Re: [net-next,v8,3/5] net: dsa: tag_ks8995: Add the KS8995 tag handling

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:

Critical: 0 · High: 2 · Medium: 0 · Low: 3

- [High] ks8995_xmit() (net/dsa/tag_ks8995.c:44-59) does not insert a
  switch tag when the frame already carries an in-band 802.1Q tag at…
- [High] ks8995_xmit() mixes two different header base pointers: the
  branch decision at net/dsa/tag_ks8995.c:44-45 reads the EtherType…
- [Low] DISCARDED (resolved later in the series): the new
  DSA_TAG_PROTO_KS8995 tagger has no in-tree consumer and no switch-side…
- [Low] DISCARDED as a functional bug, residual documentation defect
  only: the ks8995_rcv() comment at net/dsa/tag_ks8995.c:105-114 states…
- [Low] ks8995_rcv() calls netdev_err() (net/dsa/tag_ks8995.c:92) on
  every skb_vlan_untag() failure.

--

net: dsa: tag_ks8995: Add the KS8995 tag handling

This adds a new DSA tagger for the Micrel/Microchip KS8995 family. The tag
is a modified 0x8100 EtherType where the low nybble carries the target port
mask, and the tagger pushes any hardware-accelerated VLAN tag inside the
frame first so the special tag stays at offset 12.

A few questions about the transmit and receive paths below.

> diff --git a/net/dsa/tag_ks8995.c b/net/dsa/tag_ks8995.c
> new file mode 100644
> index 0000000000000..6bd5f1015c265
> --- /dev/null
> +++ b/net/dsa/tag_ks8995.c
> @@ -0,0 +1,141 @@

[ ... ]

> +static struct sk_buff *ks8995_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> +	struct vlan_ethhdr *hdr;
> +	u16 portmask;
> +
> +	/* Prepare the special KS8995 tags */
> +	portmask = dsa_xmit_port_mask(skb, dev);
> +
> +	/* The switch expects the special tag at offset 12. Move any hardware
> +	 * accelerated VLAN tag into the payload so the conduit cannot insert
> +	 * it outside the special tag.
> +	 */
> +	if (unlikely(skb_vlan_tag_present(skb))) {
> +		skb = __vlan_hwaccel_push_inside(skb);
> +		if (!skb)
> +			return NULL;
> +	}
> +
> +	hdr = skb_vlan_eth_hdr(skb);
> +	if (hdr->h_vlan_proto != htons(ETH_P_8021Q)) {
> +		skb = vlan_insert_tag(skb, KS8995M_STPID(portmask), 0);
> +		/* vlan_insert_tag() drops the skb on failure */
> +		if (!skb)
> +			return NULL;
> +		hdr = skb_vlan_eth_hdr(skb);
> +		netdev_dbg(dev, "%s: inserted VLAN TAG %04x TCI %04x\n",
> +			   __func__, hdr->h_vlan_proto, hdr->h_vlan_TCI);
> +	} else {
> +		/* VLAN tag already exists in skb head, modify it in place */
> +		hdr = vlan_eth_hdr(skb);
> +		hdr->h_vlan_proto = KS8995M_STPID(portmask);

[Severity: High]
In the else branch of ks8995_xmit(), no switch tag is inserted; only the
TPID of the frame's existing 802.1Q tag is rewritten. Does that mean the
sender's VLAN tag is consumed as the special tag?

For an 8021q upper on a DSA user port the tag ends up in-band before the
tagger runs, either through the core:

  vlan_dev_hard_start_xmit() -> validate_xmit_vlan() ->
  __vlan_hwaccel_push_inside()

or through the __vlan_hwaccel_push_inside() call at the top of
ks8995_xmit() itself. The h_vlan_proto == htons(ETH_P_8021Q) test then
selects the in-place rewrite.

The companion patch in this series configures PORT_INSERT_TAG on the CPU
port and PORT_REMOVE_TAG on the user ports, with the comment in
ksz8_port_setup():

	/* The KSZ8995XA has a special tag format in the front of the frame
	 * that need to be inserted by the CPU and then removed by each
	 * port. PORT_REMOVE_TAG simply means "remove tags coming from the
	 * CPU port" it does not affect ingress packets.
	 */

If the user port removes those four bytes, do VLAN-tagged frames leave the
user port untagged, landing in the peer's native VLAN?

There is a second effect of reusing the existing tag: the special tag's TCI
becomes whatever VID/PCP the local sender chose (an 8021q upper, or a raw
socket writing its own 802.1Q header), whereas the vlan_insert_tag() branch
always produces TCI 0. Is that asymmetry intentional?

The commit message says:

    Push any hardware-accelerated VLAN tag into the packet before adding the
    switch tag.

For frames that already carry an 802.1Q tag no switch tag is added at all,
so could the merging behaviour be spelled out either in the commit message
or in a comment?

[Severity: High]
The branch decision and the write use two different header bases. The test
reads through skb_vlan_eth_hdr(), which is (struct vlan_ethhdr *)skb->data,
while the store goes through vlan_eth_hdr(), which is
(struct vlan_ethhdr *)skb_mac_header(skb). include/linux/if_vlan.h says of
skb_vlan_eth_hdr(): "Prefer this version in TX path".

The two bases coincide only because __dev_queue_xmit() calls
skb_reset_mac_header(). __dev_direct_xmit() does not, and neither does
validate_xmit_skb() on that path. AF_XDP copy mode takes exactly that
route:

  xsk_generic_xmit() -> xsk_build_skb()   /* mac_header left unset */
    -> __dev_direct_xmit() -> netdev_start_xmit()
      -> dsa_user_xmit() -> ks8995_xmit()

xsk_build_skb() allocates with sock_alloc_send_skb() and never sets the MAC
header, so skb->mac_header keeps the unset value (~0U); skb_mac_header()
only has a DEBUG_NET_WARN_ON_ONCE for that case.

For such an skb whose bytes 12-13 happen to be 0x8100 (fully chosen by the
AF_XDP application, which writes raw L2 frames), the test picks the else
branch from the correct offset and then:

		hdr = vlan_eth_hdr(skb);
		hdr->h_vlan_proto = KS8995M_STPID(portmask);

writes two bytes at skb->head + 0xFFFF + 12. Can this scribble outside the
buffer? Would simply reusing the hdr already obtained from
skb_vlan_eth_hdr() above avoid it? Other taggers that rewrite the header on
transmit use the skb->data based accessor, for example
net/dsa/tag_sja1105.c.

> +		netdev_dbg(dev, "%s: modified VLAN TAG %04x\n",
> +			   __func__, hdr->h_vlan_proto);
> +	}
> +
> +	return skb;
> +}
> +
> +static struct sk_buff *ks8995_rcv(struct sk_buff *skb, struct net_device *dev)
> +{

[ ... ]

> +	/* Move the custom DSA+VLAN tag into the hwaccel area and strip
> +	 * it from the skb head
> +	 */
> +	skb = skb_vlan_untag(skb);
> +	if (!skb) {
> +		/* skb_vlan_untag drops the skb on failure */
> +		netdev_err(dev, "%s: unable to untag skb\n", __func__);
> +		return NULL;
> +	}

[Severity: Low]
Could this netdev_err() flood the log? It is an unratelimited KERN_ERR on a
path driven entirely by received frames, and skb_vlan_untag() has two
remote-traffic-dependent failure modes:

net/core/skbuff.c:skb_vlan_untag() {
	...
	if (unlikely(!pskb_may_pull(skb, VLAN_HLEN + sizeof(unsigned short))))
		goto err_free;
	...
	skb = skb_reorder_vlan_header(skb);
	if (unlikely(!skb))
		goto err_free;
	...
}

The first fires on short frames, the second on the skb_cow() ->
pskb_expand_head() GFP_ATOMIC allocation, which is reachable whenever the
RX skb is cloned (a packet socket tapping the conduit) and memory is tight.

The neighbouring drop reason in this same function uses netdev_dbg(), and
net/dsa/tag_lan9303.c uses dev_warn_ratelimited() on its RX drop path.
Would netdev_dbg() or a ratelimited/once variant be preferable here?

[ ... ]

> +	/* Preserve the VLAN tag if it contains a non-zero VID which is not
> +	 * identical to 0x001, or PCP, and restore its TPID to the standard
> +	 * value.
> +	 *
> +	 * If this is just an ordinary inbound package the datasheet claims
> +	 * it will "replace null VID with ingress port VID", which means
> +	 * VID set to 1: 0x8101 0001 for port 0 or 0x8102 0001 for port 1.
> +	 * So in the DSA driver we will set the default port VID to 0 so
> +	 * we can properly detect non-VLAN frames.
> +	 */
> +	if (!skb->vlan_tci) {

[Severity: Low]
This looks like a comment inaccuracy rather than a functional problem. The
comment describes preserving the tag only when the VID is "non-zero ... and
not identical to 0x001", but the code just tests !skb->vlan_tci, so there
is no VID == 1 exemption.

The code appears correct as written: the companion patch programs
ksz8_port_setup() with

	/* Use VID 0 to identify untagged frames in the special tag */
	ksz_pwrite16(dev, port, REG_PORT_CTRL_VID, 0);

so untagged ingress frames arrive with TCI 0 and the test catches them, and
a genuine VLAN 1 frame should not be treated as untagged. Could the comment
be reworded to match what the code does?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831-ks8995-to-ksz8-v8-0-fd1dbb2b76e8%40kernel.org