sysfs: allow suicide

Sysfs now allows direct suicide. Make suicidial sysfs nodes to use it and remove now unncessary callback mechanism.

Signed-off-by Tejun Heo <[email protected]>

---
 drivers/base/core.c         |   33 ----------------------
 drivers/s390/cio/ccwgroup.c |   25 +++-------------
 drivers/scsi/scsi_sysfs.c   |   13 +--------
 fs/sysfs/file.c             |   63 -------------------------------------------
 include/linux/device.h      |    6 ----
 include/linux/sysfs.h       |    9 ------
 6 files changed, 6 insertions(+), 143 deletions(-)

Quoted 4 lines · Tejun Heo

Sysfs now allows direct suicide. Make suicidial sysfs nodes to use it and remove now unncessary callback mechanism.

Signed-off-by Tejun Heo <[email protected]>

Quoted 9 lines

---
 drivers/base/core.c         |   33 ----------------------
 drivers/s390/cio/ccwgroup.c |   25 +++-------------
 drivers/scsi/scsi_sysfs.c   |   13 +--------
 fs/sysfs/file.c             |   63 -------------------------------------------
 include/linux/device.h      |    6 ----
 include/linux/sysfs.h       |    9 ------
 6 files changed, 6 insertions(+), 143 deletions(-)

Just browsing through this (I'll review and do some tests later), but you missed drivers/s390/cio/device.c::ccw_device_remove_disconnected().

Following (not event compile tested) patch (which is basically a revert of 59a8a6e227cf0bc42e5be741ebfea97c222ab9ef) should take care of it.

Signed-off-by Cornelia Huck <[email protected]>

Hi, Tejun,

I was just looking over these changes...

kernel/module.c sys_delete_module(const char __user *name_user, unsigned int flags) · line 735

Quoted 9 lines

kernel/module.c +9 -0

+	/* Don't proceed till inhibition is lifted. */
+	add_wait_queue(&module_unload_wait, &wait);
+	set_current_state(TASK_UNINTERRUPTIBLE);
+	if (atomic_read(&module_unload_inhibit_cnt))
+		schedule();
+	__set_current_state(TASK_RUNNING);
+	remove_wait_queue(&module_unload_wait, &wait);
+
+	mutex_lock(&module_mutex);

Maybe I'm missing something, but this looks racy to me. There's no check after schedule() to see if module_unload_inhibit_cnt is really zero, and nothing to keep somebody else from slipping in and raising it again afterward.

Given your description of this tool as a "sledgehammer," might it not be easier to just take and hold module_mutex for the duration of the unload block?

Quoted 3 lines

Given your description of this tool as a "sledgehammer," might it not be easier to just take and hold module_mutex for the duration of the unload block?

That would be easier but...

* It would serialize users of the sledgehammer.
* It would block loading modules (which is often more important than unloading them) when things go south.

Quoted 3 lines · Tejun Heo

Quoted 3 lines

Given your description of this tool as a "sledgehammer," might it not be easier to just take and hold module_mutex for the duration of the unload block?

Quoted 5 lines

That would be easier but...

* It would serialize users of the sledgehammer.
* It would block loading modules (which is often more important than unloading them) when things go south.

My concern is that you're dropping the module mutex around ->exit now. I don't *think* this should matter, but it's worth considering.

I really wonder if an explicit "kill_this_attribute()" is a better way to go than this...

Quoted 2 lines · Rusty Russell

My concern is that you're dropping the module mutex around ->exit now. I don't *think* this should matter, but it's worth considering.

We always did that. Before the patch the code segment looked like the following.

Quoted 2 lines

I really wonder if an explicit "kill_this_attribute()" is a better way to go than this...

I think this sort of temporary unload blocking would be useful for other cases like this.

Quoted 4 lines

Quoted 2 lines

I really wonder if an explicit "kill_this_attribute()" is a better way to go than this...

I think this sort of temporary unload blocking would be useful for other cases like this.

I hope not: this doesn't work in general. Calling into a module after ->exit has called assumes that the exit function doesn't free up or overwrite stuff the other functions need.

Quoted 5 lines · Rusty Russell

Quoted 4 lines

Quoted 2 lines

I really wonder if an explicit "kill_this_attribute()" is a better way to go than this...

I think this sort of temporary unload blocking would be useful for other cases like this.

Quoted 3 lines

I hope not: this doesn't work in general. Calling into a module after ->exit has called assumes that the exit function doesn't free up or overwrite stuff the other functions need.

Right, the sole purpose the unload inhibition is to hold onto the 'code' section from going away. The rest of object lifetime management should be implemented using separate mechanisms anyway. I was talking about similar cases where the 'code' should be protected for a short time.

As stated you cannot protect arbitrary code this way, as you are trying to do. I do not think you've broken any of the current code, but I cannot tell. You're certainly going to surprise unsuspecting future authors.

Can you really not figure out the module owner of the sysfs entry to inc its use count during this procedure? (__module_get()).

Quoted 4 lines · Rusty Russell

As stated you cannot protect arbitrary code this way, as you are trying to do. I do not think you've broken any of the current code, but I cannot tell. You're certainly going to surprise unsuspecting future authors.

Can you elaborate a bit? Why can't it protect the code?

Quoted 2 lines

Can you really not figure out the module owner of the sysfs entry to inc its use count during this procedure? (__module_get()).

I can but I don't think it's worth the effort. It will involve passing @owner parameter down through kobject to sysfs but the path is pretty obscure and thus difficult to test.

I think it's too much work for the
users of the API and it will be easy to pass the wrong @owner and go unnoticed.

Have you tested that *this* path works? Let's take your first change as an example:

(quoted diff) +4 -0

+       mutex_lock(&gdev->reg_mutex);
+       __ccwgroup_remove_symlinks(gdev);
+       device_unregister(dev);
+       mutex_unlock(&gdev->reg_mutex);

Now, are you sure that calling cleanup_ccwgroup just after device_unregister() works?

static void __exit
cleanup_ccwgroup (void)

{
	bus_unregister (&ccwgroup_bus_type);
}

Quoted 3 lines

I think it's too much work for the
users of the API and it will be easy to pass the wrong @owner and go unnoticed.

But your shortcut insists that all module authors be aware that functions can be running after exit() is called. That's a recipe for instability and disaster.

Quoted 6 lines · Tejun Heo

Quoted 4 lines · Rusty Russell

As stated you cannot protect arbitrary code this way, as you are trying to do. I do not think you've broken any of the current code, but I cannot tell. You're certainly going to surprise unsuspecting future authors.

Can you elaborate a bit? Why can't it protect the code?

Because you don't know what that code does. After all, it's assumed that module code doesn't get called after exit and you're deliberately violating that assumption.

Quoted 5 lines

Quoted 2 lines

Can you really not figure out the module owner of the sysfs entry to inc its use count during this procedure? (__module_get()).

I can but I don't think it's worth the effort. It will involve passing @owner parameter down through kobject to sysfs but the path is pretty obscure and thus difficult to test.

Have you tested that *this* path works? Let's take your first change as an example:

(quoted diff) +4 -0

+       mutex_lock(&gdev->reg_mutex);
+       __ccwgroup_remove_symlinks(gdev);
+       device_unregister(dev);
+       mutex_unlock(&gdev->reg_mutex);

Now, are you sure that calling cleanup_ccwgroup just after device_unregister() works?

static void __exit
cleanup_ccwgroup (void)

{
	bus_unregister (&ccwgroup_bus_type);
}

Quoted 3 lines

I think it's too much work for the
users of the API and it will be easy to pass the wrong @owner and go unnoticed.

But your shortcut insists that all module authors be aware that functions can be running after exit() is called. That's a recipe for instability and disaster.

Quoted 8 lines · Rusty Russell

Now, are you sure that calling cleanup_ccwgroup just after device_unregister() works?

static void __exit
cleanup_ccwgroup (void)

{
	bus_unregister (&ccwgroup_bus_type);
}

It should. After ->exit() is called, there can't be any object left behind. If a module is hosting objects which can't be destroyed from ->exit(), its module ref count shouldn't be zero. So, either 1. refcount != 0 or 2. ->exit() can destroy all objects. As Cornelia explains, for ccwgroup, it's #1. Note that unload inhibition doesn't change anything about this.

Hmmm.... There doesn't seem to any reason why the blocking should be after calling ->exit(). And, yeah, it would be more useful and intuitive if blocking happens before ->exit(). What do you think?

Quoted 3 lines · Tejun Heo

Hmmm.... There doesn't seem to any reason why the blocking should be after calling ->exit(). And, yeah, it would be more useful and intuitive if blocking happens before ->exit(). What do you think?

*That* I have no problem with.

I was going to say "just grab a reference to every module" except if a new module is loaded you don't know about it.

If you move your blocking, it seems fine.

Quoted 4 lines · Jonathan Corbet

Hi, Tejun,

I was just looking over these changes...

Quoted 14 lines

kernel/module.c sys_delete_module(const char __user *name_user, unsigned int flags) · line 735

Quoted 9 lines

kernel/module.c +9 -0

+	/* Don't proceed till inhibition is lifted. */
+	add_wait_queue(&module_unload_wait, &wait);
+	set_current_state(TASK_UNINTERRUPTIBLE);
+	if (atomic_read(&module_unload_inhibit_cnt))
+		schedule();
+	__set_current_state(TASK_RUNNING);
+	remove_wait_queue(&module_unload_wait, &wait);
+
+	mutex_lock(&module_mutex);

Maybe I'm missing something, but this looks racy to me. There's no check after schedule() to see if module_unload_inhibit_cnt is really zero, and nothing to keep somebody else from slipping in and raising it again afterward.

The unloading can proceed once module_unload_inhibit_cnt reaches zero. An unloading thread only has to care about inhibition put in effect before unloading has started, so there's no need to check again.

Quoted 3 lines

The unloading can proceed once module_unload_inhibit_cnt reaches zero. An unloading thread only has to care about inhibition put in effect before unloading has started, so there's no need to check again.

You haven't fully answered Jon's question. Suppose module_unload_inhibit_cnt is nonzero, so the task adds itself to the module_unload_wait queue, changes to TASK_UNINTERRUPTIBLE, and calls schedule. There's nothing to prevent somebody else from waking the task back up before the original inhibition has been lifted.

Quoted 4 lines · Alan Stern

Quoted 3 lines

The unloading can proceed once module_unload_inhibit_cnt reaches zero. An unloading thread only has to care about inhibition put in effect before unloading has started, so there's no need to check again.

Quoted 5 lines

You haven't fully answered Jon's question. Suppose module_unload_inhibit_cnt is nonzero, so the task adds itself to the module_unload_wait queue, changes to TASK_UNINTERRUPTIBLE, and calls schedule. There's nothing to prevent somebody else from waking the task back up before the original inhibition has been lifted.

Hmmm... I might be missing something here. Who else can wake up a thread in uninterruptible sleep?

Hello, all.

This is the second patchset of four sysfs update patchset series[1] and to be applied on top of the first patchset[1].

Quoted 9 lines · Tejun Heo

Quoted 8 lines · Alan Stern

Quoted 3 lines

The unloading can proceed once module_unload_inhibit_cnt reaches zero. An unloading thread only has to care about inhibition put in effect before unloading has started, so there's no need to check again.

You haven't fully answered Jon's question. Suppose module_unload_inhibit_cnt is nonzero, so the task adds itself to the module_unload_wait queue, changes to TASK_UNINTERRUPTIBLE, and calls schedule. There's nothing to prevent somebody else from waking the task back up before the original inhibition has been lifted.

Quoted 2 lines

Hmmm... I might be missing something here. Who else can wake up a thread in uninterruptible sleep?

In principle, anything can. There has never been any guarantee in the kernel that a task sleeping on a waitqueue will remain asleep until the waitqueue is signalled. That's part of the reason why things like __wait_event() are coded as loops.

I have no problem with changing the condition check to loop but it would be great if someone can point me to a code where this unexpected wake up is used.

Quoted 3 lines · Tejun Heo

I have no problem with changing the condition check to loop but it would be great if someone can point me to a code where this unexpected wake up is used.

This is one of those areas where we're conservative. Historically there have been random wakes, and noone is quite sure that signal code or the freezer or whatever won't do it under some circumstances.

Thus it's always seen as better to wait on a specific condition.

Quoted 7 lines

Quoted 2 lines

Hmmm... I might be missing something here. Who else can wake up a thread in uninterruptible sleep?

In principle, anything can. There has never been any guarantee in the kernel that a task sleeping on a waitqueue will remain asleep until the waitqueue is signalled. That's part of the reason why things like __wait_event() are coded as loops.

Hmmm... I always thought the queue was because the condition can change inbetween waking up and actually running. For example, if the condition is !(queue empty), another task can enter the critical section and consume the element which triggered wake up before the woken up task do.

Sysfs now allows direct suicide. Make suicidial sysfs nodes to use it and remove now unncessary callback mechanism.

Signed-off-by Tejun Heo <[email protected]>

---
 drivers/base/core.c         |   33 ----------------------
 drivers/s390/cio/ccwgroup.c |   25 +++-------------
 drivers/scsi/scsi_sysfs.c   |   13 +--------
 fs/sysfs/file.c             |   63 -------------------------------------------
 include/linux/device.h      |    6 ----
 include/linux/sysfs.h       |    9 ------
 6 files changed, 6 insertions(+), 143 deletions(-)

Quoted 4 lines · Tejun Heo

Sysfs now allows direct suicide. Make suicidial sysfs nodes to use it and remove now unncessary callback mechanism.

Signed-off-by Tejun Heo <[email protected]>

Quoted 8 lines

---
 drivers/base/core.c         |   33 ----------------------
 drivers/s390/cio/ccwgroup.c |   25 +++-------------
 drivers/scsi/scsi_sysfs.c   |   13 +--------
 fs/sysfs/file.c             |   63 -------------------------------------------
 include/linux/device.h      |    6 ----
 include/linux/sysfs.h       |    9 ------
 6 files changed, 6 insertions(+), 143 deletions(-)

ccwgroup was implicitly relying on a held reference, fix that.

Signed-off-by Cornelia Huck <[email protected]>