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] 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. >
Unless something else which is substantial shows up I will not be sending an update to this patch. cheers, jamal > -- > > 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/[email protected]/ > Link: > https://lore.kernel.org/netdev/[email protected]/ > > > 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 >
