Re: [PATCH 1/4] module: implement module_inhibit_unload()
On Wed, 26 Sep 2007, Tejun Heo wrote: > >> 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. That's the other part of the reason for using a loop. :-) Alan Stern - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
On Wed, 26 Sep 2007, Tejun Heo wrote: 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. That's the other part of the reason for using a loop. :-) Alan Stern - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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(_unload_wait, ); > >> + set_current_state(TASK_UNINTERRUPTIBLE); > >> + if (atomic_read(_unload_inhibit_cnt)) > >> + schedule(); > >> + __set_current_state(TASK_RUNNING); > >> + remove_wait_queue(_unload_wait, ); > >> + > >> + mutex_lock(_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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 (_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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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(>reg_mutex);
> + __ccwgroup_remove_symlinks(gdev);
> + device_unregister(dev);
> + mutex_unlock(>reg_mutex);
>
> Now, are you sure that calling cleanup_ccwgroup just after
> device_unregister() works?
>
> static void __exit
> cleanup_ccwgroup (void)
> {
> bus_unregister (_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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
On Tue, 25 Sep 2007 14:38:38 +1000,
Rusty Russell <[EMAIL PROTECTED]> wrote:
> Have you tested that *this* path works? Let's take your first change as
> an example:
>
> + mutex_lock(>reg_mutex);
> + __ccwgroup_remove_symlinks(gdev);
> + device_unregister(dev);
> + mutex_unlock(>reg_mutex);
>
> Now, are you sure that calling cleanup_ccwgroup just after
> device_unregister() works?
>
> static void __exit
> cleanup_ccwgroup (void)
> {
> bus_unregister (_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().
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
On Tue, 25 Sep 2007 14:38:38 +1000,
Rusty Russell [EMAIL PROTECTED] 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().
And no, I still haven't gotten around to testing and reviewing all
those patchsets, sorry
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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(>reg_mutex);
+ __ccwgroup_remove_symlinks(gdev);
+ device_unregister(dev);
+ mutex_unlock(>reg_mutex);
Now, are you sure that calling cleanup_ccwgroup just after
device_unregister() works?
static void __exit
cleanup_ccwgroup (void)
{
bus_unregister (_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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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(_mutex);
mod->exit();
mutex_lock(_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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
Jonathan Corbet wrote: > Hi, Tejun, > > I was just looking over these changes... > >> +/* Don't proceed till inhibition is lifted. */ >> +add_wait_queue(_unload_wait, ); >> +set_current_state(TASK_UNINTERRUPTIBLE); >> +if (atomic_read(_unload_inhibit_cnt)) >> +schedule(); >> +__set_current_state(TASK_RUNNING); >> +remove_wait_queue(_unload_wait, ); >> + >> +mutex_lock(_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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
Hi, Tejun, I was just looking over these changes... > + /* Don't proceed till inhibition is lifted. */ > + add_wait_queue(_unload_wait, ); > + set_current_state(TASK_UNINTERRUPTIBLE); > + if (atomic_read(_unload_inhibit_cnt)) > + schedule(); > + __set_current_state(TASK_RUNNING); > + remove_wait_queue(_unload_wait, ); > + > + mutex_lock(_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? jon - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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. 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? jon - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/4] module: implement module_inhibit_unload()
As its name suggests, module_inhibit_unload() inhibits all module
unloading till the matching module_allow_unload() is called. This
unload inhibition doesn't affect whether a module can be unloaded or
not. It just stalls the final module free till the inhibition is
lifted.
This sledgehammer mechanism is to be used briefly in obscure cases
where identifying or getting the module to prevent from unloading is
difficult or not worth the effort. Note that module unloading is
siberia-cold path. If the inhibion is relatively brief in human
scale, that is, upto a few secs at maximum, it should be fine. So, if
this sledgehammer simplifies API and fixes premature unload bugs which
unfortunately aren't too rare, there isn't much reason not to use it.
Even if something goes wrong with unload inhibition (e.g. someone
forgets to lift the inhibition), it doesn't prevent modules from being
loaded.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
Cc: Rusty Russell <[EMAIL PROTECTED]>
---
include/linux/module.h | 17 +
kernel/module.c| 59 ---
2 files changed, 72 insertions(+), 4 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index b6a646c..a835659 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -418,6 +418,9 @@ static inline int try_module_get(struct module *module)
extern void module_put(struct module *module);
+void module_inhibit_unload(void);
+void module_allow_unload(void);
+
#else /*!CONFIG_MODULE_UNLOAD*/
static inline int try_module_get(struct module *module)
{
@@ -431,6 +434,12 @@ static inline void __module_get(struct module *module)
}
#define symbol_put(x) do { } while(0)
#define symbol_put_addr(p) do { } while(0)
+static inline void module_inhibit_unload(void)
+{
+}
+static inline void module_allow_unload(void)
+{
+}
#endif /* CONFIG_MODULE_UNLOAD */
@@ -516,6 +525,14 @@ static inline void module_put(struct module *module)
{
}
+static inline void module_inhibit_unload(void)
+{
+}
+
+static inline void module_allow_unload(void)
+{
+}
+
#define module_name(mod) "kernel"
#define __unsafe(mod)
diff --git a/kernel/module.c b/kernel/module.c
index db0ead0..8daff45 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -44,6 +44,7 @@
#include
#include
#include
+#include
#include
extern int module_sysfs_initialized;
@@ -65,6 +66,8 @@ extern int module_sysfs_initialized;
* (add/delete uses stop_machine). */
static DEFINE_MUTEX(module_mutex);
static LIST_HEAD(modules);
+static atomic_t module_unload_inhibit_cnt = ATOMIC_INIT(0);
+static DECLARE_WAIT_QUEUE_HEAD(module_unload_wait);
static BLOCKING_NOTIFIER_HEAD(module_notify_list);
@@ -656,6 +659,7 @@ static void wait_for_zero_refcount(struct module *mod)
asmlinkage long
sys_delete_module(const char __user *name_user, unsigned int flags)
{
+ DECLARE_WAITQUEUE(wait, current);
struct module *mod;
char name[MODULE_NAME_LEN];
int ret, forced = 0;
@@ -714,12 +718,22 @@ sys_delete_module(const char __user *name_user, unsigned
int flags)
if (!forced && module_refcount(mod) != 0)
wait_for_zero_refcount(mod);
+ mutex_unlock(_mutex);
+
/* Final destruction now noone is using it. */
- if (mod->exit != NULL) {
- mutex_unlock(_mutex);
+ if (mod->exit != NULL)
mod->exit();
- mutex_lock(_mutex);
- }
+
+ /* Don't proceed till inhibition is lifted. */
+ add_wait_queue(_unload_wait, );
+ set_current_state(TASK_UNINTERRUPTIBLE);
+ if (atomic_read(_unload_inhibit_cnt))
+ schedule();
+ __set_current_state(TASK_RUNNING);
+ remove_wait_queue(_unload_wait, );
+
+ mutex_lock(_mutex);
+
free_module(mod);
out:
@@ -805,6 +819,43 @@ void module_put(struct module *module)
}
EXPORT_SYMBOL(module_put);
+/**
+ * module_inhibit_unload - inhibit module unload
+ *
+ * Inhibit module unload until allowed again. All module unload
+ * operations which reach zero reference count after this call
+ * has returned are guaranteed to be stalled till inhibition is
+ * lifted.
+ *
+ * This is a simple mechanism to prevent premature unload while
+ * code on a to-be-unloaded module is still executing. Unload
+ * inhibitions must be finite and relatively short.
+ *
+ * LOCKING:
+ * None.
+ */
+void module_inhibit_unload(void)
+{
+ atomic_inc(_unload_inhibit_cnt);
+}
+
+/**
+ * module_allow_unload - allow module unload
+ *
+ * Allow module unload. Must be balanced with calls to
+ * module_inhibit_unload().
+ *
+ * LOCKING:
+ * None.
+ */
+void module_allow_unload(void)
+{
+ if (atomic_dec_and_test(_unload_inhibit_cnt))
+ wake_up_all(_unload_wait);
+
+ BUG_ON(atomic_read(_unload_inhibit_cnt) < 0);
+}
+
#else /* !CONFIG_MODULE_UNLOAD */
static void
[PATCH 1/4] module: implement module_inhibit_unload()
As its name suggests, module_inhibit_unload() inhibits all module
unloading till the matching module_allow_unload() is called. This
unload inhibition doesn't affect whether a module can be unloaded or
not. It just stalls the final module free till the inhibition is
lifted.
This sledgehammer mechanism is to be used briefly in obscure cases
where identifying or getting the module to prevent from unloading is
difficult or not worth the effort. Note that module unloading is
siberia-cold path. If the inhibion is relatively brief in human
scale, that is, upto a few secs at maximum, it should be fine. So, if
this sledgehammer simplifies API and fixes premature unload bugs which
unfortunately aren't too rare, there isn't much reason not to use it.
Even if something goes wrong with unload inhibition (e.g. someone
forgets to lift the inhibition), it doesn't prevent modules from being
loaded.
Signed-off-by: Tejun Heo [EMAIL PROTECTED]
Cc: Rusty Russell [EMAIL PROTECTED]
---
include/linux/module.h | 17 +
kernel/module.c| 59 ---
2 files changed, 72 insertions(+), 4 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index b6a646c..a835659 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -418,6 +418,9 @@ static inline int try_module_get(struct module *module)
extern void module_put(struct module *module);
+void module_inhibit_unload(void);
+void module_allow_unload(void);
+
#else /*!CONFIG_MODULE_UNLOAD*/
static inline int try_module_get(struct module *module)
{
@@ -431,6 +434,12 @@ static inline void __module_get(struct module *module)
}
#define symbol_put(x) do { } while(0)
#define symbol_put_addr(p) do { } while(0)
+static inline void module_inhibit_unload(void)
+{
+}
+static inline void module_allow_unload(void)
+{
+}
#endif /* CONFIG_MODULE_UNLOAD */
@@ -516,6 +525,14 @@ static inline void module_put(struct module *module)
{
}
+static inline void module_inhibit_unload(void)
+{
+}
+
+static inline void module_allow_unload(void)
+{
+}
+
#define module_name(mod) kernel
#define __unsafe(mod)
diff --git a/kernel/module.c b/kernel/module.c
index db0ead0..8daff45 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -44,6 +44,7 @@
#include asm/uaccess.h
#include asm/semaphore.h
#include asm/cacheflush.h
+#include asm/atomic.h
#include linux/license.h
extern int module_sysfs_initialized;
@@ -65,6 +66,8 @@ extern int module_sysfs_initialized;
* (add/delete uses stop_machine). */
static DEFINE_MUTEX(module_mutex);
static LIST_HEAD(modules);
+static atomic_t module_unload_inhibit_cnt = ATOMIC_INIT(0);
+static DECLARE_WAIT_QUEUE_HEAD(module_unload_wait);
static BLOCKING_NOTIFIER_HEAD(module_notify_list);
@@ -656,6 +659,7 @@ static void wait_for_zero_refcount(struct module *mod)
asmlinkage long
sys_delete_module(const char __user *name_user, unsigned int flags)
{
+ DECLARE_WAITQUEUE(wait, current);
struct module *mod;
char name[MODULE_NAME_LEN];
int ret, forced = 0;
@@ -714,12 +718,22 @@ sys_delete_module(const char __user *name_user, unsigned
int flags)
if (!forced module_refcount(mod) != 0)
wait_for_zero_refcount(mod);
+ mutex_unlock(module_mutex);
+
/* Final destruction now noone is using it. */
- if (mod-exit != NULL) {
- mutex_unlock(module_mutex);
+ if (mod-exit != NULL)
mod-exit();
- mutex_lock(module_mutex);
- }
+
+ /* 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);
+
free_module(mod);
out:
@@ -805,6 +819,43 @@ void module_put(struct module *module)
}
EXPORT_SYMBOL(module_put);
+/**
+ * module_inhibit_unload - inhibit module unload
+ *
+ * Inhibit module unload until allowed again. All module unload
+ * operations which reach zero reference count after this call
+ * has returned are guaranteed to be stalled till inhibition is
+ * lifted.
+ *
+ * This is a simple mechanism to prevent premature unload while
+ * code on a to-be-unloaded module is still executing. Unload
+ * inhibitions must be finite and relatively short.
+ *
+ * LOCKING:
+ * None.
+ */
+void module_inhibit_unload(void)
+{
+ atomic_inc(module_unload_inhibit_cnt);
+}
+
+/**
+ * module_allow_unload - allow module unload
+ *
+ * Allow module unload. Must be balanced with calls to
+ * module_inhibit_unload().
+ *
+ * LOCKING:
+ * None.
+ */
+void module_allow_unload(void)
+{
+ if (atomic_dec_and_test(module_unload_inhibit_cnt))
+
