netdev - Re: [PATCH net-next 2/6] net: bridge: Populate the pvid flag in br_vlan_get

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]

Date:   Tue, 20 Aug 2019 03:12:04 +0300
From:   Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:     Vladimir Oltean <olteanv@...il.com>, f.fainelli@...il.com,
        vivien.didelot@...il.com, andrew@...n.ch, idosch@...sch.org,
        roopa@...ulusnetworks.com, davem@...emloft.net
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net-next 2/6] net: bridge: Populate the pvid flag in
 br_vlan_get_info

On 8/20/19 2:59 AM, Vladimir Oltean wrote:
> 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.
> 
> At a first glance it would make more sense to just handle the
> BRIDGE_VLAN_INFO_PVID flag in __vlan_add_flags. However, as Nikolay
> explains:
> 
>   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 this 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. [Obviously, I'm talking about RCU
>   pvid/vlan use cases similar to the dumps.  The locked cases are fine.
>   I would like to avoid explaining why this shouldn't be relied upon
>   without locking]
> 
> So instead of introducing the above change and making sure of the pvid
> uniqueness under RCU, simply dynamically populate the pvid flag in
> br_vlan_get_info().
> 
> Signed-off-by: Vladimir Oltean <olteanv@...il.com>
> ---
>  net/bridge/br_vlan.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index f5b2aeebbfe9..bb98984cd27d 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -1281,6 +1281,8 @@ int br_vlan_get_info(const struct net_device *dev, u16 vid,
>  
>  	p_vinfo->vid = vid;
>  	p_vinfo->flags = v->flags;
> +	if (vid == br_get_pvid(vg))
> +		p_vinfo->flags |= BRIDGE_VLAN_INFO_PVID;
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(br_vlan_get_info);
> 

Looks good, thanks!
Acked-by: Nikolay Aleksandrov <nikolay@...ulusnetworks.com>