On 10/6/26 15:04, Timothy Redaelli wrote:
The 'filter' column of the Mirror table was parsed with
parse_ofp_exact_flow(), which requires every field that is mentioned to
be given as an exact value. Specifications like "ip,nw_src=10.0.0.0/24"
were therefore rejected, even though the matching side already stores the
filter as a miniflow plus minimask and unions that mask into the
megaflow, so partially masked fields work end to end.
Add parse_ofp_masked_flow(), which parses a flow specification into a
'struct flow' and 'struct flow_wildcards' and lets each value carry a
mask. Use it for the mirror filter. Like parse_ofp_exact_flow(), it
rejects fields whose prerequisites are not met, fields that are set
more than once and fields given without a value.
Both parsers now share a helper, parse_ofp_flow__(), driven by a 'masked'
flag: it parses each value with mf_parse() (masked) or mf_parse_value()
(exact) and applies it with the generic mf_set_flow_value[_masked]() /
mf_mask_field[_masked]() primitives. This keeps the exact path's
behaviour unchanged while adding masked support and avoiding duplicated
parsing logic. parse_ofp_exact_flow() is left in place, as its other
callers do want exact values.
We should simplify this as there's too a bit too much detail and
internal code references.
Reported-at: https://github.com/openvswitch/ovs-issues/issues/349
Reported-at: https://redhat.atlassian.net/browse/FDP-2474
Co-authored-by: Kevin Traynor <[email protected]>
Signed-off-by: Kevin Traynor <[email protected]>
Signed-off-by: Timothy Redaelli <[email protected]>
We should add:
Assisted-by: Claude Opus 4.8, Claude Code
Otherwise LGTM. (One minor comment below that doesn't require a code change)
Let's give time in case there are other comments. If not, no need to
respin, I can take care of the commit message on apply. Thanks.
---
v3:
- Share parsing with parse_ofp_exact_flow() via a common
parse_ofp_flow__() helper, which also rejects empty values
again. (Kevin)
- Add a negative test case for a field without a value.
- Rebased on current main.
v2:
- Restore the prerequisite and duplicate field checks that
parse_ofp_exact_flow() does. (Mike)
- Add negative test cases for them.
- Rebased on current main.
NEWS | 3 ++
include/openvswitch/ofp-flow.h | 4 ++
lib/ofp-flow.c | 77 +++++++++++++++++++++++++++-------
ofproto/ofproto-dpif-mirror.c | 6 +--
tests/ofproto-dpif.at | 68 ++++++++++++++++++++++++++++++
vswitchd/vswitch.xml | 8 ++--
6 files changed, 145 insertions(+), 21 deletions(-)
diff --git a/NEWS b/NEWS
index de1a030ad..33ab0585b 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,8 @@
Post-v4.0.0
--------------------
+ - ovs-vswitchd:
+ * Mirror filters now accept masked fields, e.g. "ip,nw_src=10.0.0.0/24"
+ in the "filter" column of the Mirror table.
v4.0.0 - 17 Aug 2026
diff --git a/include/openvswitch/ofp-flow.h b/include/openvswitch/ofp-flow.h
index f2223d90b..ad39dc04a 100644
--- a/include/openvswitch/ofp-flow.h
+++ b/include/openvswitch/ofp-flow.h
@@ -155,6 +155,10 @@ char *parse_ofp_exact_flow(struct flow *flow, struct
flow_wildcards *wc,
const struct tun_table *tun_table, const char *s,
const struct ofputil_port_map *port_map);
+char *parse_ofp_masked_flow(struct flow *flow, struct flow_wildcards *wc,
+ const struct tun_table *tun_table, const char *s,
+ const struct ofputil_port_map *port_map);
+
/* Flow stats or aggregate stats request, independent of protocol. */
struct ofputil_flow_stats_request {
bool aggregate; /* Aggregate results? */
diff --git a/lib/ofp-flow.c b/lib/ofp-flow.c
index 3bc744f78..fcf19ee22 100644
--- a/lib/ofp-flow.c
+++ b/lib/ofp-flow.c
@@ -1916,19 +1916,24 @@ parse_ofp_flow_mod_file(const char *file_name,
return NULL;
}
-/* Parses a specification of a flow from 's' into 'flow'. 's' must take the
- * form FIELD=VALUE[,FIELD=VALUE]... where each FIELD is the name of a
- * mf_field. Fields must be specified in a natural order for satisfying
- * prerequisites. If 'wc' is specified, masks the field in 'wc' for each of the
- * field specified in flow. If the map, 'names_portno' is specfied, converts
- * the in_port name into port no while setting the 'flow'.
+/* Parses a specification of a flow from 's' into 'flow' (and 'wc', if
+ * nonnull). 's' must take the form FIELD=VALUE[,FIELD=VALUE]... where each
+ * FIELD is the name of an mf_field. Fields must be specified in a natural
+ * order for satisfying prerequisites. If 'wc' is specified, masks the field
+ * in 'wc' for each field specified in 'flow'. If the map 'port_map' is
+ * specified, converts the in_port name into port number while setting the
+ * 'flow'.
+ *
+ * If 'masked' is true, each VALUE may include a mask (e.g.
+ * "nw_src=10.0.0.0/24"), so a field can be partially wildcarded; otherwise
+ * every field must be given as an exact value.
*
* Returns NULL on success, otherwise a malloc()'d string that explains the
* problem. */
-char *
-parse_ofp_exact_flow(struct flow *flow, struct flow_wildcards *wc,
- const struct tun_table *tun_table, const char *s,
- const struct ofputil_port_map *port_map)
+static char *
+parse_ofp_flow__(struct flow *flow, struct flow_wildcards *wc,
+ const struct tun_table *tun_table, const char *s,
+ const struct ofputil_port_map *port_map, bool masked)
{
char *pos, *key, *value_s;
char *error = NULL;
@@ -1966,7 +1971,7 @@ parse_ofp_exact_flow(struct flow *flow, struct
flow_wildcards *wc,
}
} else {
const struct mf_field *mf;
- union mf_value value;
+ union mf_value value, mask;
char *field_error;
mf = mf_from_name(key);
@@ -1986,7 +1991,11 @@ parse_ofp_exact_flow(struct flow *flow, struct
flow_wildcards *wc,
goto exit;
}
- field_error = mf_parse_value(mf, value_s, port_map, &value);
+ if (masked) {
+ field_error = mf_parse(mf, value_s, port_map, &value, &mask);
+ } else {
+ field_error = mf_parse_value(mf, value_s, port_map, &value);
+ }
if (field_error) {
error = xasprintf("%s: bad value for %s (%s)",
s, key, field_error);
@@ -1994,9 +2003,16 @@ parse_ofp_exact_flow(struct flow *flow, struct
flow_wildcards *wc,
goto exit;
}
- mf_set_flow_value(mf, &value, flow);
- if (wc) {
- mf_mask_field(mf, wc);
+ if (masked) {
+ mf_set_flow_value_masked(mf, &value, &mask, flow);
+ if (wc) {
In the current code, this check is redundant because of the ovs_assert
in parse_ofp_masked_flow(). I'd still be inclined to keep it to be
defensive as there's no check for a (masked && !wc) combination earlier
in this fn.
+ mf_mask_field_masked(mf, &mask, wc);
+ }
+ } else {
+ mf_set_flow_value(mf, &value, flow);
+ if (wc) {
+ mf_mask_field(mf, wc);
+ }
}
}
}
@@ -2016,3 +2032,34 @@ exit:
}
return error;
}
+
+/* Parses a specification of a flow from 's' into 'flow'. Each field must be
+ * given as an exact value; masks are not accepted. See parse_ofp_flow__() for
+ * the full description of the syntax and the other arguments.
+ *
+ * Returns NULL on success, otherwise a malloc()'d string that explains the
+ * problem. */
+char *
+parse_ofp_exact_flow(struct flow *flow, struct flow_wildcards *wc,
+ const struct tun_table *tun_table, const char *s,
+ const struct ofputil_port_map *port_map)
+{
+ return parse_ofp_flow__(flow, wc, tun_table, s, port_map, false);
+}
+
+/* Parses a specification of a flow from 's' into 'flow' and 'wc'. Unlike
+ * parse_ofp_exact_flow(), each value may include a mask (e.g.
+ * "nw_src=10.0.0.0/24"), so a field can be partially wildcarded. 'wc' must be
+ * nonnull, since it receives the mask for each field. See parse_ofp_flow__()
+ * for the full description of the syntax and the other arguments.
+ *
+ * Returns NULL on success, otherwise a malloc()'d string that explains the
+ * problem. */
+char *
+parse_ofp_masked_flow(struct flow *flow, struct flow_wildcards *wc,
+ const struct tun_table *tun_table, const char *s,
+ const struct ofputil_port_map *port_map)
+{
+ ovs_assert(wc);
+ return parse_ofp_flow__(flow, wc, tun_table, s, port_map, true);
+}
diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c
index e8a2830fb..4159289c3 100644
--- a/ofproto/ofproto-dpif-mirror.c
+++ b/ofproto/ofproto-dpif-mirror.c
@@ -336,9 +336,9 @@ mirror_set(struct mbridge *mbridge, const struct ofproto
*ofproto,
char *err;
ofproto_append_ports_to_map(&map, ofproto->ports);
- err = parse_ofp_exact_flow(&flow, &wc,
- ofproto_get_tun_tab(ofproto),
- ms->filter, &map);
+ err = parse_ofp_masked_flow(&flow, &wc,
+ ofproto_get_tun_tab(ofproto),
+ ms->filter, &map);
ofputil_port_map_destroy(&map);
if (err) {
VLOG_WARN("filter is invalid: %s", err);
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index efb36e058..256f5d68d 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -5908,6 +5908,74 @@ OVS_VSWITCHD_STOP(["/filter is invalid: invalid: unknown
field invalid/d
/mirror mymirror configuration is invalid/d"])
AT_CLEANUP
+AT_SETUP([ofproto-dpif - mirroring, filter with wildcards])
+AT_KEYWORDS([mirror mirrors mirroring])
+OVS_VSWITCHD_START
+add_of_ports br0 1 2 3
+AT_CHECK([ovs-vsctl \
+ set Bridge br0 mirrors=@m -- \
+ --id=@p3 get Port p3 -- \
+ --id=@m create Mirror name=mymirror select_all=true output_port=@p3 \
+ filter="\"ip,nw_src=192.168.0.0/24\""], [0], [ignore])
+
+AT_CHECK([ovs-ofctl add-flow br0 "in_port=1 actions=output:2"])
+
+match_flow="eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=128,frag=no),tcp(dst=80)"
+nomatch_flow="eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=10.0.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=128,frag=no),tcp(dst=80)"
+
+dnl A masked filter should be accepted and only matching flows mirrored.
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$match_flow"], [0],
[stdout])
+AT_CHECK_UNQUOTED([tail -1 stdout], [0],
+ [Datapath actions: 3,2
+])
+
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$nomatch_flow"], [0],
[stdout])
+AT_CHECK_UNQUOTED([tail -1 stdout], [0],
+ [Datapath actions: 2
+])
+
+dnl A masked L4 port filter should compose with the wildcards too.
+AT_CHECK([ovs-vsctl set mirror mymirror
filter="\"tcp,tcp_dst=0x0050/0xfff0\""], [0])
+
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$match_flow"], [0],
[stdout])
+AT_CHECK_UNQUOTED([tail -1 stdout], [0],
+ [Datapath actions: 3,2
+])
+
+nomatch_port_flow="eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=128,frag=no),tcp(dst=443)"
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$nomatch_port_flow"],
[0], [stdout])
+AT_CHECK_UNQUOTED([tail -1 stdout], [0],
+ [Datapath actions: 2
+])
+
+dnl Missing prerequisites, missing values and duplicate fields are still
+dnl rejected.
+AT_CHECK([ovs-vsctl set mirror mymirror filter="\"nw_src=192.168.0.0/24\""],
[0])
+AT_CHECK([ovs-vsctl set mirror mymirror \
+ filter="\"ip,nw_src=192.168.0.0/24,nw_src=10.0.0.0/8\""], [0])
+AT_CHECK([ovs-vsctl set mirror mymirror filter="\"ip,ipv6\""], [0])
+AT_CHECK([ovs-vsctl set mirror mymirror filter="\"ip,nw_src\""], [0])
+
+dnl Each of the above four lines should produce two log messages.
+OVS_WAIT_UNTIL([test $(grep -Ec "filter is invalid|mirror mymirror configuration is
invalid" ovs-vswitchd.log) -eq 8])
+AT_CHECK([grep -c "prerequisites not met for setting nw_src"
ovs-vswitchd.log], [0], [1
+])
+AT_CHECK([grep -c "field nw_src set multiple times" ovs-vswitchd.log], [0], [1
+])
+AT_CHECK([grep -c "Ethernet type set multiple times" ovs-vswitchd.log], [0], [1
+])
+AT_CHECK([grep -c "bad value for nw_src" ovs-vswitchd.log], [0], [1
+])
+
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$match_flow"], [0],
[stdout])
+AT_CHECK_UNQUOTED([tail -1 stdout], [0],
+ [Datapath actions: 2
+])
+
+OVS_VSWITCHD_STOP(["/filter is invalid: /d
+/mirror mymirror configuration is invalid/d"])
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - mirroring, select_all])
AT_KEYWORDS([mirror mirrors mirroring])
OVS_VSWITCHD_START
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 4eec70fe4..1da1a2f29 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -5319,9 +5319,11 @@ ovs-vsctl add-port br0 p1 -- \
When set, only packets that match <ref column="filter"/> are
selected for mirroring. Packets that do not match are ignored
by thie mirror. The <ref column="filter"/> syntax is described
- in <code>ovs-fields</code>(7). However, the <code>in_port</code>
- field is not supported; <ref column="select_src_port"/> should be
- used to limit the mirror to a source port.
+ in <code>ovs-fields</code>(7). A field may be given with a mask,
+ e.g. <code>ip,nw_src=10.0.0.0/24</code>.
+ However, the <code>in_port</code> field is not supported;
+ <ref column="select_src_port"/> should be used to limit the
+ mirror to a source port.
</p>
<p>
This filter is applied after <ref column="select_all"/>, <ref
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
