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. 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]> --- 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) { + 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 -- 2.55.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
