[PATCH net-next] selftests/net/openvswitch: add flow modify test

[PATCH net-next] selftests/net/openvswitch: add flow modify test

[Minxi Hou]

Add mod_flow() and the mod-flow CLI command to ovs-dpctl.py, exercising
OVS_FLOW_CMD_SET. Add test_flow_set which first modifies an existing
flow with new actions and verifies the change via traffic, then modifies
the same flow without actions and verifies the kernel handles the
no-actions case gracefully.

The no-actions path is unreachable from userspace OVS tools (dpctl
mod-flow requires actions) but reachable via raw netlink. This is the
code path where Adrian Moreno found a possible kfree_skb of ERR_PTR
when reply allocation fails after locking.

Make parse() skip OVS_FLOW_ATTR_ACTIONS when actstr is None so the
kernel enters the post-lock allocation branch in ovs_flow_cmd_set().

Suggested-by: Aaron Conole <aconole@redhat.com>
Signed-off-by: Minxi Hou <houminxi@gmail.com>
---
 .../selftests/net/openvswitch/openvswitch.sh  | 72 +++++++++++++++++++
 .../selftests/net/openvswitch/ovs-dpctl.py    | 39 +++++++++-
 2 files changed, 108 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
index a415e9dec8cd..71190bc662f9 100755
--- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
+++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
@@ -30,6 +30,7 @@ tests="
 	drop_reason				drop: test drop reasons are emitted
 	pop_vlan				vlan: POP_VLAN action strips tag
 	dec_ttl					ttl: dec_ttl decrements IP TTL
+	flow_set				flow-set: Flow modify
 	psample					psample: Sampling packets with psample"
 
 info() {
@@ -193,6 +194,23 @@ ovs_add_flow () {
 	return 0
 }
 
+ovs_mod_flow () {
+	if [ -n "$4" ]; then
+		info "Modifying flow: sbx:$1 br:$2 flow:$3 act:$4"
+		ovs_sbx "$1" python3 $ovs_base/ovs-dpctl.py \
+			mod-flow "$2" "$3" "$4"
+	else
+		info "Modifying flow (no actions): sbx:$1 br:$2 flow:$3"
+		ovs_sbx "$1" python3 $ovs_base/ovs-dpctl.py \
+			mod-flow "$2" "$3"
+	fi
+	if [ $? -ne 0 ]; then
+		info "Flow modify [ $3 ] failed"
+		return 1
+	fi
+	return 0
+}
+
 ovs_del_flows () {
 	info "Deleting all flows from DP: sbx:$1 br:$2"
 	ovs_sbx "$1" python3 $ovs_base/ovs-dpctl.py del-flows "$2"
@@ -300,6 +318,60 @@ test_dec_ttl() {
 	return 0
 }
 
+test_flow_set() {
+	sbx_add "test_flow_set" || return $?
+	ovs_add_dp "test_flow_set" flowset || return 1
+
+	info "create namespaces"
+	for ns in client server; do
+		ovs_add_netns_and_veths "test_flow_set" "flowset" "$ns" \
+			"${ns:0:1}0" "${ns:0:1}1" || return 1
+	done
+
+	ip netns exec client ip addr add 10.0.0.1/24 dev c1
+	ip netns exec client ip link set c1 up
+	ip netns exec server ip addr add 10.0.0.2/24 dev s1
+	ip netns exec server ip link set s1 up
+
+	ovs_add_flow "test_flow_set" flowset \
+		'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1
+	ovs_add_flow "test_flow_set" flowset \
+		'in_port(2),eth(),eth_type(0x0806),arp()' '1' || return 1
+
+	local fwd_flow="ufid:00000001-0002-0003-0004-000500060007"
+	fwd_flow="$fwd_flow,in_port(1),eth(),eth_type(0x0800),ipv4()"
+
+	ovs_add_flow "test_flow_set" flowset "$fwd_flow" '2' \
+		|| return 1
+	ovs_add_flow "test_flow_set" flowset \
+		'in_port(2),eth(),eth_type(0x0800),ipv4()' '1' || return 1
+
+	info "verify initial forwarding"
+	ovs_sbx "test_flow_set" ip netns exec client ping -c 1 -W 2 \
+		10.0.0.2 || return 1
+
+	info "mod-flow with new actions (change to drop)"
+	ovs_mod_flow "test_flow_set" flowset "$fwd_flow" 'drop' \
+		|| return 1
+
+	info "verify traffic is now dropped"
+	ovs_sbx "test_flow_set" ip netns exec client ping -c 1 -W 2 \
+		10.0.0.2 >/dev/null 2>&1 \
+		&& { info "FAIL: ping should fail after mod-flow to drop"
+		     return 1; }
+
+	info "mod-flow without actions"
+	ovs_mod_flow "test_flow_set" flowset "$fwd_flow" || return 1
+
+	info "verify drop actions unchanged"
+	ovs_sbx "test_flow_set" ip netns exec client ping -c 1 -W 2 \
+		10.0.0.2 >/dev/null 2>&1 \
+		&& { info "FAIL: ping should still fail after no-actions set"
+		     return 1; }
+
+	return 0
+}
+
 # psample test
 # - use psample to observe packets
 test_psample() {
diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
index 3342e295293d..9113473d425f 100644
--- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
+++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
@@ -2702,9 +2702,10 @@ class OvsFlow(GenericNetlinkSocket):
             self["attrs"].append(["OVS_FLOW_ATTR_KEY", k])
             self["attrs"].append(["OVS_FLOW_ATTR_MASK", m])
 
-            a = ovsactions()
-            a.parse(actstr)
-            self["attrs"].append(["OVS_FLOW_ATTR_ACTIONS", a])
+            if actstr is not None:
+                a = ovsactions()
+                a.parse(actstr)
+                self["attrs"].append(["OVS_FLOW_ATTR_ACTIONS", a])
 
     def __init__(self):
         GenericNetlinkSocket.__init__(self)
@@ -2738,6 +2739,24 @@ class OvsFlow(GenericNetlinkSocket):
             raise ne
         return reply
 
+    def mod_flow(self, dpifindex, flowmsg):
+        flowmsg["cmd"] = OVS_FLOW_CMD_SET
+        flowmsg["version"] = OVS_DATAPATH_VERSION
+        flowmsg["reserved"] = 0
+        flowmsg["dpifindex"] = dpifindex
+
+        try:
+            reply = self.nlm_request(
+                flowmsg,
+                msg_type=self.prid,
+                msg_flags=NLM_F_REQUEST | NLM_F_ACK,
+            )
+            reply = reply[0]
+        except NetlinkError as ne:
+            print(flowmsg)
+            raise ne
+        return reply
+
     def del_flows(self, dpifindex):
         """
         Send a del message to the kernel that will drop all flows.
@@ -3013,6 +3032,12 @@ def main(argv):
     addflcmd.add_argument("flow", help="Flow specification")
     addflcmd.add_argument("acts", help="Flow actions")
 
+    modflcmd = subparsers.add_parser("mod-flow")
+    modflcmd.add_argument("modbr", help="Datapath name")
+    modflcmd.add_argument("modflow", help="Flow specification")
+    modflcmd.add_argument("modacts", help="Flow actions",
+                          nargs="?", default=None)
+
     delfscmd = subparsers.add_parser("del-flows")
     delfscmd.add_argument("flsbr", help="Datapath name")
 
@@ -3110,6 +3135,14 @@ def main(argv):
         flow = OvsFlow.ovs_flow_msg()
         flow.parse(args.flow, args.acts, rep["dpifindex"])
         ovsflow.add_flow(rep["dpifindex"], flow)
+    elif hasattr(args, "modbr"):
+        rep = ovsdp.info(args.modbr, 0)
+        if rep is None:
+            print("DP '%s' not found." % args.modbr)
+            return 1
+        flow = OvsFlow.ovs_flow_msg()
+        flow.parse(args.modflow, args.modacts, rep["dpifindex"])
+        ovsflow.mod_flow(rep["dpifindex"], flow)
     elif hasattr(args, "flsbr"):
         rep = ovsdp.info(args.flsbr, 0)
         if rep is None:
-- 
2.54.0

Re: [PATCH net-next] selftests/net/openvswitch: add flow modify test

[Aaron Conole]

Minxi Hou <houminxi@gmail.com> writes:

> Add mod_flow() and the mod-flow CLI command to ovs-dpctl.py, exercising
> OVS_FLOW_CMD_SET. Add test_flow_set which first modifies an existing
> flow with new actions and verifies the change via traffic, then modifies
> the same flow without actions and verifies the kernel handles the
> no-actions case gracefully.
>
> The no-actions path is unreachable from userspace OVS tools (dpctl
> mod-flow requires actions) but reachable via raw netlink. This is the
> code path where Adrian Moreno found a possible kfree_skb of ERR_PTR
> when reply allocation fails after locking.
>
> Make parse() skip OVS_FLOW_ATTR_ACTIONS when actstr is None so the
> kernel enters the post-lock allocation branch in ovs_flow_cmd_set().
>
> Suggested-by: Aaron Conole <aconole@redhat.com>
> Signed-off-by: Minxi Hou <houminxi@gmail.com>
> ---

Thanks for the really quick turnaround.  I'll do a deeper review of the
code on Monday; I wanted to reply though because I was expecting to see
a splat in the selftest, but instead it passed.  This confused me at
first, because I didn't realize that the net- targeted patches were also
applied in the net-next test run:

https://github.com/linux-netdev/testing/commit/30e78e3cb99d9abb5182c387abdf5ede29fb9589  ("net: openvswitch: fix possible kfree_skb of ERR_PTR")

But we will want to make sure that whenever this selftest would finally
get applied the above kfree_skb fix is also applied.

>  .../selftests/net/openvswitch/openvswitch.sh  | 72 +++++++++++++++++++
>  .../selftests/net/openvswitch/ovs-dpctl.py    | 39 +++++++++-
>  2 files changed, 108 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> index a415e9dec8cd..71190bc662f9 100755
> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> @@ -30,6 +30,7 @@ tests="
>  	drop_reason				drop: test drop reasons are emitted
>  	pop_vlan				vlan: POP_VLAN action strips tag
>  	dec_ttl					ttl: dec_ttl decrements IP TTL
> +	flow_set				flow-set: Flow modify
>  	psample					psample: Sampling packets with psample"
>  
>  info() {
> @@ -193,6 +194,23 @@ ovs_add_flow () {
>  	return 0
>  }
>  
> +ovs_mod_flow () {
> +	if [ -n "$4" ]; then
> +		info "Modifying flow: sbx:$1 br:$2 flow:$3 act:$4"
> +		ovs_sbx "$1" python3 $ovs_base/ovs-dpctl.py \
> +			mod-flow "$2" "$3" "$4"
> +	else
> +		info "Modifying flow (no actions): sbx:$1 br:$2 flow:$3"
> +		ovs_sbx "$1" python3 $ovs_base/ovs-dpctl.py \
> +			mod-flow "$2" "$3"
> +	fi
> +	if [ $? -ne 0 ]; then
> +		info "Flow modify [ $3 ] failed"
> +		return 1
> +	fi
> +	return 0
> +}
> +
>  ovs_del_flows () {
>  	info "Deleting all flows from DP: sbx:$1 br:$2"
>  	ovs_sbx "$1" python3 $ovs_base/ovs-dpctl.py del-flows "$2"
> @@ -300,6 +318,60 @@ test_dec_ttl() {
>  	return 0
>  }
>  
> +test_flow_set() {
> +	sbx_add "test_flow_set" || return $?
> +	ovs_add_dp "test_flow_set" flowset || return 1
> +
> +	info "create namespaces"
> +	for ns in client server; do
> +		ovs_add_netns_and_veths "test_flow_set" "flowset" "$ns" \
> +			"${ns:0:1}0" "${ns:0:1}1" || return 1
> +	done
> +
> +	ip netns exec client ip addr add 10.0.0.1/24 dev c1
> +	ip netns exec client ip link set c1 up
> +	ip netns exec server ip addr add 10.0.0.2/24 dev s1
> +	ip netns exec server ip link set s1 up
> +
> +	ovs_add_flow "test_flow_set" flowset \
> +		'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1
> +	ovs_add_flow "test_flow_set" flowset \
> +		'in_port(2),eth(),eth_type(0x0806),arp()' '1' || return 1
> +
> +	local fwd_flow="ufid:00000001-0002-0003-0004-000500060007"
> +	fwd_flow="$fwd_flow,in_port(1),eth(),eth_type(0x0800),ipv4()"
> +
> +	ovs_add_flow "test_flow_set" flowset "$fwd_flow" '2' \
> +		|| return 1
> +	ovs_add_flow "test_flow_set" flowset \
> +		'in_port(2),eth(),eth_type(0x0800),ipv4()' '1' || return 1
> +
> +	info "verify initial forwarding"
> +	ovs_sbx "test_flow_set" ip netns exec client ping -c 1 -W 2 \
> +		10.0.0.2 || return 1
> +
> +	info "mod-flow with new actions (change to drop)"
> +	ovs_mod_flow "test_flow_set" flowset "$fwd_flow" 'drop' \
> +		|| return 1
> +
> +	info "verify traffic is now dropped"
> +	ovs_sbx "test_flow_set" ip netns exec client ping -c 1 -W 2 \
> +		10.0.0.2 >/dev/null 2>&1 \
> +		&& { info "FAIL: ping should fail after mod-flow to drop"
> +		     return 1; }
> +
> +	info "mod-flow without actions"
> +	ovs_mod_flow "test_flow_set" flowset "$fwd_flow" || return 1
> +
> +	info "verify drop actions unchanged"
> +	ovs_sbx "test_flow_set" ip netns exec client ping -c 1 -W 2 \
> +		10.0.0.2 >/dev/null 2>&1 \
> +		&& { info "FAIL: ping should still fail after no-actions set"
> +		     return 1; }
> +
> +	return 0
> +}
> +
>  # psample test
>  # - use psample to observe packets
>  test_psample() {
> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> index 3342e295293d..9113473d425f 100644
> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> @@ -2702,9 +2702,10 @@ class OvsFlow(GenericNetlinkSocket):
>              self["attrs"].append(["OVS_FLOW_ATTR_KEY", k])
>              self["attrs"].append(["OVS_FLOW_ATTR_MASK", m])
>  
> -            a = ovsactions()
> -            a.parse(actstr)
> -            self["attrs"].append(["OVS_FLOW_ATTR_ACTIONS", a])
> +            if actstr is not None:
> +                a = ovsactions()
> +                a.parse(actstr)
> +                self["attrs"].append(["OVS_FLOW_ATTR_ACTIONS", a])
>  
>      def __init__(self):
>          GenericNetlinkSocket.__init__(self)
> @@ -2738,6 +2739,24 @@ class OvsFlow(GenericNetlinkSocket):
>              raise ne
>          return reply
>  
> +    def mod_flow(self, dpifindex, flowmsg):
> +        flowmsg["cmd"] = OVS_FLOW_CMD_SET
> +        flowmsg["version"] = OVS_DATAPATH_VERSION
> +        flowmsg["reserved"] = 0
> +        flowmsg["dpifindex"] = dpifindex
> +
> +        try:
> +            reply = self.nlm_request(
> +                flowmsg,
> +                msg_type=self.prid,
> +                msg_flags=NLM_F_REQUEST | NLM_F_ACK,
> +            )
> +            reply = reply[0]
> +        except NetlinkError as ne:
> +            print(flowmsg)
> +            raise ne
> +        return reply
> +
>      def del_flows(self, dpifindex):
>          """
>          Send a del message to the kernel that will drop all flows.
> @@ -3013,6 +3032,12 @@ def main(argv):
>      addflcmd.add_argument("flow", help="Flow specification")
>      addflcmd.add_argument("acts", help="Flow actions")
>  
> +    modflcmd = subparsers.add_parser("mod-flow")
> +    modflcmd.add_argument("modbr", help="Datapath name")
> +    modflcmd.add_argument("modflow", help="Flow specification")
> +    modflcmd.add_argument("modacts", help="Flow actions",
> +                          nargs="?", default=None)
> +
>      delfscmd = subparsers.add_parser("del-flows")
>      delfscmd.add_argument("flsbr", help="Datapath name")
>  
> @@ -3110,6 +3135,14 @@ def main(argv):
>          flow = OvsFlow.ovs_flow_msg()
>          flow.parse(args.flow, args.acts, rep["dpifindex"])
>          ovsflow.add_flow(rep["dpifindex"], flow)
> +    elif hasattr(args, "modbr"):
> +        rep = ovsdp.info(args.modbr, 0)
> +        if rep is None:
> +            print("DP '%s' not found." % args.modbr)
> +            return 1
> +        flow = OvsFlow.ovs_flow_msg()
> +        flow.parse(args.modflow, args.modacts, rep["dpifindex"])
> +        ovsflow.mod_flow(rep["dpifindex"], flow)
>      elif hasattr(args, "flsbr"):
>          rep = ovsdp.info(args.flsbr, 0)
>          if rep is None:

Re: [PATCH net-next] selftests/net/openvswitch: add flow modify test

[Ilya Maximets]

On 6/6/26 1:42 AM, 侯敏熙 wrote:
> Also sending a v2 shortly
> to fix two pylint warnings that nipa flagged (+2 new warnings: missing
> docstring on mod_flow, and a %-format string).
Hi, Minxi.  Thanks for working on all the test improvements, but,
please, do not send new versions just yet.  Wait for Aaron's review.

Simply re-sending the same patch with minor changes flagged by CI
creates a lot of churn and discourages people from reviewing the
series, as they don't really have time to look at version N while
there is already N+1.

As a general suggestion, try to wait for a human review.  It's hard
to define what the "good" amount of waiting time should be.  But if
the number of human replies is noticeably smaller than the patch
version number, that might be a good sign to slow down a little.

I would also suggest to wait for the Adrian's fix to be merged and
synced into net-next before sending the new version of the test to
avoid any potential mishaps.

Best regards, Ilya Maximets.