Re: [RFC bpf-next v2 3/3] selftests/bpf: Add a test for the PPPoE encap/decap adjust_room flags

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 6141 bytes --]

> selftests/bpf: Add a test for the PPPoE encap/decap adjust_room flags

TL;DR:
- Test issue: reject-flag-mix and reject-encap-shrink pass even without the
  new ENCAP_PPPOE checks
- No IPv6 truncated-decap subtest, so the inner_len check goes untested

> diff --git a/tools/testing/selftests/bpf/prog_tests/tc_pppoe.c b/tools/testing/selftests/bpf/prog_tests/tc_pppoe.c
> new file mode 100644
> index 000000000000..73e602c91230
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/tc_pppoe.c
> @@ -0,0 +1,254 @@

[ ... ]

> +void test_tc_pppoe(void)
> +{

[ ... ]

> +	/*
> +	 * A PPPoE packet whose payload is too short to still contain a
> +	 * full IP header after decapsulation.
> +	 */
> +	static const __u8 truncated_pkt[] = {
> +		0x11, 0x22, 0x33, 0x44, 0x55, 0x66,
> +		0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff,
> +		0x88, 0x64,
> +		0x11, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0x21,
> +		0x45, 0x00,
> +	};

[ ... ]

> +	test_decap_reject_input(skel, bad_ppp_pkt, sizeof(bad_ppp_pkt),
> +				"decap-bad-ppp-proto");
> +	test_decap_reject_input(skel, truncated_pkt, sizeof(truncated_pkt),
> +				"decap-truncated");

Does decap-truncated reach the new "full inner L3 header" check in
bpf_skb_net_shrink()?

truncated_pkt is 24 bytes, so in bpf_skb_adjust_room() the generic length
check fires first:

    len_cur = 24 - 14 = 10
    len_cur - len_diff_abs = 2, which is < len_min (sizeof(struct iphdr))

and the call returns -ENOTSUPP before bpf_skb_net_shrink() runs.

The check added by the decap patch:

    /* A full inner L3 header must remain after decapsulation. */
    if (skb->len - off - PPPOE_SES_HLEN < inner_len)
        return -EINVAL;

is redundant for PPP_IP here, because inner_len == len_min == 20.  It is the
only guard for PPP_IPV6, where inner_len is 40, and no subtest feeds a
PPPoE/IPv6 packet with 20-39 bytes after the PPP header.

If that check were removed or broken, an skb could be marked ETH_P_IPV6
without a full IPv6 header, and none of the subtests would notice.

Would it make sense to add an IPv6 truncated case, for example PPP protocol
0x0057 with 20 bytes of payload, so the inner_len check is exercised?

> +	test_reject(skel, 6, fake_ppp_pkt, sizeof(fake_ppp_pkt),
> +		    "reject-decap-fake-ppp-proto");

[ ... ]

> +	test_reject(skel, 1, ip4_pkt, sizeof(ip4_pkt), "reject-encap-len");
> +	test_reject(skel, 2, ip4_pkt, sizeof(ip4_pkt), "reject-encap-mode");
> +	test_reject(skel, 3, ip4_pkt, sizeof(ip4_pkt), "reject-encap-shrink");
> +	test_reject(skel, 4, ip4_pkt, sizeof(ip4_pkt), "reject-flag-mix");

Can reject-flag-mix or reject-encap-shrink fail on a kernel that lacks the
new ENCAP_PPPOE checks?

For reject-flag-mix, case 4 passes ENCAP_PPPOE | DECAP_PPPOE on ip4_pkt, so
skb->protocol is ETH_P_IP.  bpf_skb_adjust_room() returns early at the
decap protocol gate:

    if (unlikely(flags & BPF_F_ADJ_ROOM_DECAP_PPPOE)) {
        if (proto != htons(ETH_P_PPP_SES))
            return -ENOTSUPP;

which is before the ENCAP_PPPOE flag combination check is reached.  Even with
a PPPoE input, a grow with any decap flag is already rejected by the existing
"if (!shrink) return -EINVAL;" in the BPF_F_ADJ_ROOM_DECAP_MASK block.

For reject-encap-shrink, without the "shrink ||" clause in the ENCAP_PPPOE
check, bpf_skb_net_shrink() would still return -EINVAL, because
BPF_F_ADJ_ROOM_ENCAP_PPPOE is outside its allowed flag mask.

tc_pppoe_reject only records !ret, so both subtests pass whether or not the
new checks exist.  A negative test should give a different result on a
kernel that is missing the check it targets.

Should case 4 use ENCAP_PPPOE together with another encap flag, for example
BPF_F_ADJ_ROOM_ENCAP_L3_IPV4 or BPF_F_ADJ_ROOM_ENCAP_L2_ETH, so the new
rejection is actually exercised?

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/tc_pppoe.c b/tools/testing/selftests/bpf/progs/tc_pppoe.c
> new file mode 100644
> index 000000000000..b1624f0f9d8a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/tc_pppoe.c
> @@ -0,0 +1,163 @@

[ ... ]

> +SEC("tc")
> +int tc_pppoe_reject(struct __sk_buff *skb)
> +{
> +	int ret = 0;
> +
> +	switch (reject_case) {

[ ... ]

> +	case 4:
> +		/* Encap flag combined with a decap flag. */
> +		ret = bpf_skb_adjust_room(skb, PPPOE_SES_HLEN_TEST,
> +					  BPF_ADJ_ROOM_MAC,
> +					  BPF_F_ADJ_ROOM_ENCAP_PPPOE |
> +					  BPF_F_ADJ_ROOM_DECAP_PPPOE);
> +		break;

Does this case reach the ENCAP_PPPOE flag combination check it is meant to
exercise?

prog_tests/tc_pppoe.c runs it as test_reject(skel, 4, ip4_pkt, ...), so
eth_type_trans() in bpf_prog_test_run_skb() sets skb->protocol to
htons(ETH_P_IP).  Because BPF_F_ADJ_ROOM_DECAP_PPPOE is set,
bpf_skb_adjust_room() returns -ENOTSUPP at the protocol check, before the
new validation is reached:

    flags & ((BPF_F_ADJ_ROOM_ENCAP_MASK |
              BPF_F_ADJ_ROOM_DECAP_MASK) &
             ~BPF_F_ADJ_ROOM_ENCAP_PPPOE))
        return -EINVAL;

The call is rejected for the same reason as case 6 (reject-decap-non-pppoe
on the same ip4_pkt), so this subtest adds no coverage.  With a PPPoE input,
a positive len_diff combined with any decap flag would still be caught by
the existing "if (!shrink) return -EINVAL;" in the DECAP_MASK block.

The combination that only the new check rejects is not tested at all:
ENCAP_PPPOE together with another encap flag, for example:

    bpf_skb_adjust_room(skb, PPPOE_SES_HLEN_TEST, BPF_ADJ_ROOM_MAC,
                        BPF_F_ADJ_ROOM_ENCAP_PPPOE |
                        BPF_F_ADJ_ROOM_ENCAP_L3_IPV4);

If that check were missing, this call would go into bpf_skb_net_grow() with
encap == true and succeed, and reject_unexpected would catch it.  The test
already builds fake_ppp_pkt so that case 6 can fail on a kernel without the
protocol check.

Could case 4 use a second encap flag in the same way, so that it also fails
on a kernel without the check it targets?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/37148983810