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: <5284DDCE.4090901@gmail.com>
Date:	Thu, 14 Nov 2013 09:27:26 -0500
From:	Vlad Yasevich <vyasevich@...il.com>
To:	Veaceslav Falico <vfalico@...hat.com>,
	Vlad Yasevich <vyasevic@...hat.com>
CC:	Stefan Priebe <s.priebe@...fihost.ag>,
	Linux Netdev List <netdev@...r.kernel.org>
Subject: Re: how to mix bridges and bonding inc. vlans correctly on Kernel
 > 3.10

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.

> 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