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

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();

> 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.

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

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>