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().
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.
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 ;-)
Best Regards,
Petr
