[PATCH net] net/sched: cls_bpf: accept dev-bound programs on shared blocks

[PATCH net] net/sched: cls_bpf: accept dev-bound programs on shared blocks

[Jamal Hadi Salim]

Follow-up to commit 120977e2c096 ("net/sched: cls_bpf: reject dev-bound
programs bound to a different device"), which derived the target device
from block->q:

	dev = block->q ? qdisc_dev(block->q) : NULL;

tcf_block_create() deliberately leaves block->q NULL for shared blocks, so
a device-bound program attached through a shared block is rejected with
-EINVAL even when it is bound to a netdev that owns the block.  The
offload path (cls_bpf_offload_cmd()) never consults block->q and drives
the per-device callbacks for shared blocks as before.

Accept the attach on a shared block when bpf_offload_dev_match() matches
every netdev bound to the block, and reject an empty block.  Every member
must match because the offload path broadcasts the program to every
callback registered on the block, so an any-member check would re-admit
the wrong-device attach on a mixed block.

Conditions to recreate the bug:
  - CONFIG_NETDEVSIM=y, CONFIG_NET_CLS_BPF=y
  - load a dev-bound SCHED_CLS program for netdevsim device B
    (prog_ifindex=B), pin it in bpffs
  - tc qdisc add dev B ingress_block 22 clsact
  - tc filter add block 22 ingress bpf da object-pinned <pin> skip_sw
  Unfixed, the filter add fails with -EINVAL ("Program is bound to a
  different device"); fixed, it succeeds.  Adding a second, independently
  backed netdevsim device to the same block makes the attach fail again.

Fixes: 120977e2c096deea4e866e4273be9220b957c29e ("net/sched: cls_bpf: reject dev-bound programs bound to a different device")
Reported-by: Sashiko (nipa) <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260809094418.901607-1-jhs@mojatatu.com
Link: https://lore.kernel.org/netdev/20260809094418.901607-1-jhs@mojatatu.com/
Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/cls_bpf.c | 26 ++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 188cf0f949dd..fe3bf251a39b 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -392,9 +392,29 @@ static int cls_bpf_prog_from_efd(struct nlattr **tb, struct cls_bpf_prog *prog,
 	if (bpf_prog_is_dev_bound(fp->aux)) {
 		struct tcf_block *block = tp->chain->block;
 		struct net_device *dev;
-
-		dev = block->q ? qdisc_dev(block->q) : NULL;
-		if (!dev || !bpf_offload_dev_match(fp, dev)) {
+		unsigned long ifindex;
+		bool found = false;
+		bool match = false;
+
+		/* A shared block has no qdisc (block->q == NULL) but may
+		 * bind several netdevs; the program is offloaded to all of
+		 * them, so it must match all of them.
+		 */
+		if (!tcf_block_shared(block)) {
+			match = bpf_offload_dev_match(fp, qdisc_dev(tcf_block_q(block)));
+		} else {
+			xa_for_each(&block->ports, ifindex, dev) {
+				found = true;
+				if (!bpf_offload_dev_match(fp, dev)) {
+					match = false;
+					break;
+				}
+				match = true;
+			}
+			if (!found)
+				match = false;
+		}
+		if (!match) {
 			NL_SET_ERR_MSG(extack,
 				       "Program is bound to a different device");
 			bpf_prog_put(fp);
--
2.43.0

Re: [PATCH net] net/sched: cls_bpf: accept dev-bound programs on shared blocks

[sashiko-bot]

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

commit 335af23f9a1d13cab5f84cecaf4fd5a4dda63d7f
Author: Jamal Hadi Salim <jhs@mojatatu.com>

net/sched: cls_bpf: accept dev-bound programs on shared blocks

This commit allows device-bound programs to be attached through shared blocks
by ensuring the program matches every netdev bound to the block. Prior to
this, attaching to a shared block would incorrectly return -EINVAL because the
block's qdisc is NULL.

Link: https://lore.kernel.org/netdev/20260809094418.901607-1-jhs@mojatatu.com/

> diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
> index 188cf0f949dd4..6a54f8a7b8218 100644
> --- a/net/sched/cls_bpf.c
> +++ b/net/sched/cls_bpf.c
> @@ -392,9 +392,29 @@ static int cls_bpf_prog_from_efd(struct nlattr **tb, struct cls_bpf_prog *prog,
>  	if (bpf_prog_is_dev_bound(fp->aux)) {
>  		struct tcf_block *block = tp->chain->block;
>  		struct net_device *dev;
> -
> -		dev = block->q ? qdisc_dev(block->q) : NULL;
> -		if (!dev || !bpf_offload_dev_match(fp, dev)) {
> +		unsigned long ifindex;
> +		bool found = false;
> +		bool match = false;
> +
> +		/* A shared block has no qdisc (block->q == NULL) but may
> +		 * bind several netdevs; the program is offloaded to all of
> +		 * them, so it must match all of them.
> +		 */

[Severity: Low]
This isn't a bug, but does this multi-line comment follow the documented BPF
subsystem coding style? The BPF subsystem expects the opening "/*" to be on
its own line, rather than having text on the same line as the opening.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/QDISC-PYC7.v1.20261003103747@mojatatu.com?part=1

Re: [PATCH net] net/sched: cls_bpf: accept dev-bound programs on shared blocks

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] This patch's 'every member must match' rule for dev-bound
  cls_bpf programs on shared blocks is checked only once.
- [Low] bpf: multi-line comment style violation in cls_bpf.c

--

net/sched: cls_bpf: accept dev-bound programs on shared blocks

This patch lets a device-bound cls_bpf program attach through a shared tc
block, as long as bpf_offload_dev_match() succeeds for every netdev in
block->ports. An empty shared block is rejected. Non-shared blocks are still
checked against the qdisc's device.

Link: https://lore.kernel.org/netdev/20260809094418.901607-1-jhs@mojatatu.com/

> diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
> index 188cf0f949dd4..6a54f8a7b8218 100644
> --- a/net/sched/cls_bpf.c
> +++ b/net/sched/cls_bpf.c
> @@ -392,9 +392,29 @@ static int cls_bpf_prog_from_efd(struct nlattr **tb, struct cls_bpf_prog *prog,
>  	if (bpf_prog_is_dev_bound(fp->aux)) {
>  		struct tcf_block *block = tp->chain->block;
>  		struct net_device *dev;
> -
> -		dev = block->q ? qdisc_dev(block->q) : NULL;
> -		if (!dev || !bpf_offload_dev_match(fp, dev)) {
> +		unsigned long ifindex;
> +		bool found = false;
> +		bool match = false;
> +
> +		/* A shared block has no qdisc (block->q == NULL) but may
> +		 * bind several netdevs; the program is offloaded to all of
> +		 * them, so it must match all of them.
> +		 */
> +		if (!tcf_block_shared(block)) {
> +			match = bpf_offload_dev_match(fp, qdisc_dev(tcf_block_q(block)));
> +		} else {
> +			xa_for_each(&block->ports, ifindex, dev) {
> +				found = true;
> +				if (!bpf_offload_dev_match(fp, dev)) {

[Severity: Low]
This is a minor style nit. The new block comment in
cls_bpf_prog_from_efd() starts its text on the same line as the
opening /*.

cls_bpf.c is a BPF-related path, and BPF code uses the multi-line
comment style with the opening /* on a line of its own. That applies
even where the surrounding code in the file uses the older form.

Could you move the text down so the comment reads like this?

		/*
		 * A shared block has no qdisc (block->q == NULL) but may
		 * bind several netdevs; the program is offloaded to all of
		 * them, so it must match all of them.
		 */

This has no functional impact. If the comment text changes because of
the block->ports concern above, it is worth fixing in the same respin.

[Severity: High]

This check runs only once, when the filter is added or replaced. It uses
whatever block->ports holds at that moment. What happens if a netdev from a
different bpf_offload_dev joins the shared block later?

For example:

  tc qdisc add dev B ingress_block 22 clsact
  tc filter add block 22 ingress bpf da object-pinned <pin> skip_sw
  tc qdisc add dev C ingress_block 22 clsact

On the third command, tcf_block_get_ext() finds the existing block and
replays its filters to C's callback. This happens before C is added to
block->ports:

net/sched/cls_api.c:tcf_block_get_ext() {
    ...
	err = tcf_block_offload_bind(block, q, ei, extack);
	if (err)
		goto err_block_offload_bind;

	if (tcf_block_tracks_dev(block, ei)) {
		err = xa_insert(&block->ports, dev->ifindex, dev, GFP_KERNEL);
    ...
}

The call chain is tcf_block_offload_bind()->tcf_block_bind()->
tcf_block_playback_offloads()->cls_bpf_reoffload(). cls_bpf_reoffload()
passes prog->filter to the new callback without calling
bpf_offload_dev_match():

net/sched/cls_bpf.c:cls_bpf_reoffload() {
    ...
		cls_bpf.prog = add ? prog->filter : NULL;
    ...
		err = tc_setup_cb_reoffload(block, tp, add, cb, TC_SETUP_CLSBPF,
					    &cls_bpf, cb_priv, &prog->gen_flags,
					    &prog->in_hw_count);
    ...
}

netdevsim's nsim_bpf_setup_tc_block_cb() does not check the device either.
It only requires prog->aux->offload to be non-NULL before calling
nsim_bpf_offload(). That sets C's bpf_offloaded and marks the program as
loaded in state owned by B's nsim_dev.

Doesn't this build the mixed block that the commit message says the
all-members check prevents?

  "an any-member check would re-admit the wrong-device attach on a mixed
  block."

Before this patch, block->q is NULL on a shared block, so every dev-bound
program was rejected there. That kept dev-bound programs off this replay
path.

This looks like the problem that commit 120977e2c096 fixed. When B is
unregistered, __bpf_offload_dev_netdev_unregister() has no altdev, so it
calls __bpf_prog_offload_destroy():

kernel/bpf/offload.c:__bpf_prog_offload_destroy() {
    ...
	if (offload->dev_state)
		offload->offdev->ops->destroy(prog);

	list_del_init(&offload->offloads);
	kfree(offload);
	prog->aux->offload = NULL;
}

If is_loaded is still set, nsim_bpf_destroy_prog() can hit its WARN here.

The filter and block stay alive because C still holds them. Deleting the
filter or C's clsact later goes through
nsim_bpf_offload()->nsim_prog_set_loaded(). aux->offload_requested is still
true, so the bpf_prog_is_offloaded() check passes, and then:

drivers/net/netdevsim/bpf.c:nsim_prog_set_loaded() {
    ...
	state = prog->aux->offload->dev_priv;
	state->is_loaded = loaded;
}

Can this dereference a NULL prog->aux->offload?

The reproducer in the commit message only adds the second device before the
attach. Does the check also need to run on the bind/reoffload side? Or
should dev-bound programs stay rejected on shared blocks?

There is a smaller related gap. tcf_block_tracks_dev() only records clsact
ingress and egress binders in block->ports:

net/sched/cls_api.c:tcf_block_tracks_dev() {
	return tcf_block_shared(block) &&
	       (ei->binder_type == FLOW_BLOCK_BINDER_TYPE_CLSACT_INGRESS ||
		ei->binder_type == FLOW_BLOCK_BINDER_TYPE_CLSACT_EGRESS);
}

Other shared-block binders, such as those set up by tcf_qevent_init(), also
register offload callbacks. They never appear in block->ports, so the new
xa_for_each() loop does not see them. Could a device bound this way receive
the program without being checked?

> +					match = false;
> +					break;
> +				}
> +				match = true;
> +			}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-PYC7.v1.20261003103747%40mojatatu.com