On Sun, Oct 11, 2026 at 5:03 PM <[email protected]> wrote: > > 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] The v1->v2 notes say: "Style (gemini + nipa Low) -- FIXED: the > shared-block comment opens with /* on its own line." The diff does not… > - [Low] The commit message ("matches every netdev bound to the block"), > the code comment ("require a match with every member") and the extack… > - [Low] The stable metadata does not express the patch's known > dependencies. >
Unless something else which is substantial shows up I will not be sending an update to this patch. cheers, jamal > -- > > net/sched: cls_bpf: accept dev-bound programs on shared blocks > > This patch follows up commit 120977e2c096, which took the target device > from block->q and so rejected every dev-bound cls_bpf program attached > through a shared block. On shared blocks, cls_bpf_prog_from_efd() now > walks block->ports and accepts the attach only when > bpf_offload_dev_match() succeeds for every entry. It rejects an empty > block. > > Link: https://lore.kernel.org/netdev/[email protected]/ > Link: > https://lore.kernel.org/netdev/[email protected]/ > > > Fixes: 120977e2c096 ("net/sched: cls_bpf: reject dev-bound programs bound > > to a different device") > > Reported-by: Sashiko (nipa) <[email protected]> > > [ ... ] > > Cc: [email protected] > > [Severity: Low] > Do these tags give stable tooling enough to backport this safely? > > Commit 120977e2c096 carries "Fixes: 2b3486bc2d23" ("bpf: Introduce > device-bound XDP programs", v6.3). Stable trees from v6.3 onward, such > as 6.6.y, can therefore pick it up and inherit the shared-block > rejection. > > This follow-up iterates block->ports in cls_bpf_prog_from_efd(). That > field was added by 913b47d3424e ("net/sched: Introduce tc block netdev > tracking infra", v6.8-rc1), so the patch will not build on those older > trees. They would keep rejecting every dev-bound attach on a shared > block. > > The ordering against the netdevsim patch ("patch 1 must be applied > first, including in stable") is only stated in prose. Would a version > bound on the Cc: stable line, or a prerequisite annotation, express > both dependencies in a form the tooling can use? > > > diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c > > index 188cf0f949dd4..07ad9556889d4 100644 > > --- a/net/sched/cls_bpf.c > > +++ b/net/sched/cls_bpf.c > > @@ -392,11 +392,31 @@ 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; > > + unsigned long ifindex; > > + bool match = false; > > + > > + /* A shared block has no qdisc of its own, but it may bind > > + * several netdevs and the program is offloaded to all of > > them; > > + * require a match with every member. > > + */ > > [Severity: Low] > This isn't a bug, but the v1 -> v2 notes say: > > Style (gemini + nipa Low) -- FIXED: the shared-block comment opens > with /* on its own line. > > This comment in cls_bpf_prog_from_efd() still has text on the opening > line. The only new comment in the series with /* on its own line is in > nsim_bpf_setup_tc_block_cb(), in the netdevsim patch. > > Was the style fix lost when v2 was split into three patches? > > > + if (!tcf_block_shared(block)) { > > + dev = qdisc_dev(tcf_block_q(block)); > > + match = bpf_offload_dev_match(fp, dev); > > + if (!match) > > + NL_SET_ERR_MSG(extack, > > + "Program is bound to a > > different device"); > > + } else { > > + xa_for_each(&block->ports, ifindex, dev) { > > + match = bpf_offload_dev_match(fp, dev); > > + if (!match) > > + break; > > + } > > [Severity: Low] > Does block->ports really hold every netdev bound to the shared block? > The commit message ("matches every netdev bound to the block"), the > comment above and the extack text all assume it does. > > In tcf_block_get_ext(), the offload callback is bound for every binder > type, but the device only goes into ports when tcf_block_tracks_dev() > is true: > > 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); > ... > } > > tcf_block_tracks_dev() only accepts FLOW_BLOCK_BINDER_TYPE_CLSACT_INGRESS > and FLOW_BLOCK_BINDER_TYPE_CLSACT_EGRESS. > > sch_red binds shared blocks through tcf_qevent_init() using > FLOW_BLOCK_BINDER_TYPE_RED_EARLY_DROP and FLOW_BLOCK_BINDER_TYPE_RED_MARK. > Those netdevs own the block and receive the cls_bpf offload broadcast, > but this loop never visits them. > > That leads to two cases: > > - If a block's only binders are RED qevents, ports is empty. A dev-bound > attach then fails with "Program is not bound to every device in the > block", which is misleading. The old code also returned -EINVAL here, > so the return value has not changed. > > - On a mixed clsact plus qevent block, the check passes without looking > at the qevent member. > > The mixed case is safe in-tree today only because of other checks. nfp > and netdevsim register their callbacks ingress-only, so a qevent bind > raises nooffloaddevcnt, and tc_setup_cb_add() rejects skip_sw: > > /* Make sure all netdevs sharing this block are offload-capable. */ > if (block->nooffloaddevcnt && err_stop) { > ok_count = -EOPNOTSUPP; > goto err_unlock; > } > > Both drivers also re-check bpf_offload_dev_match() in their callbacks. > > Should the commit message, comment and extack say that this check only > covers clsact-tracked members? The rest of the protection comes from > nooffloaddevcnt and the driver callbacks. > > > + if (!match) > > + NL_SET_ERR_MSG(extack, > > + "Program is not bound to every > > device in the block"); > > + } > > [ ... ] > > -- > Sashiko AI review · > https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-PYC7.v2.20261010165947%40mojatatu.com >
