* Re: [PATCH net] net/sched: cls_bpf: accept dev-bound programs on shared blocks
2026-10-04 8:21 [PATCH net] net/sched: cls_bpf: accept dev-bound programs on shared blocks Jamal Hadi Salim
2026-10-05 8:21 ` sashiko-bot
@ 2026-10-05 8:57 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: netdev-bot+sashiko @ 2026-10-05 8:57 UTC (permalink / raw)
To: jhs
Cc: netdev, ast, daniel, andrii, eddyz87, memxor, martin.lau, song,
yonghong.song, jolsa, emil, ihor.solodrai, john.fastabend, sdf,
davem, edumazet, kuba, pabeni, horms, jiri, bpf, victor, stable,
sashiko-bot
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
^ permalink raw reply [flat|nested] 3+ messages in thread