Re: [PATCH v2 net-next 09/14] net: dsa: add NETC switch tag support

Simon Horman
2026-03-24T16:54:28Z

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: dsa: add NETC switch tag support

This commit introduces support for the NXP NETC switch tag protocol,
a proprietary header used by NETC switches. The tag supports three
types (Forward, To_Port, and To_Host) with variable length (6-14 bytes).

> diff --git a/include/linux/dsa/tag_netc.h b/include/linux/dsa/tag_netc.h
> new file mode 100644
> index 000000000000..fe964722e5b0
> --- /dev/null
> +++ b/include/linux/dsa/tag_netc.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * Copyright 2025-2026 NXP
> + */
> +
> +#ifndef __NET_DSA_TAG_NETC_H
> +#define __NET_DSA_TAG_NETC_H
> +
> +#include <linux/skbuff.h>
> +#include <net/dsa.h>
> +
> +#define NETC_TAG_MAX_LEN 14
> +
> +#endif

[ ... ]

> diff --git a/net/dsa/tag_netc.c b/net/dsa/tag_netc.c
> new file mode 100644
> index 000000000000..15b0dd5c5408
> --- /dev/null
> +++ b/net/dsa/tag_netc.c

[ ... ]

> @@ -0,0 +1,180 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2025-2026 NXP
> + */
> +
> +#include <linux/dsa/tag_netc.h>
> +
> +#include "tag.h"
> +
> +#define NETC_NAME "nxp_netc"
> +
> +/* Forward NXP switch tag */
> +#define NETC_TAG_FORWARD 0
> +
> +/* To_Port NXP switch tag */
> +#define NETC_TAG_TO_PORT 1
> +/* SubType0: No request to perform timestamping */
> +#define NETC_TAG_TP_SUBTYPE0 0
> +
> +/* To_Host NXP switch tag */
> +#define NETC_TAG_TO_HOST 2
> +/* SubType0: frames redirected or copied to CPU port */
> +#define NETC_TAG_TH_SUBTYPE0 0
> +/* SubType1: frames redirected or copied to CPU port with timestamp */
> +#define NETC_TAG_TH_SUBTYPE1 1
> +/* SubType2: Transmit timestamp response (two-step timestamping) */
> +#define NETC_TAG_TH_SUBTYPE2 2
> +
> +/* NETC switch tag lengths */
> +#define NETC_TAG_FORWARD_LEN 6
> +#define NETC_TAG_TP_SUBTYPE0_LEN 6
> +#define NETC_TAG_TH_SUBTYPE0_LEN 6
> +#define NETC_TAG_TH_SUBTYPE1_LEN 14
> +#define NETC_TAG_TH_SUBTYPE2_LEN 14
> +#define NETC_TAG_CMN_LEN 5
> +
> +#define NETC_TAG_SUBTYPE GENMASK(3, 0)
> +#define NETC_TAG_TYPE GENMASK(7, 4)
> +#define NETC_TAG_QV BIT(0)
> +#define NETC_TAG_IPV GENMASK(4, 2)
> +#define NETC_TAG_SWITCH GENMASK(2, 0)
> +#define NETC_TAG_PORT GENMASK(7, 3)
> +
> +struct netc_tag_cmn {
> + __be16 tpid;
> + u8 type;
> + u8 qos;
> + u8 switch_port;
> +} __packed;

[ ... ]

> +static int netc_get_rx_tag_len(int rx_type)
> +{
> + int type = FIELD_GET(NETC_TAG_TYPE, rx_type);
> +
> + if (type == NETC_TAG_TO_HOST) {
> + u8 subtype = rx_type & NETC_TAG_SUBTYPE;
> +
> + if (subtype == NETC_TAG_TH_SUBTYPE1)
> + return NETC_TAG_TH_SUBTYPE1_LEN;
> + else if (subtype == NETC_TAG_TH_SUBTYPE2)
> + return NETC_TAG_TH_SUBTYPE2_LEN;
> + else
> + return NETC_TAG_TH_SUBTYPE0_LEN;
> + }
> +
> + return NETC_TAG_FORWARD_LEN;
> +}
> +
> +static struct sk_buff *netc_rcv(struct sk_buff *skb,
> + struct net_device *ndev)
> +{
> + struct netc_tag_cmn *tag_cmn = dsa_etype_header_pos_rx(skb);
> + int tag_len = netc_get_rx_tag_len(tag_cmn->type);
^^^^^^^^^^^^

Is the tag header guaranteed to be in the linear region of the skb?
Looking at other EtherType-based DSA tag drivers (tag_ar9331, tag_lan9303,
tag_dsa, tag_edsa, tag_a5psw, tag_brcm_legacy, tag_mxl-gsw1xx,
tag_gswip, and tag_mtk), they all call pskb_may_pull() before accessing
tag data to ensure the header is linearized.

For example, tag_ar9331.c:

static struct sk_buff *ar9331_tag_rcv(struct sk_buff *skb,
struct net_device *ndev)
{
if (unlikely(!pskb_may_pull(skb, AR9331_HDR_LEN)))
return NULL;
...
}

Similarly, tag_lan9303.c:

static struct sk_buff *lan9303_rcv(struct sk_buff *skb,
struct net_device *ndev)
{
if (unlikely(!pskb_may_pull(skb, LAN9303_TAG_LEN)))
return NULL;
...
}

Without this check, netc_rcv() accesses tag_cmn->type, tag_cmn->tpid,
tag_cmn->qos, and tag_cmn->switch_port before verifying these bytes are
linearized. The struct netc_tag_cmn is 5 bytes, with 3 bytes at
skb->data through skb->data+2 that may not be guaranteed to be in the
linear region.

Additionally, netc_get_rx_tag_len() can return up to 14 bytes
(NETC_TAG_TH_SUBTYPE1_LEN or NETC_TAG_TH_SUBTYPE2_LEN), and this value
is passed directly to skb_pull_rcsum():

> + int tag_len = netc_get_rx_tag_len(tag_cmn->type);
> + int sw_id, port;
> +
> + if (ntohs(tag_cmn->tpid) != ETH_P_NXP_NETC) {
> + dev_warn_ratelimited(&ndev->dev, "Unknown TPID 0x%04x\n",
> + ntohs(tag_cmn->tpid));
> +
> + return NULL;
> + }
> +
> + if (tag_cmn->qos & NETC_TAG_QV)
> + skb->priority = FIELD_GET(NETC_TAG_IPV, tag_cmn->qos);
> +
> + sw_id = NETC_TAG_SWITCH & tag_cmn->switch_port;
> + /* ENETC VEPA switch ID (0) is not supported yet */
> + if (!sw_id) {
> + dev_warn_ratelimited(&ndev->dev,
> + "VEPA switch ID is not supported yet\n");
> +
> + return NULL;
> + }
> +
> + port = FIELD_GET(NETC_TAG_PORT, tag_cmn->switch_port);
> + skb->dev = dsa_conduit_find_user(ndev, sw_id, port);
> + if (!skb->dev)
> + return NULL;
> +
> + if (tag_cmn->type == NETC_TAG_FORWARD)
> + dsa_default_offload_fwd_mark(skb);
> +
> + /* Remove Switch tag from the frame */
> + skb_pull_rcsum(skb, tag_len);
^^^^^^^^^^^^^^^^^^

The skb_pull_rcsum() function contains BUG_ON(len > skb->len). If a
truncated or malformed packet arrives with fewer than tag_len bytes
remaining, the kernel will panic rather than gracefully handling the
error.

Should a check like this be added at the beginning of netc_rcv():

if (unlikely(!pskb_may_pull(skb, NETC_TAG_MAX_LEN - 2)))
return NULL;

The -2 would account for the EtherType bytes already consumed by
eth_type_trans().

> + dsa_strip_etype_header(skb, tag_len);
> +
> + return skb;
> +}

[ ... ]