Re: [PATCH net] llc: reset transport_header offset as value is inaccurate when buffer is processed by DSA

[Antonio Pastor <antonio.pastor@gmail.com>]

 From 46c5a1ad90905e054b4a459e86b9ef98eca26df9 Mon Sep 17 00:00:00 2001
From: Antonio Pastor <antonio.pastor@gmail.com>
Date: Tue, 10 Dec 2024 19:45:20 -0500
Subject: [RFC PATCH] llc: llc_input: explicitly set skb->transport_header

Reset transport_header offset and apply the LLC header size increment, 
instead
of applying the increment on current value.
With DSA is enabled skb->transport_header is 2 bytes off, causing
net/802/psnap/snap_rcv to fail OUI:PID match and drop skb.

Signed-off-by: Antonio Pastor <antonio.pastor@gmail.com>
---
  net/llc/llc_input.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/llc/llc_input.c b/net/llc/llc_input.c
index 51bccfb00a9c..6f33ae9095f8 100644
--- a/net/llc/llc_input.c
+++ b/net/llc/llc_input.c
@@ -124,7 +124,7 @@ static inline int llc_fixup_skb(struct sk_buff *skb)
      if (unlikely(!pskb_may_pull(skb, llc_len)))
          return 0;

-    skb->transport_header += llc_len;
+    skb_set_transport_header(skb, llc_len);
      skb_pull(skb, llc_len);
      if (skb->protocol == htons(ETH_P_802_2)) {
          __be16 pdulen;
-- 
2.43.0

On 2024-12-09 13:36, Antonio Pastor wrote:
> Hi,
>
> While testing 802.2+LLC+SNAP processing of inbound packets in OpenWrt, 
> it was found that network_header offset is 2 bytes short (before 
> sbk->data) when the packet was received through OpenWrt's DSA 
> (Distributed Switch Architecture). This causes SNAP OUI:PID mismatch 
> and packet is silently dropped by snap_rcv().
>
> Here a trace:
>
>           <idle>-0       [001] ..s..  8744.047176: find_snap_client 
> <-snap_rcv
>           <idle>-0       [001] ..s..  8744.047218: <stack trace>
>  => snap_rcv
>  => llc_rcv
>  => __netif_receive_skb_one_core
>  => netif_receive_skb
>  => br_handle_frame_finish
>  => br_handle_frame
>  => __netif_receive_skb_core.constprop.0
>  => __netif_receive_skb_list_core
>  => netif_receive_skb_list_internal
>  => napi_complete_done
>  => gro_cell_poll
>  => __napi_poll.constprop.0
>  => net_rx_action
>  => handle_softirqs
>  => irq_exit
>  => call_with_stack
>  => __irq_svc
>  => default_idle_call
>  => do_idle
>  => cpu_startup_entry
>  => secondary_start_kernel
>  => 0x42301294
>
> The offsets were detected as incorrect as early as 
> napi_complete_done() and I gave up on tracking where the problem comes 
> from. Running with GRO disabled makes no difference.
>
> Curiously enough, __netif_receive_skb_list_core() resets 
> network_header offset, but leaves transport_header offset alone if it 
> was set, assuming it is correct. On non-DSA OpenWrt images it is, but 
> since images were migrated to use DSA this issue appears. For locally 
> generated packets transport_header offset is not set (0xffff) so 
> __netif_receive_skb_list_core() resets it, which solves the issue. 
> That is why inbound packets received from an external system exhibit 
> the problem but locally generated traffic is processed OK.
>
> I can only assume this has been an issue for a while but since 
> presumably it only impacts 802.2+LLC+SNAP (which I'm aware is not much 
> used today) it has not been flagged before. I wouldn't be surprised if 
> any protocols using Ethernet II frames reset transport_header offset 
> before they have anything to do with it.
>
> The kernel code does not touch transport_header offset until llc_rcv() 
> where it is moved forward based on the length of the LLC header as it 
> is assumed correct, which is the issue.
>
> Patch below proposes modifying llc_rcv() to reset transport_header 
> offset and then push forward by the LLC header length. While a better 
> solution might lurk elsewhere by tackling the root cause of why 
> transport_header offset is off after DSA set it to begin with, that is 
> taking too much effort to identify and risks widespread impact. A 
> patch could be made to __netif_receive_skb_list_core() to always reset 
> transport_header offset, but that would also impact all frames. This 
> is a lower risk patch that will not impact any non 802.2+LLC frames, 
> and presumably only SNAP ones. It follows the approach of 
> __netif_receive_skb_list_core() of not trusting the offset as received 
> and resetting it before snap_rcv() has a need for it.
>
> Patch:
>
>  net/llc/llc_input.c | 2 +-
>  1 file changed, 1 insertions(+), 1 deletions(-)
>
> --- a/net/llc/llc_input.c
> +++ b/net/llc/llc_input.c
> @@ -124,7 +124,7 @@ static inline int llc_fixup_skb(struct s
>      if (unlikely(!pskb_may_pull(skb, llc_len)))
>          return 0;
>
> -    skb->transport_header += llc_len;
> +    skb_set_transport_header(skb, llc_len);
>      skb_pull(skb, llc_len);
>      if (skb->protocol == htons(ETH_P_802_2)) {
>          __be16 pdulen;
>
>
> Can you share your opinions on this patch and suggest next actions for 
> its adoption (or modification) please?
>
> Regards,
>
> AP
>