netdev - Re: [PATCH net-next v2 08/12] net: dsa: vsc73xx: Implement the tag

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: Thu, 20 Jun 2024 17:09:43 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Pawel Dembicki <paweldembicki@...il.com>
Cc: netdev@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>,
	Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>,
	Claudiu Manoil <claudiu.manoil@....com>,
	Alexandre Belloni <alexandre.belloni@...tlin.com>,
	UNGLinuxDriver@...rochip.com, Russell King <linux@...linux.org.uk>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 08/12] net: dsa: vsc73xx: Implement the
 tag_8021q VLAN operations

On Wed, Jun 19, 2024 at 10:52:14PM +0200, Pawel Dembicki wrote:
> +static int vsc73xx_tag_8021q_vlan_add(struct dsa_switch *ds, int port, u16 vid,
> +				      u16 flags)
> +{
> +	bool untagged = flags & BRIDGE_VLAN_INFO_UNTAGGED;
> +	bool pvid = flags & BRIDGE_VLAN_INFO_PVID;
> +	struct vsc73xx_portinfo *portinfo;
> +	struct vsc73xx *vsc = ds->priv;
> +	bool commit_to_hardware;
> +	int ret;
> +
> +	portinfo = &vsc->portinfo[port];
> +
> +	if (untagged) {
> +		portinfo->untagged_tag_8021q_configured = true;
> +		portinfo->untagged_tag_8021q = vid;
> +	}

This is something I don't really like, but I understand why you're
doing it. dsa_tag_8021q_bridge_join() first adds the bridge_vid and
only then deletes the standalone_vid. Both are egress-untagged.

The fact that you have storage in this driver for a single egress-tagged
tag_8021q VLAN makes it awkward if not impossible to track the deletion
properly from tag_8021q_vlan_del(). You just record the last added VID
in tag_8021q_vlan_add() and silently discard any previous one during the
VSC73XX_TXUPDCFG_TX_UNTAGGED_VID setting, earlier than the deletion API.

I'm wondering if this isn't actually a self-inflicted problem created by
the data structures in use. I see that when the port is in VSC73XX_VLAN_IGNORE
(which it is for tag_8021q), VSC73XX_TXUPDCFG_TX_INSERT_TAG will be
unset. So the port is egress-untagged for all VIDs, and there's little
point in tracking any one single egress untagged VID coming from
tag_8021q. No?

Wouldn't it be better, assuming that is the case, to just return an
error if this is a user port and tag_8021q is requesting a VID which
is _not_ egress-untagged? If you also respond positively to my comment
about returning early in vsc73xx_vlan_commit_untagged() without doing
anything in the tag_8021q case, then you can actually get rid of
untagged_tag_8021q_configured and untagged_tag_8021q from portinfo.

> +	if (pvid) {
> +		portinfo->pvid_tag_8021q_configured = true;
> +		portinfo->pvid_tag_8021q = vid;
> +	}
> +
> +	commit_to_hardware = vsc73xx_tag_8021q_active(dsa_to_port(ds, port));
> +	if (commit_to_hardware) {
> +		ret = vsc73xx_vlan_commit_untagged(vsc, port);
> +		if (ret)
> +			return ret;
> +
> +		ret = vsc73xx_vlan_commit_pvid(vsc, port);
> +		if (ret)
> +			return ret;

Hmm, here I notice that the vsc73xx_vlan_commit_conf() call is missing.
Am I right that if you add it here, then the entire vsc73xx_vlan_commit_*()
string of 3 calls from vsc73xx_port_enable() becomes unnecessary?

The tag_8021q VLAN add operations are initiated by you when you call
dsa_tag_8021q_register(). So you have good control over when the VLAN
configuration of the port for standalone mode is done.

> +	}
> +
> +	return vsc73xx_update_vlan_table(vsc, port, vid, true);
> +}
> +
> +static int vsc73xx_tag_8021q_vlan_del(struct dsa_switch *ds, int port, u16 vid)
> +{
> +	struct vsc73xx *vsc = ds->priv;
> +

As a matter of API compliance, I guess you could add a snippet:

	if (portinfo->pvid_tag_8021q_configured &&
	    portinfo->pvid_tag_8021q == vid) {
		portinfo->pvid_tag_8021q_configured = false;

		if (commit_to_hardware) {
			err = vsc73xx_vlan_commit_pvid(vsc, port);
			if (err)
				return err;
		}
	}

Of course, this is not to say that tag_8021q will _currently_ configure
ports for an operating mode without PVID. But it's something that the
API permits.

> +	return vsc73xx_update_vlan_table(vsc, port, vid, false);
> +}