netdev - Re: [PATCH v3 net-next 03/11] net: bridge: vlan: make __vlan_add

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: <d396a685-11c6-4927-26fa-e8a9c8a51ffe@nvidia.com>
Date:   Wed, 16 Feb 2022 13:07:51 +0200
From:   Nikolay Aleksandrov <nikolay@...dia.com>
To:     Vladimir Oltean <vladimir.oltean@....com>, <netdev@...r.kernel.org>
CC:     Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        "Vivien Didelot" <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        Roopa Prabhu <roopa@...dia.com>, Jiri Pirko <jiri@...dia.com>,
        Ido Schimmel <idosch@...dia.com>,
        Rafael Richter <rafael.richter@....de>,
        Daniel Klauer <daniel.klauer@....de>,
        Tobias Waldekranz <tobias@...dekranz.com>
Subject: Re: [PATCH v3 net-next 03/11] net: bridge: vlan: make
 __vlan_add_flags react only to PVID and UNTAGGED

On 16/02/2022 13:03, Nikolay Aleksandrov wrote:
> On 15/02/2022 19:02, Vladimir Oltean wrote:
>> Currently there is a very subtle aspect to the behavior of
>> __vlan_add_flags(): it changes the struct net_bridge_vlan flags and
>> pvid, yet it returns true ("changed") even if none of those changed,
>> just a transition of br_vlan_is_brentry(v) took place from false to
>> true.
>>
>> This can be seen in br_vlan_add_existing(), however we do not actually
>> rely on this subtle behavior, since the "if" condition that checks that
>> the vlan wasn't a brentry before had a useless (until now) assignment:
>>
>> 	*changed = true;
>>
>> Make things more obvious by actually making __vlan_add_flags() do what's
>> written on the box, and be more specific about what is actually written
>> on the box. This is needed because further transformations will be done
>> to __vlan_add_flags().
>>
>> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
>> ---
>> v2->v3: patch is new
>>
>>  net/bridge/br_vlan.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
>> index 498cc297b492..89e2cfed7bdb 100644
>> --- a/net/bridge/br_vlan.c
>> +++ b/net/bridge/br_vlan.c
>> @@ -58,7 +58,9 @@ static bool __vlan_delete_pvid(struct net_bridge_vlan_group *vg, u16 vid)
>>  	return true;
>>  }
>>  
>> -/* return true if anything changed, false otherwise */
>> +/* Returns true if the BRIDGE_VLAN_INFO_PVID and BRIDGE_VLAN_INFO_UNTAGGED bits
>> + * of @flags produced any change onto @v, false otherwise
>> + */
>>  static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
>>  {
>>  	struct net_bridge_vlan_group *vg;
>> @@ -80,7 +82,7 @@ static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
>>  	else
>>  		v->flags &= ~BRIDGE_VLAN_INFO_UNTAGGED;
>>  
>> -	return ret || !!(old_flags ^ v->flags);
>> +	return ret || !!((old_flags ^ v->flags) & BRIDGE_VLAN_INFO_UNTAGGED);
>>  }
>>  
>>  static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
> 
> This patch is unnecessary and can be dropped given the next one.

OTOH being explicit is not bad, nevermind my comment. I'd just have put
this change after the refactoring, but it's ok either way.

Acked-by: Nikolay Aleksandrov <nikolay@...dia.com>