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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20071215131016.GA28705@gondor.apana.org.au>
Date:	Sat, 15 Dec 2007 21:10:16 +0800
From:	Herbert Xu <herbert@...dor.apana.org.au>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
	"Eric W. Biederman" <ebiederm@...ssion.com>
Subject: Re: [patch 2/4] net: use mutex_is_locked() for ASSERT_RTNL()

On Sat, Dec 15, 2007 at 02:48:10AM -0800, Andrew Morton wrote:
>
> When Eric said
> 
> > Way way deep in mutex debugging on the slowpath there is a unreadable
> > and incomprehensible WARN_ON in muxtex_trylock that will trigger if
> > you have 10 tons of debugging turned on, and you are in,
> > interrupt context, and you manage to hit the slow path.  I think that
> > is a pretty unlikely scenario.
> 
> I think he's still right.  That's if the warning which he managed to find
> even still exists.

Well at the very start he said:

: When mutex debugging is on taking a mutex complains if we are not
: allowed to sleep.  At that point we have called netif_tx_lock_bh
: so we are clearly not allowed to sleep.  Arguably this is not a
: problem for mutex_trylock.

This is what threw me off as it implied that we were warning about
ASSERT_RTNL calls in any atomic context, including those with just
soft IRQs disabled, which is an important distinction for the
networking subsystem.

Had he said that it was just IRQ handlers rather than soft IRQs
then we wouldn't be having this discussion now :)

> I think the change which Eric proposed is a good one: it converts
> ASSERT_RTNL() from an atomic rmw which dirties a cacheline which will
> sometimes be owned by a different CPU into a plain old read.  It's going to
> make ASSERT_RTNL() heaps cheaper.

I agree.  Although paths using ASSERT_RTNL shouldn't be performance
critical since the RTNL should only be held for control operations
as opposed to data transport.

> Now as a separate issue we (ie: you) need to work out what _other_ things
> you want ASSERT_RTNL to check apart from "rtnl must be held".

Since we have now established that ASSERT_RTNL never actually
warned about usage on paths with BH off, I think Eric's original
patch is fine as it is and I owe him an apology.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@...dor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
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