netdev - Re: how to mix bridges and bonding inc. vlans correctly on Kernel

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: <5284DE65.40105@profihost.ag>
Date:	Thu, 14 Nov 2013 15:29:57 +0100
From:	Stefan Priebe - Profihost AG <s.priebe@...fihost.ag>
To:	Vlad Yasevich <vyasevich@...il.com>,
	Veaceslav Falico <vfalico@...hat.com>,
	Vlad Yasevich <vyasevic@...hat.com>
CC:	Linux Netdev List <netdev@...r.kernel.org>
Subject: Re: how to mix bridges and bonding inc. vlans correctly on Kernel
 > 3.10

Am 14.11.2013 15:27, schrieb Vlad Yasevich:
> On 11/14/2013 06:54 AM, Veaceslav Falico wrote:
>> On Wed, Nov 13, 2013 at 10:09:30PM -0500, Vlad Yasevich wrote:
>>> Can you try out attached patch please.  I ran it in my environment and
>>> always get promisc link when attaching devices to the bridge.
>>
>> It's interesting, actually, why do we need to check for IFF_UP at all
>> when
>> changing flags.
>>
>> The addition of IFF_UP goes up to:
>>
>> commit b6c40d68ff6498b7f63ddf97cf0aa818d748dee7
>> Author: Patrick McHardy <kaber@...sh.net>
>> Date:   Tue Oct 7 15:26:48 2008 -0700
>>
>>      net: only invoke dev->change_rx_flags when device is UP
>>
>>      Jesper Dangaard Brouer <hawk@...x.dk> reported a bug when setting a
>> VLAN
>>      device down that is in promiscous mode:
>>
>>      When the VLAN device is set down, the promiscous count on the real
>>      device is decremented by one by vlan_dev_stop(). When removing the
>>      promiscous flag from the VLAN device afterwards, the promiscous
>>      count on the real device is decremented a second time by the
>>      vlan_change_rx_flags() callback.
>>
>> ... snip ...
>>
> 
> This was applied in 2.6.27 timeframe.
> 
>> However, I'm not sure that this is still needed, cause:
>>
>> commit deede2fabe24e00bd7e246eb81cd5767dc6fcfc7
>> Author: Matthijs Kooijman <matthijs@...in.nl>
>> Date:   Mon Oct 31 04:53:13 2011 +0000
>>
>>      vlan: Don't propagate flag changes on down interfaces.
>>
>>      When (de)configuring a vlan interface, the IFF_ALLMULTI ans
>> IFF_PROMISC
>>      flags are cleared or set on the underlying interface. So, if these
>> flags
>>      are changed on a vlan interface that is not up, the flags underlying
>>      interface might be set or cleared twice.
>>
>>      Only propagating flag changes when a device is up makes sure this
>> does
>>      not happen. It also makes sure that an underlying device is not
>> set to
>>      promiscuous or allmulti mode for a vlan device that is down.
>>
>> ... snip ...
>>
> 
> And this in 3.2.  So how did this work for Stefan in 3.9?  Unless there
> is something in Ubuntu that changed how the stacked interfaces are
> configured and is keeping interfaces down longer then it used to.

Stop here ;-) in 3.9 it was the other way round.

This one worked:
> eth2
>      \
>       -- bond1 -- vmbr1
>      /              \
> eth3                 \ vmbr1.3000
>                            \ ---- tap114i1

this one did not:
> eth2
>      \
>       -- bond1 -- vmbr1
>      /     \
> eth3        ----- bond1.3000 --- vmbr1v3000
>                                      \ ---- tap114i1

Now with the patch - the 2nd one works the first one does not.

Stefan

>> which fixed completely the initial issue with vlans.
>>
>> Maybe we should just remove the IFF_UP check in dev_change_rx_flags(),
>> and
>> let the drivers using it handle its own logic, as vlan does?
>>
> 
> This should work, but this might regress a few drivers.  Looking through
> the callers of dev_set_promiscuity at least DSA driver suffers
> the same fate as VLAN.
> 
> -vlad
> 
>> Something like this:
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 8ffc52e..9615cd7 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -4995,7 +4995,7 @@ static void dev_change_rx_flags(struct net_device
>> *dev, int flags)
>>   {
>>       const struct net_device_ops *ops = dev->netdev_ops;
>>
>> -    if ((dev->flags & IFF_UP) && ops->ndo_change_rx_flags)
>> +    if (ops->ndo_change_rx_flags)
>>           ops->ndo_change_rx_flags(dev, flags);
>>   }
>>
>>
>>>
>>> Thanks
>>> -vlad
> 
--
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