Re: [PATCH bpf-next 7/7] selftests/bpf: Add tests for BPF ethtool hooks

[Anton Protopopov <a.s.protopopov@gmail.com>]

On 26/08/31 12:07PM, bot+bpf-ci@kernel.org wrote:
> >     This is how the list off sub-tests maps to call sites:
> >
> >       linkstate_get_doit:    netlink.c (ethnl_default_doit)
> >       linkstate_get_dump:    netlink.c (ethnl_default_dump_one)
> >       cable_test_act:        cabletest.c (ethnl_act_cable_test)
> >       cable_test_tdr_act:    cabletest.c (ethnl_act_cable_test_tdr)
> >       features_set:          features.c (ethnl_set_features)
> >       module_fw_flash_act:   module.c (ethnl_act_module_fw_flash)
> >       tunnel_info_get_doit:  tunnels.c (ethnl_tunnel_info_doit)
> >       tunnel_info_get_dump:  tunnels.c (ethnl_tunnel_info_dumpit)
> >       tsinfo_get_dump:       tsinfo.c (ethnl_tsinfo_dump_one_netdev)
> >       rss_get_dump:          rss.c (ethnl_rss_dumpit)
> >       channels_set_doit:     netlink.c (ethnl_default_set_doit)
> >       ioctl:                 ioctl.c (ethtool_bpf_ioctl_hook)
> >       ioctl_sub_cmd:         ioctl.c (ethtool_bpf_ioctl_hook)
> >       no tests:             rss.c (ethnl_rss_{create,delete}_doit)
> 
> The changelog's sub-tests table does not match the sub-tests the patch
> adds. The code registers 15 sub-tests, including cable_test_phy_index
> (check_doit with TEST_PHY_INDEX, exercising the phy_index argument of
> the doit hook) and strset_get_phy_index_dump (check_dump with
> TEST_PHY_INDEX, exercising the phy_index argument of the dump hook),
> neither of which appears in the 13-row table.
> 
> These are the only two sub-tests that cover the phy_index parameter at
> all, i.e. the least obvious part of the new hook ABI, so their absence
> from the mapping is the part a reviewer is most likely to want
> documented.
> 
> Two smaller inconsistencies in the same block: the prose names
> ethnl_tsinfo_dump_one_phydev() as unreachable via netdevsim, but the
> 'no tests' row lists only rss.c (ethnl_rss_{create,delete}_doit); and
> 'the list off sub-tests' should read 'the list of sub-tests'.

Yes, thanks, I haven't updated the commit message after the last changes.

> > diff --git a/tools/testing/selftests/bpf/prog_tests/ethtool_lsm.c b/tools/testing/selftests/bpf/prog_tests/ethtool_lsm.c
> > new file mode 100644
> > index 0000000000000..f4662fb1ae3ce
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/ethtool_lsm.c
> > @@ -0,0 +1,330 @@
> 
> [ ... ]
> 
> > +static int ethnl_request(int fd, __u16 family_id, __u8 cmd, __u16 hdr_attr,
> > +			 __u16 extra_nest, __u32 ifindex, __u32 phy_index, bool dump)
> > +{
> > +	static __u32 sequence = 10;
> > +	struct genl_req req = {};
> > +	__u32 seq = sequence++;
> > +	struct rtattr *nest;
> > +	int err;
> > +
> > +	req.nlh.nlmsg_len = NLMSG_LENGTH(GENL_HDRLEN);
> > +	req.nlh.nlmsg_type = family_id;
> > +	req.nlh.nlmsg_flags = NLM_F_REQUEST | (dump ? NLM_F_DUMP : 0);
> > +	req.nlh.nlmsg_seq = seq;
> > +	req.genl.cmd = cmd;
> > +	req.genl.version = ETHTOOL_GENL_VERSION;
> > +
> > +	nest = addattr_nest(&req.nlh, sizeof(req), hdr_attr | NLA_F_NESTED);
> > +	if (ifindex && addattr32(&req.nlh, sizeof(req), ETHTOOL_A_HEADER_DEV_INDEX, ifindex))
> > +		return -EMSGSIZE;
> > +	if (phy_index && addattr32(&req.nlh, sizeof(req), ETHTOOL_A_HEADER_PHY_INDEX, phy_index))
> > +		return -EMSGSIZE;
> > +	if (addattr32(&req.nlh, sizeof(req), ETHTOOL_A_HEADER_FLAGS, ETHTOOL_FLAG_COMPACT_BITSETS))
> > +		return -EMSGSIZE;
> > +	addattr_nest_end(&req.nlh, nest);
> > +
> > +	if (extra_nest) {
> > +		nest = addattr_nest(&req.nlh, sizeof(req), extra_nest | NLA_F_NESTED);
> > +		addattr_nest_end(&req.nlh, nest);
> > +	}
> > +
> > +	err = genl_send(fd, &req.nlh);
> > +	if (err)
> > +		return err;
> > +
> > +	return genl_recv(fd, seq, family_id, dump);
> > +}
> 
> ethnl_request() never sets NLM_F_ACK, so a doit request that succeeds
> without producing a reply message gets no netlink response at all:
> netlink_rcv_skb() only calls netlink_ack() when
> 'nlh->nlmsg_flags & NLM_F_ACK || err'.
> 
> This is exactly what happens in the allow arm of the channels_set_doit
> subtest. ETHTOOL_MSG_CHANNELS_SET is dispatched through
> ethnl_default_set_doit(); ethnl_set_channels_validate() returns 1 on
> netdevsim (both get_channels and set_channels are implemented), so the
> hook is reached and the deny arm works, but the request carries only
> the header nest, so ethnl_set_channels() takes 'if (!mod) return 0;'
> and the SET handler produces neither a reply nor an error.
> 
> genl_recv() then blocks until the SO_RCVTIMEO set in genl_open()
> (struct timeval { .tv_sec = 1 }) expires and returns -EAGAIN.
> Consequences: (a) 'if (!ASSERT_NEQ(err, -TEST_ERRNO, "doit (allow)"))'
> passes vacuously - -EAGAIN would also be returned if the kernel had
> wrongly dropped the request, so the allow arm of this subtest pins
> nothing; (b) the subtest silently costs a full second of wall time, and
> would grow with each additional SET-style subtest added later.
> 
> Setting NLM_F_ACK in nlmsg_flags (rtnl_talk() in netlink_helpers.c
> does this itself when no answer is requested) makes the kernel ack the
> successful set, so the allow arm returns 0 immediately and actually
> distinguishes success from 'nothing came back'.

Yes, thanks.

> [ ... ]
> 
> > diff --git a/tools/testing/selftests/bpf/progs/ethtool_lsm.c b/tools/testing/selftests/bpf/progs/ethtool_lsm.c
> > new file mode 100644
> > index 0000000000000..c6b21aeed6fec
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/ethtool_lsm.c
> > @@ -0,0 +1,168 @@
> 
> [ ... ]
> 
> > +/*
> > + * This test is used for all hooks, just checks that it is ours,
> > + * and allows/denies based on the "allow" global variable
> > + */
> > +static int test_policy(const struct net_device *dev, __u32 cmd, __u32 sub_cmd, __u32 phy_index)
> > +{
> > +	__u32 pid;
> > +
> > +	pid = bpf_get_current_pid_tgid() >> 32;
> > +	if (pid != monitored_pid)
> > +		return 0;
> > +
> > +	if (!dev || (target_ifindex && dev->ifindex != target_ifindex))
> > +		return 0;
> > +
> > +	if (target_cmd && cmd != target_cmd)
> > +		return 0;
> > +
> > +	if (target_sub_cmd && sub_cmd != target_sub_cmd)
> > +		return 0;
> > +
> > +	if (phy_index != target_phy_index)
> > +		return -EDOTDOT;
> > +
> > +	return allow ? 0 : -EDOTDOT; /* unlikely to see this errno outside this test */
> > +}
> 
> The comment on test_policy() says the helper 'just checks that it is
> ours, and allows/denies based on the "allow" global variable', but the
> function has a third behaviour the comment does not mention: the
> phy_index test returns -EDOTDOT unconditionally, i.e. it denies
> regardless of the value of allow.
> 
> That is not a cosmetic difference: it is the only place where the deny
> errno is produced independently of allow, and it is precisely the
> mechanism the phy_index sub-tests (cable_test_phy_index,
> strset_get_phy_index_dump in prog_tests/ethtool_lsm.c) rely on - a
> mismatched phy_index makes the allow phase return -EDOTDOT and
> therefore fail ASSERT_NEQ(err, -TEST_ERRNO, "doit (allow)").
> 
> Note also that every other filter in the function bails out with
> 'return 0' (allow) when the request does not match, while this one
> bails out with a denial, so the asymmetry is easy to misread as a
> copy/paste slip.
> 
> A reader who trusts the comment will assume allow == true always means
> 'permit', and a later change to the phy_index handling can silently
> turn the phy_index sub-tests into no-ops. Suggest documenting that the
> phy_index comparison is an assertion that the hook argument was
> propagated correctly (or gating it on target_phy_index like the other
> filters and asserting the value from user space instead).

Yes, thanks. I haven't updated the comment.
I will see what looks better, to refactor code or comment.

> [ ... ]
> 
> > +/*
> > + * CVE-2025-21701 is an example of a bug which must be mitigated under a lock,
> > + * as access to dev->reg_state must be protected.
> > + */
> > +SEC("lsm/ethtool_netlink_doit")
> > +int BPF_PROG(cve_2025_21701, const struct net_device *dev, __u32 cmd, __u32 phy_index, int ret)
> > +{
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!dev)
> > +		return 0;
> > +
> > +	return dev->reg_state >= NETREG_UNREGISTERING ?  -ENODEV : 0;
> > +}
> 
> The example policy for CVE-2025-21701 installs only an
> lsm/ethtool_netlink_doit program, but the hook-adding commit in this
> same series (36bc9190590e4) classifies CVE-2025-21701 under 'more than
> one hook required', in contrast to CVE-2024-46834 which is given both
> an ioctl and a doit program a few lines below.
> 
> The upstream fix for that CVE (12e070eb6964b, 'net: avoid race between
> device unregistration and ethnl ops') widened the test in
> ethnl_ops_begin() to 'dev->reg_state >= NETREG_UNREGISTERING', and
> ethnl_ops_begin() is reached from the dump path as well as from doit:
> 
> net/ethtool/netlink.c:ethnl_default_dump_one()
>   -> ctx->ops->prepare_data()
>     -> net/ethtool/channels.c:35 / net/ethtool/rss.c:158
>       -> ethnl_ops_begin()
> 
> A doit-only program therefore leaves the dump entry into the same
> window unpoliced, so as an 'illustration of how an actual policy might
> look' it is incomplete for the CVE it names. Either add the matching
> lsm/ethtool_netlink_dump program (as done for cve_2024_46834_*), or
> state in the comment that only the doit half is shown and why.

The actual CVE wasn't reached in .dump, only .doit. However, the commit
you've listed makes sense, as there can be similar CVEs reachable via both paths.

> Secondary nit on the same line: '?  -ENODEV' contains a doubled space
> after the '?'.

yes thanks

> 
> 
> ---
> 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/33386073074