Re: [PATCH V4 3/3] livepatch: free klp_patch object synchronously

[Ming Lei]

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