[PATCH] net: dsa: microchip: linearize skb for tail-tagging switches

[PATCH] net: dsa: microchip: linearize skb for tail-tagging switches

[Jakob Unterwurzacher]

The pointer arithmentic for accessing the tail tag does not
seem to handle nonlinear skbs.

For nonlinear skbs, it reads uninitialized memory inside the
skb headroom, essentially randomizing the tag, breaking user
traffic.

Example where ksz9477_rcv thinks that the packet from port 1 comes
from port 6 (which does not exist for the ksz9896 that's in use),
dropping the packet. Debug prints added by me (not included in
this patch):

	[  256.645337] ksz9477_rcv:323 tag0=6
	[  256.645349] skb len=47 headroom=78 headlen=0 tailroom=0
	               mac=(64,14) mac_len=14 net=(78,0) trans=78
	               shinfo(txflags=0 nr_frags=1 gso(size=0 type=0 segs=0))
	               csum(0x0 start=0 offset=0 ip_summed=0 complete_sw=0 valid=0 level=0)
	               hash(0x0 sw=0 l4=0) proto=0x00f8 pkttype=1 iif=3
	               priority=0x0 mark=0x0 alloc_cpu=0 vlan_all=0x0
	               encapsulation=0 inner(proto=0x0000, mac=0, net=0, trans=0)
	[  256.645377] dev name=end1 feat=0x0002e10200114bb3
	[  256.645386] skb headroom: 00000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
	[  256.645395] skb headroom: 00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
	[  256.645403] skb headroom: 00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
	[  256.645411] skb headroom: 00000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
	[  256.645420] skb headroom: 00000040: ff ff ff ff ff ff 00 1c 19 f2 e2 db 08 06
	[  256.645428] skb frag:     00000000: 00 01 08 00 06 04 00 01 00 1c 19 f2 e2 db 0a 02
	[  256.645436] skb frag:     00000010: 00 83 00 00 00 00 00 00 0a 02 a0 2f 00 00 00 00
	[  256.645444] skb frag:     00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01
	[  256.645452] ksz_common_rcv:92 dsa_conduit_find_user returned NULL

Call skb_linearize before trying to access the tag.

This patch fixes ksz9477_rcv which is used by the ksz9896 I have at
hand, and also applies the same fix to ksz8795_rcv which seems to have
the same problem.

Tested on v6.12.19 and today's master (d76bb1ebb5587f66b).

Signed-off-by: Jakob Unterwurzacher <jakob.unterwurzacher@cherry.de>
---
 net/dsa/tag_ksz.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index c33d4bf17929..7fbcdb7f152a 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c
@@ -140,7 +140,12 @@ static struct sk_buff *ksz8795_xmit(struct sk_buff *skb, struct net_device *dev)
 
 static struct sk_buff *ksz8795_rcv(struct sk_buff *skb, struct net_device *dev)
 {
-	u8 *tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
+	u8 *tag;
+
+	if (skb_linearize(skb))
+		return NULL;
+
+	tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
 
 	return ksz_common_rcv(skb, dev, tag[0] & KSZ8795_TAIL_TAG_EG_PORT_M,
 			      KSZ_EGRESS_TAG_LEN);
@@ -311,8 +316,13 @@ static struct sk_buff *ksz9477_xmit(struct sk_buff *skb,
 
 static struct sk_buff *ksz9477_rcv(struct sk_buff *skb, struct net_device *dev)
 {
+	u8 *tag;
+
+	if (skb_linearize(skb))
+		return NULL;
+
 	/* Tag decoding */
-	u8 *tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
+	tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
 	unsigned int port = tag[0] & KSZ9477_TAIL_TAG_EG_PORT_M;
 	unsigned int len = KSZ_EGRESS_TAG_LEN;
 
-- 
2.39.5

Re: [PATCH] net: dsa: microchip: linearize skb for tail-tagging switches

[Andrew Lunn]

On Fri, May 09, 2025 at 09:18:19AM +0200, Jakob Unterwurzacher wrote:
> The pointer arithmentic for accessing the tail tag does not
> seem to handle nonlinear skbs.
> 
> For nonlinear skbs, it reads uninitialized memory inside the
> skb headroom, essentially randomizing the tag, breaking user
> traffic.

Both tag_rtl8_4.c & tag_trailer.c also linearize, so i would say this
is correct.

What is interesting is that both xrs700x_rcv() and
sja1110_rcv_inband_control_extension() also don't call
skb_linearize().

Vladimir? George?

> Tested on v6.12.19 and today's master (d76bb1ebb5587f66b).

Please read:

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

This patch should be for net, and you need a Fixes: tag.

    Andrew

---
pw-bot: cr

Re: [PATCH] net: dsa: microchip: linearize skb for tail-tagging switches

[Vladimir Oltean]

On Fri, May 09, 2025 at 02:31:00PM +0200, Andrew Lunn wrote:
> On Fri, May 09, 2025 at 09:18:19AM +0200, Jakob Unterwurzacher wrote:
> > The pointer arithmentic for accessing the tail tag does not
> > seem to handle nonlinear skbs.
> > 
> > For nonlinear skbs, it reads uninitialized memory inside the
> > skb headroom, essentially randomizing the tag, breaking user
> > traffic.
> 
> Both tag_rtl8_4.c & tag_trailer.c also linearize, so i would say this
> is correct.
> 
> What is interesting is that both xrs700x_rcv() and
> sja1110_rcv_inband_control_extension() also don't call
> skb_linearize().
> 
> Vladimir? George?

Yes, it should be a more widespread problem.

Have non-zero needed_tailroom:
trailer
ksz8795
ksz9477
ksz9893
lan937x
hellcreek
sja1110
xrs700x

Call skb_linearize():
trailer
rtl8_4t

It should be only a matter of chance that the other taggers haven't come
across non-linear skbs.

My opinion is that we should let taggers linearize when and if it is
necessary, rather than doing so in the core. For example, sja1110 only
needs to do so if (rx_header & SJA1110_RX_HEADER_HAS_TRAILER), which the
core obviously does not know. Thus, I agree with the proposed fix.

Jakob, when you resend v2 retargeted to "net" and with the Fixes: tag
added, could you also address xrs700x and sja1110, or should I?

Re: [PATCH] net: dsa: microchip: linearize skb for tail-tagging switches

[Jakob Unterwurzacher]

On 09.05.25 14:56, Vladimir Oltean wrote:
> Jakob, when you resend v2 retargeted to "net" and with the Fixes: tag
> added, could you also address xrs700x and sja1110, or should I?

xrs700x seems clear enough, but sja1110 looks... complicated.
I'd prefer to only touch ksz.

I will send v2 on monday.

Thanks, Jakob

RE: [PATCH] net: dsa: microchip: linearize skb for tail-tagging switches

[Tristram.Ha]

> The pointer arithmentic for accessing the tail tag does not
> seem to handle nonlinear skbs.
> 
> For nonlinear skbs, it reads uninitialized memory inside the
> skb headroom, essentially randomizing the tag, breaking user
> traffic.
> 
> Example where ksz9477_rcv thinks that the packet from port 1 comes
> from port 6 (which does not exist for the ksz9896 that's in use),
> dropping the packet. Debug prints added by me (not included in
> this patch):
> 
>         [  256.645337] ksz9477_rcv:323 tag0=6
>         [  256.645349] skb len=47 headroom=78 headlen=0 tailroom=0
>                        mac=(64,14) mac_len=14 net=(78,0) trans=78
>                        shinfo(txflags=0 nr_frags=1 gso(size=0 type=0 segs=0))
>                        csum(0x0 start=0 offset=0 ip_summed=0 complete_sw=0 valid=0
> level=0)
>                        hash(0x0 sw=0 l4=0) proto=0x00f8 pkttype=1 iif=3
>                        priority=0x0 mark=0x0 alloc_cpu=0 vlan_all=0x0
>                        encapsulation=0 inner(proto=0x0000, mac=0, net=0, trans=0)
>         [  256.645377] dev name=end1 feat=0x0002e10200114bb3
>         [  256.645386] skb headroom: 00000000: 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00
>         [  256.645395] skb headroom: 00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00
>         [  256.645403] skb headroom: 00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00
>         [  256.645411] skb headroom: 00000030: 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00
>         [  256.645420] skb headroom: 00000040: ff ff ff ff ff ff 00 1c 19 f2 e2 db 08 06
>         [  256.645428] skb frag:     00000000: 00 01 08 00 06 04 00 01 00 1c 19 f2 e2 db
> 0a 02
>         [  256.645436] skb frag:     00000010: 00 83 00 00 00 00 00 00 0a 02 a0 2f 00 00
> 00 00
>         [  256.645444] skb frag:     00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 01
>         [  256.645452] ksz_common_rcv:92 dsa_conduit_find_user returned NULL
> 
> Call skb_linearize before trying to access the tag.
> 
> This patch fixes ksz9477_rcv which is used by the ksz9896 I have at
> hand, and also applies the same fix to ksz8795_rcv which seems to have
> the same problem.
> 
> Tested on v6.12.19 and today's master (d76bb1ebb5587f66b).
> 
> Signed-off-by: Jakob Unterwurzacher <jakob.unterwurzacher@cherry.de>
> ---
>  net/dsa/tag_ksz.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
> index c33d4bf17929..7fbcdb7f152a 100644
> --- a/net/dsa/tag_ksz.c
> +++ b/net/dsa/tag_ksz.c
> @@ -140,7 +140,12 @@ static struct sk_buff *ksz8795_xmit(struct sk_buff *skb,
> struct net_device *dev)
> 
>  static struct sk_buff *ksz8795_rcv(struct sk_buff *skb, struct net_device *dev)
>  {
> -       u8 *tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
> +       u8 *tag;
> +
> +       if (skb_linearize(skb))
> +               return NULL;
> +
> +       tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
> 
>         return ksz_common_rcv(skb, dev, tag[0] & KSZ8795_TAIL_TAG_EG_PORT_M,
>                               KSZ_EGRESS_TAG_LEN);
> @@ -311,8 +316,13 @@ static struct sk_buff *ksz9477_xmit(struct sk_buff *skb,
> 
>  static struct sk_buff *ksz9477_rcv(struct sk_buff *skb, struct net_device *dev)
>  {
> +       u8 *tag;
> +
> +       if (skb_linearize(skb))
> +               return NULL;
> +
>         /* Tag decoding */
> -       u8 *tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
> +       tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
>         unsigned int port = tag[0] & KSZ9477_TAIL_TAG_EG_PORT_M;
>         unsigned int len = KSZ_EGRESS_TAG_LEN;
> 

I am curious about what MAC controller are you using to return socket
buffer with fragments?

I thought DSA already disables most advanced hardware acceleration
features so that the tag manipulation functions do not need to worry
about accessing the socket buffer incorrectly.

Now that the socket buffer has fragment is it better to remove the tail
tag inside the fragment to preserve the performance of receiving such
socket buffer?

I know using skb_linearize() is an easy fix, but is it worthwhile to
check the receive performance?

This is out of topic.

The transmit performance can be improved by keeping the fragments while
adding the tail tag.  At one time there is an API in the kernel to do
that, but it was removed as nobody used it.  I see the hardware checksum
generation feature is now preserved so the tag manipulation functions
need to handle it.  Is there a way to allocate a few more bytes in the
fragments so that the socket buffer does not need to be consolidated to
add tail tag?

Re: [PATCH] net: dsa: microchip: linearize skb for tail-tagging switches

[Jakob Unterwurzacher]

On 10.05.25 00:18, Tristram.Ha@microchip.com wrote:
> 
> I am curious about what MAC controller are you using to return socket
> buffer with fragments?

Hi, we have the KSZ9896C connected to gmac1 on the Rockchip RK3588, dts [1][2] 
says:

	gmac1: ethernet@fe1c0000 {
		compatible = "rockchip,rk3588-gmac", "snps,dwmac-4.20a";

Thanks, Jakob

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi?h=v6.15-rc5#n1811
[2] 
https://www.kernel.org/doc/Documentation/devicetree/bindings/net/snps%2Cdwmac.yaml