netdev - Re: [PATCH] rtnl: Simplify 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: <20071003060603.GA6457@gondor.apana.org.au>
Date:	Wed, 3 Oct 2007 14:06:03 +0800
From:	Herbert Xu <herbert@...dor.apana.org.au>
To:	Patrick McHardy <kaber@...sh.net>
Cc:	"Eric W. Biederman" <ebiederm@...ssion.com>, davem@...emloft.net,
	netdev@...r.kernel.org, oliver@...kum.name,
	linux-usb-devel@...ts.sourceforge.net
Subject: Re: [PATCH] rtnl: Simplify ASSERT_RTNL

On Tue, Oct 02, 2007 at 05:29:11PM +0200, Patrick McHardy wrote:
>
> I think this doesn't completely fix it, when dev_unicast_add is
> interrupted by dev_mc_add before the unicast changes are performed,
> they will get committed in the dev_mc_add context, so we might still
> call change_flags with BH disabled. Taking the TX lock around the
> dev->uc_count and dev->uc_promisc checks and changes in __dev_set_rx_mode
> should fix this.

Good catch.  Digging back in history it seems that you added
the change_rx_flags function so that the driver didn't have to
do it under TX lock, right?

The problem with this is that the stack can now call
change_rx_flags and set_multicast_list simultaneously
which presents a potential headache for the driver
author (if they were to use change_rx_flags).

It seems to me what we could do is in fact separate out the
part that adds the address and the part that syncs it with
hardware.

That way we can call the hardware from a process context later
and use the RTNL to guarantee that we only enter the driver
once.

So dev_mc_add would look like:

1) Hold some form of lock L.
2) Modify mc list A (a copy of the current mc list).
3) Drop lock.
4) Schedule an update to the hardware.

The update to the hardware would look lie:

1) Hold RTNL.
2) Hold lock L.
3) Copy list A to list B (B would be our current list).
4) Drop lock L.
5) Call the hardware.
6) Drop RTNL.

For compatibility, set_multicast_list would still be invoked
under the TX lock while set_rx_mode would do exactly the same
thing but would only hold the RTNL.

What do you think about this approach?

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