Re: [PATCH] rtnl: Simplify ASSERT_RTNL

[Patrick McHardy <kaber@trash.net>]

Herbert Xu wrote:
> On Sat, Sep 29, 2007 at 05:32:41PM +0200, Patrick McHardy wrote:
> 
>>For unicast addresses its not strictly necessary since they may
>>only be changed under the RTNL anyway. The reason why it takes
>>the tx_lock is for consistency with multicast address handling,
>>which can't rely on the RTNL since IPv6 changes them from
>>BH context. The idea was that the ->set_rx_mode function should
>>handle both secondary unicast and multicast addresses for
>>simplicity.
> 
> 
> In any case, coming back to the original question, the RTNL
> assertion is simply wrong in this case because if we're being
> called from IPv6 then the RTNL won't even be held.
> 
> So I think we need to
> 
> 1) Move the assert into dev_set_promiscuity.
> 2) Take the TX lock in dev_set_promiscuity.


In the IPv6 case we're only changing the multicast list,
so we're never calling into __dev_set_promiscuity.

I actually even added a comment about this :)

        /* Unicast addresses changes may only happen under the rtnl,
         * therefore calling __dev_set_promiscuity here is safe.
         */

I would prefer to keep the ASSERT_RTNL in __dev_set_promiscuity
since it also covers the __dev_set_rx_mode path. How about
adding an ASSERT_RTNL_ATOMIC without the might_sleep or simply
open coding it?