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

[Simon Horman <horms@kernel.org>]

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;
> +}

[ ... ]