lkml.org 
[lkml]   [2021]   [Nov]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V4 3/3] livepatch: free klp_patch object synchronously
    On Wed, Nov 03, 2021 at 02:55:19PM +0100, Petr Mladek wrote:
    > On Tue 2021-11-02 22:59:32, Ming Lei wrote:
    > > klp_mutex isn't acquired before calling kobject_put(klp_patch), so it is
    > > fine to free klp_patch object synchronously.
    > >
    > > One issue is that enabled store() method, in which the klp_patch kobject
    > > itself is deleted & released. However, sysfs has provided APIs for dealing
    > > with this corner case, so use sysfs_break_active_protection() and
    > > sysfs_unbreak_active_protection() for releasing klp_patch kobject from
    > > enabled_store(), meantime the enabled attribute has to be removed
    > > before deleting the klp_patch kobject.
    > >
    > > --- a/kernel/livepatch/core.c
    > > +++ b/kernel/livepatch/core.c
    > > @@ -369,10 +370,18 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
    > > out:
    > > mutex_unlock(&klp_mutex);
    > >
    > > - klp_free_patches_async(&to_free);
    > > -
    > > if (ret)
    > > return ret;
    > > +
    > > + if (!list_empty(&to_free)) {
    > > + kn = sysfs_break_active_protection(kobj, &attr->attr);
    > > + WARN_ON_ONCE(!kn);
    > > + sysfs_remove_file(kobj, &attr->attr);
    > > + klp_free_patches(&to_free);
    > > + if (kn)
    > > + sysfs_unbreak_active_protection(kn);
    > > + }
    >
    > I agree that using workqueues for free_work looks like a hack.
    > But this looks even more tricky and fragile to me. It feels like
    > playing with sysfs/kernfs internals.
    >
    > It might look less tricky when using sysfs_remove_file_self().

    The protection needs to cover removing both 'enabled' attribute and
    the patch kobject, so sysfs_remove_file_self() isn't good here.

    >
    > Anyway, there are only few users of these APIs:
    >
    > + sysfs_break_active_protection() is used only scsi
    > + kernfs_break_active_protection() is used by cgroups, cpusets, and rdtgroup.
    > + sysfs_remove_file_self() is used by some RDMA-related stuff.
    >
    > It means that there are some users but it is not widely used API.

    It is used by generic pci device and scsi device, both are the most popular
    devices in the world, either one of the two subsystem should have huge amount
    of users, so it means the interface itself has been proved/verified for long
    time by many enough real users.

    >
    > I would personally prefer to keep it as is. I do not see any
    > fundamental advantage of the new code. But I might be biased
    > because the current code was written by me ;-)

    The fundamental advantage is that the API has been used/verified by
    enough real users. Also killing attribute/kobject itself isn't unique
    for livepatch, that is actually one common pattern, so it needn't
    such hacky implementation.


    Thanks,
    Ming

    \
     
     \ /
      Last update: 2021-11-05 09:02    [W:4.870 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site