information about kernel locking issue

information about kernel locking issue

[Giacomo S.]

Good morning.

Some time ago i wrote a module in kernel space which does some filtering.

While running `dhclient eth0'  and plugging/unplugging the ethernet cable
from the network card, kernel freezed saying the culprit was the following
function (EIP is at get_ifaddr_by_name() ) :

/* returns in *addr the internet address having the name ifname */
int get_ifaddr_by_name(const char *ifname, __u32 * addr)
{
       struct net_device *pnet_device;
       struct in_device *pin_device;

       read_lock_bh(&dev_base_lock);
       pnet_device = dev_base;
       while (pnet_device != NULL)
         {
                 if ((netif_running(pnet_device))
                     && (pnet_device->ip_ptr != NULL)
                     && (strcmp(pnet_device->name, ifname) == 0))
                   {
                           pin_device =
                               (struct in_device *) pnet_device->ip_ptr;
                           /* ifa_local: ifa_address is the remote point in 
ppp */
                           *addr = (pin_device->ifa_list->ifa_local);
                           read_unlock_bh(&dev_base_lock);
                           return 1;
                   }
                pnet_device = pnet_device->next;
         }
         read_unlock_bh(&dev_base_lock);
       return -1;              /* address not found! */
}

What could be wrong?
I think the problem is that read_lock_bh is not correct, perhaps
i would need read_lock_irq instead??

Can anyone suggest if i am right.

Thanks a lot

Giacomo.
-- 
Giacomo S.

www.giacomos.it

em@il:
delleceste@gmail.com
jacum@libero.it
giacomo.strangolino@elettra.trieste.it
 	

Running IPFIRE-wall on debian GNU/Linux:
www.giacomos.it/ipfire

www.debian.org

Re: information about kernel locking issue

[Massimiliano Hofer]

On Saturday 1 July 2006 1:55 pm, Giacomo S. wrote:

> I think the problem is that read_lock_bh is not correct, perhaps
> i would need read_lock_irq instead??

Your code snippet doesn't show how the function is called. If this is invoked 
by (*match)() you are in a soft interrupt context and should use read_lock().
read_lock_bh() is what you use in process context when a soft interrupt may 
use write_lock().
I don't think this needs read_lock_irq() and I can't find any IRQ using it, 
but someone with more experience may disagree with me.

-- 
Bye,
   Massimiliano Hofer

Re: information about kernel locking issue

[Giacomo]

Sorry, i did not specify an important detail:

this function is invoked by other functions, the first of those is hooked
to all the netfilter hooks.

So, depending on the 'direction' of the flow (input, output, fwd), we are
in user context (for output) or soft interrupt (in, fwd).

2006/7/1, Massimiliano Hofer <max@nucleus.it>:
> On Saturday 1 July 2006 1:55 pm, Giacomo S. wrote:
>
> > I think the problem is that read_lock_bh is not correct, perhaps
> > i would need read_lock_irq instead??
>
> Your code snippet doesn't show how the function is called. If this is invoked
> by (*match)() you are in a soft interrupt context and should use read_lock().
> read_lock_bh() is what you use in process context when a soft interrupt may
> use write_lock().
> I don't think this needs read_lock_irq() and I can't find any IRQ using it,
> but someone with more experience may disagree with me.
>
> --
> Bye,
>    Massimiliano Hofer
>


-- 
Giacomo S.
http://www.giacomos.it

- - - - - - - - - - - - - - - - - - - - - -

Proteggi il tuo PC provando il mio semplice FIREWALL:
http://www.giacomos.it/ipfire

mailto:
delleceste@gmail.com
giacomo.strangolino@elettra.trieste.it
jacum@libero.it

- - - - - - - - - - - - - - - - - - - - - -

 . ''  `.
:   :'    :
 `.  ` '
    `- Debian GNU/Linux -- The power of freedom
        http://www.debian.org

Re: information about kernel locking issue

[Gregor Maier]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Massimiliano Hofer wrote:
> On Saturday 1 July 2006 1:55 pm, Giacomo S. wrote:
> 
> 
>>I think the problem is that read_lock_bh is not correct, perhaps
>>i would need read_lock_irq instead??
> 
> 
> Your code snippet doesn't show how the function is called. If this is invoked 
> by (*match)() you are in a soft interrupt context and should use read_lock().
> read_lock_bh() is what you use in process context when a soft interrupt may 
> use write_lock().

That's wrong. When you run in interrupt context, you must ensure that
you disable interrupts (be it soft or hardware interrupts) while holding
a spinlock.
The _irq und _irqsave locking functions will disable hardware interrupts
on the local cpu while holding the lock (and thus softirqs are also
disabled).  The _bh functions will disable softirqs on the local cpu
while you hold the lock. This means, that the code executed with the
lock held cannot be interrupted by a hardware irq resp. softirq.

i.e.
- - if you run in hardware interrupt context you must use a *_irq
or *_irqsave lock (which guarentees that no other hardware interrupt is
serviced until you release the lock)
- - if you run in soft interrupt context you must use a *_bh locking
function (the _irq _irqsave are theoretically also ok, but you want to
avoid disabling hardware irqs when it's not necesarry)
- - in other cases (process context, you can use any lock function).

If you don't disable interrupts while holding a lock in interrupt
context, your piece of code may get interrupted while still holding the
lock. Which may (and will) result in deadlock if the new interrupt
routine also tries to aquire the same lock.

> I don't think this needs read_lock_irq() and I can't find any IRQ using it, 
> but someone with more experience may disagree with me.

so, since the code runs ins softirq context, lock_bh is fine. I suppose
the problem is, that one of the functions in the critical area may
sleep. When using spinlocks, you must ensure, that you do not call a
function that can sleep. So you may want to check if.

Have a look at the Writing Linux Device Drivers book from O'Reilly.
Chapter 5 covers locking. A online version can be found at
http://lwn.net/Kernel/LDD3/


hth
gregor


- --
Gregor Maier                                      Lehrstuhl Informatik 8
gregor@net.in.tum.de                              Tel: +49 89  289-18010
http://www.net.in.tum.de                                     TU Muenchen
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (Darwin)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFEstc7dGiwgbikMYMRAr4UAKCd9NvHTPiqxvJZyQPUe0QV37Z7PwCgl7bq
oYuRhn8jh2OSMMRnGmeNM78=
=gvyd
-----END PGP SIGNATURE-----

Re: information about kernel locking issue

[Massimiliano Hofer]

On Tuesday 11 July 2006 12:39 am, Gregor Maier wrote:

> > Your code snippet doesn't show how the function is called. If this is
> > invoked by (*match)() you are in a soft interrupt context and should use
> > read_lock(). read_lock_bh() is what you use in process context when a
> > soft interrupt may use write_lock().
>
> That's wrong. When you run in interrupt context, you must ensure that
> you disable interrupts (be it soft or hardware interrupts) while holding
> a spinlock.

You're right and I made a poor choice of words, but in his case the lock 
protects dev_base and, to the best of my knowledge, it is never written by 
interrupts (soft or hard). Correct me if I'm mistaken on this point.

In this case, if you take a read lock and no other interrupt ever takes 
anything more than a read lock, while user context always disables interrupts 
while taking a write lock, you shouldn't be able to trigger a deadlock.

-- 
Bye,
   Massimiliano Hofer