Re: [PATCH net v2 1/3] netdevsim: reject cls_bpf programs bound to another device

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] On the tree this patch applies to, the bug described in the
  commit message cannot happen.

Pre-existing issues:
- [Medium] Offload accounting imbalance when a shared-block late-join
  replay is rolled back.

--

netdevsim: reject cls_bpf programs bound to another device

This adds a check to nsim_bpf_setup_tc_block_cb(). The check rejects a
device-bound cls_bpf program whose offload device does not match the
target netdev, and returns -EINVAL with an extack message. The commit
message says this closes a shared-block late-join replay path through
cls_bpf_reoffload().

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

> diff --git a/drivers/net/netdevsim/bpf.c b/drivers/net/netdevsim/bpf.c
> index 16aa882783984..e6799947db834 100644
> --- a/drivers/net/netdevsim/bpf.c
> +++ b/drivers/net/netdevsim/bpf.c
> @@ -144,6 +144,18 @@ int nsim_bpf_setup_tc_block_cb(enum tc_setup_type type,
>  		return -EOPNOTSUPP;
>  	}
>  
> +	/*
> +	 * A dev-bound program can be replayed here when a netdev joins a
> +	 * shared block, after the core attach-time check has run; enforce
> +	 * the device match as nfp does.
> +	 */

[Severity: Low]

Can this replay path be reached on the tree this patch applies to? The
commit message says:

  The core cls_bpf attach path rejects a mismatching program, but a
  netdev that joins a shared block later is replayed the stored filter by
  cls_bpf_reoffload() before it is inserted into block->ports, so the
  attach-time check cannot see it.

In net/sched/cls_bpf.c, cls_bpf_prog_from_efd() does this attach check:

	dev = block->q ? qdisc_dev(block->q) : NULL;
	if (!dev || !bpf_offload_dev_match(fp, dev)) {
		NL_SET_ERR_MSG(extack,
			       "Program is bound to a different device");

In net/sched/cls_api.c, tcf_block_create() never sets block->q for a
shared block:

	/* Don't store q pointer for blocks which are shared */
	if (!tcf_block_shared(block))
		block->q = q;

So it looks like every dev-bound program on a shared block is rejected
at attach time. Wouldn't step 3 of the reproducer ("install the program
on a shared block owned by B") fail with "Program is bound to a
different device"?

If so, cls_bpf_reoffload() never holds a dev-bound filter on a shared
block that it could replay to a joining device. A non-shared block has
only one netdev, and that netdev already passed the attach check.

The late-join path seems to open only with the companion patch
"net/sched: cls_bpf: accept dev-bound programs on shared blocks". That
patch replaces the block->q check with the every-member predicate
mentioned in the v1 -> v2 notes. This comment only becomes accurate once
that patch is applied.

Could the commit message and this comment say that the cls_bpf patch
opens the replay path? Or could they describe the path that the Fixes:
and Cc: stable tags actually cover on older trees, which is direct
attach before 120977e2c096? The code change itself looks fine.

> +	if (prog && bpf_prog_is_dev_bound(prog->aux) &&
> +	    !bpf_offload_dev_match(prog, ns->netdev)) {
> +		NSIM_EA(cls_bpf->common.extack,
> +			"program bound to a different device");
> +		return -EINVAL;
> +	}

[Severity: Medium]

This is a pre-existing issue that this patch did not introduce, but the
new -EINVAL return adds another way to reach it. The companion cls_bpf
patch lets a dev-bound program sit on a shared block. After that, what
happens to the offload counters when this rejection triggers a rollback?

Say device B holds a skip_sw cls_bpf filter with a B-bound program on a
shared block. So in_hw_count == 1, TCA_CLS_FLAGS_IN_HW is set, and
block->offloadcnt == 1. Then netdevsim device C, which has a different
bpf_offload_dev, binds the block:

tcf_block_bind()
  tcf_block_playback_offloads(add=true)
    cls_bpf_reoffload()
      tc_setup_cb_reoffload()
        nsim_bpf_setup_tc_block_cb()   <- returns -EINVAL here

The filter is skip_sw, so the error propagates and
tcf_block_playback_offloads() jumps to err_playback_remove. That replays
add=false over every filter, including the one whose add just failed.

With prog == NULL the new guard is skipped. ns->bpf_offloaded is NULL,
so it is != oldprog, and C's callback takes this path:

	if (ns->bpf_offloaded != oldprog) {
		oldprog = NULL;
		if (!cls_bpf->prog)
			return 0;

tc_setup_cb_reoffload() treats that 0 as a successful removal and calls
tc_cls_offload_cnt_update(..., 1, false):

	*cnt -= diff;
	if (!*cnt)
		tcf_block_offload_dec(block, flags);

Wouldn't in_hw_count then drop from 1 to 0, clearing IN_HW and
decrementing block->offloadcnt, while B still has the program offloaded?
Each later failed join would decrement the u32 in_hw_count again and
wrap it.

Once block->offloadcnt reads 0, tcf_block_offload_in_use() returns
false. Could a device without ndo_setup_tc then bind a block that still
carries a skip_sw filter?

The same imbalance looks reachable on the baseline through netdevsim's
existing rejections, which only apply to adds. One case is an unbound
skip_sw program that B accepts (bpf_tc_non_bound_accept=1) but a joining
C rejects with the default bpf_tc_non_bound_accept=0. Another is the
-EBUSY "driver and netdev offload states mismatch" path.

The root cause seems to have two parts. The core rolls back without
tracking which callbacks accepted the add. Also, netdevsim returns 0 when
asked to remove a program it does not hold.

[ ... ]

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