[PATCH net v2 0/2] net: openvswitch: fix a feature leak on a failed change

[PATCH net v2 0/2] net: openvswitch: fix a feature leak on a failed change

[Minxi Hou]

Changes in v2:
- Patch 1 limits its closing claim to OVS_DP_CMD_SET, and says that a
  failed OVS_DP_CMD_NEW and the reset that follows it still leave the
  key one enable ahead.
- Patch 1 names the reachability and the effect: only CAP_NET_ADMIN is
  needed, datapath_policy does not validate the per-cpu-pids attribute,
  and a disable on a count of zero warns in static_key_dec_not_one().
- Patch 2 treats only EOPNOTSUPP as the missing key and skips with the
  kselftest skip code; any other failure fails the test.
- Patch 2 selects CONFIG_NET_CLS_ACT and CONFIG_NET_TC_SKB_EXT in the
  selftest config fragment, so the recirc round runs in CI.
- Patch 2 sends the features the datapath reports when --features is
  left out, instead of clearing them.

v1: https://lore.kernel.org/netdev/20261107113403.797779-1-houminxi@gmail.com/

ovs_dp_change() records the requested user features before it calls the
update that can fail, so a request rejected for an empty per-cpu-pids
leaves the datapath claiming a feature it never enabled. The tc skb
extension key is a reference count shared with cls_flower, and the code
that would keep it in step is skipped on that path.

Patch 1 fixes the ordering. Patch 2 adds a set-dp command to the
openvswitch selftest tool, and a test that sends such a request and
checks the features the datapath reports afterwards. It fails without
the fix and passes with it.

The problem turned up while the YNL spec for this family was being
written: describing the ovs_datapath set command meant reading
ovs_dp_change(), where the ordering stands out.

Signed-off-by: Minxi Hou <houminxi@gmail.com>

Minxi Hou (2):
Minxi Hou (2):
  net: openvswitch: do not record user features when the change fails
  selftests/net/openvswitch: check that a failed set keeps the features

 net/openvswitch/datapath.c                    | 10 +--
 .../testing/selftests/net/openvswitch/config  |  2 +
 .../selftests/net/openvswitch/openvswitch.sh  | 67 +++++++++++++++++++
 .../selftests/net/openvswitch/ovs-dpctl.py    | 53 +++++++++++++++
 4 files changed, 127 insertions(+), 5 deletions(-)


base-commit: 30c71476d842b094651bd7712f6c8ee8716147d0
-- 
2.56.0

[PATCH net v2 1/2] net: openvswitch: do not record user features when the change fails

[Minxi Hou]

ovs_dp_change() stores the new features in dp->user_features before it
updates the per-cpu upcall port IDs, and that update fails when the
per-cpu-pids attribute is empty or its length is not a multiple of four.
The request returns -EINVAL with the new features already recorded.

The features decide whether the tc skb extension static key is enabled,
but the code that enables or disables the key runs after the failing
call, so it does not run for the failed request. dp->user_features then
claims TC_RECIRC_SHARING is set while tc_skb_ext_tc_enable() was not
called, and a later request that clears the bit calls
tc_skb_ext_tc_disable() for an enable that never happened.

tc_skb_ext_tc is one static key for the whole host. cls_flower, the
bridge, the flow dissector and OVS hold references to it, and the key
stays on while any of them holds one. The count ends up one short and
reaches zero while other users still hold a reference, so recirc sharing
in every datapath goes off with it, as does the skb extension allocation
in tc, in the flow dissector and in the bridge. A disable on a count of
zero also reaches the imbalance check in static_key_dec_not_one() in
kernel/jump_label.c and warns there, which panics a host that runs with
panic_on_warn. The opposite ordering is possible as well: a failing
request that drops the bit leaves the key enabled for good.

The request only needs CAP_NET_ADMIN, which a user namespace that owns a
network namespace provides. datapath_policy has no entry for
OVS_DP_ATTR_PER_CPU_PIDS and the operation skips strict validation, so an
empty attribute reaches the check that fails.

Keep the new features in the local variable until the request has gone
through, and use that variable for the enable and disable decisions. A
failed OVS_DP_CMD_SET then leaves both the features and the key alone.

Two other paths touch the bit without keeping the key in step:
ovs_dp_reset_user_features() clears it without disabling the key, and a
new datapath that fails after ovs_dp_change() succeeded is freed without
disabling it. Both predate this change and are left alone here, so a
failed OVS_DP_CMD_NEW and the reset that follows it still leave the key
one enable ahead.

The selftest in the next patch sends a set request that fails on an
empty per-cpu-pids and reads the features back; it fails without this
change and passes with it. The openvswitch selftest suite passes with
the change applied.

Fixes: 35d39fecbc24 ("net/sched: Enable tc skb ext allocation on chain miss only when needed")
Cc: stable@vger.kernel.org
Signed-off-by: Minxi Hou <houminxi@gmail.com>
---
 net/openvswitch/datapath.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index b2a0b09fd4a5..00043aef2d03 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -1801,9 +1801,7 @@ static int ovs_dp_change(struct datapath *dp, struct nlattr *a[])
 			return err;
 	}
 
-	dp->user_features = user_features;
-
-	if (dp->user_features & OVS_DP_F_DISPATCH_UPCALL_PER_CPU &&
+	if (user_features & OVS_DP_F_DISPATCH_UPCALL_PER_CPU &&
 	    a[OVS_DP_ATTR_PER_CPU_PIDS]) {
 		/* Upcall Netlink Port IDs have been updated */
 		err = ovs_dp_set_upcall_portids(dp,
@@ -1812,10 +1810,12 @@ static int ovs_dp_change(struct datapath *dp, struct nlattr *a[])
 			return err;
 	}
 
-	if ((dp->user_features & OVS_DP_F_TC_RECIRC_SHARING) &&
+	dp->user_features = user_features;
+
+	if ((user_features & OVS_DP_F_TC_RECIRC_SHARING) &&
 	    !(old_features & OVS_DP_F_TC_RECIRC_SHARING))
 		tc_skb_ext_tc_enable();
-	else if (!(dp->user_features & OVS_DP_F_TC_RECIRC_SHARING) &&
+	else if (!(user_features & OVS_DP_F_TC_RECIRC_SHARING) &&
 		 (old_features & OVS_DP_F_TC_RECIRC_SHARING))
 		tc_skb_ext_tc_disable();
 
-- 
2.56.0

Re: [PATCH net v2 1/2] net: openvswitch: do not record user features when the change fails

[netdev-bot+sashiko]

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 commit message says "cls_flower, the bridge, the flow
  dissector and OVS hold references to it" about the tc_skb_ext_tc…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but each failed
  OVS_DP_CMD_NEW leaks one tc_skb_ext_tc static key reference, with no…

--

net: openvswitch: do not record user features when the change fails

This patch moves the store to dp->user_features in ovs_dp_change() so it
happens after the per-cpu upcall port ID update. A failed OVS_DP_CMD_SET
therefore no longer records the new features. The tc_skb_ext_tc enable and
disable decisions now use the local user_features variable.

> tc_skb_ext_tc is one static key for the whole host. cls_flower, the
> bridge, the flow dissector and OVS hold references to it, and the key
> stays on while any of them holds one.

[Severity: Low]
Is this list of reference holders accurate? Only cls_flower and
openvswitch call tc_skb_ext_tc_enable() and tc_skb_ext_tc_disable(). In
net/sched/cls_api.c those two wrap static_branch_inc() and
static_branch_dec().

The bridge and the flow dissector only read the key through
tc_skb_ext_tc_enabled():

net/core/flow_dissector.c:
	if (tc_skb_ext_tc_enabled()) {

net/bridge/br_private.h:
	if (!tc_skb_ext_tc_enabled())

The description of what happens to these readers is still correct. Could
the sentence say that cls_flower and OVS hold references, and that the
bridge and the flow dissector only depend on the key?

> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index b2a0b09fd4a56..00043aef2d030 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c

[ ... ]

> @@ -1812,10 +1810,12 @@ static int ovs_dp_change(struct datapath *dp, struct nlattr *a[])
>  			return err;
>  	}
>  
> -	if ((dp->user_features & OVS_DP_F_TC_RECIRC_SHARING) &&
> +	dp->user_features = user_features;
> +
> +	if ((user_features & OVS_DP_F_TC_RECIRC_SHARING) &&
>  	    !(old_features & OVS_DP_F_TC_RECIRC_SHARING))
>  		tc_skb_ext_tc_enable();

[Severity: High]
This is a pre-existing issue that this patch did not introduce. Can every
failed OVS_DP_CMD_NEW leak one tc_skb_ext_tc reference?

In ovs_dp_cmd_new() the dp is newly allocated, so old_features is 0. A
request that carries OVS_DP_F_TC_RECIRC_SHARING always gets to
tc_skb_ext_tc_enable() here.

If new_vport() then fails, for example with -EEXIST because the name is
already taken, the error path frees dp without a matching disable:

net/openvswitch/datapath.c:ovs_dp_cmd_new() {
    ...
	err = ovs_dp_change(dp, a);
	if (err)
		goto err_unlock_and_destroy_meters;

	vport = new_vport(&parms);
	if (IS_ERR(vport)) {
	...
		goto err_destroy_portids;
	}
    ...
err_destroy_portids:
	kfree(rcu_dereference_raw(dp->upcall_portids));
    ...
err_destroy_dp:
	kfree(dp);
    ...
}

The only disable at teardown is in __dp_destroy(), and this path never
gets there:

	if (dp->user_features & OVS_DP_F_TC_RECIRC_SHARING)
		tc_skb_ext_tc_disable();

ovs_dp_reset_user_features() has the same gap. It sets
dp->user_features = 0 on an existing datapath without a disable. After
that, __dp_destroy() will not drop the reference, and a later SET that
turns the bit back on takes another one.

The commit message says these paths "still leave the key one enable
ahead". OVS_DP_CMD_NEW only needs CAP_NET_ADMIN in a user namespace that
owns the netns. Can't the sequence be repeated, adding one enable for
each failure? For example, create datapath X, then keep sending NEW for
X with TC_RECIRC_SHARING.

That would leave the host-wide key stuck on, and cls_flower and OVS
could no longer turn it off. When the count reaches INT_MAX,
static_key_fast_inc_not_disabled() refuses the increment. The
WARN_ON_ONCE() in static_key_slow_inc_cpuslocked() then fires, and that
panics hosts running with panic_on_warn.

The next patch in the series only changes selftests, so this is still
there after the series. Would it make sense to add a
tc_skb_ext_tc_disable() to the ovs_dp_cmd_new() error path and to
ovs_dp_reset_user_features()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108123424.2825403-1-houminxi%40gmail.com

Re: [PATCH net v2 1/2] net: openvswitch: do not record user features when the change fails

[Minxi Hou]

On 2026-11-09 08:23, Sashiko wrote:
> [Low] Is this list of reference holders accurate? Only cls_flower and
> openvswitch call tc_skb_ext_tc_enable() and tc_skb_ext_tc_disable().

Correct, and v3 says it that way: cls_flower and OVS take and drop
references, and the bridge, the flow dissector and the OVS flow path only
read the key and depend on it staying on while they do.

> [High] Can every failed OVS_DP_CMD_NEW leak one tc_skb_ext_tc reference?
> [High] ovs_dp_reset_user_features() has the same gap.
> Would it make sense to add a tc_skb_ext_tc_disable() to the
> ovs_dp_cmd_new() error path and to ovs_dp_reset_user_features()?

Both are real, and v3 fixes them along with the one the series started
with. All the paths that change or clear the bit now go through
ovs_dp_set_user_features(), which takes or drops the reference as it
records the field, so the four places that write it can no longer
disagree with the count. The failing NEW is the datapath that never
reaches __dp_destroy(), and the reset runs ahead of it for the name that
already exists.

The two tests cover the reset path: an outdated client sends NEW for a
datapath that exists, and the test checks that the datapath keeps
answering sets afterwards.

pw-bot: cr

[PATCH net v2 2/2] selftests/net/openvswitch: check that a failed set keeps the features

[Minxi Hou]

Add a set-dp command to ovs-dpctl for the kernel's datapath SET command,
and a test that uses it.

The test clears the features of a fresh datapath, sets the dispatch bit
with a valid pid list, then sends a set that would set the same bit with
an empty per-cpu-pids. The request has to fail with EINVAL, and the
datapath has to keep reporting the features it had. A second round
covers the other direction: with the recirc bit set, a failing request
that would drop it has to leave it in place.

That round needs CONFIG_NET_TC_SKB_EXT. The selftest config fragment did
not select it, so a kernel built from the fragment rejected the recirc
bit with EOPNOTSUPP and the round could not run. Select the key and the
classful action support it depends on, and skip the test with the
kselftest skip code when the kernel still reports EOPNOTSUPP. Any other
failure of that request fails the test rather than skipping it.

set-dp sends the features it is given. When --features is left out it
sends the features the datapath reports, so a request that only changes
the pid list does not clear the other bits.

Both rounds fail without the fix in the previous patch, which reports
the features the failed request asked for instead of the old ones.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
---
 .../testing/selftests/net/openvswitch/config  |  2 +
 .../selftests/net/openvswitch/openvswitch.sh  | 67 +++++++++++++++++++
 .../selftests/net/openvswitch/ovs-dpctl.py    | 53 +++++++++++++++
 3 files changed, 122 insertions(+)

diff --git a/tools/testing/selftests/net/openvswitch/config b/tools/testing/selftests/net/openvswitch/config
index 15685d242813..d93eac0f08f7 100644
--- a/tools/testing/selftests/net/openvswitch/config
+++ b/tools/testing/selftests/net/openvswitch/config
@@ -2,6 +2,8 @@ CONFIG_GENEVE=m
 CONFIG_INET_DIAG=y
 CONFIG_IP_SCTP=y
 CONFIG_IPV6=y
+CONFIG_NET_CLS_ACT=y
+CONFIG_NET_TC_SKB_EXT=y
 CONFIG_NETFILTER=y
 CONFIG_NETFILTER_ADVANCED=y
 CONFIG_NET_IPGRE=m
diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
index 21444657a040..e2aba1e40d55 100755
--- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
+++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
@@ -24,6 +24,7 @@ tests="
 	nat_connect_v4				ip4-nat-xon: Basic ipv4 tcp connection via NAT
 	nat_related_v4				ip4-nat-related: ICMP related matches work with SNAT
 	netlink_checks				ovsnl: validate netlink attrs and settings
+	dp_features				ovsdp: keep the set features
 	upcall_interfaces			ovs: test the upcall interfaces
 	tunnel_metadata				ovs: test extraction of tunnel metadata
 	drop_reason				drop: test drop reasons are emitted
@@ -1409,6 +1410,72 @@ test_netlink_checks () {
 	return 0
 }
 
+test_dp_features() {
+	dp_features_check() {
+		python3 $ovs_base/ovs-dpctl.py show "$1" |
+			grep -q "features: 0x$2\$"
+	}
+
+	dp_features_set() {
+		python3 $ovs_base/ovs-dpctl.py set-dp "$@"
+	}
+
+	sbx_add "test_dp_features" || return $?
+	ovs_add_dp "test_dp_features" dpfeat || return 1
+
+	# 8 is OVS_DP_F_DISPATCH_UPCALL_PER_CPU, the bit that makes the
+	# kernel read per-cpu-pids.
+	info "clear the datapath features"
+	ovs_sbx "test_dp_features" dp_features_set dpfeat --features 0 \
+	    || return 1
+	ovs_sbx "test_dp_features" dp_features_check dpfeat 0 \
+	    || { info "features were not cleared"; return 1; }
+
+	info "a set with the dispatch bit and pids is accepted"
+	ovs_sbx "test_dp_features" dp_features_set dpfeat --features 8 \
+	    --per-cpu-pids 1,2 || return 1
+	ovs_sbx "test_dp_features" dp_features_check dpfeat 8 \
+	    || { info "the accepted set was not applied"; return 1; }
+
+	ovs_sbx "test_dp_features" dp_features_set dpfeat --features 0 \
+	    || return 1
+
+	info "a set with an empty per-cpu-pids is rejected"
+	ovs_sbx "test_dp_features" dp_features_set dpfeat --features 8 \
+	    --per-cpu-pids "" &> /dev/null
+	[ $? -eq 22 ] \
+	    || { info "the empty per-cpu-pids set did not fail with EINVAL"; \
+		 return 1; }
+
+	info "the rejected set keeps the features it had"
+	ovs_sbx "test_dp_features" dp_features_check dpfeat 0 \
+	    || { info "the rejected set was recorded"; return 1; }
+
+	# 4 is OVS_DP_F_TC_RECIRC_SHARING, which the kernel only accepts when
+	# the tc skb extension is compiled in. A rejected set that drops the
+	# bit must leave the key alone, so the features have to stay as they
+	# were.
+	ovs_sbx "test_dp_features" dp_features_set dpfeat --features 4 \
+	    &> /dev/null
+	rc=$?
+	if [ $rc -eq 95 ]; then
+		info "tc skb extension is not compiled in, skipping recirc"
+		return $ksft_skip
+	elif [ $rc -ne 0 ]; then
+		info "the recirc set failed with $rc"
+		return 1
+	fi
+	info "a rejected set does not drop the recirc bit"
+	ovs_sbx "test_dp_features" dp_features_set dpfeat --features 8 \
+	    --per-cpu-pids "" &> /dev/null
+	[ $? -eq 22 ] \
+	    || { info "the clearing set did not fail with EINVAL"; return 1; }
+	ovs_sbx "test_dp_features" dp_features_check dpfeat 4 \
+	    || { info "the rejected set dropped the recirc bit"; return 1; }
+
+	return 0
+}
+
 test_upcall_interfaces() {
 	sbx_add "test_upcall_interfaces" || return 1
 
diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
index 1615843c225e..1b02d383e49e 100644
--- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
+++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
@@ -2301,6 +2301,24 @@ class OvsDatapath(GenericNetlinkSocket):
 
         return reply
 
+    def set(self, dpname, ifindex=0, features=None, per_cpu_pids=None):
+        msg = OvsDatapath.dp_cmd_msg()
+        msg["cmd"] = OVS_DP_CMD_SET
+        msg["version"] = OVS_DATAPATH_VERSION
+        msg["reserved"] = 0
+        msg["dpifindex"] = ifindex
+        msg["attrs"].append(["OVS_DP_ATTR_NAME", dpname])
+        if features is not None:
+            msg["attrs"].append(["OVS_DP_ATTR_USER_FEATURES", features])
+        if per_cpu_pids is not None:
+            msg["attrs"].append(["OVS_DP_ATTR_PER_CPU_PIDS", per_cpu_pids])
+
+        reply = self.nlm_request(
+            msg, msg_type=self.prid, msg_flags=NLM_F_REQUEST | NLM_F_ACK
+        )
+
+        return reply[0]
+
     def create(
         self, dpname, shouldUpcall=False, versionStr=None, p=OvsPacket()
     ):
@@ -2961,6 +2979,23 @@ def main(argv):
     deldpcmd = subparsers.add_parser("del-dp")
     deldpcmd.add_argument("deldp", help="Datapath Name")
 
+    setdpcmd = subparsers.add_parser("set-dp")
+    setdpcmd.add_argument("setdp", help="Datapath Name")
+    setdpcmd.add_argument(
+        "-f",
+        "--features",
+        type=lambda x: int(x, 0),
+        default=None,
+        help="Datapath user features, defaults to the current value",
+    )
+    setdpcmd.add_argument(
+        "-p",
+        "--per-cpu-pids",
+        type=str,
+        default=None,
+        help="Comma separated upcall pids, an empty string sends none",
+    )
+
     addifcmd = subparsers.add_parser("add-if")
     addifcmd.add_argument("dpname", help="Datapath Name")
     addifcmd.add_argument("addif", help="Interface name for adding")
@@ -3057,6 +3092,24 @@ def main(argv):
             ovspk.upcall_handler(ovsflow)
     elif hasattr(args, "deldp"):
         ovsdp.destroy(args.deldp)
+    elif hasattr(args, "setdp"):
+        rep = ovsdp.info(args.setdp, 0)
+        if rep is None:
+            print("DP '%s' not found." % args.setdp)
+            return 1
+        pids = None
+        if args.per_cpu_pids is not None:
+            pids = [int(x) for x in args.per_cpu_pids.split(",")
+                    if x != ""]
+        features = args.features
+        if features is None:
+            features = rep.get_attr("OVS_DP_ATTR_USER_FEATURES")
+        try:
+            ovsdp.set(args.setdp, rep["dpifindex"], features, pids)
+        except NetlinkError as ne:
+            print("set failed: %s" % ne)
+            return ne.code
+        print("DP '%s' updated" % args.setdp)
     elif hasattr(args, "addif"):
         rep = ovsdp.info(args.dpname, 0)
         if rep is None:
-- 
2.56.0

Re: [PATCH net v2 2/2] selftests/net/openvswitch: check that a failed set keeps the features

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 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] In test_dp_features
  (tools/testing/selftests/net/openvswitch/openvswitch.sh), the…
- [Low] The patch adds CONFIG_NET_CLS_ACT=y and CONFIG_NET_TC_SKB_EXT=y
  to tools/testing/selftests/net/openvswitch/config but leaves out…
- [Low] selftests/net/openvswitch: unsorted CONFIG_NET_TC_SKB_EXT in
  config

--

selftests/net/openvswitch: check that a failed set keeps the features

This adds a set-dp command to ovs-dpctl.py and a new dp_features test to
openvswitch.sh. The test checks that a rejected OVS_DP_CMD_SET leaves the
datapath user features unchanged. It also adds CONFIG_NET_CLS_ACT and
CONFIG_NET_TC_SKB_EXT to the selftest config fragment so the recirc round
can run.

> diff --git a/tools/testing/selftests/net/openvswitch/config b/tools/testing/selftests/net/openvswitch/config
> index 15685d2428139..d93eac0f08f7b 100644
> --- a/tools/testing/selftests/net/openvswitch/config
> +++ b/tools/testing/selftests/net/openvswitch/config
> @@ -2,6 +2,8 @@ CONFIG_GENEVE=m
>  CONFIG_INET_DIAG=y
>  CONFIG_IP_SCTP=y
>  CONFIG_IPV6=y
> +CONFIG_NET_CLS_ACT=y
> +CONFIG_NET_TC_SKB_EXT=y
>  CONFIG_NETFILTER=y
>  CONFIG_NETFILTER_ADVANCED=y
>  CONFIG_NET_IPGRE=m

[Severity: Low]
This isn't in the right place in the sort order.

The fragment is kept sorted with underscores ignored, which is the
order the netdev selftest config checks expect. In that order
CONFIG_NET_CLS_ACT fits where you put it, before CONFIG_NETFILTER.

CONFIG_NET_TC_SKB_EXT does not. Compared without underscores,
NETTCSKBEXT sorts after NETFILTER, NETFILTERADVANCED, NETIPGRE and
NETIPGREDEMUX. It should go after CONFIG_NET_IPGRE_DEMUX=m and before
the CONFIG_NF_* entries, which is also where
selftests/net/forwarding/config has it.

Putting it straight after CONFIG_NET_CLS_ACT=y breaks the ordering
for the rest of the file. Could it be moved down in the next version?

[Severity: Low]
Should CONFIG_NET_SCHED=y be added to the fragment too?

Both new symbols sit inside the if NET_SCHED block in net/sched/Kconfig.
Nothing in the tree selects NET_SCHED, and OPENVSWITCH doesn't either:

net/sched/Kconfig:
    if NET_SCHED
    ...
    config NET_CLS_ACT
    	bool "Actions"
    ...
    config NET_TC_SKB_EXT
    	bool "TC recirculation support"
    	depends on NET_CLS_ACT
    ...
    endif # NET_SCHED

When the fragment is merged onto a base without NET_SCHED (for example
tinyconfig or allnoconfig plus the fragment), merge_config drops both
symbols. ovs_dp_change() then still returns -EOPNOTSUPP for
OVS_DP_F_TC_RECIRC_SHARING, and test_dp_features skips the recirc round.
That is the case the commit message says this change fixes.

hid/config.common and tc-testing/config both list CONFIG_NET_SCHED=y next
to these symbols.

Also, the commit message says:

    Select the key and the classful action support it depends on

The Kconfig prompt for NET_CLS_ACT is "Actions" (tc actions), not
classful actions. Could the wording be adjusted?

> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> index 21444657a0404..e2aba1e40d559 100755
> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh

[ ... ]

> @@ -1409,6 +1410,72 @@ test_netlink_checks () {
>  	return 0
>  }
>  
> +test_dp_features() {

[ ... ]

> +	sbx_add "test_dp_features" || return $?
> +	ovs_add_dp "test_dp_features" dpfeat || return 1
> +

[ ... ]

> +	ovs_sbx "test_dp_features" dp_features_set dpfeat --features 4 \
> +	    &> /dev/null
> +	rc=$?
> +	if [ $rc -eq 95 ]; then
> +		info "tc skb extension is not compiled in, skipping recirc"
> +		return $ksft_skip

[Severity: Medium]
Does this leave the dpfeat datapath behind on kernels without
CONFIG_NET_TC_SKB_EXT?

ovs_add_dp() creates the datapath but only queues its removal:

ovs_add_dp () {
	...
	ovs_sbx "$sbxname" python3 $ovs_base/ovs-dpctl.py add-dp $*
	on_exit "ovs_sbx $sbxname python3 $ovs_base/ovs-dpctl.py del-dp $1;"
}

The queued del-dp only runs from ovs_exit_sig. run_test() calls
ovs_exit_sig for OK and FAIL, but not for a skip:

	elif [ $ret -eq $ksft_skip ]; then
		printf "TEST: %-60s  [SKIP]\n" "${tdesc}"

The script-level EXIT trap doesn't catch this either. run_test runs its
body in a subshell, and ovs_dir is only set inside that subshell.

Every other test that skips after setting up the sandbox (dec_ttl,
icmpv6, psample, drop_reason, ct/nat) calls ovs_exit_sig right before
return $ksft_skip.

Without that call, the dpfeat datapath and its netdev stay on the host
after the run. Later runs then reuse it without any warning, because
add-dp reports "already exists" and exits 0.

Should ovs_exit_sig be called here before returning $ksft_skip?

> +	elif [ $rc -ne 0 ]; then
> +		info "the recirc set failed with $rc"
> +		return 1
> +	fi

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108123424.2825403-1-houminxi%40gmail.com

Re: [PATCH net v2 2/2] selftests/net/openvswitch: check that a failed set keeps the features

[Minxi Hou]

On 2026-11-09 08:23, Sashiko wrote:
> [Medium] In test_dp_features, the skip path does not call ovs_exit_sig.
> [Low] CONFIG_NET_TC_SKB_EXT is not in sort order.
> [Low] Should CONFIG_NET_SCHED=y be added to the fragment too?

All three are in v3. The recirc round is a test of its own now, and it
calls ovs_exit_sig before it returns the skip code, like the other skips
in the middle of a test in that file.

The fragment has CONFIG_NET_SCHED=y, CONFIG_NET_CLS_ACT=y and
CONFIG_NET_TC_SKB_EXT=y, with the last one after CONFIG_NET_IPGRE_DEMUX
in the order that ignores underscores.

Two more changes on the way. The skip reason is printed to stdout, so a
run that skipped the recirc round no longer ends as a plain pass with no
sign that the round did not run. And the expected errno is read from
python rather than written as 95, because EOPNOTSUPP is 45 on alpha and
sparc, 122 on mips and 223 on parisc, where a hardcoded number would
turn a real failure into a skip.