[RFC PATCH net 0/1] transport_header set incorrectly when using veth

[RFC PATCH net 0/1] transport_header set incorrectly when using veth

[sreedevi.joshi]

From: Sreedevi Joshi <sreedevi.joshi@intel.com>

When testing a use-case on veth by attaching XDP and tc ingress hooks, it was 
noticed that the transport_header is set incorrectly and causes the tc_ingress
hook that is using bpf_skb_change_tail() call to report a failure.

Here is the flow:
veth ingress:
veth_convert_skb_to_xdp_buff()- [Example: skb->trannsport_header=65535 skb->network_header=0]
..>skb_pp_cow_data()
....>skb_headers_offset_update() - adds offset without checking and this
		results in transport_header value roll over.
		[off: 192: results in  skb->transport_header = 191, skb->network_header=192]
tc_ingress hook: bpf_skb_change_tail()
  - Since transport_header < network_header, min_len is negative and it fails.

Two possbible solutions:
option 1: introducing the check in the skb_headers_offset_update() to skip adding offset 
	to transport_header when it is not set. (patch attached)
option 2: reset transport header in veth_xdp_rcv_skb()

Option 1 seems to be better as it will apply to any other interfaces that may
use skb_headers_offset_update and there seems to similar logic in the same function
to check if mac_header was set before adding offset.

Seeking your inputs on this.

NOTES:
1. If veth is used without XDP hook attached, this issue is not observed
as the logic uses __netif_rx() directly and the transport header is reset
in __netif_receive_skb_core() as it detects it is not set.

2. Tested on i40e driver and confirmed it does not have this issue as the
skb_headers_offset_update() is not in the processing path.


Instructions to reproduce the issue along with the XDP and tc ingress programs
is attached below.

-------------------------------8<-------------------------------
instructions:

#build XDP and tc programs
clang -O2 -g -target bpf -D__TARGET_ARCH_x86 -c xdp_prog.c -o xdp_prog.o
clang -O2 -g -target bpf -D__TARGET_ARCH_x86 -c tc_bpf_prog.c -o tc_bpf_prog.o

# create the veth pair
ip link add veth0 numtxqueues 1 numrxqueues 1 type veth peer name veth1 \
   numtxqueues 1 numrxqueues 1

ip addr add 10.0.1.0/24 dev veth0
ip addr add 10.0.1.1/24 dev veth1
ip link set veth0 address 02:00:00:00:00:00
ip link set veth1 address 02:00:00:00:00:01
ip link set veth0 up
ip link set veth1 up

if [ -f /proc/net/if_inet6 ]; then
    echo 1 > /proc/sys/net/ipv6/conf/veth0/disable_ipv6
    echo 1 > /proc/sys/net/ipv6/conf/veth1/disable_ipv6
fi

#attach xdp hook and tc ingress hooks to veth1
xdp-loader load veth1 xdp_prog.o

tc qdisc add dev veth1 clsact
tc filter add dev veth1 ingress bpf da obj tc_bpf_prog.o sec prog

# generate traffic from veth0 egress -> veth1 ingress
ping -c e 10.0.1.3 -I veth0

# observe the trace pipe (make sure tracing is on)
# The following prints will appear
# ping-5330    [072] ..s2. 18266.403464: bpf_trace_printk: Failure.. new len=52 ret=-22
cat /sys/kernel/debug/tracing/trace_pipe

-------------------------------8<-------------------------------
xdp_prog.c:

#include <linux/bpf.h>
#include <bpf/bpf_helpers.h>

SEC("xdp") int netd_xdp_prog(struct xdp_md *xdp)
{ 
        /* Squash compiler warning. */
        (void)xdp;

        return XDP_PASS;
}

char _license[] SEC("license") = "GPL";

-------------------------------8<-------------------------------
test_bpf_prog.c:

#include <linux/bpf.h>
#include <bpf/bpf_helpers.h>
#include <linux/pkt_cls.h>

SEC("prog") int netd_tc_test_ingress(struct __sk_buff *skb)
{       
        long ret;

        /* extend skb length by 10 */
        ret = bpf_skb_change_tail(skb, skb->len + 10, 0);
        if (ret < 0) {
                bpf_printk("Failure.. new len=%d ret=%d\n", skb->len+10, ret);
                return TC_ACT_SHOT;
        }

        bpf_printk("Success new len:%d \n", skb->len+10);

        return TC_ACT_UNSPEC;
}

char _license[] SEC("license") = "GPL";

-------------------------------8<-------------------------------

Sreedevi Joshi (1):
  net: check transport_header before adding offset

 net/core/skbuff.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.25.1

[RFC PATCH net 1/1] net: check transport_header before adding offset

[sreedevi.joshi]

From: Sreedevi Joshi <sreedevi.joshi@intel.com>

skb_headers_offset_update() adds offset to the transport_header
of skb without checking if it was set. When the transport header
is not set, it's value is 65535. Adding offset to this causes it to
roll over and makes the transport_header value to be less than
network_header.
When a tc ingress hook is attached and it invokes bpf_skb_change_tail()
(to strip off extra bytes at the end of packet or to attach some
extra bytes), the logic in __bpf_skb_change_tail() that calculates
the min_len fails due to the transport_header being incorrectly set.

This issue was discovered when testing with veth interface with both xdp and
tc ingress hooks are attached. veth_convert_skb_to_xdp_buff() calls
skb_pp_cow_data() and it results in this function being called. Since
transport_header is incremented without checking, it results in the condition
where transport_header < network_header. __netif_receive_skb_core() when it
receives this skb, skips reset of the transport header as it is already set.

This is specific to XDP path. When there is no XDP hook, the logic takes a
different route (__netif_rx()) and the reset of the transport header happens in
__netif_receive_skb_core() before it reaches tc ingress hook.

Fixes: f5b1729443fd ("net: Add skb_headers_offset_update helper function.")
Signed-off-by: Sreedevi Joshi <sreedevi.joshi@intel.com>
---
 net/core/skbuff.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index a441613a1e6c..79b10abd95f1 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2098,7 +2098,8 @@ void skb_headers_offset_update(struct sk_buff *skb, int off)
 	if (skb->ip_summed == CHECKSUM_PARTIAL)
 		skb->csum_start += off;
 	/* {transport,network,mac}_header and tail are relative to skb->head */
-	skb->transport_header += off;
+	if (skb_transport_header_was_set(skb))
+		skb->transport_header += off;
 	skb->network_header   += off;
 	if (skb_mac_header_was_set(skb))
 		skb->mac_header += off;
-- 
2.25.1

RE: [RFC PATCH net 0/1] transport_header set incorrectly when using veth

[Joshi, Sreedevi]

> -----Original Message-----
> From: sreedevi.joshi <joshisre@ecsmtp.an.intel.com>
> Sent: Thursday, February 6, 2025 1:06 PM
> To: edumazet@gmail.com; kuba@kernel.org; pabeni@redhat.com; horms@kernel.org; ast@kernel.org; daniel@iogearbox.net
> Cc: Karlsson, Magnus <magnus.karlsson@intel.com>; Fijalkowski, Maciej <maciej.fijalkowski@intel.com>; hawk@kernel.org;
> john.fastabend@gmail.com; almasrymina@google.com; asml.silence@gmail.com; lorenzo@kernel.org; Lobakin, Aleksander
> <aleksander.lobakin@intel.com>; chopps@labn.net; bigeasy@linutronix.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> bpf@vger.kernel.org; Joshi, Sreedevi <sreedevi.joshi@intel.com>
> Subject: [RFC PATCH net 0/1] transport_header set incorrectly when using veth
> 
> From: Sreedevi Joshi <sreedevi.joshi@intel.com>
> 
> When testing a use-case on veth by attaching XDP and tc ingress hooks, it was noticed that the transport_header is set incorrectly and
> causes the tc_ingress hook that is using bpf_skb_change_tail() call to report a failure.
> 
> Here is the flow:
> veth ingress:
> veth_convert_skb_to_xdp_buff()- [Example: skb->trannsport_header=65535 skb->network_header=0]
> ..>skb_pp_cow_data()
> ....>skb_headers_offset_update() - adds offset without checking and this
> 		results in transport_header value roll over.
> 		[off: 192: results in  skb->transport_header = 191, skb->network_header=192] tc_ingress hook: bpf_skb_change_tail()
>   - Since transport_header < network_header, min_len is negative and it fails.
> 
> Two possbible solutions:
> option 1: introducing the check in the skb_headers_offset_update() to skip adding offset
> 	to transport_header when it is not set. (patch attached) option 2: reset transport header in veth_xdp_rcv_skb()
> 
> Option 1 seems to be better as it will apply to any other interfaces that may use skb_headers_offset_update and there seems to
> similar logic in the same function to check if mac_header was set before adding offset.
> 
> Seeking your inputs on this.
> 
> NOTES:
> 1. If veth is used without XDP hook attached, this issue is not observed as the logic uses __netif_rx() directly and the transport header
> is reset in __netif_receive_skb_core() as it detects it is not set.
> 
> 2. Tested on i40e driver and confirmed it does not have this issue as the
> skb_headers_offset_update() is not in the processing path.
> 
> 
> Instructions to reproduce the issue along with the XDP and tc ingress programs is attached below.
> 
> -------------------------------8<-------------------------------
> instructions:
> 
> #build XDP and tc programs
> clang -O2 -g -target bpf -D__TARGET_ARCH_x86 -c xdp_prog.c -o xdp_prog.o clang -O2 -g -target bpf -D__TARGET_ARCH_x86 -c
> tc_bpf_prog.c -o tc_bpf_prog.o
> 
> # create the veth pair
> ip link add veth0 numtxqueues 1 numrxqueues 1 type veth peer name veth1 \
>    numtxqueues 1 numrxqueues 1
> 
> ip addr add 10.0.1.0/24 dev veth0
> ip addr add 10.0.1.1/24 dev veth1
> ip link set veth0 address 02:00:00:00:00:00 ip link set veth1 address 02:00:00:00:00:01 ip link set veth0 up ip link set veth1 up
> 
> if [ -f /proc/net/if_inet6 ]; then
>     echo 1 > /proc/sys/net/ipv6/conf/veth0/disable_ipv6
>     echo 1 > /proc/sys/net/ipv6/conf/veth1/disable_ipv6
> fi
> 
> #attach xdp hook and tc ingress hooks to veth1 xdp-loader load veth1 xdp_prog.o
> 
> tc qdisc add dev veth1 clsact
> tc filter add dev veth1 ingress bpf da obj tc_bpf_prog.o sec prog
> 
> # generate traffic from veth0 egress -> veth1 ingress ping -c e 10.0.1.3 -I veth0
> 
> # observe the trace pipe (make sure tracing is on) # The following prints will appear
> # ping-5330    [072] ..s2. 18266.403464: bpf_trace_printk: Failure.. new len=52 ret=-22
> cat /sys/kernel/debug/tracing/trace_pipe
> 
> -------------------------------8<-------------------------------
> xdp_prog.c:
> 
> #include <linux/bpf.h>
> #include <bpf/bpf_helpers.h>
> 
> SEC("xdp") int netd_xdp_prog(struct xdp_md *xdp) {
>         /* Squash compiler warning. */
>         (void)xdp;
> 
>         return XDP_PASS;
> }
> 
> char _license[] SEC("license") = "GPL";
> 
> -------------------------------8<-------------------------------
> test_bpf_prog.c:
> 
> #include <linux/bpf.h>
> #include <bpf/bpf_helpers.h>
> #include <linux/pkt_cls.h>
> 
> SEC("prog") int netd_tc_test_ingress(struct __sk_buff *skb)
> {
>         long ret;
> 
>         /* extend skb length by 10 */
>         ret = bpf_skb_change_tail(skb, skb->len + 10, 0);
>         if (ret < 0) {
>                 bpf_printk("Failure.. new len=%d ret=%d\n", skb->len+10, ret);
>                 return TC_ACT_SHOT;
>         }
> 
>         bpf_printk("Success new len:%d \n", skb->len+10);
> 
>         return TC_ACT_UNSPEC;
> }
> 
> char _license[] SEC("license") = "GPL";
> 
> -------------------------------8<-------------------------------
> 
> Sreedevi Joshi (1):
>   net: check transport_header before adding offset
> 
>  net/core/skbuff.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> --
> 2.25.1
[] 
Apologies for resending. Mail server had some issues earlier and didn't reach some recipients. 

RE: [RFC PATCH net 1/1] net: check transport_header before adding offset

[Joshi, Sreedevi]

> -----Original Message-----
> From: sreedevi.joshi <joshisre@ecsmtp.an.intel.com>
> Sent: Thursday, February 6, 2025 1:06 PM
> To: edumazet@gmail.com; kuba@kernel.org; pabeni@redhat.com; horms@kernel.org; ast@kernel.org; daniel@iogearbox.net
> Cc: Karlsson, Magnus <magnus.karlsson@intel.com>; Fijalkowski, Maciej <maciej.fijalkowski@intel.com>; hawk@kernel.org;
> john.fastabend@gmail.com; almasrymina@google.com; asml.silence@gmail.com; lorenzo@kernel.org; Lobakin, Aleksander
> <aleksander.lobakin@intel.com>; chopps@labn.net; bigeasy@linutronix.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> bpf@vger.kernel.org; Joshi, Sreedevi <sreedevi.joshi@intel.com>
> Subject: [RFC PATCH net 1/1] net: check transport_header before adding offset
> 
> From: Sreedevi Joshi <sreedevi.joshi@intel.com>
> 
> skb_headers_offset_update() adds offset to the transport_header
> of skb without checking if it was set. When the transport header
> is not set, it's value is 65535. Adding offset to this causes it to
> roll over and makes the transport_header value to be less than
> network_header.
> When a tc ingress hook is attached and it invokes bpf_skb_change_tail()
> (to strip off extra bytes at the end of packet or to attach some
> extra bytes), the logic in __bpf_skb_change_tail() that calculates
> the min_len fails due to the transport_header being incorrectly set.
> 
> This issue was discovered when testing with veth interface with both xdp and
> tc ingress hooks are attached. veth_convert_skb_to_xdp_buff() calls
> skb_pp_cow_data() and it results in this function being called. Since
> transport_header is incremented without checking, it results in the condition
> where transport_header < network_header. __netif_receive_skb_core() when it
> receives this skb, skips reset of the transport header as it is already set.
> 
> This is specific to XDP path. When there is no XDP hook, the logic takes a
> different route (__netif_rx()) and the reset of the transport header happens in
> __netif_receive_skb_core() before it reaches tc ingress hook.
> 
> Fixes: f5b1729443fd ("net: Add skb_headers_offset_update helper function.")
> Signed-off-by: Sreedevi Joshi <sreedevi.joshi@intel.com>
> ---
>  net/core/skbuff.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index a441613a1e6c..79b10abd95f1 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -2098,7 +2098,8 @@ void skb_headers_offset_update(struct sk_buff *skb, int off)
>  	if (skb->ip_summed == CHECKSUM_PARTIAL)
>  		skb->csum_start += off;
>  	/* {transport,network,mac}_header and tail are relative to skb->head */
> -	skb->transport_header += off;
> +	if (skb_transport_header_was_set(skb))
> +		skb->transport_header += off;
>  	skb->network_header   += off;
>  	if (skb_mac_header_was_set(skb))
>  		skb->mac_header += off;
> --
> 2.25.1

[] resending due to mail server issues.

RE: [RFC PATCH net 0/1] transport_header set incorrectly when using veth

[Joshi, Sreedevi]

> -----Original Message-----
> From: Joshi, Sreedevi
> Sent: Friday, February 7, 2025 9:28 AM
> To: sreedevi.joshi <joshisre@ecsmtp.an.intel.com>; edumazet@gmail.com; kuba@kernel.org; pabeni@redhat.com; horms@kernel.org;
> ast@kernel.org; daniel@iogearbox.net
> Cc: Karlsson, Magnus <magnus.karlsson@intel.com>; Fijalkowski, Maciej <maciej.fijalkowski@intel.com>; hawk@kernel.org;
> john.fastabend@gmail.com; almasrymina@google.com; asml.silence@gmail.com; lorenzo@kernel.org; Lobakin, Aleksander
> <aleksander.lobakin@intel.com>; chopps@labn.net; bigeasy@linutronix.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> bpf@vger.kernel.org
> Subject: RE: [RFC PATCH net 0/1] transport_header set incorrectly when using veth
> 
> 
> > -----Original Message-----
> > From: sreedevi.joshi <joshisre@ecsmtp.an.intel.com>
> > Sent: Thursday, February 6, 2025 1:06 PM
> > To: edumazet@gmail.com; kuba@kernel.org; pabeni@redhat.com; horms@kernel.org; ast@kernel.org; daniel@iogearbox.net
> > Cc: Karlsson, Magnus <magnus.karlsson@intel.com>; Fijalkowski, Maciej <maciej.fijalkowski@intel.com>; hawk@kernel.org;
> > john.fastabend@gmail.com; almasrymina@google.com; asml.silence@gmail.com; lorenzo@kernel.org; Lobakin, Aleksander
> > <aleksander.lobakin@intel.com>; chopps@labn.net; bigeasy@linutronix.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> > bpf@vger.kernel.org; Joshi, Sreedevi <sreedevi.joshi@intel.com>
> > Subject: [RFC PATCH net 0/1] transport_header set incorrectly when using veth
> >
> > From: Sreedevi Joshi <sreedevi.joshi@intel.com>
> >
> > When testing a use-case on veth by attaching XDP and tc ingress hooks, it was noticed that the transport_header is set incorrectly and
> > causes the tc_ingress hook that is using bpf_skb_change_tail() call to report a failure.
> >
> > Here is the flow:
> > veth ingress:
> > veth_convert_skb_to_xdp_buff()- [Example: skb->trannsport_header=65535 skb->network_header=0]
> > ..>skb_pp_cow_data()
> > ....>skb_headers_offset_update() - adds offset without checking and this
> > 		results in transport_header value roll over.
> > 		[off: 192: results in  skb->transport_header = 191, skb->network_header=192] tc_ingress hook: bpf_skb_change_tail()
> >   - Since transport_header < network_header, min_len is negative and it fails.
> >
> > Two possbible solutions:
> > option 1: introducing the check in the skb_headers_offset_update() to skip adding offset
> > 	to transport_header when it is not set. (patch attached) option 2: reset transport header in veth_xdp_rcv_skb()
> >
> > Option 1 seems to be better as it will apply to any other interfaces that may use skb_headers_offset_update and there seems to
> > similar logic in the same function to check if mac_header was set before adding offset.
> >
> > Seeking your inputs on this.
> >
> > NOTES:
> > 1. If veth is used without XDP hook attached, this issue is not observed as the logic uses __netif_rx() directly and the transport header
> > is reset in __netif_receive_skb_core() as it detects it is not set.
> >
> > 2. Tested on i40e driver and confirmed it does not have this issue as the
> > skb_headers_offset_update() is not in the processing path.
> >
> >
> > Instructions to reproduce the issue along with the XDP and tc ingress programs is attached below.
> >
> > -------------------------------8<-------------------------------
> > instructions:
> >
> > #build XDP and tc programs
> > clang -O2 -g -target bpf -D__TARGET_ARCH_x86 -c xdp_prog.c -o xdp_prog.o clang -O2 -g -target bpf -D__TARGET_ARCH_x86 -c
> > tc_bpf_prog.c -o tc_bpf_prog.o
> >
> > # create the veth pair
> > ip link add veth0 numtxqueues 1 numrxqueues 1 type veth peer name veth1 \
> >    numtxqueues 1 numrxqueues 1
> >
> > ip addr add 10.0.1.0/24 dev veth0
> > ip addr add 10.0.1.1/24 dev veth1
> > ip link set veth0 address 02:00:00:00:00:00 ip link set veth1 address 02:00:00:00:00:01 ip link set veth0 up ip link set veth1 up
> >
> > if [ -f /proc/net/if_inet6 ]; then
> >     echo 1 > /proc/sys/net/ipv6/conf/veth0/disable_ipv6
> >     echo 1 > /proc/sys/net/ipv6/conf/veth1/disable_ipv6
> > fi
> >
> > #attach xdp hook and tc ingress hooks to veth1 xdp-loader load veth1 xdp_prog.o
> >
> > tc qdisc add dev veth1 clsact
> > tc filter add dev veth1 ingress bpf da obj tc_bpf_prog.o sec prog
> >
> > # generate traffic from veth0 egress -> veth1 ingress ping -c e 10.0.1.3 -I veth0
> >
> > # observe the trace pipe (make sure tracing is on) # The following prints will appear
> > # ping-5330    [072] ..s2. 18266.403464: bpf_trace_printk: Failure.. new len=52 ret=-22
> > cat /sys/kernel/debug/tracing/trace_pipe
> >
> > -------------------------------8<-------------------------------
> > xdp_prog.c:
> >
> > #include <linux/bpf.h>
> > #include <bpf/bpf_helpers.h>
> >
> > SEC("xdp") int netd_xdp_prog(struct xdp_md *xdp) {
> >         /* Squash compiler warning. */
> >         (void)xdp;
> >
> >         return XDP_PASS;
> > }
> >
> > char _license[] SEC("license") = "GPL";
> >
> > -------------------------------8<-------------------------------
> > test_bpf_prog.c:
> >
> > #include <linux/bpf.h>
> > #include <bpf/bpf_helpers.h>
> > #include <linux/pkt_cls.h>
> >
> > SEC("prog") int netd_tc_test_ingress(struct __sk_buff *skb)
> > {
> >         long ret;
> >
> >         /* extend skb length by 10 */
> >         ret = bpf_skb_change_tail(skb, skb->len + 10, 0);
> >         if (ret < 0) {
> >                 bpf_printk("Failure.. new len=%d ret=%d\n", skb->len+10, ret);
> >                 return TC_ACT_SHOT;
> >         }
> >
> >         bpf_printk("Success new len:%d \n", skb->len+10);
> >
> >         return TC_ACT_UNSPEC;
> > }
> >
> > char _license[] SEC("license") = "GPL";
> >
> > -------------------------------8<-------------------------------
> >
> > Sreedevi Joshi (1):
> >   net: check transport_header before adding offset
> >
> >  net/core/skbuff.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > --
> > 2.25.1
> []
> Apologies for resending. Mail server had some issues earlier and didn't reach some recipients.

The following commit that was introduced in 6.12.8 addresses this issue. We no longer
need to follow up on this RFC.

Thanks for your time!
Sreedevi

commit 9ecc4d858b92c1bb0673ad9c327298e600c55659
Author: Cong Wang <cong.wang@bytedance.com>
Date:   Thu Dec 12 19:40:54 2024 -0800

    bpf: Check negative offsets in __bpf_skb_min_len()

    skb_network_offset() and skb_transport_offset() can be negative when
    they are called after we pull the transport header, for example, when
    we use eBPF sockmap at the point of ->sk_data_ready().

    __bpf_skb_min_len() uses an unsigned int to get these offsets, this
    leads to a very large number which then causes bpf_skb_change_tail()
    failed unexpectedly.

    Fix this by using a signed int to get these offsets and ensure the
    minimum is at least zero.

    Fixes: 5293efe62df8 ("bpf: add bpf_skb_change_tail helper")
    Signed-off-by: Cong Wang <cong.wang@bytedance.com>
    Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
    Acked-by: John Fastabend <john.fastabend@gmail.com>
    Link: https://lore.kernel.org/bpf/20241213034057.246437-2-xiyou.wangcong@gmail.com