| 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
| ||
|
Message-ID: <CAAXyoMNh-6_NtYGBYYBhbiH0UPWCOoiZNhMkgeGqPzKP3HA-_g@mail.gmail.com>
Date: Tue, 26 Aug 2025 09:47:34 +0800
From: Yangfl <mmyangfl@...il.com>
To: Vladimir Oltean <olteanv@...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 6:15 AM Vladimir Oltean <olteanv@...il.com> wrote:
>
> On Sun, Aug 24, 2025 at 08:51:10AM +0800, David Yang wrote:
> > diff --git a/net/dsa/Makefile b/net/dsa/Makefile
> > index 555c07cfeb71..4b011a1d5c87 100644
> > --- a/net/dsa/Makefile
> > +++ b/net/dsa/Makefile
> > @@ -39,6 +39,7 @@ obj-$(CONFIG_NET_DSA_TAG_SJA1105) += tag_sja1105.o
> > obj-$(CONFIG_NET_DSA_TAG_TRAILER) += tag_trailer.o
> > obj-$(CONFIG_NET_DSA_TAG_VSC73XX_8021Q) += tag_vsc73xx_8021q.o
> > obj-$(CONFIG_NET_DSA_TAG_XRS700X) += tag_xrs700x.o
> > +obj-$(CONFIG_NET_DSA_TAG_YT921X) += tag_yt921x.o
> >
> > # for tracing framework to find trace.h
> > CFLAGS_trace.o := -I$(src)
> > diff --git a/net/dsa/tag_yt921x.c b/net/dsa/tag_yt921x.c
> > new file mode 100644
> > index 000000000000..ab7f97367e76
> > --- /dev/null
> > +++ b/net/dsa/tag_yt921x.c
> > @@ -0,0 +1,126 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Motorcomm YT921x Switch Extended CPU Port Tagging
> > + *
> > + * Copyright (c) 2025 David Yang <mmyangfl@...il.com>
> > + *
> > + * +----+----+-------+-----+----+---------
> > + * | DA | SA | TagET | Tag | ET | Payload ...
> > + * +----+----+-------+-----+----+---------
> > + * 6 6 2 6 2 N
> > + *
> > + * Tag Ethertype: CPU_TAG_TPID_TPID (default: 0x9988)
> > + * Tag:
> > + * 2: Service VLAN Tag
> > + * 2: Rx Port
> > + * 15b: Rx Port Valid
> > + * 14b-11b: Rx Port
> > + * 10b-0b: Unknown value 0x80
> > + * 2: Tx Port(s)
> > + * 15b: Tx Port(s) Valid
> > + * 10b-0b: Tx Port(s) Mask
> > + */
> > +
> > +#include <linux/etherdevice.h>
> > +#include <linux/list.h>
> > +#include <linux/slab.h>
>
> Why include list.h and slab.h?
>
> > +
> > +#include "tag.h"
> > +
> > +#define YT921X_NAME "yt921x"
> > +
> > +#define YT921X_TAG_LEN 8
> > +
> > +#define ETH_P_YT921X 0x9988
>
> You can add a header in include/linux/dsa/ which is shared with the
> switch driver, to avoid duplicate definitions.
>
> > +
> > +#define YT921X_TAG_PORT_EN BIT(15)
> > +#define YT921X_TAG_RX_PORT_M GENMASK(14, 11)
> > +#define YT921X_TAG_RX_CMD_M GENMASK(10, 0)
> > +#define YT921X_TAG_RX_CMD(x) FIELD_PREP(YT921X_TAG_RX_CMD_M, (x))
> > +#define YT921X_TAG_RX_CMD_UNK_NORMAL 0x80
> > +#define YT921X_TAG_TX_PORTS_M GENMASK(10, 0)
> > +#define YT921X_TAG_TX_PORTn(port) BIT(port)
> > +
> > +static struct sk_buff *
> > +yt921x_tag_xmit(struct sk_buff *skb, struct net_device *netdev)
> > +{
> > + struct dsa_port *dp = dsa_user_to_port(netdev);
> > + unsigned int port = dp->index;
> > + __be16 *tag;
> > + u16 tx;
> > +
> > + skb_push(skb, YT921X_TAG_LEN);
> > + dsa_alloc_etype_header(skb, YT921X_TAG_LEN);
> > +
> > + tag = dsa_etype_header_pos_tx(skb);
> > +
> > + /* We might use yt921x_priv::tag_eth_p, but
> > + * 1. CPU_TAG_TPID could be configured anyway;
> > + * 2. Are you using the right chip?
>
> 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.
>
> > + */
> > + tag[0] = htons(ETH_P_YT921X);
> > + /* Service VLAN tag not used */
> > + tag[1] = 0;
> > + tag[2] = 0;
> > + tx = YT921X_TAG_PORT_EN | YT921X_TAG_TX_PORTn(port);
> > + tag[3] = htons(tx);
> > +
> > + /* Now tell the conduit network device about the desired output queue
> > + * as well
> > + */
> > + skb_set_queue_mapping(skb, port);
>
> This is generally used for integrated DSA switches, for lossless
> backpressure during CPU transmission, where the conduit interface driver
> is known, and has set up its queues in a special way, as a result of the
> fact that it is attached to a known DSA switch (made by the same vendor).
> What do you need it for, in a discrete MDIO-controlled switch?
>
> > +
> > + return skb;
> > +}
> > +
> > +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.
> > +
> > + /* Locate which port this is coming from */
> > + rx = ntohs(tag[1]);
> > + if (unlikely((rx & YT921X_TAG_PORT_EN) == 0)) {
> > + dev_warn_ratelimited(&netdev->dev,
> > + "Unexpected rx tag 0x%04x\n", rx);
> > + return NULL;
> > + }
> > +
> > + port = FIELD_GET(YT921X_TAG_RX_PORT_M, rx);
> > + skb->dev = dsa_conduit_find_user(netdev, 0, port);
> > + if (unlikely(!skb->dev)) {
> > + dev_warn_ratelimited(&netdev->dev,
> > + "Couldn't decode source port %u\n", port);
> > + return NULL;
> > + }
> > +
> > + /* Remove YT921x tag and update checksum */
> > + skb_pull_rcsum(skb, YT921X_TAG_LEN);
> > +
> > + dsa_default_offload_fwd_mark(skb);
> > +
> > + dsa_strip_etype_header(skb, YT921X_TAG_LEN);
> > +
> > + return skb;
> > +}
> > +
> > +static const struct dsa_device_ops yt921x_netdev_ops = {
> > + .name = YT921X_NAME,
> > + .proto = DSA_TAG_PROTO_YT921X,
> > + .xmit = yt921x_tag_xmit,
> > + .rcv = yt921x_tag_rcv,
> > + .needed_headroom = YT921X_TAG_LEN,
> > +};
> > +
> > +MODULE_DESCRIPTION("DSA tag driver for Motorcomm YT921x switches");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_YT921X, YT921X_NAME);
> > +
> > +module_dsa_tag_driver(yt921x_netdev_ops);
> > --
> > 2.50.1
> >
>
Powered by blists - more mailing lists