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
