netdev - Re: [patch 10/15] ppp_generic: fix lockdep warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 26 Apr 2007 13:28:12 +0200
From:	Jarek Poplawski <jarkao2@...pl>
To:	Paul Mackerras <paulus@...ba.org>
Cc:	akpm@...ux-foundation.org, davem@...emloft.net,
	netdev@...r.kernel.org, jura@...ams.com
Subject: Re: [patch 10/15] ppp_generic: fix lockdep warning

On Thu, Apr 26, 2007 at 08:04:36PM +1000, Paul Mackerras wrote:
> akpm@...ux-foundation.org writes:
> 
> > lockdep has seen locks "-> #0" - "-> #3" taken in circular order, but IMHO,
> > lock "-> #3" (&pch->downl) taken after "-> #2" (&ppp->wlock) differs from
> > &pch->downl lock taken in "-> #0" (before &vlan_netdev_xmit_lock_key) and
> > lockdep should be notified about this.
> > 
> > Reported & tested by: "Yuriy N. Shkandybin" <jura@...ams.com>
> > Signed-off-by: Jarek Poplawski <jarkao2@...pl>
> > Cc: Paul Mackerras <paulus@...ba.org>
> > Signed-off-by: Andrew Morton <akpm@...ux-foundation.org>
> > ---
> > 
> >  drivers/net/ppp_generic.c |    6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff -puN drivers/net/ppp_generic.c~ppp_generic-fix-lockdep-warning drivers/net/ppp_generic.c
> > --- a/drivers/net/ppp_generic.c~ppp_generic-fix-lockdep-warning
> > +++ a/drivers/net/ppp_generic.c
> > @@ -1433,7 +1433,8 @@ ppp_channel_push(struct channel *pch)
> >  	struct sk_buff *skb;
> >  	struct ppp *ppp;
> >  
> > -	spin_lock_bh(&pch->downl);
> > +	local_bh_disable();
> > +	spin_lock_nested(&pch->downl, SINGLE_DEPTH_NESTING);
> 
> This looks like a band-aid to me.  I don't feel that I understand
> exactly how the recursive locking situation arose, or why saying
> "SINGLE_DEPTH_NESTING" (whatever that means exactly) is a suitable
> fix.

I think I've cc-d this patch proposal to you, and there was
something about: don't apply without  maintainer's opinion and
Yuriy's testing. I think Andrew was not afraid to risk -mm
stability, and took this earlier.

In this case lockdep sees locks taken in different order,
and one of this locks is pch->downl, taken in two different
functions. Lockdep thinks it's the same lock, but these
functions serve two different type of channels, which
cannot wait for/take this lock at the same time.
SINGLE_DEPTH_NESTING means here this is another lock, so
lockdep doesn't see nothing wrong with this:

f1()
{
spin_lock_nested(&pch->downl, SINGLE_DEPTH_NESTING);
spin_lock(&B);
...
}

f2()
{
spin_lock(&B);
spin_lock(&pch->downl);
...
}

which would generate an error without nesting.
So, if you think it's wrong, the patch should be dumped,
and other measures be taken, to stop bother users with this.

Regards,
Jarek P.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html