* [PATCH net v2 0/3] net/sched: cls_bpf: accept dev-bound programs on shared blocks
@ 2026-10-10 21:02 Jamal Hadi Salim
2026-10-10 21:02 ` [PATCH net v2 3/3] selftests: net: exercise dev-bound cls_bpf " Jamal Hadi Salim
0 siblings, 1 reply; 6+ messages in thread
From: Jamal Hadi Salim @ 2026-10-10 21:02 UTC (permalink / raw)
To: netdev
Cc: Jamal Hadi Salim, ast, daniel, andrii, eddyz87, memxor,
martin.lau, song, yonghong.song, jolsa, emil, ihor.solodrai,
john.fastabend, sdf, davem, edumazet, kuba, pabeni, horms, jiri,
bpf, stable, sashiko-bot, victor, shuah, linux-kselftest,
andrew+netdev
This series fixes the regression where a device-bound SCHED_CLS program
attached through a shared block is rejected even when it is bound to a
netdev that owns the block, and closes the bind/reoffload replay hole a
later-joining device would otherwise exploit.
Patch 1 gives netdevsim's cls_bpf TC block callback the device-match
check nfp already has; the callback is the authoritative per-device
acceptance boundary and the defect there predates the cls_bpf core
change.
Patch 2 makes the core attach-time check derive the target device from
the shared block's member set (block->q is NULL for a shared block) and
require every member to match, mirroring the broadcast fan-out of the
offload path. Patch 1 is a prerequisite of patch 2 on v7.2+ trees:
commit 120977e2c096 ("net/sched: cls_bpf: reject dev-bound programs
bound to a different device") made the replay path unreachable through
shared blocks as a side effect, and patch 2 re-exposes it, so a stable
tree must take both together.
Patch 3 adds shared-block and late-join coverage to bpf_offload.py; it
uses the existing sample_ret0.bpf.o with an explicit `type classifier`
override instead of adding a new sample object.
---
Changelog:
v1 -> v2
- Patch 1 (netdevsim): enforce bpf_offload_dev_match() on the cls_bpf TC
block callback, closing the bind/reoffload replay path (nipa High).
- Patch 2 (cls_bpf): reject an empty shared block; reformat the comment
(gemini + nipa Low).
- Patch 3 (selftests): new shared-block + late-join coverage in
bpf_offload.py.
- Split the v2 draft into three patches; patches 1 and 2 each carry their
own Fixes: and stable metadata (the selftest companion carries neither).
Jamal Hadi Salim (3):
netdevsim: reject cls_bpf programs bound to another device
net/sched: cls_bpf: accept dev-bound programs on shared blocks
selftests: net: exercise dev-bound cls_bpf on shared blocks
drivers/net/netdevsim/bpf.c | 12 ++
net/sched/cls_bpf.c | 28 ++++-
tools/testing/selftests/net/bpf_offload.py | 139 +++++++++++++++++++++
3 files changed, 175 insertions(+), 4 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH net v2 3/3] selftests: net: exercise dev-bound cls_bpf on shared blocks 2026-10-10 21:02 [PATCH net v2 0/3] net/sched: cls_bpf: accept dev-bound programs on shared blocks Jamal Hadi Salim @ 2026-10-10 21:02 ` Jamal Hadi Salim 2026-10-11 21:03 ` netdev-bot+sashiko 2026-10-16 18:23 ` Simon Horman 0 siblings, 2 replies; 6+ messages in thread From: Jamal Hadi Salim @ 2026-10-10 21:02 UTC (permalink / raw) To: netdev Cc: Jamal Hadi Salim, shuah, linux-kselftest, davem, edumazet, kuba, pabeni, horms, jiri, bpf, victor The issue: bpf_offload.py only exercises per-device ingress qdiscs today, so neither the shared-block accept path nor the late-join replay is tested. The Fix: Add a test for a device-bound SCHED_CLS program attached through a shared block. It loads a dev-bound SCHED_CLS program for a netdevsim device and checks: - the program attaches to a shared block owned by its device, and the filter teardown leaves the device unloaded; - a block that also holds an independently backed device rejects the attach, because the offload path broadcasts the program to all members; - an independently backed device that joins a populated block is not loaded by the bind/replay path, and the driver's extack is asserted; - a second port of the same netdevsim device (same offload device) is accepted together with the bound port. The program is loaded from the existing sample_ret0.bpf.o with an explicit `type classifier` override, so no new sample object is needed. Reviewed-by: Victor Nogueira <victor@mojatatu.com> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com> --- v1 -> v2 (v2 of an on-list patch; v1 posted 2026-10-04): - New selftest: shared-block + late-join coverage in bpf_offload.py. - Split the single v2 draft into three patches; this companion carries no Fixes: and no Cc: stable (a selftest is not backported; it depends on both code patches). --- tools/testing/selftests/net/bpf_offload.py | 139 +++++++++++++++++++++ 1 file changed, 139 insertions(+) diff --git a/tools/testing/selftests/net/bpf_offload.py b/tools/testing/selftests/net/bpf_offload.py index c856d266c8f3..87e244d8ff05 100755 --- a/tools/testing/selftests/net/bpf_offload.py +++ b/tools/testing/selftests/net/bpf_offload.py @@ -452,6 +452,38 @@ class BpfNetdevSim(NetdevSim): self.tc_add_ingress() self.wait_for_flush(bound=bound, total=total) + def tc_add_block(self, block, fail=True, include_stderr=False): + return tc("qdisc add dev %s ingress_block %d clsact" % + (self['ifname'], block), fail=fail, + include_stderr=include_stderr) + + def tc_del_block(self, block): + tc("qdisc del dev %s clsact" % (self['ifname']), fail=False) + + def tc_add_block_filter(self, block, bpf, op="add", prio=None, handle=None, + da=False, verbose=False, skip_sw=False, + skip_hw=False, fail=True, include_stderr=False): + spec = "" + if prio is not None: + spec += " prio %d" % (prio) + if handle: + spec += " handle %s" % (handle) + + params = "" + if da: + params += " da" + if verbose: + params += " verbose" + if skip_sw: + params += " skip_sw" + if skip_hw: + params += " skip_hw" + + return tc("filter {op} block {block} {spec} bpf {bpf} {params}" + .format(op=op, block=block, spec=spec, bpf=bpf, + params=params), + fail=fail, include_stderr=include_stderr) + def tc_show_ingress(self, expected=None): # No JSON support, oh well... flags = ["skip_sw", "skip_hw", "in_hw"] @@ -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(): + """ + Device-bound cls_bpf program attached through a shared block. + """ + # A shared block has no qdisc, so the target device is derived from the + # block's bound netdevs and the program must match every member, since + # the offload path broadcasts it to every callback on the block. A + # netdev joining a populated block later is replayed the stored filter + # before it is inserted into block->ports, so the driver callback must + # enforce the device match as well. + simdev = BpfNetdevSimDev() + sim, = simdev.nsims + sim.set_ethtool_tc_offloads(True) + + start_test("Test TC shared-block load dev-bound program...") + bpftool_prog_load("sample_ret0.bpf.o", "/sys/fs/bpf/tc_shared", + prog_type="classifier", dev=sim['ifname']) + + 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) + + 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() + sim2, = simdev2.nsims + sim2.set_ethtool_tc_offloads(True) + sim.tc_add_block(40) + sim2.tc_add_block(40) + ret, _, err = sim.tc_add_block_filter(40, bpf_pinned("/sys/fs/bpf/tc_shared"), + da=True, skip_sw=True, + fail=False, include_stderr=True) + fail(ret == 0, "dev-bound program broadcast to a foreign shared-block member") + check_extack(err, "Program is not bound to every device in the block.", args) + sim2.tc_del_block(40) + sim.tc_del_block(40) + simdev2.remove() + + start_test("Test TC shared-block late join replay...") + # Build a block owned by sim's device with the matching program + # installed, then let an independently backed device join it. The + # join replays the stored filter to the joining device's callback + # before the device is recorded in block->ports; the driver must + # reject the replay so the foreign device is never loaded. + # sim2 is created here (not inherited) so the TC-offload state it + # depends on is explicit. + simdev2 = BpfNetdevSimDev() + sim2, = simdev2.nsims + sim2.set_ethtool_tc_offloads(True) + 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") + # The add above was expected to fail, so the qdisc may not exist; the + # delete is harmless and only cleans up when it does. + sim2.tc_del_block(30) + sim.tc_del_block(30) + simdev2.remove() + + start_test("Test TC shared-block same-offload-device member...") + # A second port of the same netdevsim device shares the offload device, + # so a program bound to one port may legitimately be offloaded to both. + simdev3 = BpfNetdevSimDev(2) + sim3, sim3b = simdev3.nsims + sim3.set_ethtool_tc_offloads(True) + sim3b.set_ethtool_tc_offloads(True) + bpftool_prog_load("sample_ret0.bpf.o", "/sys/fs/bpf/tc_shared_multi", + prog_type="classifier", dev=sim3['ifname']) + sim3.tc_add_block(50) + sim3b.tc_add_block(50) + # err kept for post-mortem value on a failure. + ret, _, err = sim3.tc_add_block_filter(50, bpf_pinned("/sys/fs/bpf/tc_shared_multi"), + da=True, skip_sw=True, + fail=False, include_stderr=True) + fail(ret != 0, "same-offload-device member rejected on a shared block") + fail(sim3.dfs_read("bpf_offloaded_id") == "0", + "program not offloaded to the bound port") + 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) + + simdev3.remove() + simdev.remove() + rm("/sys/fs/bpf/tc_shared_multi") + rm("/sys/fs/bpf/tc_shared") + bpftool_prog_list_wait(expected=0) + def test_multi_prog(simdev, sim, obj, modename, modeid): start_test("Test multi-attachment XDP - %s + offload..." % (modename or "default", )) @@ -1349,6 +1486,8 @@ try: ret, out = bpftool("prog show %s" % (progB), fail=False) fail(ret != 0, "couldn't get information about orphaned program") + test_tc_shared_block() + print("%s: OK" % (os.path.basename(__file__))) finally: -- 2.43.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net v2 3/3] selftests: net: exercise dev-bound cls_bpf on shared blocks 2026-10-10 21:02 ` [PATCH net v2 3/3] selftests: net: exercise dev-bound cls_bpf " Jamal Hadi Salim @ 2026-10-11 21:03 ` netdev-bot+sashiko 2026-10-11 23:49 ` Jamal Hadi Salim 2026-10-11 23:50 ` Jamal Hadi Salim 2026-10-16 18:23 ` Simon Horman 1 sibling, 2 replies; 6+ messages in thread From: netdev-bot+sashiko @ 2026-10-11 21:03 UTC (permalink / raw) To: jhs Cc: netdev, shuah, linux-kselftest, davem, edumazet, kuba, pabeni, horms, jiri, bpf, victor 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. -- 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2 3/3] selftests: net: exercise dev-bound cls_bpf on shared blocks 2026-10-11 21:03 ` netdev-bot+sashiko @ 2026-10-11 23:49 ` Jamal Hadi Salim 2026-10-11 23:50 ` Jamal Hadi Salim 1 sibling, 0 replies; 6+ messages in thread From: Jamal Hadi Salim @ 2026-10-11 23:49 UTC (permalink / raw) To: netdev-bot+sashiko Cc: netdev, shuah, linux-kselftest, davem, edumazet, kuba, pabeni, horms, jiri, bpf, victor On Sun, Oct 11, 2026 at 5:03 PM <netdev-bot+sashiko@kernel.org> 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2 3/3] selftests: net: exercise dev-bound cls_bpf on shared blocks 2026-10-11 21:03 ` netdev-bot+sashiko 2026-10-11 23:49 ` Jamal Hadi Salim @ 2026-10-11 23:50 ` Jamal Hadi Salim 1 sibling, 0 replies; 6+ messages in thread From: Jamal Hadi Salim @ 2026-10-11 23:50 UTC (permalink / raw) To: netdev-bot+sashiko Cc: netdev, shuah, linux-kselftest, davem, edumazet, kuba, pabeni, horms, jiri, bpf, victor On Sun, Oct 11, 2026 at 5:03 PM <netdev-bot+sashiko@kernel.org> 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2 3/3] selftests: net: exercise dev-bound cls_bpf on shared blocks 2026-10-10 21:02 ` [PATCH net v2 3/3] selftests: net: exercise dev-bound cls_bpf " Jamal Hadi Salim 2026-10-11 21:03 ` netdev-bot+sashiko @ 2026-10-16 18:23 ` Simon Horman 1 sibling, 0 replies; 6+ messages in thread From: Simon Horman @ 2026-10-16 18:23 UTC (permalink / raw) To: Jamal Hadi Salim Cc: netdev, shuah, linux-kselftest, davem, edumazet, kuba, pabeni, jiri, bpf, victor On Sat, Oct 10, 2026 at 05:02:50PM -0400, Jamal Hadi Salim wrote: > The issue: > bpf_offload.py only exercises per-device ingress qdiscs today, so neither > the shared-block accept path nor the late-join replay is tested. > > The Fix: > Add a test for a device-bound SCHED_CLS program attached through a shared > block. It loads a dev-bound SCHED_CLS program for a netdevsim device and > checks: > - the program attaches to a shared block owned by its device, and the > filter teardown leaves the device unloaded; > - a block that also holds an independently backed device rejects the > attach, because the offload path broadcasts the program to all > members; > - an independently backed device that joins a populated block is not > loaded by the bind/replay path, and the driver's extack is asserted; > - a second port of the same netdevsim device (same offload device) is > accepted together with the bound port. > > The program is loaded from the existing sample_ret0.bpf.o with an explicit > `type classifier` override, so no new sample object is needed. > > Reviewed-by: Victor Nogueira <victor@mojatatu.com> > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com> Reviewed-by: Simon Horman <horms@kernel.org> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-10-16 18:23 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-10-10 21:02 [PATCH net v2 0/3] net/sched: cls_bpf: accept dev-bound programs on shared blocks Jamal Hadi Salim 2026-10-10 21:02 ` [PATCH net v2 3/3] selftests: net: exercise dev-bound cls_bpf " Jamal Hadi Salim 2026-10-11 21:03 ` netdev-bot+sashiko 2026-10-11 23:49 ` Jamal Hadi Salim 2026-10-11 23:50 ` Jamal Hadi Salim 2026-10-16 18:23 ` Simon Horman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox