> -----Original Message-----
> From: sreedevi.joshi <joshisre@xxxxxxxxxxxxxxxxxxx>
> Sent: Thursday, February 6, 2025 1:06 PM
> To: edumazet@xxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; horms@xxxxxxxxxx; ast@xxxxxxxxxx; daniel@xxxxxxxxxxxxx
> Cc: Karlsson, Magnus <magnus.karlsson@xxxxxxxxx>; Fijalkowski, Maciej <maciej.fijalkowski@xxxxxxxxx>; hawk@xxxxxxxxxx;
> john.fastabend@xxxxxxxxx; almasrymina@xxxxxxxxxx; asml.silence@xxxxxxxxx; lorenzo@xxxxxxxxxx; Lobakin, Aleksander
> <aleksander.lobakin@xxxxxxxxx>; chopps@xxxxxxxx; bigeasy@xxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> bpf@xxxxxxxxxxxxxxx; Joshi, Sreedevi <sreedevi.joshi@xxxxxxxxx>
> Subject: [RFC PATCH net 0/1] transport_header set incorrectly when using veth
>
> From: Sreedevi Joshi <sreedevi.joshi@xxxxxxxxx>
>
> 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.
