Message-ID: <ef68689e-7e0b-4702-a762-d214c7d76e3b@gmail.com>
Date: Mon, 9 Dec 2024 13:36:55 -0500
From: Antonio Pastor <antonio.pastor@...il.com>
To: netdev@...r.kernel.org
Subject: [PATCH net] llc: reset transport_header offset as value is inaccurate
 when buffer is processed by DSA

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

Powered by blists - more mailing lists