On Sun, Oct 11, 2026 at 5:03 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0 (2)
>
> 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).
>
> - [Medium] The commit message says the first subtest checks that "the
> program attaches to a shared block owned by its device, and the filter…
>
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but the late-
> join subtest (block 30) runs a core offload-accounting imbalance and…
> - [Low] This problem wasn't introduced by this patch, but the same-
> offload-device subtest hits a netdevsim state-tracking defect.
> - [Low] This problem wasn't introduced by this patch, but if one of the
> new subtests fails, a netdevsim device can be left behind.
>
Unless something else which is substantial shows up I will not be
sending an update to this patch.
cheers,
jamal
> --
>
> selftests: net: exercise dev-bound cls_bpf on shared blocks
>
> This adds test_tc_shared_block() to bpf_offload.py. The test attaches a
> device-bound SCHED_CLS program to netdevsim through shared tc blocks. It
> covers four cases: attach on a block owned by the bound device, rejection
> when a foreign device is a member, rejection of a foreign device that
> joins late during replay, and acceptance on a second port of the same
> netdevsim device.
>
> > diff --git a/tools/testing/selftests/net/bpf_offload.py
> > b/tools/testing/selftests/net/bpf_offload.py
> > index c856d266c8f36..8c1da9b51be5a 100755
> > --- a/tools/testing/selftests/net/bpf_offload.py
> > +++ b/tools/testing/selftests/net/bpf_offload.py
>
> [ ... ]
>
> > @@ -629,6 +661,111 @@ def test_spurios_extack(sim, obj, skip_hw, needle):
> > include_stderr=True)
> > check_no_extack(res, needle)
> >
> > +def test_tc_shared_block():
>
> [ ... ]
>
> > + start_test("Test TC shared-block dev-bound attach...")
> > + # The pinned program is dev-bound to this device; attaching it through
> > + # a shared block must succeed even though block->q is NULL.
> > + sim.tc_add_block(22)
> > + # err is kept (include_stderr) for post-mortem value on a failure.
> > + ret, _, err = sim.tc_add_block_filter(22,
> > bpf_pinned("/sys/fs/bpf/tc_shared"),
> > + da=True, skip_sw=True,
> > + fail=False, include_stderr=True)
> > + fail(ret != 0, "dev-bound program rejected on a matching shared block")
> > + fail(sim.dfs_read("bpf_offloaded_id") == "0",
> > + "dev-bound program not offloaded on a matching shared block")
> > + sim.tc_del_block(22)
>
> [Severity: Medium]
> The commit message says this subtest checks that "the program attaches to
> a shared block owned by its device, and the filter teardown leaves the
> device unloaded". Is the teardown part checked anywhere?
>
> After sim.tc_del_block(22), nothing reads bpf_offloaded_id again or waits
> for the flush. sim.tc_del_block(30) has the same gap. So does the
> sim3b.tc_del_block(50) / sim3.tc_del_block(50) pair, which covers the
> unbind remove replay and the last-member block destroy.
>
> The tc_del_block() helper also ignores failure:
>
> def tc_del_block(self, block):
> tc("qdisc del dev %s clsact" % (self['ifname']), fail=False)
>
> The only check left is bpftool_prog_list_wait(expected=0) at the end. It
> runs after simdev3.remove(), simdev.remove() and the rm of both pins. It
> counts programs, not per-port offload state, so it can't tell an unload
> caused by filter teardown from one caused by device removal.
>
> Suppose the remove replay through
> cls_bpf_reoffload()->nsim_bpf_setup_tc_block_cb()->nsim_bpf_offload(ns,
> NULL, true) stopped clearing ns->bpf_offloaded:
>
> For blocks 30 and 50, the only sign would be the
> WARN_ON(ns->bpf_offloaded) in nsim_bpf_uninit() in dmesg.
>
> For block 22, the failure would show up later as -EBUSY ("driver and
> netdev offload states mismatch") on the block 30 attach. The test would
> then report it as "dev-bound program rejected on a matching shared block".
>
> Could a check like this be added after each tc_del_block() for sim, sim3
> and sim3b, while the devices still exist?
>
> fail(sim.dfs_read("bpf_offloaded_id") != "0", ...)
>
> It could also be worth checking that sim3 is still loaded after sim3b
> leaves block 50. Otherwise, the teardown claim could be dropped from the
> commit message.
>
> > +
> > + start_test("Test TC shared-block broadcast to a foreign member...")
> > + # Add an independently backed device to the SAME block before the
> > + # attach. The program is bound to sim's device only, and the offload
> > + # path fans it out to every member, so the attach must be rejected.
> > + simdev2 = BpfNetdevSimDev()
>
> [Severity: Low]
> This is a pre-existing issue, not one introduced by this patch, but can a
> failure in one of these subtests leave a netdevsim device behind?
>
> clean_up() loops over the same list that each removal modifies:
>
> for dev in devs:
> dev.remove()
>
> and BpfNetdevSimDev.remove() does:
>
> def remove(self):
> super().remove()
> devs.remove(self)
>
> As a result, every removal skips the next entry in the list.
>
> Here simdev and simdev2 (or simdev3) are both alive. A fail() in the
> broadcast, late-join or same-offload-device subtest raises before the
> local simdevN.remove(). The outer finally then calls clean_up(), which
> removes the first device and skips the second. The second device and its
> clsact qdisc are left in place.
>
> Looping over a copy of the list (for dev in list(devs):) would avoid
> this.
>
> [ ... ]
>
> > + sim.tc_add_block(30)
> > + ret, _, err = sim.tc_add_block_filter(30,
> > bpf_pinned("/sys/fs/bpf/tc_shared"),
> > + da=True, skip_sw=True,
> > + fail=False, include_stderr=True)
> > + fail(ret != 0, "dev-bound program rejected on a matching shared block")
> > + ret, _, err = sim2.tc_add_block(30, fail=False, include_stderr=True)
> > + fail(ret == 0, "foreign device joined a populated dev-bound shared
> > block")
> > + check_extack_nsim(err, "program bound to a different device.", args)
> > + fail(sim2.dfs_read("bpf_offloaded_id") != "0",
> > + "replayed program loaded onto a foreign device")
>
> [Severity: Medium]
> This isn't a bug introduced by this patch, but the rejected join here
> seems to break the core offload accounting, and these assertions can't
> catch it.
>
> When sim2 joins block 30, tcf_block_bind()->tcf_block_playback_offloads()
> with add=true replays the filter to sim2's callback. The callback rejects
> it with -EINVAL. Because the filter is skip_sw, tc_setup_cb_reoffload()
> returns the error and the playback unwinds:
>
> net/sched/cls_api.c:tcf_block_playback_offloads() {
> ...
> err_playback_remove:
> tcf_proto_put(tp, true, NULL);
> tcf_chain_put(chain);
> tcf_block_playback_offloads(block, cb, cb_priv, false, offload_in_use,
> extack);
> ...
> }
>
> This sends a remove to the same sim2 callback, including for the tp whose
> add just failed. nsim returns success for removing a program it never
> held:
>
> drivers/net/netdevsim/bpf.c:nsim_bpf_setup_tc_block_cb() {
> ...
> if (ns->bpf_offloaded != oldprog) {
> oldprog = NULL;
> if (!cls_bpf->prog)
> return 0;
> ...
> }
>
> So tc_setup_cb_reoffload() calls tc_cls_offload_cnt_update() with
> add=false. The filter's in_hw_count drops from 1 (sim's real offload) to
> 0. TCA_CLS_FLAGS_IN_HW is cleared and block->offloadcnt drops to 0, while
> sim still has the program loaded.
>
> When sim.tc_del_block(30) runs, the unbind decrements in_hw_count again.
> The u32 wraps to 0xFFFFFFFF and stays there until block destruction calls
> tc_cls_offload_cnt_reset(). Until then, tcf_block_offload_in_use() is
> false, so tcf_block_offload_bind() skips its "dev has offload disabled"
> check.
>
> Would it be worth also checking sim's offload state and the filter's
> in_hw flag after the rejected join?
>
> The same remove-returns-0 behaviour can already be reached through nsim's
> -EBUSY add rejection path, and nfp's callback has the same shape.
>
> [ ... ]
>
> > + fail(sim3b.dfs_read("bpf_offloaded_id") == "0",
> > + "program not offloaded to the same-offload-device port")
> > + sim3b.tc_del_block(50)
> > + sim3.tc_del_block(50)
>
> [Severity: Low]
> This is a pre-existing netdevsim issue, not one introduced by this patch,
> but with this teardown order the program's debugfs loaded state is wrong
> while sim3 is still offloaded.
>
> netdevsim tracks loaded state in a single per-program boolean that every
> port shares:
>
> drivers/net/netdevsim/bpf.c:nsim_prog_set_loaded() {
> ...
> state = prog->aux->offload->dev_priv;
> state->is_loaded = loaded;
> }
>
> sim3b.tc_del_block(50) replays a remove to sim3b. That runs
> nsim_bpf_offload(ns, NULL, true), which calls
> nsim_prog_set_loaded(ns->bpf_offloaded, false). This clears is_loaded
> even though sim3's ns->bpf_offloaded still points at the program.
>
> Until sim3 leaves the block, bpf_bound_progs/<id>/loaded reads as not
> loaded. The WARN(state->is_loaded, ...) in nsim_bpf_destroy_prog() also
> won't fire in this case.
>
> The existing XDP multi-dev ASIC tests already follow the same pattern.
>
> [ ... ]
>
> --
> Sashiko AI review ·
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-PYC7.v2.20261010165947%40mojatatu.com
