Re: how to mix bridges and bonding inc. vlans correctly on Kernel > 3.10

[Stefan Priebe <s.priebe@profihost.ag>]

Hi,

sorry for the long delay.

Am 14.11.2013 15:41, schrieb Vlad Yasevich:
> On 11/14/2013 09:29 AM, Stefan Priebe - Profihost AG wrote:
>> 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@trash.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@comx.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@stdin.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
>>
>
> How are you attaching tap to the vlan device?  Or is it attached to
> vmbr1?

No, to vmbr1.3000.

>> 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.
>
> Ok, in this case, you have a VLAN (bond1.3000) configured on top of
> bond interface.  That vlan is then attached to the bridge (vmbr1v3000).
> The tap interface for your VM (tap114i1) is also attached to the same
> bridge.  In this case, VM will receive only traffic for VLAN 3000 and
> it we receive it untagged.

Yes and this is also what i want to archieve.

> I guess I'd like to understand what you are trying to achieve.
>
> Thanks working with us on this.
> -vlad
>
>>
>> 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
>>>
>