netdev - Re: [patch 2/4] net: use mutex_is_locked() for ASSERT

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20071217072601.GA1654@ff.dom.local>
Date:	Mon, 17 Dec 2007 08:26:01 +0100
From:	Jarek Poplawski <jarkao2@...il.com>
To:	Herbert Xu <herbert@...dor.apana.org.au>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [patch 2/4] net: use mutex_is_locked() for ASSERT_RTNL()

On Mon, Dec 17, 2007 at 09:26:32AM +0800, Herbert Xu wrote:
> On Sun, Dec 16, 2007 at 07:06:41PM +0100, Jarek Poplawski wrote:
> >
> > It seemed to exist a few days ago:
> > http://kerneltrap.org/mailarchive/linux-netdev/2007/12/4/473123
> > 
> > Btw., I don't know which of the patches: Eric's or yours will be chosen,
> > but, IMHO, there is no reason to remove rtnl_trylock(), which can be still
> > useful, just like mutex_trylock() is.
> 
> Doh! Andrew was too convincing :) I misread the grep result on
> in_interrupt.  Of course that function returns true if we're
> either in an IRQ handler or have BH off.
> 
> I retract what I've said in this thread and continue to oppose
> this change without a might_sleep.
> 

...And I think some change is needed here. Btw., I proposed to change
this long time ago too. There were no response - only Ben Greear
mentioned about some applications, which could rely on the trylock
way. I didn't understand what he was talking about at all - and it
didn't change until I've read this and Eric's patch thread!

So, I was surprised, probably just like Eric, ASSERT_RTNL is 2 in 1,
with this atomic somewhere deep in mind. IMHO this should be better
commented at least. But it's still dubious to me: using trylock this
way makes impossible to verify (eg. by lockdep) recursion cases,
when lock is taken with trylock in a loop.

So, I think using might_sleep() explicitly would be much more
readable or, otherwise, Patrick's proposal with adding
ASSERT_RTNL_ATOMIC would implicitly signal the real meaning of the
other one.

Btw. #2: David Miller gave this example of ASSERT_RTNL use:

	ASSERT_RTNL();
	page = alloc_page(GFP_KERNEL);

But isn't there a debugging duplication: it seems alloc_page() is used
in so many places and this check for GFP is/should_be there already?

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