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

[Nikolay Aleksandrov <nikolay@cumulusnetworks.com>]

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@gmail.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@gmail.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
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
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