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

[Paolo Abeni <pabeni@redhat.com>]

On 12/9/24 19:36, Antonio Pastor wrote:
> 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?

IMHO llc should avoid entirely using the transport_header field as
AFAICS the relevant information do not belong to such layer. Anyhow such
change looks like way too invasive as a fix.

Your approach is IMHO correct, but the patch itself is white-space
damage - be sure to generate it on top of current net tree and double
your client is not corrupting it. Additionally, as a fix the patch must
include a suitable Fixes tag. Have an accurate read to the process
documentation before formally submitting the fix.

Thanks,

Paolo