diff mbox series

[net] net: xdp: pull ethernet header off packet after computing skb->protocol

Message ID 20200815072930.4564-1-Jason@zx2c4.com
State New
Headers show
Series [net] net: xdp: pull ethernet header off packet after computing skb->protocol | expand

Commit Message

Jason A. Donenfeld Aug. 15, 2020, 7:29 a.m. UTC 
When an XDP program changes the ethernet header protocol field,
eth_type_trans is used to recalculate skb->protocol. In order for
eth_type_trans to work correctly, the ethernet header must actually be
part of the skb data segment, so the code first pushes that onto the
head of the skb. However, it subsequently forgets to pull it back off,
making the behavior of the passed-on packet inconsistent between the
protocol modifying case and the static protocol case. This patch fixes
the issue by simply pulling the ethernet header back off of the skb
head.

Fixes: 297249569932 ("net: fix generic XDP to handle if eth header was mangled")
Cc: Jesper Dangaard Brouer <brouer@redhat.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

---
 net/core/dev.c | 1 +
 1 file changed, 1 insertion(+)

-- 
2.28.0

Comments

David Miller Aug. 16, 2020, 10:29 p.m. UTC | #1 
From: "Jason A. Donenfeld" <Jason@zx2c4.com>

Date: Sat, 15 Aug 2020 09:29:30 +0200

> When an XDP program changes the ethernet header protocol field,

> eth_type_trans is used to recalculate skb->protocol. In order for

> eth_type_trans to work correctly, the ethernet header must actually be

> part of the skb data segment, so the code first pushes that onto the

> head of the skb. However, it subsequently forgets to pull it back off,

> making the behavior of the passed-on packet inconsistent between the

> protocol modifying case and the static protocol case. This patch fixes

> the issue by simply pulling the ethernet header back off of the skb

> head.

> 

> Fixes: 297249569932 ("net: fix generic XDP to handle if eth header was mangled")

> Cc: Jesper Dangaard Brouer <brouer@redhat.com>

> Cc: David S. Miller <davem@davemloft.net>

> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>


Applied and queued up for -stable, thanks.

Jesper, I wonder how your original patch was tested because it pushes a packet
with skb->data pointing at the ethernet header into the stack.  That should be
popped at this point as per this fix here.

Thanks.
Jesper Dangaard Brouer Aug. 17, 2020, 6:01 a.m. UTC | #2 
On Sun, 16 Aug 2020 15:29:37 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: "Jason A. Donenfeld" <Jason@zx2c4.com>

> Date: Sat, 15 Aug 2020 09:29:30 +0200

> 

> > When an XDP program changes the ethernet header protocol field,

> > eth_type_trans is used to recalculate skb->protocol. In order for

> > eth_type_trans to work correctly, the ethernet header must actually be

> > part of the skb data segment, so the code first pushes that onto the

> > head of the skb. However, it subsequently forgets to pull it back off,

> > making the behavior of the passed-on packet inconsistent between the

> > protocol modifying case and the static protocol case. This patch fixes

> > the issue by simply pulling the ethernet header back off of the skb

> > head.

> > 

> > Fixes: 297249569932 ("net: fix generic XDP to handle if eth header was mangled")

> > Cc: Jesper Dangaard Brouer <brouer@redhat.com>

> > Cc: David S. Miller <davem@davemloft.net>

> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>  

> 

> Applied and queued up for -stable, thanks.

> 

> Jesper, I wonder how your original patch was tested because it pushes a packet

> with skb->data pointing at the ethernet header into the stack.  That should be

> popped at this point as per this fix here.


I think this patch is wrong, because eth_type_trans() also does a
skb_pull_inline(skb, ETH_HLEN).

To Jason, are you sure about this fix?
How did you test this?


I usually test with:

 $ cd tools/testing/selftests/bpf/
 $ sudo ./test_xdp_vlan_mode_generic.sh 

But currently I get a build error on net-next in:
 net-next/tools/testing/selftests/bpf/tools/build/bpftool/pid_iter.bpf.o
So, I could not run this test.

- - 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

On Sat, 15 Aug 2020 09:29:30 +0200
"Jason A. Donenfeld" <Jason@zx2c4.com> wrote:

> diff --git a/net/core/dev.c b/net/core/dev.c

> index 7df6c9617321..151f1651439f 100644

> --- a/net/core/dev.c

> +++ b/net/core/dev.c

> @@ -4676,6 +4676,7 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,

>  	    (orig_bcast != is_multicast_ether_addr_64bits(eth->h_dest))) {

>  		__skb_push(skb, ETH_HLEN);

>  		skb->protocol = eth_type_trans(skb, skb->dev);

> +		__skb_pull(skb, ETH_HLEN);

>  	}

>  

>  	switch (act) {
Jason A. Donenfeld Aug. 17, 2020, 7:48 a.m. UTC | #3 
On 8/17/20, Jesper Dangaard Brouer <brouer@redhat.com> wrote:
> On Sun, 16 Aug 2020 15:29:37 -0700 (PDT)

> David Miller <davem@davemloft.net> wrote:

>

>> From: "Jason A. Donenfeld" <Jason@zx2c4.com>

>> Date: Sat, 15 Aug 2020 09:29:30 +0200

>>

>> > When an XDP program changes the ethernet header protocol field,

>> > eth_type_trans is used to recalculate skb->protocol. In order for

>> > eth_type_trans to work correctly, the ethernet header must actually be

>> > part of the skb data segment, so the code first pushes that onto the

>> > head of the skb. However, it subsequently forgets to pull it back off,

>> > making the behavior of the passed-on packet inconsistent between the

>> > protocol modifying case and the static protocol case. This patch fixes

>> > the issue by simply pulling the ethernet header back off of the skb

>> > head.

>> >

>> > Fixes: 297249569932 ("net: fix generic XDP to handle if eth header was

>> > mangled")

>> > Cc: Jesper Dangaard Brouer <brouer@redhat.com>

>> > Cc: David S. Miller <davem@davemloft.net>

>> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

>>

>> Applied and queued up for -stable, thanks.

>>

>> Jesper, I wonder how your original patch was tested because it pushes a

>> packet

>> with skb->data pointing at the ethernet header into the stack.  That

>> should be

>> popped at this point as per this fix here.

>

> I think this patch is wrong, because eth_type_trans() also does a

> skb_pull_inline(skb, ETH_HLEN).


Huh, wow. That's one unusual and confusing function. But indeed it
seems like I'm the one who needs to reevaluate testing methodology
here. I'm very sorry for the noise and hassle.

Jason
David Miller Aug. 17, 2020, 6:48 p.m. UTC | #4 
From: "Jason A. Donenfeld" <Jason@zx2c4.com>

Date: Mon, 17 Aug 2020 09:48:10 +0200

> On 8/17/20, Jesper Dangaard Brouer <brouer@redhat.com> wrote:

>> On Sun, 16 Aug 2020 15:29:37 -0700 (PDT)

>> David Miller <davem@davemloft.net> wrote:

>>

>>> From: "Jason A. Donenfeld" <Jason@zx2c4.com>

>>> Date: Sat, 15 Aug 2020 09:29:30 +0200

>>>

>>> > When an XDP program changes the ethernet header protocol field,

>>> > eth_type_trans is used to recalculate skb->protocol. In order for

>>> > eth_type_trans to work correctly, the ethernet header must actually be

>>> > part of the skb data segment, so the code first pushes that onto the

>>> > head of the skb. However, it subsequently forgets to pull it back off,

>>> > making the behavior of the passed-on packet inconsistent between the

>>> > protocol modifying case and the static protocol case. This patch fixes

>>> > the issue by simply pulling the ethernet header back off of the skb

>>> > head.

>>> >

>>> > Fixes: 297249569932 ("net: fix generic XDP to handle if eth header was

>>> > mangled")

>>> > Cc: Jesper Dangaard Brouer <brouer@redhat.com>

>>> > Cc: David S. Miller <davem@davemloft.net>

>>> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

>>>

>>> Applied and queued up for -stable, thanks.

>>>

>>> Jesper, I wonder how your original patch was tested because it pushes a

>>> packet

>>> with skb->data pointing at the ethernet header into the stack.  That

>>> should be

>>> popped at this point as per this fix here.

>>

>> I think this patch is wrong, because eth_type_trans() also does a

>> skb_pull_inline(skb, ETH_HLEN).

> 

> Huh, wow. That's one unusual and confusing function. But indeed it

> seems like I'm the one who needs to reevaluate testing methodology

> here. I'm very sorry for the noise and hassle.


I've reverted this change from my tree.
diff mbox series

Patch

diff --git a/net/core/dev.c b/net/core/dev.c
index 7df6c9617321..151f1651439f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4676,6 +4676,7 @@  static u32 netif_receive_generic_xdp(struct sk_buff *skb,
 	    (orig_bcast != is_multicast_ether_addr_64bits(eth->h_dest))) {
 		__skb_push(skb, ETH_HLEN);
 		skb->protocol = eth_type_trans(skb, skb->dev);
+		__skb_pull(skb, ETH_HLEN);
 	}
 
 	switch (act) {