Jonathan Corbet wrote:
> Hi, Tejun,
>
> I was just looking over these changes...
>
>> + /* 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.

> 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.

--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Tue, 2007-09-25 at 08:18 +0900, Tejun Heo wrote:
> > 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.

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...

Rusty.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Rusty Russell wrote:
> On Tue, 2007-09-25 at 08:18 +0900, Tejun Heo wrote:
>>> 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.
>
> 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.

/* Final destruction now noone is using it. */
if (mod->exit != NULL) {
mutex_unlock(&module_mutex);
mod->exit();
mutex_lock(&module_mutex);
}

> 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.

Thanks.

--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Tue, 2007-09-25 at 10:40 +0900, Tejun Heo wrote:
> Rusty Russell wrote:
> > 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.

Hi Tejun,

Thanks, misread patch.

> > 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.

Cheers,
Rusty.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Rusty Russell wrote:
>>> 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.

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.

Thanks.

--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Tue, 2007-09-25 at 11:39 +0900, Tejun Heo wrote:
> Rusty Russell wrote:
> >>> 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.
>
> 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()).

Rusty.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Rusty Russell wrote:
> 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?

> 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.

Thanks.

--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Tue, 2007-09-25 at 12:36 +0900, Tejun Heo wrote:
> Rusty Russell wrote:
> > 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.

> > 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:

+ 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);
}

> 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.

Rusty.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Tue, 25 Sep 2007 14:38:38 +1000,
Rusty Russell wrote:

> Have you tested that *this* path works? Let's take your first change as
> an example:
>
> + 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);
> }
>

ccwgroup is a bit special. The ccwgroup drivers (say, qeth) will
unregister their ccwgroup_driver in their exit function. ccwgroup will
then unregister all devices bound to this driver (a ccwgroup device
without a driver does not make sense, since they are artifically
created by writing to a 'group' attribute provided by the driver). This
makes sure that ccwgroup cannot be unloaded while there are still
devices on the bus, so your example won't hit.

> > 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.

There are already problems like this with ->release().

those patchsets, sorry>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Rusty Russell wrote:
> On Tue, 2007-09-25 at 12:36 +0900, Tejun Heo wrote:
>> Rusty Russell wrote:
>>> 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.

What I meant by protecting 'code' was the 'code' itself. Those pages
containing instructions that cpu executes. It of course can't protect
against all the things they do.

>>> 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:
>
> + 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);
> }

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.

>> 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.

No, it doesn't change that at all. All unload inhibition does is
postponing removal of code (and data too of course) section a bit so
that a module can host code which issues unloading of itself. Object
synchronization rules remain exactly the same. Formerly broken code is
still broken and I don't even think unload inhibition would mask them
too much either.

I think the naming is too ambiguous. Maybe it should be named something
like "hold_module_for_suicide".

Thanks.

--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Tejun Heo wrote:
> Rusty Russell wrote:
>> 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?

Thanks.

--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Tue, 2007-09-25 at 17:36 +0900, Tejun Heo wrote:
> 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.

Thanks!
Rusty.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Rusty Russell wrote:
> On Tue, 2007-09-25 at 17:36 +0900, Tejun Heo wrote:
>> 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.

Great, I think I was too occupied with the sysfs case when I was writing
it. I'll update the patch. Thanks a lot.

--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Tue, 25 Sep 2007, Tejun Heo wrote:

> Jonathan Corbet wrote:
> > Hi, Tejun,
> >
> > I was just looking over these changes...
> >
> >> + /* 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.

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.

Alan Stern

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Alan Stern wrote:
>> 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.

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

Thanks.

--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Tue, 25 Sep 2007, Tejun Heo wrote:

> Alan Stern wrote:
> >> 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.
>
> 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.

Alan Stern

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Alan Stern wrote:
> On Tue, 25 Sep 2007, Tejun Heo wrote:
>
>> Alan Stern wrote:
>>>> 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.
>> 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.

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.

Thanks.

--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Wed, 2007-09-26 at 08:15 +0900, Tejun Heo wrote:
> 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.

Cheers,
Rusty.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Rusty Russell wrote:
> On Wed, 2007-09-26 at 08:15 +0900, Tejun Heo wrote:
>> 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.

I see. I'll update the code then. Thanks.

--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/