[PATCH] net: llc: explicitly set skb->transport_header

[PATCH] net: llc: explicitly set skb->transport_header

[Antonio Pastor]

802.2+LLC+SNAP frames received by napi_complete_done with GRO and DSA
have skb->transport_header set two bytes short, or pointing 2 bytes
before network_header & skb->data. As snap_rcv expects transport_header
to point to SNAP header (OID:PID) after LLC processing advances offset
over LLC header (llc_rcv & llc_fixup_skb), code doesn't find a match
and packet is dropped.

Between napi_complete_done and snap_rcv, transport_header is not used
until __netif_receive_skb_core, where originally it was being reset.
Commit fda55eca5a33 ("net: introduce skb_transport_header_was_set()")
only does so if not set, on the assumption the value was set correctly
by GRO (and also on assumption that "network stacks usually reset the
transport header anyway"). Afterwards it is moved forward by
llc_fixup_skb.

Locally generated traffic shows up at __netif_receive_skb_core with no
transport_header set and is processed without issue. On a setup with
GRO but no DSA, transport_header and network_header are both set to
point to skb->data which is also correct.

As issue is LLC specific, to avoid impacting non-LLC traffic, and to
follow up on original assumption made on previous code change,
llc_fixup_skb to reset and advance the offset. llc_fixup_skb already
assumes the LLC header is at skb->data, and by definition SNAP header
immediately follows.

Fixes: fda55eca5a33 ("net: introduce skb_transport_header_was_set()")
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

[PATCH net v2] net: llc: explicitly set skb->transport_header

[Antonio Pastor]

802.2+LLC+SNAP frames received by napi_complete_done with GRO and DSA
have skb->transport_header set two bytes short, or pointing 2 bytes
before network_header & skb->data. As snap_rcv expects transport_header
to point to SNAP header (OID:PID) after LLC processing advances offset
over LLC header (llc_rcv & llc_fixup_skb), code doesn't find a match
and packet is dropped.

Between napi_complete_done and snap_rcv, transport_header is not used
until __netif_receive_skb_core, where originally it was being reset.
Commit fda55eca5a33 ("net: introduce skb_transport_header_was_set()")
only does so if not set, on the assumption the value was set correctly
by GRO (and also on assumption that "network stacks usually reset the
transport header anyway"). Afterwards it is moved forward by
llc_fixup_skb.

Locally generated traffic shows up at __netif_receive_skb_core with no
transport_header set and is processed without issue. On a setup with
GRO but no DSA, transport_header and network_header are both set to
point to skb->data which is also correct.

As issue is LLC specific, to avoid impacting non-LLC traffic, and to
follow up on original assumption made on previous code change,
llc_fixup_skb to reset and advance the offset. llc_fixup_skb already
assumes the LLC header is at skb->data, and by definition SNAP header
immediately follows.

Fixes: fda55eca5a33 ("net: introduce skb_transport_header_was_set()")
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

Re: [PATCH net v2] net: llc: explicitly set skb->transport_header

[Eric Dumazet]

On Mon, Dec 23, 2024 at 2:40 PM Antonio Pastor <antonio.pastor@gmail.com> wrote:
>
> 802.2+LLC+SNAP frames received by napi_complete_done with GRO and DSA
> have skb->transport_header set two bytes short, or pointing 2 bytes
> before network_header & skb->data. As snap_rcv expects transport_header
> to point to SNAP header (OID:PID) after LLC processing advances offset
> over LLC header (llc_rcv & llc_fixup_skb), code doesn't find a match
> and packet is dropped.
>
> Between napi_complete_done and snap_rcv, transport_header is not used
> until __netif_receive_skb_core, where originally it was being reset.
> Commit fda55eca5a33 ("net: introduce skb_transport_header_was_set()")
> only does so if not set, on the assumption the value was set correctly
> by GRO (and also on assumption that "network stacks usually reset the
> transport header anyway"). Afterwards it is moved forward by
> llc_fixup_skb.
>
> Locally generated traffic shows up at __netif_receive_skb_core with no
> transport_header set and is processed without issue. On a setup with
> GRO but no DSA, transport_header and network_header are both set to
> point to skb->data which is also correct.
>
> As issue is LLC specific, to avoid impacting non-LLC traffic, and to
> follow up on original assumption made on previous code change,
> llc_fixup_skb to reset and advance the offset. llc_fixup_skb already
> assumes the LLC header is at skb->data, and by definition SNAP header
> immediately follows.
>
> Fixes: fda55eca5a33 ("net: introduce skb_transport_header_was_set()")
> Signed-off-by: Antonio Pastor <antonio.pastor@gmail.com>


11 years with this stuff being broken...
I wonder if we could remove it from the kernel, given nobody cared.

Can you at the same time fix net/802/psnap.c,
snap_rcv() is probably having the same issue ?

I would also use skb_reset_transport_header() as in

diff --git a/net/llc/llc_input.c b/net/llc/llc_input.c
index 51bccfb00a9cd9f16318bbe9a8cc3fe2460912b1..61b0159b2fbee60bdc2623b6c73ed1651b17a050
100644
--- a/net/llc/llc_input.c
+++ b/net/llc/llc_input.c
@@ -124,8 +124,8 @@ 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_pull(skb, llc_len);
+       skb_reset_transport_header(skb);
        if (skb->protocol == htons(ETH_P_802_2)) {
                __be16 pdulen;
                s32 data_size;

Re: [PATCH net v2] net: llc: explicitly set skb->transport_header

[Eric Dumazet]

On Mon, Dec 23, 2024 at 6:00 PM Antonio Pastor <antonio.pastor@gmail.com> wrote:
>
> On 2024-12-23 08:54, Eric Dumazet wrote:
>
> On Mon, Dec 23, 2024 at 2:40 PM Antonio Pastor <antonio.pastor@gmail.com> wrote:
>
> 802.2+LLC+SNAP frames received by napi_complete_done with GRO and DSA
> have skb->transport_header set two bytes short, or pointing 2 bytes
> before network_header & skb->data. As snap_rcv expects transport_header
> to point to SNAP header (OID:PID) after LLC processing advances offset
> over LLC header (llc_rcv & llc_fixup_skb), code doesn't find a match
> and packet is dropped.
>
> Between napi_complete_done and snap_rcv, transport_header is not used
> until __netif_receive_skb_core, where originally it was being reset.
> Commit fda55eca5a33 ("net: introduce skb_transport_header_was_set()")
> only does so if not set, on the assumption the value was set correctly
> by GRO (and also on assumption that "network stacks usually reset the
> transport header anyway"). Afterwards it is moved forward by
> llc_fixup_skb.
>
> Locally generated traffic shows up at __netif_receive_skb_core with no
> transport_header set and is processed without issue. On a setup with
> GRO but no DSA, transport_header and network_header are both set to
> point to skb->data which is also correct.
>
> As issue is LLC specific, to avoid impacting non-LLC traffic, and to
> follow up on original assumption made on previous code change,
> llc_fixup_skb to reset and advance the offset. llc_fixup_skb already
> assumes the LLC header is at skb->data, and by definition SNAP header
> immediately follows.
>
> Fixes: fda55eca5a33 ("net: introduce skb_transport_header_was_set()")
> Signed-off-by: Antonio Pastor <antonio.pastor@gmail.com>
>
> Thanks for your reply.
>
> 11 years with this stuff being broken...
>
> It works great without DSA thrown in the mix, for some reason I have not
> been able to determine, not for the lack of trying. Wonder how many
> configurations use DSA and care about SNAP frames.
> DSA was added recently to the implementation I'm working on, so for
> practical purposes this is just broken since 1-2 years ago. Tripped on it
> about 6 months ago. It's fresh. Or well aged. :)
>
> I wonder if we could remove it from the kernel, given nobody cared.
>
> Well, at least I do, and ballpark 40 other people for what I can tell.
> Some others might be lurking in the sidelines.
>
> Can you at the same time fix net/802/psnap.c,
> snap_rcv() is probably having the same issue ?
>
> That's how I got here, because of snap_rcv(). But no fix is required at
> snap_rcv(). This patch is to fix problem there.
> snap_rcv() doesn't advance the transport_header offset or pull the skb as
> far as I saw so adding a reset there, that would be redundant. Once
> this piece of code determines LLC header size SNAP header always follows
> for SNAP frames.
> I'll double-check and submit a v3 w/that included if I think it makes
> sense but honestly I don't think it does.

I see skb->transport_header being advanced at line 61 :

    /* Pass the frame on. */
    skb->transport_header += 5;


Note that setting the transport header (conditionally or not) in
__netif_receive_skb()
is probably a mistake. Only network (ipv4, ipv6) handlers can possibly
know the concept
of transport header.

Hopefully at some point we can remove this defensive code.

diff --git a/net/core/dev.c b/net/core/dev.c
index c7f3dea3e0eb9eb05865e49dd7a8535afb974149..b6722e11ee4e171e6a2f379fc1d0197dfcb1a842
100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5474,8 +5474,6 @@ static int __netif_receive_skb_core(struct
sk_buff **pskb, bool pfmemalloc,
        orig_dev = skb->dev;

        skb_reset_network_header(skb);
-       if (!skb_transport_header_was_set(skb))
-               skb_reset_transport_header(skb);
        skb_reset_mac_len(skb);

        pt_prev = NULL;

[PATCH net v3] net: llc: reset skb->transport_header

[Antonio Pastor]

802.2+LLC+SNAP frames received by napi_complete_done with GRO and DSA
have skb->transport_header set two bytes short, or pointing 2 bytes
before network_header & skb->data. As snap_rcv expects transport_header
to point to SNAP header (OID:PID) after LLC processing advances offset
over LLC header (llc_rcv & llc_fixup_skb), code doesn't find a match
and packet is dropped.

Between napi_complete_done and snap_rcv, transport_header is not used
until __netif_receive_skb_core, where originally it was being reset.
Commit fda55eca5a33 ("net: introduce skb_transport_header_was_set()")
only does so if not set, on the assumption the value was set correctly
by GRO (and also on assumption that "network stacks usually reset the
transport header anyway"). Afterwards it is moved forward by
llc_fixup_skb.

Locally generated traffic shows up at __netif_receive_skb_core with no
transport_header set and is processed without issue. On a setup with
GRO but no DSA, transport_header and network_header are both set to
point to skb->data which is also correct.

As issue is LLC specific, to avoid impacting non-LLC traffic, and to
follow up on original assumption made on previous code change,
llc_fixup_skb to reset the offset after skb pull. llc_fixup_skb
assumes the LLC header is at skb->data, and by definition SNAP header
immediately follows.

Fixes: fda55eca5a33 ("net: introduce skb_transport_header_was_set()")
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..61b0159b2fbe 100644
--- a/net/llc/llc_input.c
+++ b/net/llc/llc_input.c
@@ -124,8 +124,8 @@ 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_pull(skb, llc_len);
+	skb_reset_transport_header(skb);
 	if (skb->protocol == htons(ETH_P_802_2)) {
 		__be16 pdulen;
 		s32 data_size;
-- 
2.43.0

Re: [PATCH net v2] net: llc: explicitly set skb->transport_header

[Antonio Pastor]

On 2024-12-23 13:18, Eric Dumazet wrote:
> I see skb->transport_header being advanced at line 61 :
>
>      /* Pass the frame on. */
>      skb->transport_header += 5;

Same treatment as before? Reset after pull?

@@ -58,8 +58,8 @@ static int snap_rcv(struct sk_buff *skb, struct 
net_device *dev,
         proto = find_snap_client(skb_transport_header(skb));
         if (proto) {
                 /* Pass the frame on. */
-               skb->transport_header += 5;
                 skb_pull_rcsum(skb, 5);
+               skb_reset_transport_header(skb);
                 rc = proto->rcvfunc(skb, dev, &snap_packet_type, orig_dev);
         }
         rcu_read_unlock();

I'll submit that as a separate patch.

> Note that setting the transport header (conditionally or not) in
> __netif_receive_skb()
> is probably a mistake. Only network (ipv4, ipv6) handlers can possibly
> know the concept
> of transport header.
Not too sure about that, but I don't have specifics. Non-IP stacks are 
probably all ancient, but some might care.
> Hopefully at some point we can remove this defensive code.
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index c7f3dea3e0eb9eb05865e49dd7a8535afb974149..b6722e11ee4e171e6a2f379fc1d0197dfcb1a842
> 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5474,8 +5474,6 @@ static int __netif_receive_skb_core(struct
> sk_buff **pskb, bool pfmemalloc,
>          orig_dev = skb->dev;
>
>          skb_reset_network_header(skb);
> -       if (!skb_transport_header_was_set(skb))
> -               skb_reset_transport_header(skb);
>          skb_reset_mac_len(skb);
>
>          pt_prev = NULL;

I don't know the code well enough to identify all possible paths after 
this point to ensure all of them set transport header. I'd expect 
IPv4/IPv6 are OK and we are making LLC whole, but don't know what else 
to test beyond that. My testing is limited to SNAP... taking that out 
requires regression testing at a level I'm not comfortable running.

Re: [PATCH net v3] net: llc: reset skb->transport_header

[Eric Dumazet]

On Wed, Dec 25, 2024 at 2:08 AM Antonio Pastor <antonio.pastor@gmail.com> wrote:
>
> 802.2+LLC+SNAP frames received by napi_complete_done with GRO and DSA
> have skb->transport_header set two bytes short, or pointing 2 bytes
> before network_header & skb->data. As snap_rcv expects transport_header
> to point to SNAP header (OID:PID) after LLC processing advances offset
> over LLC header (llc_rcv & llc_fixup_skb), code doesn't find a match
> and packet is dropped.
>
> Between napi_complete_done and snap_rcv, transport_header is not used
> until __netif_receive_skb_core, where originally it was being reset.
> Commit fda55eca5a33 ("net: introduce skb_transport_header_was_set()")
> only does so if not set, on the assumption the value was set correctly
> by GRO (and also on assumption that "network stacks usually reset the
> transport header anyway"). Afterwards it is moved forward by
> llc_fixup_skb.
>
> Locally generated traffic shows up at __netif_receive_skb_core with no
> transport_header set and is processed without issue. On a setup with
> GRO but no DSA, transport_header and network_header are both set to
> point to skb->data which is also correct.
>
> As issue is LLC specific, to avoid impacting non-LLC traffic, and to
> follow up on original assumption made on previous code change,
> llc_fixup_skb to reset the offset after skb pull. llc_fixup_skb
> assumes the LLC header is at skb->data, and by definition SNAP header
> immediately follows.
>
> Fixes: fda55eca5a33 ("net: introduce skb_transport_header_was_set()")
> Signed-off-by: Antonio Pastor <antonio.pastor@gmail.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net v3] net: llc: reset skb->transport_header

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 24 Dec 2024 20:07:20 -0500 you wrote:
> 802.2+LLC+SNAP frames received by napi_complete_done with GRO and DSA
> have skb->transport_header set two bytes short, or pointing 2 bytes
> before network_header & skb->data. As snap_rcv expects transport_header
> to point to SNAP header (OID:PID) after LLC processing advances offset
> over LLC header (llc_rcv & llc_fixup_skb), code doesn't find a match
> and packet is dropped.
> 
> [...]

Here is the summary with links:
  - [net,v3] net: llc: reset skb->transport_header
    https://git.kernel.org/netdev/net/c/a024e377efed

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

[PATCH net] net: 802: reset skb->transport_header

[Antonio Pastor]

802.2+LLC+SNAP frames received by napi_complete_done() with GRO and DSA
have skb->transport_header set two bytes short, or pointing 2 bytes
before network_header & skb->data. This was an issue as snap_rcv()
expected offset to point to SNAP header (OID:PID), causing packet to
be dropped.

A fix at llc_fixup_skb() (a024e377efed) resets transport_header,
addressing the issue. This patch is additional clean up to snap_rcv()
so that it resets the offset after pulling the skb instead of
incrementing it to match the pull.

Fixes: fda55eca5a33 ("net: introduce skb_transport_header_was_set()")
Signed-off-by: Antonio Pastor <antonio.pastor@gmail.com>
---
 net/802/psnap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/802/psnap.c b/net/802/psnap.c
index fca9d454905f..252006f81afa 100644
--- a/net/802/psnap.c
+++ b/net/802/psnap.c
@@ -58,8 +58,8 @@ static int snap_rcv(struct sk_buff *skb, struct net_device *dev,
 	proto = find_snap_client(skb_transport_header(skb));
 	if (proto) {
 		/* Pass the frame on. */
-		skb->transport_header += 5;
 		skb_pull_rcsum(skb, 5);
+		skb_reset_transport_header(skb);
 		rc = proto->rcvfunc(skb, dev, &snap_packet_type, orig_dev);
 	}
 	rcu_read_unlock();
-- 
2.43.0

Re: [PATCH net] net: 802: reset skb->transport_header

[Eric Dumazet]

On Sat, Dec 28, 2024 at 3:13 AM Antonio Pastor <antonio.pastor@gmail.com> wrote:
>
> 802.2+LLC+SNAP frames received by napi_complete_done() with GRO and DSA
> have skb->transport_header set two bytes short, or pointing 2 bytes
> before network_header & skb->data. This was an issue as snap_rcv()
> expected offset to point to SNAP header (OID:PID), causing packet to
> be dropped.
>
> A fix at llc_fixup_skb() (a024e377efed) resets transport_header,
> addressing the issue. This patch is additional clean up to snap_rcv()
> so that it resets the offset after pulling the skb instead of
> incrementing it to match the pull.
>
> Fixes: fda55eca5a33 ("net: introduce skb_transport_header_was_set()")
> Signed-off-by: Antonio Pastor <antonio.pastor@gmail.com>
> ---
>  net/802/psnap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/802/psnap.c b/net/802/psnap.c
> index fca9d454905f..252006f81afa 100644
> --- a/net/802/psnap.c
> +++ b/net/802/psnap.c
> @@ -58,8 +58,8 @@ static int snap_rcv(struct sk_buff *skb, struct net_device *dev,
>         proto = find_snap_client(skb_transport_header(skb));
>         if (proto) {
>                 /* Pass the frame on. */
> -               skb->transport_header += 5;
>                 skb_pull_rcsum(skb, 5);
> +               skb_reset_transport_header(skb);
>                 rc = proto->rcvfunc(skb, dev, &snap_packet_type, orig_dev);
>         }
>         rcu_read_unlock();
> --
> 2.43.0
>

Sorry, this patch is wrong, it does not fix the potential issue yet.

Note how skb_transport_header(skb) is used in
find_snap_client(skb_transport_header(skb));

The proper way to fix the issue is to not rely on the transport header
at all, only reset it after pulling the network header.


diff --git a/net/802/psnap.c b/net/802/psnap.c
index fca9d454905fe37d6b838f0f00b3a16767e44e74..389df460c8c4b92f9ec6198247db0ba15bfb8f2e
100644
--- a/net/802/psnap.c
+++ b/net/802/psnap.c
@@ -55,11 +55,11 @@ static int snap_rcv(struct sk_buff *skb, struct
net_device *dev,
                goto drop;

        rcu_read_lock();
-       proto = find_snap_client(skb_transport_header(skb));
+       proto = find_snap_client(skb->data);
        if (proto) {
                /* Pass the frame on. */
-               skb->transport_header += 5;
                skb_pull_rcsum(skb, 5);
+               skb_reset_transport_header(skb);
                rc = proto->rcvfunc(skb, dev, &snap_packet_type, orig_dev);
        }
        rcu_read_unlock();

Re: [PATCH net] net: 802: reset skb->transport_header

[Antonio Pastor]

> Sorry, this patch is wrong, it does not fix the potential issue yet.


No worries! Thanks for your patience with this. Much appreciated.


> Note how skb_transport_header(skb) is used in
> find_snap_client(skb_transport_header(skb));


I've spent so much time trying to figure out why the offset is wrong I 
lost sight that the core issue is that it is being used to begin with. 
Paolo Abeni hinted at that too.


> The proper way to fix the issue is to not rely on the transport header
> at all, only reset it after pulling the network header.
>
>
> diff --git a/net/802/psnap.c b/net/802/psnap.c
> index fca9d454905fe37d6b838f0f00b3a16767e44e74..389df460c8c4b92f9ec6198247db0ba15bfb8f2e
> 100644
> --- a/net/802/psnap.c
> +++ b/net/802/psnap.c
> @@ -55,11 +55,11 @@ static int snap_rcv(struct sk_buff *skb, struct
> net_device *dev,
>                  goto drop;
>
>          rcu_read_lock();
> -       proto = find_snap_client(skb_transport_header(skb));
> +       proto = find_snap_client(skb->data);
>          if (proto) {
>                  /* Pass the frame on. */
> -               skb->transport_header += 5;
>                  skb_pull_rcsum(skb, 5);
> +               skb_reset_transport_header(skb);
>                  rc = proto->rcvfunc(skb, dev, &snap_packet_type, orig_dev);
>          }
>          rcu_read_unlock();


Will send V2.

[PATCH net v2] net: 802: LLC+SNAP OID:PID lookup on start of skb data

[Antonio Pastor]

802.2+LLC+SNAP frames received by napi_complete_done() with GRO and DSA
have skb->transport_header set two bytes short, or pointing 2 bytes
before network_header & skb->data. This was an issue as snap_rcv()
expected offset to point to SNAP header (OID:PID), causing packet to
be dropped.

A fix at llc_fixup_skb() (a024e377efed) resets transport_header for any
LLC consumers that may care about it, and stops SNAP packets from being
dropped, but doesn't fix the problem which is that LLC and SNAP should
not use transport_header offset.

Ths patch eliminates the use of transport_header offset for SNAP lookup
of OID:PID so that SNAP does not rely on the offset at all.
The offset is reset after pull for any SNAP packet consumers that may
(but shouldn't) use it.

Fixes: fda55eca5a33 ("net: introduce skb_transport_header_was_set()")
Signed-off-by: Antonio Pastor <antonio.pastor@gmail.com>
---
 net/802/psnap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/802/psnap.c b/net/802/psnap.c
index fca9d454905f..389df460c8c4 100644
--- a/net/802/psnap.c
+++ b/net/802/psnap.c
@@ -55,11 +55,11 @@ static int snap_rcv(struct sk_buff *skb, struct net_device *dev,
 		goto drop;
 
 	rcu_read_lock();
-	proto = find_snap_client(skb_transport_header(skb));
+	proto = find_snap_client(skb->data);
 	if (proto) {
 		/* Pass the frame on. */
-		skb->transport_header += 5;
 		skb_pull_rcsum(skb, 5);
+		skb_reset_transport_header(skb);
 		rc = proto->rcvfunc(skb, dev, &snap_packet_type, orig_dev);
 	}
 	rcu_read_unlock();
-- 
2.43.0

Re: [PATCH net v2] net: 802: LLC+SNAP OID:PID lookup on start of skb data

[Eric Dumazet]

On Fri, Jan 3, 2025 at 2:23 AM Antonio Pastor <antonio.pastor@gmail.com> wrote:
>
> 802.2+LLC+SNAP frames received by napi_complete_done() with GRO and DSA
> have skb->transport_header set two bytes short, or pointing 2 bytes
> before network_header & skb->data. This was an issue as snap_rcv()
> expected offset to point to SNAP header (OID:PID), causing packet to
> be dropped.
>
> A fix at llc_fixup_skb() (a024e377efed) resets transport_header for any
> LLC consumers that may care about it, and stops SNAP packets from being
> dropped, but doesn't fix the problem which is that LLC and SNAP should
> not use transport_header offset.
>
> Ths patch eliminates the use of transport_header offset for SNAP lookup
> of OID:PID so that SNAP does not rely on the offset at all.
> The offset is reset after pull for any SNAP packet consumers that may
> (but shouldn't) use it.
>
> Fixes: fda55eca5a33 ("net: introduce skb_transport_header_was_set()")
> Signed-off-by: Antonio Pastor <antonio.pastor@gmail.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net v2] net: 802: LLC+SNAP OID:PID lookup on start of skb data

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu,  2 Jan 2025 20:23:00 -0500 you wrote:
> 802.2+LLC+SNAP frames received by napi_complete_done() with GRO and DSA
> have skb->transport_header set two bytes short, or pointing 2 bytes
> before network_header & skb->data. This was an issue as snap_rcv()
> expected offset to point to SNAP header (OID:PID), causing packet to
> be dropped.
> 
> A fix at llc_fixup_skb() (a024e377efed) resets transport_header for any
> LLC consumers that may care about it, and stops SNAP packets from being
> dropped, but doesn't fix the problem which is that LLC and SNAP should
> not use transport_header offset.
> 
> [...]

Here is the summary with links:
  - [net,v2] net: 802: LLC+SNAP OID:PID lookup on start of skb data
    https://git.kernel.org/netdev/net/c/1e9b0e1c550c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html