netdev - Re: [PATCH net-next v2 4/7] net: dsa: vsc73xx: Add dsa tagging based on 8021q

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: <ZJg2M+Qvg3Fv73CH@corigine.com>
Date: Sun, 25 Jun 2023 14:42:27 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Pawel Dembicki <paweldembicki@...il.com>
Cc: netdev@...r.kernel.org, Linus Walleij <linus.walleij@...aro.org>,
	Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>,
	Vladimir Oltean <olteanv@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	linux-kernel@...r.kernel.org,
	Dan Carpenter <dan.carpenter@...aro.org>
Subject: Re: [PATCH net-next v2 4/7] net: dsa: vsc73xx: Add dsa tagging based
 on 8021q

+ Dan Carpenter

On Sun, Jun 25, 2023 at 01:53:39PM +0200, Pawel Dembicki wrote:
> This patch is simple implementation of 8021q tagging in vsc73xx driver.
> At this moment devices with DSA_TAG_PROTO_NONE are useless. VSC73XX
> family doesn't provide any tag support for external ethernet ports.
> 
> The only way is vlan-based tagging. It require constant hardware vlan
> filtering. VSC73XX family support provider bridging but QinQ only without
> fully implemented 802.1AD. It allow only doubled 0x8100 TPID.
> 
> In simple port mode QinQ is enabled to preserve forwarding vlan tagged
> frames.
> 
> Tag driver introduce most simple funcionality required for proper taging
> support.
> 
> Reviewed-by: Linus Walleij <linus.walleij@...aro.org>
> Signed-off-by: Pawel Dembicki <paweldembicki@...il.com>

...

> +static void vsc73xx_vlan_rcv(struct sk_buff *skb, int *source_port,
> +			     int *switch_id, int *vbid, u16 *vid)
> +{
> +	if (vid_is_dsa_8021q(skb_vlan_tag_get(skb) & VLAN_VID_MASK))
> +		return dsa_8021q_rcv(skb, source_port, switch_id, vbid);
> +
> +	/* Try our best with imprecise RX */
> +	*vid = skb_vlan_tag_get(skb) & VLAN_VID_MASK;
> +}
> +
> +static struct sk_buff *vsc73xx_rcv(struct sk_buff *skb,
> +				   struct net_device *netdev)
> +{
> +	int src_port = -1, switch_id = -1, vbid = -1;
> +	u16 vid;
> +
> +	if (skb_vlan_tag_present(skb))
> +		/* Normal traffic path. */
> +		vsc73xx_vlan_rcv(skb, &src_port, &switch_id, &vbid, &vid);
> +
> +	if (vbid >= 1)
> +		skb->dev = dsa_tag_8021q_find_port_by_vbid(netdev, vbid);
> +	else if (src_port == -1 || switch_id == -1)
> +		skb->dev = dsa_find_designated_bridge_port_by_vid(netdev, vid);

Hi Pawel,

Smatch warns that vid may be used uninitialised here.
And it's not clear to me why that cannot be the case.

> +	else
> +		skb->dev = dsa_master_find_slave(netdev, switch_id, src_port);
> +	if (!skb->dev) {
> +		netdev_warn(netdev, "Couldn't decode source port\n");
> +		return NULL;
> +	}
> +
> +	dsa_default_offload_fwd_mark(skb);
> +
> +	if (dsa_port_is_vlan_filtering(dsa_slave_to_port(skb->dev)) &&
> +	    eth_hdr(skb)->h_proto == htons(ETH_P_8021Q))
> +		__vlan_hwaccel_clear_tag(skb);
> +
> +	return skb;
> +}

...