try_module_get and friends

try_module_get and friends

[Jon Masters]

Yo,

Ok...so I might just be missing something and you guys will set me right
if that is indeed the case, but...

What's protecting the -RT kernel from assumptions about atomicity of
module symbol references? Specifically, the kernel doesn't normally need
to protect against inter-module symbol references to EXPORT_SYMBOL(s),
however, it requires special protection when a function pointer is being
passed around - that's why we use things like try_module_get. But it is
assumed that it is ok not to do this in atomic contexts.

So, what's protecting the RT kernel from falling over if a CPU frequency
governor module is removed at the wrong moment?

Jon.

Re: try_module_get and friends

[Jon Masters]

On Tue, 2008-03-04 at 00:20 -0500, Jon Masters wrote:

> So, what's protecting the RT kernel from falling over if a CPU frequency
> governor module is removed at the wrong moment?

That's a contrived example, it does the right thing in the ones I've
looked it, but it just occurred to me that this might not be general for
kernel code where random pointers to modules are getting thrown around.

Jon.

Re: try_module_get and friends

[Jon Masters]

On Tue, 2008-03-04 at 00:28 -0500, Jon Masters wrote:
> On Tue, 2008-03-04 at 00:20 -0500, Jon Masters wrote:
> 
> > So, what's protecting the RT kernel from falling over if a CPU frequency
> > governor module is removed at the wrong moment?
> 
> That's a contrived example, it does the right thing in the ones I've
> looked it, but it just occurred to me that this might not be general for
> kernel code where random pointers to modules are getting thrown around.

I recommend not setting CONFIG_MODULE_UNLOAD on RT kernels for now. I
looked at programmatically implementing this in Kconfig, but it seems
that CONFIG_MODULE_UNLOAD is never explicitly set anywhere, it's simply
in the deconfig for each architecture - anyone know Kconfig well enough
to tell us how to fix up Kconfig.preempt to do the right thing?

Jon.

Re: try_module_get and friends

[Daniel Walker]

On Tue, 2008-03-04 at 15:07 -0500, Jon Masters wrote:
> On Tue, 2008-03-04 at 00:28 -0500, Jon Masters wrote:
> > On Tue, 2008-03-04 at 00:20 -0500, Jon Masters wrote:
> > 
> > > So, what's protecting the RT kernel from falling over if a CPU frequency
> > > governor module is removed at the wrong moment?
> > 
> > That's a contrived example, it does the right thing in the ones I've
> > looked it, but it just occurred to me that this might not be general for
> > kernel code where random pointers to modules are getting thrown around.
> 
> I recommend not setting CONFIG_MODULE_UNLOAD on RT kernels for now. I
> looked at programmatically implementing this in Kconfig, but it seems
> that CONFIG_MODULE_UNLOAD is never explicitly set anywhere, it's simply
> in the deconfig for each architecture - anyone know Kconfig well enough
> to tell us how to fix up Kconfig.preempt to do the right thing?

Could you give a different of better example of the problem with
modules? This thread has been fairly scant on details ..

Daniel

Re: try_module_get and friends

[Steven Rostedt]

On Wed, 5 Mar 2008, Daniel Walker wrote:
>
> Could you give a different of better example of the problem with
> modules? This thread has been fairly scant on details ..

There's a theoretical potential for the system to crash in -rt when
unloading a module. Simply because interrupts are threads.

Some device has an interrupt handler that is called and preempted.
At that moment the module for that device is unloaded. This calls things
like kstop_machine that should make sure that all interrupt handlers have
been resolved. But this does not work with -rt, since the interrupt
handlers can now be preempted and resume after the kstop_machine and the
moduling being unloaded.

Since the code for the interrupt handler no longer exists... KABOOM!

Enough details?

-- Steve

Re: try_module_get and friends

[Daniel Walker]

On Wed, 2008-03-05 at 14:47 -0500, Steven Rostedt wrote:
> Some device has an interrupt handler that is called and preempted.
> At that moment the module for that device is unloaded. This calls things
> like kstop_machine that should make sure that all interrupt handlers have
> been resolved. But this does not work with -rt, since the interrupt
> handlers can now be preempted and resume after the kstop_machine and the
> moduling being unloaded.
> 
> Since the code for the interrupt handler no longer exists... KABOOM!
> 
> Enough details?

yeah, you have to assume the device is not in use, but still handling
interrupts?

Daniel

Re: try_module_get and friends

[Frank Ch. Eigler]

Steven Rostedt <rostedt@goodmis.org> writes:

> [...]
> There's a theoretical potential for the system to crash in -rt when
> unloading a module. Simply because interrupts are threads.
>
> Some device has an interrupt handler that is called and preempted.
> At that moment the module for that device is unloaded. [...]
> Since the code for the interrupt handler no longer exists... KABOOM!

Perhaps you can identify those modules that have asked for interrupts,
and block only them from unloading.  Or, would it be outrageously
expensive to increment the module refcount while one of its interrupt
handlers is running?

- FChE

Re: try_module_get and friends

[Steven Rostedt]

On Wed, 5 Mar 2008, Frank Ch. Eigler wrote:
> > There's a theoretical potential for the system to crash in -rt when
> > unloading a module. Simply because interrupts are threads.
> >
> > Some device has an interrupt handler that is called and preempted.
> > At that moment the module for that device is unloaded. [...]
> > Since the code for the interrupt handler no longer exists... KABOOM!
>
> Perhaps you can identify those modules that have asked for interrupts,
> and block only them from unloading.  Or, would it be outrageously
> expensive to increment the module refcount while one of its interrupt
> handlers is running?

Actually, we were simply thinking of calling an rcu_lock before checking
the handlers, and releasing the lock when all are done. Then before doing
the module unload, we do a rcu_sync. Note, no interrupt handler should
ever do a rcu_sync.

-- Steve

Re: try_module_get and friends

[Steven Rostedt]

On Wed, 5 Mar 2008, Frank Ch. Eigler wrote:
> > There's a theoretical potential for the system to crash in -rt when
> > unloading a module. Simply because interrupts are threads.
> >
> > Some device has an interrupt handler that is called and preempted.
> > At that moment the module for that device is unloaded. [...]
> > Since the code for the interrupt handler no longer exists... KABOOM!
>
> Perhaps you can identify those modules that have asked for interrupts,
> and block only them from unloading.  Or, would it be outrageously
> expensive to increment the module refcount while one of its interrupt
> handlers is running?

Actually this whole thing is theoretical. Since the device would have to
free the irq before unloading and interrupt desc->lock protects races like
this. Then there is the question of softirqs and tasklets. And random
passing of pointers. But this can also happen in mainline since tasklets
and softirqs can run in a thread.

We need to look deeper. I'm not sure there is an issue here.

-- Steve

Re: try_module_get and friends

[Daniel Walker]

On Wed, 2008-03-05 at 15:54 -0500, Steven Rostedt wrote:

> Actually, we were simply thinking of calling an rcu_lock before checking
> the handlers, and releasing the lock when all are done. Then before doing
> the module unload, we do a rcu_sync. Note, no interrupt handler should
> ever do a rcu_sync.

There's already some smp synchronize inside free_irq, are you sure
that's not already handling this for smp? For -rt we could just enable
it for UP ..

Daniel

Re: try_module_get and friends

[Paul E. McKenney]

On Wed, Mar 05, 2008 at 01:15:02PM -0800, Daniel Walker wrote:
> 
> On Wed, 2008-03-05 at 15:54 -0500, Steven Rostedt wrote:
> 
> > Actually, we were simply thinking of calling an rcu_lock before checking
> > the handlers, and releasing the lock when all are done. Then before doing
> > the module unload, we do a rcu_sync. Note, no interrupt handler should
> > ever do a rcu_sync.
> 
> There's already some smp synchronize inside free_irq, are you sure
> that's not already handling this for smp? For -rt we could just enable
> it for UP ..

If (desc->status & IRQ_INPROGRESS) remains true throughout the
execution of the threaded (and thus preemptible) interrupt handler,
then the synchronize_irq() should do the trick.

							Thanx, Paul

Re: try_module_get and friends

[Jon Masters]

On Wed, 2008-03-05 at 16:14 -0500, Steven Rostedt wrote:
> On Wed, 5 Mar 2008, Frank Ch. Eigler wrote:
> > > There's a theoretical potential for the system to crash in -rt when
> > > unloading a module. Simply because interrupts are threads.
> > >
> > > Some device has an interrupt handler that is called and preempted.
> > > At that moment the module for that device is unloaded. [...]
> > > Since the code for the interrupt handler no longer exists... KABOOM!
> >
> > Perhaps you can identify those modules that have asked for interrupts,
> > and block only them from unloading.  Or, would it be outrageously
> > expensive to increment the module refcount while one of its interrupt
> > handlers is running?
> 
> Actually this whole thing is theoretical. Since the device would have to
> free the irq before unloading and interrupt desc->lock protects races like
> this. Then there is the question of softirqs and tasklets. And random
> passing of pointers. But this can also happen in mainline since tasklets
> and softirqs can run in a thread.
> 
> We need to look deeper. I'm not sure there is an issue here.

I can poke some more and look at what actually happens on module unload
(for now I think a quick test script loading/unloading modules in a loop
should show us whether this is enough of a problem in reality), but I do
want to point out that I was never concerned with interrupt handlers
themselves - only the assumptions mainline makes about "interrupt
context" use of pointers without an explicit try_module_get.

Rusty didn't have a simple answer either, so I'll poke.

Jon.