Re: [RFC bpf-next v3 3/3] selftests/bpf: Add a test for the PPPoE encap/decap kfuncs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] selftests/bpf: single-word handle used in Signed-off-by trailer
- [Low] selftests/bpf: missing subtest isolation for tc_pppoe_fail in
  test_tc_pppoe()
- [Low] selftests/bpf: uninitialized reads on run_prog() error in
  test_encap_decap()
--

commit 0a18ebb6cc8079a048ec95f72882a33d91363974
Author: ThisSeanZhang <thisseanzhang@gmail.com>

> selftests/bpf: Add a test for the PPPoE encap/decap kfuncs
>
> Add a tc test that exercises the bpf_skb_pppoe_encap() and
> bpf_skb_pppoe_decap() kfuncs.
>
> The encap program is run over plain Ethernet/IPv4 and Ethernet/IPv6
> packets. The test checks that the kernel writes a correct PPPoE
> session header between the Ethernet and network headers (version,
> type, code, session id, length and PPP protocol), that skb->protocol
> becomes ETH_P_PPP_SES, and that the payload is preserved. It then
> runs the decap program and checks that the original packet and
> protocol metadata are restored.
>
> The rejection paths are driven by the packet input: encapsulation of
> a non-IP packet (ARP), decapsulation of a packet whose protocol is
> not ETH_P_PPP_SES (plain IPv4, and an IPv4 packet whose bytes at the
> PPP protocol offset decode as PPP_IP), decapsulation of a PPPoE
> packet with an unsupported PPP protocol (IPCP), and decapsulation of
> a packet too short to still hold an IP header.
>
> A separate program that holds a packet pointer across a call to
> bpf_skb_pppoe_encap() must fail verification, covering the
> KF_CHANGES_PKT invalidation from the first patch.
>
> Signed-off-by: ThisSeanZhang <thisseanzhang@gmail.com>

[Severity: High]
The Signed-off-by trailer, commit author, and file copyright headers use
the single-word handle "ThisSeanZhang".  Per
Documentation/process/submitting-patches.rst, the Developer's Certificate
of Origin requires a space-separated real full name.

> 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 0000000000000..5420d3bf321ac
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/tc_pppoe.c
[ ... ]
> +static int run_prog(int prog_fd, const void *data_in, __u32 size_in,
> +		    void *data_out, __u32 size_out, __u32 *retval,
> +		    __u32 *size_out_actual)
> +{
> +	LIBBPF_OPTS(bpf_test_run_opts, opts,
> +		    .data_in = (void *)data_in,
> +		    .data_size_in = size_in,
> +		    .data_out = data_out,
> +		    .data_size_out = size_out,
> +	);
> +	int ret;
> +
> +	ret = bpf_prog_test_run_opts(prog_fd, &opts);
> +	if (!ret && retval)
> +		*retval = opts.retval;
> +	if (!ret && size_out_actual)
> +		*size_out_actual = opts.data_size_out;
> +
> +	return ret;
> +}
> +
> +static void test_encap_decap(struct tc_pppoe *skel, const char *subtest,
> +			     const __u8 *pkt, __u32 pkt_len, int ethertype,
> +			     __u8 ppp_proto)
> +{
> +	__u8 encap_pkt[128];
> +	__u8 decap_pkt[128];
> +	__u32 retval, out_len;
> +	int ret;
> +
> +	if (!test__start_subtest(subtest))
> +		return;
> +
> +	skel->bss->encap_proto = 0;
> +	ret = run_prog(bpf_program__fd(skel->progs.tc_pppoe_encap),
> +		       pkt, pkt_len, encap_pkt, sizeof(encap_pkt),
> +		       &retval, &out_len);
> +	ASSERT_OK(ret, "encap test_run");
> +	ASSERT_OK(retval, "encap retval");
> +	ASSERT_EQ(out_len, pkt_len + PPPOE_SES_HLEN, "encap pkt len");

[Severity: Low]
Can test_encap_decap() read uninitialized stack memory if run_prog()
fails?

run_prog() only populates *retval and *size_out_actual when
bpf_prog_test_run_opts() returns 0, and ASSERT_OK(ret, ...) does not
return on failure.  If either run_prog() call fails, execution continues
to evaluate assertions on uninitialized retval, out_len, encap_pkt, and
decap_pkt (which can also trigger -Werror=maybe-uninitialized when
run_prog() is inlined).  Returning early when !ASSERT_OK(ret, ...) avoids
this.

[ ... ]

> +	/*
> +	 * KF_CHANGES_PKT must make the verifier reject a program that
> +	 * keeps a packet pointer across the call.
> +	 */
> +	fail_skel = tc_pppoe_fail__open_and_load();
> +	if (!ASSERT_ERR_PTR(fail_skel, "stale packet pointer rejected"))
> +		tc_pppoe_fail__destroy(fail_skel);
> +}

[Severity: Low]
Does calling tc_pppoe_fail__open_and_load() without starting a subtest
attach its result to the preceding subtest?

In test_progs, the "decap-non-pppoe-fake-proto" subtest stays active until
the next test__start_subtest() call or until test_tc_pppoe() returns, and
filtered runs (such as test_progs -t tc_pppoe/encap-decap-v4) will still
execute tc_pppoe_fail__open_and_load() outside any subtest.  Using
RUN_TESTS(tc_pppoe_fail) with __failure and __msg() annotations from
bpf_misc.h on tc_pppoe_stale_ptr(), or guarding this check with
test__start_subtest(), would isolate it properly and verify the expected
verifier error message.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111032141.525444-1-thisseanzhang@gmail.com?part=3