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

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

[Minxi Hou]

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 +--
 .../selftests/net/openvswitch/openvswitch.sh  | 64 +++++++++++++++++++
 .../selftests/net/openvswitch/ovs-dpctl.py    | 50 +++++++++++++++
 3 files changed, 119 insertions(+), 5 deletions(-)


base-commit: 30c71476d842b094651bd7712f6c8ee8716147d0
-- 
2.56.0

[PATCH net v1 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. That key is a
reference count shared with cls_flower, so the count ends up one short
and reaches zero while other users still hold a reference. 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. The opposite
ordering is possible as well: a failing request that drops the bit
leaves the key enabled for good.

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 request 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.

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 v1 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 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

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 "A failed request then leaves both the
  features and the key alone".
- [Low] The commit message understates who can trigger the fixed bug and
  what happens.

--

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

This patch moves the store of the new features into dp->user_features in
ovs_dp_change() so it happens after ovs_dp_set_upcall_portids() succeeds.
The enable and disable decisions for the tc skb extension static key now
use the local user_features variable. A failed request should no longer
leave the recorded features and the key out of step.

> That key is a
> reference count shared with cls_flower, so the count ends up one short
> and reaches zero while other users still hold a reference.

[Severity: Low]
Could the commit message say more about who can trigger this and what
happens?

OVS_DP_CMD_SET and OVS_DP_CMD_NEW use GENL_UNS_ADMIN_PERM, and
dp_datapath_genl_family sets netnsok, so CAP_NET_ADMIN in a user namespace
is enough. The ops use GENL_DONT_VALIDATE_STRICT and datapath_policy has
no entry for OVS_DP_ATTR_PER_CPU_PIDS. An empty attribute therefore gets
through validation and reaches the -EINVAL in ovs_dp_set_upcall_portids().

tc_skb_ext_tc is a single global key in net/sched/cls_api.c, read by tc,
the bridge, the flow dissector and OVS. One crafted SET from a container
can change skb-ext behaviour for the whole host.

When OVS is the only user of the key, the pre-patch sequence is: a failed
SET records the bit, then a later SET clears it. That calls
tc_skb_ext_tc_disable() with the count at zero, which hits the WARN in
static_key_dec_not_one():

kernel/jump_label.c:static_key_dec_not_one() {
    ...
	if (WARN_ON_ONCE(v == 0))
		return true;
    ...
}

When cls_flower does hold a reference, its own later disable hits the same
WARN. With panic_on_warn set, the system panics.

Would it be worth mentioning the WARN and the user namespace reachability
in the changelog, so stable and security triage can weigh the fix?

> 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: Medium]
The commit message says "A failed request then leaves both the features
and the key alone". Is that true for a failed OVS_DP_CMD_NEW?

In ovs_dp_cmd_new(), ovs_dp_change() runs on a fresh dp where
old_features is 0. A request that carries OVS_DP_F_TC_RECIRC_SHARING
therefore takes the tc_skb_ext_tc_enable() branch above.

new_vport() can then fail. Examples are -EEXIST when a netdev with that
name already exists, -ENOMEM, or an ifindex conflict:

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_unlock_and_destroy_meters:
	ovs_unlock();
	ovs_meters_exit(dp);
    ...
err_destroy_dp:
	kfree(dp);
    ...
}

That unwind never calls tc_skb_ext_tc_disable(). The dp was never added
to ovs_net->dps, so __dp_destroy() does not run for it either.

Does each such request leak one reference on the global tc_skb_ext_tc
key? That would leave the key "enabled for good", which the commit
message lists as part of the bug.

The -EEXIST branch has a second path. When genlhdr->version is below
OVS_DP_VER_FEATURES, it calls ovs_dp_reset_user_features() on the
existing datapath:

net/openvswitch/datapath.c:ovs_dp_reset_user_features() {
    ...
	dp->user_features = 0;
}

This clears OVS_DP_F_TC_RECIRC_SHARING without calling
tc_skb_ext_tc_disable(). __dp_destroy() decides from dp->user_features:

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

so that reference is never dropped. A later SET that sets the bit again
takes another one.

The commit message does name both paths and leaves them out of scope. Both
end in the same state this patch fixes in ovs_dp_change(): the bit in
dp->user_features no longer matches exactly one held key reference. Both
can be reached with CAP_NET_ADMIN in a user namespace, and the effect
applies to the whole host.

Two possible fixes:

- In the NEW unwind, call tc_skb_ext_tc_disable() when dp->user_features
  has the bit, or delay the enable until the request succeeds.

- In ovs_dp_reset_user_features(), call tc_skb_ext_tc_disable() when it
  clears the bit.

Since the patch has Cc: stable, could the closing sentence be narrowed?
Otherwise backporters may read it as meaning failed requests now keep the
key balanced in all cases.

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

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

[Minxi Hou]

Both points are right, and v2 changes the description.

The Medium one is a wording defect. "A failed request" reads as covering
every request that fails, and the paths you list do still leave the key
away from the features: the NEW unwind after new_vport() fails, and
ovs_dp_reset_user_features() on the -EEXIST branch. They predate this
change and stay out of it, as the changelog says, but the closing
sentence promised more than ovs_dp_change() can deliver. v2 says "A
failed OVS_DP_CMD_SET then leaves both the features and the key alone",
and adds that a failed OVS_DP_CMD_NEW and the reset that follows it still
leave the key one enable ahead.

For the Low one, v2 names the reachability and the effect. The request
needs only CAP_NET_ADMIN, which a user namespace that owns a network
namespace carries. 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. A disable on a count of
zero reaches the WARN_ON_ONCE in static_key_dec_not_one(), which panics a
host that runs with panic_on_warn.

pw-bot: cr

[PATCH net v1 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 case needs
CONFIG_NET_TC_SKB_EXT and is skipped without it.

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>
---
 .../selftests/net/openvswitch/openvswitch.sh  | 64 +++++++++++++++++++
 .../selftests/net/openvswitch/ovs-dpctl.py    | 50 +++++++++++++++
 2 files changed, 114 insertions(+)

diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
index 21444657a040..f26c01a17b49 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,69 @@ 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 only exists 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
+	if [ $? -eq 0 ]; then
+		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; }
+	else
+		info "tc skb extension is not available, skipping recirc"
+	fi
+
+	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..be41ec72faf2 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",
+    )
+    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,21 @@ 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 != ""]
+        try:
+            ovsdp.set(args.setdp, rep["dpifindex"], args.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 v1 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 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

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 recirc-sharing round of the new test_dp_features()
  selftest (tools/testing/selftests/net/openvswitch/openvswitch.sh) can…
- [Low] The new `set-dp` subcommand in ovs-dpctl.py makes `--features`
  optional (default None), and OvsDatapath.set() only appends…

--

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

This patch adds a set-dp subcommand to ovs-dpctl.py that issues
OVS_DP_CMD_SET. It also adds a test_dp_features selftest. The test checks
that a rejected set request leaves the datapath user features unchanged,
first for the dispatch bit and then for the recirc sharing bit.

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

[ ... ]

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

[ ... ]

> +	# 4 is OVS_DP_F_TC_RECIRC_SHARING, which only exists 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
> +	if [ $? -eq 0 ]; then
> +		info "a rejected set does not drop the recirc bit"

[ ... ]

> +	else
> +		info "tc skb extension is not available, skipping recirc"
> +	fi
> +
> +	return 0
> +}

[Severity: Medium]
Does this probe skip the recirc round on any failure, and not only when
CONFIG_NET_TC_SKB_EXT is missing?

ovs_dp_change() reports the missing option with -EOPNOTSUPP:

#if !IS_ENABLED(CONFIG_NET_TC_SKB_EXT)
		if (user_features & OVS_DP_F_TC_RECIRC_SHARING)
			return -EOPNOTSUPP;
#endif

In that case set-dp exits with 95. Other failures also reach the else
branch, for example a Python exception, a missing datapath (both exit 1)
or a different errno.

In the else branch, info only writes to debug.log unless -v is given. The
function then returns 0, so run_test reports PASS rather than SKIP.

Could the probe check for 95 explicitly and return $ksft_skip, or at least
make the skip visible?

Also, tools/testing/selftests/net/openvswitch/config has neither
CONFIG_NET_CLS_ACT nor CONFIG_NET_TC_SKB_EXT. NET_TC_SKB_EXT is a bool
with no default, and openvswitch does not select it. Kernels built from
this fragment would therefore always skip this round.

This is the only round that touches OVS_DP_F_TC_RECIRC_SHARING and the
tc_skb_ext static key. The first round only toggles
OVS_DP_F_DISPATCH_UPCALL_PER_CPU, which never touches the key.

Should CONFIG_NET_CLS_ACT=y and CONFIG_NET_TC_SKB_EXT=y be added to the
config fragment? Without them, the statement "Both rounds fail without
the fix in the previous patch" in the commit message would not hold for
CI runs.

> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> index 1615843c225e4..be41ec72faf2a 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])

[Severity: Low]
When --features is left out, OVS_DP_ATTR_USER_FEATURES is not sent.
ovs_dp_change() starts with user_features at 0 and stores it on every
successful set:

	u32 user_features = 0, old_features = dp->user_features;
	...
	dp->user_features = user_features;

Does that mean "ovs-dpctl.py set-dp <dp>" with no --features clears every
feature bit on the datapath?

Running "set-dp <dp> --per-cpu-pids 1,2" without "--features 8" also
looks surprising. The pid handling only runs under this check:

	if (user_features & OVS_DP_F_DISPATCH_UPCALL_PER_CPU &&
	    a[OVS_DP_ATTR_PER_CPU_PIDS]) {

So the pid list is ignored and the features are cleared, but the tool
still prints "DP '...' updated" and exits 0.

The set-dp handler in main() already calls ovsdp.info(args.setdp, 0).
Could args.features default to the current OVS_DP_ATTR_USER_FEATURES from
that reply? Alternatively, the "Datapath user features" help text could
say that leaving out --features clears the features.

The new test always passes --features, so the test itself is not
affected.

[ ... ]

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

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

[Minxi Hou]

Both are real, and v2 fixes them.

The Medium one: the else branch treated any failure as "the key is not
there". v2 reads the exit status of the recirc set, takes EOPNOTSUPP
(95, what set-dp returns when ovs_dp_change() rejects the recirc bit
under !IS_ENABLED(CONFIG_NET_TC_SKB_EXT)) as the reason to return
$ksft_skip, and fails the test on any other status.

You are also right about the fragment. tools/testing/selftests/net/
openvswitch/config selected neither CONFIG_NET_CLS_ACT nor
CONFIG_NET_TC_SKB_EXT, so a kernel built from it could never run that
round, and the claim that both rounds fail without the fix did not hold
for such a build. v2 adds both symbols. I built a kernel from the
fragment before the change and one from the fragment after it: the first
reports the skip with the test harness marking it SKIP, the second has
both symbols set and runs the round.

The Low one: ovs_dp_change() starts user_features at 0, so a set-dp
without --features cleared every bit, and a pid list sent without the
dispatch bit was ignored while the command still printed "updated". v2
reads the current features from the GET reply and sends those when
--features is left out, and the help text says so.