Re: [PATCH V4 3/3] livepatch: free klp_patch object synchronously
From: Petr Mladek <pmladek@suse.com>
Date: 2021-11-03 13:55:28
Also in:
lkml
On Tue 2021-11-02 22:59:32, Ming Lei wrote:
quoted hunk ↗ jump to hunk
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