netdev - Re: [PATCH net v4 2/3] net: wangxun: match VLAN CTAG and STAG features

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: <20240516194544.67d9249c@kernel.org>
Date: Thu, 16 May 2024 19:45:44 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Jiawen Wu <jiawenwu@...stnetic.com>
Cc: davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
 rmk+kernel@...linux.org.uk, andrew@...n.ch, netdev@...r.kernel.org,
 mengyuanlou@...-swift.com, duanqiangwen@...-swift.com, Sai Krishna
 <saikrishnag@...vell.com>, Simon Horman <horms@...nel.org>
Subject: Re: [PATCH net v4 2/3] net: wangxun: match VLAN CTAG and STAG
 features

On Tue, 14 May 2024 15:23:29 +0800 Jiawen Wu wrote:
> +#define NETIF_VLAN_STRIPPING_FEATURES	(NETIF_F_HW_VLAN_CTAG_RX | \
> +					 NETIF_F_HW_VLAN_STAG_RX)


> +	if (changed & NETIF_VLAN_STRIPPING_FEATURES) {
> +		if (features & NETIF_F_HW_VLAN_CTAG_RX &&
> +		    features & NETIF_F_HW_VLAN_STAG_RX) {
> +			features |= NETIF_VLAN_STRIPPING_FEATURES;

this is a noop, right? It's like checking:

	if (value & 1)
		value |= 1; 

features already have both bits set ORing them in doesn't change much.
Or am I misreading?

All I would have expected was:

	if (features & NETIF_VLAN_STRIPPING_FEATURES != NETIF_VLAN_STRIPPING_FEATURES) {
		/* your "else" clause which resets both */
	}

> +		} else if (!(features & NETIF_F_HW_VLAN_CTAG_RX) &&
> +			 !(features & NETIF_F_HW_VLAN_STAG_RX)) {
> +			features &= ~NETIF_VLAN_STRIPPING_FEATURES;
> +		} else {
> +			features &= ~NETIF_VLAN_STRIPPING_FEATURES;
> +			features |= netdev->features & NETIF_VLAN_STRIPPING_FEATURES;
> +			wx_err(wx, "802.1Q and 802.1ad VLAN stripping must be either both on or both off.");
> +		}
> +	}