linux-kernel - Re: [PATCH net-next v6 2/3] net: dsa: tag_yt921x: add support for Motorcomm YT921x tags

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: <20250826143847.wwczaqgafl6y7ped@skbuf>
Date: Tue, 26 Aug 2025 17:38:47 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Yangfl <mmyangfl@...il.com>
Cc: netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Simon Horman <horms@...nel.org>,
	Russell King <linux@...linux.org.uk>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v6 2/3] net: dsa: tag_yt921x: add support for
 Motorcomm YT921x tags

On Tue, Aug 26, 2025 at 09:47:34AM +0800, Yangfl wrote:
> > The tag format sort of becomes fixed ABI as soon as user space is able
> > to run "cat /sys/class/net/eth0/dsa/tagging", see "yt921x", and record
> > it to a pcap file. Unless the EtherType bears some other meaning rather
> > than being a fixed value, then if you change it later to some other
> > value than 0x9988, you'd better also change the protocol name to
> > distinguish it from "yt921x".
> 
> "EtherType" here does not necessarily become EtherType; better to
> think it is a key to enable port control over the switch. It could be
> a dynamic random value as long as everyone gets the same value all
> over the kernel, see the setup process of the switch driver. Ideally
> only the remaining content of the tag should become the ABI (and is
> actually enforced by the switch), but making a dynamic "EtherType" is
> clearly a worse idea so I don't know how to clarify the fact...
> 
> > Also, you can _not_ use yt921x_priv :: tag_eth_p, because doing so would
> > assume that typeof(ds->priv) == struct yt921x_priv. In principle we
> > would like to be able to run the tagging protocols on the dsa_loop
> > driver as well, which can be attached to any network interface. Very
> > few, if any, tagging protocol drivers don't work on dsa_loop.
> > > +
> > > +static struct sk_buff *
> > > +yt921x_tag_rcv(struct sk_buff *skb, struct net_device *netdev)
> > > +{
> > > +     unsigned int port;
> > > +     __be16 *tag;
> > > +     u16 rx;
> > > +
> > > +     if (unlikely(!pskb_may_pull(skb, YT921X_TAG_LEN)))
> > > +             return NULL;
> > > +
> > > +     tag = (__be16 *)skb->data;
> >
> > Use dsa_etype_header_pos_rx() and validate the CPU_TAG_TPID_TPID as well.
> 
> See the above explanation why rx "EtherType" is not considered part of ABI.

So what I don't understand is: it's impossible to separate RX from TX in
current DSA taggers. If TX hardcodes the assumption that the switch was
configured to use EtherType 0x9988, why would we expect RX to use anything else?
What valid configuration would we prevent, if we ensured that RX packets
have the same EtherType?

It should be ok if you documented that the EtherType is self-assigned
and must be hardcoded to 0x9988 by local convention for the moment, but
is otherwise configurable, and the meaning of a different value is
undefined. Even if self-assigned, I suppose you could still add the
value to include/uapi/linux/if_ether.h to avoid conflicts with other
uses.

Even better if you clarify the expectations by writing a libpcap
dissector for the new tagging protocol, which you need to do anyway for
recent tcpdump versions to work on the conduit interface (otherwise it
fails with "unsupported protocol"). See
https://github.com/the-tcpdump-group/libpcap/pull/1463
The libpcap dissector uses the /sys/class/net/eth0/dsa/tagging text to
identify the protocol in use, not the EtherType.