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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] bpf: incorrect multi-line comment style
--

commit fe908855753e03da84cf9e3447581e4350dd47f1
Author: ThisSeanZhang <thisseanzhang@gmail.com>

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

This commit adds a tc selftest that exercises the new PPPoE flags of
bpf_skb_adjust_room(). It verifies encapsulation and decapsulation behaviors
over IPv4 and IPv6, as well as multiple error conditions.

> 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..ee914e828d4db
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/tc_pppoe.c

[ ... ]

> +static void test_encap_decap(struct tc_pppoe *skel, const char *subtest,
> +			     const __u8 *pkt, __u32 pkt_len, int ethertype,
> +			     __u8 ppp_proto)
> +{

[ ... ]

> +	/* Ethernet header, with the ethertype changed to PPPoE session. */
> +	ASSERT_EQ(encap_pkt[12], 0x88, "encap eth h_proto");
> +	ASSERT_EQ(encap_pkt[13], 0x64, "encap eth h_proto");
> +	/* PPPoE session header: ver/type/code, session id, length and
> +	 * PPP protocol.
> +	 */

[Severity: Low]
This isn't a bug, but the BPF subsystem prefers the opening /* of multi-line
comments to be on its own line.

> +	ASSERT_EQ(encap_pkt[14], 0x11, "encap pppoe ver/type");
> +	ASSERT_EQ(encap_pkt[15], 0x00, "encap pppoe code");

[ ... ]

> +	ASSERT_EQ(encap_pkt[20], 0x00, "encap ppp proto");
> +	ASSERT_EQ(encap_pkt[21], ppp_proto, "encap ppp proto");
> +	/* The original packet must be shifted unchanged behind the
> +	 * new header.
> +	 */

[Severity: Low]
Could the opening /* be placed on its own line to match the BPF multi-line
comment style?

> +	ASSERT_MEMEQ(encap_pkt + 14 + PPP_SES_HLEN, pkt + 14,
> +		     pkt_len - 14, "encap payload");

[ ... ]

> +void test_tc_pppoe(void)
> +{
> +	/* A PPPoE packet whose PPP protocol is neither IPv4 nor IPv6:
> +	 * IP control protocol (0x8021) in this case.
> +	 */

[Severity: Low]
Does this follow the modern kernel multi-line comment style expected in the
BPF subsystem?

> +	static const __u8 bad_ppp_pkt[] = {
> +		0x11, 0x22, 0x33, 0x44, 0x55, 0x66,

[ ... ]

> +		0x00, 0x00, 0x00, 0x00,
> +	};
> +	/* A PPPoE packet whose payload is too short to still contain a
> +	 * full IP header after decapsulation.
> +	 */

[Severity: Low]
It looks like the opening /* is not on its own line here either.

> +	static const __u8 truncated_pkt[] = {
> +		0x11, 0x22, 0x33, 0x44, 0x55, 0x66,

[ ... ]

> +	test_decap_reject_input(skel, truncated_pkt, sizeof(truncated_pkt),
> +				"decap-truncated");
> +	test_reject(skel, 6, fake_ppp_pkt, sizeof(fake_ppp_pkt),
> +		    "reject-decap-fake-ppp-proto");
> +
> +	/* Encapsulate a v4 packet once more to get a PPPoE packet as
> +	 * input for the "decap without the flag" rejection case.
> +	 */

[Severity: Low]
This is a minor style observation, but multi-line comments in BPF tests
should start with /* on a separate line.

> +	ASSERT_OK(run_prog(bpf_program__fd(skel->progs.tc_pppoe_encap),
> +			   ip4_pkt, sizeof(ip4_pkt), encap_pkt,

[ ... ]

> 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 0000000000000..357e1b5706f7a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/tc_pppoe.c

[ ... ]

> +/* skb->protocol as observed after the helper call. */
> +int encap_proto;
> +int decap_proto;
> +
> +/* Set when tc_pppoe_reject observes a call that should have been
> + * rejected by the helper.
> + */

[Severity: Low]
This isn't a functional issue, but BPF style guidelines recommend placing the
opening /* of a multi-line comment on its own line.

> +int reject_unexpected;
> +
> +SEC("tc")
> +int tc_pppoe_encap(struct __sk_buff *skb)
> +{
> +	__u8 hdr[PPPOE_SES_HLEN_TEST] = {
> +		0x11, 0x00,			/* ver, type, code */
> +		0xde, 0xad,			/* session id */
> +		0x00, 0x00,			/* length, set below */
> +		0x00, 0x00,			/* PPP protocol, set below */
> +	};
> +	struct ethhdr eth;
> +	/* The PPPoE length field covers everything after the 6 byte
> +	 * session header: the PPP protocol field plus the payload.
> +	 */

[Severity: Low]
Could this comment be formatted to match the multi-line comment style used
in the BPF subsystem?

> +	__u16 plen = skb->len - ETH_HLEN_TEST + 2;
> +
> +	hdr[4] = (plen >> 8) & 0xff;

[ ... ]

> +SEC("tc")
> +int tc_pppoe_decap(struct __sk_buff *skb)
> +{
> +	struct ethhdr eth;

[ ... ]

> +	decap_proto = skb->protocol;
> +
> +	/* Restore the ethertype to the protocol of the decapsulated
> +	 * payload, as picked by the kernel from the PPP protocol field.
> +	 */

[Severity: Low]
Does this multi-line comment align with the BPF coding style?

> +	eth.h_proto = (__be16)skb->protocol;
> +	if (bpf_skb_store_bytes(skb, 0, &eth, sizeof(eth), 0))
> +		return TC_ACT_SHOT_TEST;
> +
> +	return TC_ACT_OK_TEST;
> +}
> +
> +/* Every bpf_skb_adjust_room() call below must be rejected by the
> + * helper; tc_pppoe_reject reports (and fails the test) if one of
> + * them unexpectedly succeeds.
> + */

[Severity: Low]
This is a minor style observation, but the opening /* here should ideally be
on its own line.

> +SEC("tc")
> +int tc_pppoe_reject(struct __sk_buff *skb)
> +{

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