Message-Id: <20250206180551.1716413-1-sreedevi.joshi@intel.com>
Date: Thu,  6 Feb 2025 12:05:50 -0600
From: "sreedevi.joshi" <joshisre@...mtp.an.intel.com>
To: edumazet@...il.com,
	kuba@...nel.org,
	pabeni@...hat.com,
	horms@...nel.org,
	ast@...nel.org,
	daniel@...earbox.net
Cc: magnus.karlsson@...el.com,
	maciej.fijalkowski@...el.com,
	hawk@...nel.org,
	john.fastabend@...il.com,
	almasrymina@...gle.com,
	asml.silence@...il.com,
	lorenzo@...nel.org,
	aleksander.lobakin@...el.com,
	chopps@...n.net,
	bigeasy@...utronix.de,
	netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	bpf@...r.kernel.org,
	Sreedevi Joshi <sreedevi.joshi@...el.com>
Subject: [RFC PATCH net 0/1] transport_header set incorrectly when using veth

From: Sreedevi Joshi <sreedevi.joshi@...el.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

Powered by blists - more mailing lists