netdev - Re: What to do when a bridge port gets its pvid deleted?

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] [day] [month] [year] [list]

Date:   Tue, 20 Aug 2019 02:12:26 +0300
From:   Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:     Vladimir Oltean <olteanv@...il.com>,
        Ido Schimmel <idosch@...sch.org>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Roopa Prabhu <roopa@...ulusnetworks.com>,
        netdev <netdev@...r.kernel.org>, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        stephen@...workplumber.org
Subject: Re: What to do when a bridge port gets its pvid deleted?

On 8/20/19 2:01 AM, Nikolay Aleksandrov wrote:
> On 8/20/19 12:10 AM, Vladimir Oltean wrote:
> [snip]
>> It's good to know that it's there (like you said, it explains some
>> things) but I can't exactly say that removing it helps in any way.
>> In fact, removing it only overwrites the dsa_8021q VLANs with 1 during
>> bridge_join, while not actually doing anything upon a vlan_filtering
>> toggle.
>> So the sja1105 driver is in a way shielded by DSA from the bridge, for
>> the better.
>> It still appears to be true that the bridge doesn't think it's
>> necessary to notify through SWITCHDEV_OBJ_ID_PORT_VLAN again. So my
>> best bet is to restore the pvid on my own.
>> However I've already stumbled upon an apparent bug while trying to do
>> that. Does this look off? If it doesn't, I'll submit it as a patch:
>>
>> commit 788f03991aa576fc0b4b26ca330af0f412c55582
>> Author: Vladimir Oltean <olteanv@...il.com>
>> Date:   Mon Aug 19 22:57:00 2019 +0300
>>
>>     net: bridge: Keep the BRIDGE_VLAN_INFO_PVID flag in net_bridge_vlan
>>
>>     Currently this simplified code snippet fails:
>>
>>             br_vlan_get_pvid(netdev, &pvid);
>>             br_vlan_get_info(netdev, pvid, &vinfo);
>>             ASSERT(!(vinfo.flags & BRIDGE_VLAN_INFO_PVID));
>>
>>     It is intuitive that the pvid of a netdevice should have the
>>     BRIDGE_VLAN_INFO_PVID flag set.
>>
>>     However I can't seem to pinpoint a commit where this behavior was
>>     introduced. It seems like it's been like that since forever.
>>
>>     Signed-off-by: Vladimir Oltean <olteanv@...il.com>
>>
>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
>> index 021cc9f66804..f49b2758bcab 100644
>> --- a/net/bridge/br_vlan.c
>> +++ b/net/bridge/br_vlan.c
>> @@ -68,10 +68,13 @@ static bool __vlan_add_flags(struct
>> net_bridge_vlan *v, u16 flags)
>>      else
>>          vg = nbp_vlan_group(v->port);
>>
>> -    if (flags & BRIDGE_VLAN_INFO_PVID)
>> +    if (flags & BRIDGE_VLAN_INFO_PVID) {
>>          ret = __vlan_add_pvid(vg, v->vid);
>> -    else
>> +        v->flags |= BRIDGE_VLAN_INFO_PVID;
>> +    } else {
>>          ret = __vlan_delete_pvid(vg, v->vid);
>> +        v->flags &= ~BRIDGE_VLAN_INFO_PVID;
>> +    }
>>
>>      if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
>>          v->flags |= BRIDGE_VLAN_INFO_UNTAGGED;
>>
> 
> Hi Vladimir,
> I know it looks logical to have it, but there are a few reasons why we don't
> do it, most importantly because we need to have only one visible pvid at any single
> time, even if it's stale - it must be just one. Right now that rule will not
> be violated  by your change, but people will try using this flag and could see

Obviously, I'm talking about RCU pvid/vlan use cases similar to the dumps.
The locked cases are fine.

> two pvids simultaneously. You can see that the pvid code is even using memory
> barriers to propagate the new value faster and everywhere the pvid is read only once.
> That is the reason the flag is set dynamically when dumping entries, too.
> A second (weaker) argument against would be given the above we don't want
> another way to do the same thing, specifically if it can provide us with

i.e. I would like to avoid explaining why this shouldn't be relied upon without locking

> two pvids (e.g. if walking the vlan list) or if it can provide us with a pvid
> different from the one set in the vg.
> If you do need that flag, you can change br_vlan_get_info() to set the flag like
> the other places do - if the vid matches the current vg pvid, or you could change
> the whole pvid handling code to this new scheme as long as it can guarantee a single
> pvid when walking the vlan list and fast pvid retrieval in the fast-path.
> 
> Cheers,
>  Nik
>  
>