Thread (13 messages) flat view 13 messages, 3 authors, 2021-11-10

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help