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: <4709B4D6.3040805@trash.net>
Date:	Mon, 08 Oct 2007 06:40:54 +0200
From:	Patrick McHardy <kaber@...sh.net>
To:	Herbert Xu <herbert@...dor.apana.org.au>
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

Herbert Xu wrote:
> 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?


Yes, and to make sure the RTNL is held.

> 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).


The change_rx_flags function was intended to be used by software
devices that want to synchronize flags to a different device,
in which case they shouldn't interfere since set_multicast_list
would only be used for the multicast list and not the 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 sounds like a really good idea to get rid of all the
confusion.

> 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?


Sounds great :)
-
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