Message ID | 20240501145404.959173-2-mkp@redhat.com |
---|---|
State | Under Review |
Delegated to: | Ilya Maximets |
Headers | show |
Series | [ovs-dev,v9,1/2] ofproto-dpif-mirror: Reduce number of function parameters. | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
ovsrobot/intel-ovs-compilation | success | test: success |
On 1 May 2024, at 16:54, Mike Pattrick wrote: > Currently a bridge mirror will collect all packets and tools like > ovs-tcpdump can apply additional filters after they have already been > duplicated by vswitchd. This can result in inefficient collection. > > This patch adds support to apply pre-selection to bridge mirrors, which > can limit which packets are mirrored based on flow metadata. This > significantly improves overall vswitchd performance during mirroring if > only a subset of traffic is required. > > Signed-off-by: Mike Pattrick <mkp@redhat.com> > --- > v8: > - Corrected code from v7 related to sequence and in_port. Mirrors > reject filters with an in_port set as this could cause confusion. > - Combined ovsrcu pointers into a new struct, minimatch wasn't used > because the minimatch_* functions didn't fit the usage here. > - Added a test to check for modifying filters when partially > overlapping flows already exist. > - Corrected documentation. > v9: > - Explicitly cleared mirror_config.filter* when not set > --- > Documentation/ref/ovs-tcpdump.8.rst | 8 +- > NEWS | 6 + > lib/flow.h | 9 ++ > ofproto/ofproto-dpif-mirror.c | 104 +++++++++++++++++- > ofproto/ofproto-dpif-mirror.h | 9 +- > ofproto/ofproto-dpif-xlate.c | 15 ++- > ofproto/ofproto-dpif.c | 12 +- > ofproto/ofproto.h | 3 + > tests/ofproto-dpif.at | 165 ++++++++++++++++++++++++++++ > utilities/ovs-tcpdump.in | 13 ++- > vswitchd/bridge.c | 13 ++- > vswitchd/vswitch.ovsschema | 7 +- > vswitchd/vswitch.xml | 16 +++ > 13 files changed, 365 insertions(+), 15 deletions(-) > > diff --git a/Documentation/ref/ovs-tcpdump.8.rst b/Documentation/ref/ovs-tcpdump.8.rst > index b9f8cdf6f..e21e61211 100644 > --- a/Documentation/ref/ovs-tcpdump.8.rst > +++ b/Documentation/ref/ovs-tcpdump.8.rst > @@ -61,8 +61,14 @@ Options > > If specified, mirror all ports (optional). > > +* ``--filter <flow>`` > + > + If specified, only mirror flows that match the provided OpenFlow filter. > + The available fields are documented in ``ovs-fields(7)``. > + > See Also > ======== > > ``ovs-appctl(8)``, ``ovs-vswitchd(8)``, ``ovs-pcap(1)``, > -``ovs-tcpundump(1)``, ``tcpdump(8)``, ``wireshark(8)``. > +``ovs-fields(7)``, ``ovs-tcpundump(1)``, ``tcpdump(8)``, > +``wireshark(8)``. > diff --git a/NEWS b/NEWS > index b92cec532..f3a4bf076 100644 > --- a/NEWS > +++ b/NEWS > @@ -7,6 +7,12 @@ Post-v3.3.0 > - The primary development branch has been renamed from 'master' to 'main'. > The OVS tree remains hosted on GitHub. > https://github.com/openvswitch/ovs.git > + - ovs-vsctl: > + * Added a new filter column in the Mirror table which can be used to > + apply filters to mirror ports. > + - ovs-tcpdump: > + * Added command line parameter --filter to enable filtering the flows > + that are captured by tcpdump. > > > v3.3.0 - 16 Feb 2024 > diff --git a/lib/flow.h b/lib/flow.h > index 75a9be3c1..60ec4b0d7 100644 > --- a/lib/flow.h > +++ b/lib/flow.h > @@ -939,6 +939,15 @@ flow_union_with_miniflow(struct flow *dst, const struct miniflow *src) > flow_union_with_miniflow_subset(dst, src, src->map); > } > > +/* Perform a bitwise OR of minimask 'src' mask data with the equivalent > + * fields in 'dst', storing the result in 'dst'. */ > +static inline void > +flow_wildcards_union_with_minimask(struct flow_wildcards *dst, > + const struct minimask *src) > +{ > + flow_union_with_miniflow_subset(&dst->masks, &src->masks, src->masks.map); > +} > + > static inline bool is_ct_valid(const struct flow *flow, > const struct flow_wildcards *mask, > struct flow_wildcards *wc) > diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c > index 4967ecc9a..6d89d13a5 100644 > --- a/ofproto/ofproto-dpif-mirror.c > +++ b/ofproto/ofproto-dpif-mirror.c > @@ -21,6 +21,7 @@ > #include "cmap.h" > #include "hmapx.h" > #include "ofproto.h" > +#include "ofproto-dpif-trace.h" > #include "vlan-bitmap.h" > #include "openvswitch/vlog.h" > > @@ -48,6 +49,11 @@ struct mbundle { > mirror_mask_t mirror_out; /* Mirrors that output to this mbundle. */ > }; > > +struct filtermask { > + struct miniflow *flow; > + struct minimask *mask; > +}; > + > struct mirror { > struct mbridge *mbridge; /* Owning ofproto. */ > size_t idx; /* In ofproto's "mirrors" array. */ > @@ -57,6 +63,10 @@ struct mirror { > struct hmapx srcs; /* Contains "struct mbundle*"s. */ > struct hmapx dsts; /* Contains "struct mbundle*"s. */ > > + /* Filter criteria. */ > + OVSRCU_TYPE(struct filtermask *) filter_mask; > + char *filter_str; > + > /* This is accessed by handler threads assuming RCU protection (see > * mirror_get()), but can be manipulated by mirror_set() without any > * explicit synchronization. */ > @@ -83,6 +93,23 @@ static void mbundle_lookup_multiple(const struct mbridge *, struct ofbundle **, > static int mirror_scan(struct mbridge *); > static void mirror_update_dups(struct mbridge *); > > +static void > +filtermask_free(struct filtermask *fm) > +{ > + free(fm->flow); > + free(fm->mask); > + free(fm); > +} > + > +static struct filtermask * > +filtermask_create(struct flow *flow, struct flow_wildcards *wc) > +{ > + struct filtermask *fm = xmalloc(sizeof *fm); > + fm->flow = miniflow_create(flow); > + fm->mask = minimask_create(wc); > + return fm; > +} > + > struct mbridge * > mbridge_create(void) > { > @@ -207,8 +234,8 @@ mirror_bundle_dst(struct mbridge *mbridge, struct ofbundle *ofbundle) > } > > int > -mirror_set(struct mbridge *mbridge, void *aux, > - const struct ofproto_mirror_settings *ms, > +mirror_set(struct mbridge *mbridge, const struct ofproto *ofproto, > + void *aux, const struct ofproto_mirror_settings *ms, > const struct mirror_bundles *mb) > { > struct mbundle *mbundle, *out; > @@ -264,11 +291,13 @@ mirror_set(struct mbridge *mbridge, void *aux, > && vlan_bitmap_equal(vlans, ms->src_vlans) > && mirror->out == out > && mirror->out_vlan == out_vlan > - && mirror->snaplen == ms->snaplen) > + && mirror->snaplen == ms->snaplen > + && nullable_string_is_equal(mirror->filter_str, ms->filter) > + && !ms->filter) > { > hmapx_destroy(&srcs_map); > hmapx_destroy(&dsts_map); > - return 0; > + return ECANCELED; > } > > /* XXX: Not sure if these need to be thread safe. */ > @@ -288,6 +317,51 @@ mirror_set(struct mbridge *mbridge, void *aux, > mirror->out_vlan = out_vlan; > mirror->snaplen = ms->snaplen; > > + if (!nullable_string_is_equal(mirror->filter_str, ms->filter) || > + (mirror->filter_str || ms->filter)) { > + if (mirror->filter_str) { > + ovsrcu_postpone(filtermask_free, > + ovsrcu_get_protected(struct filtermask *, > + &mirror->filter_mask)); > + free(mirror->filter_str); > + mirror->filter_str = NULL; > + ovsrcu_set(&mirror->filter_mask, NULL); > + } > + > + if (ms->filter && strlen(ms->filter)) { > + struct ofputil_port_map map = OFPUTIL_PORT_MAP_INITIALIZER(&map); > + struct flow_wildcards wc; > + struct flow flow; > + 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); > + ofputil_port_map_destroy(&map); > + if (err) { > + VLOG_WARN("filter is invalid: %s", err); > + free(err); > + mirror_destroy(mbridge, mirror->aux); > + return EINVAL; > + } > + > + /* If the user wants to filter on in_port, they should use the srcs > + * bundle. Users setting in_port could experience unexpected > + * behavior, and it would be overly complex to detect all possible > + * issues. So instead we attempt to extract the in_port and error > + * if successful. */ > + if (wc.masks.in_port.ofp_port) { > + VLOG_WARN("filter is invalid due to in_port field."); > + mirror_destroy(mbridge, mirror->aux); > + return EINVAL; > + } > + > + mirror->filter_str = xstrdup(ms->filter); > + ovsrcu_set(&mirror->filter_mask, filtermask_create(&flow, &wc)); > + } > + } > + > /* Update mbundles. */ > mirror_bit = MIRROR_MASK_C(1) << mirror->idx; > CMAP_FOR_EACH (mbundle, cmap_node, &mirror->mbridge->mbundles) { > @@ -343,6 +417,15 @@ mirror_destroy(struct mbridge *mbridge, void *aux) > ovsrcu_postpone(free, vlans); > } > > + if (mirror->filter_str) { > + ovsrcu_postpone(filtermask_free, > + ovsrcu_get_protected(struct filtermask *, > + &mirror->filter_mask)); > + free(mirror->filter_str); > + mirror->filter_str = NULL; > + ovsrcu_set(&mirror->filter_mask, NULL); > + } > + > mbridge->mirrors[mirror->idx] = NULL; > /* mirror_get() might have just read the pointer, so we must postpone the > * free. */ > @@ -414,7 +497,9 @@ mirror_update_stats(struct mbridge *mbridge, mirror_mask_t mirrors, > * in which a 1-bit indicates that the mirror includes a particular VLAN, > * 'mc->dup_mirrors' receives a bitmap of mirrors whose output duplicates > * mirror 'index', 'mc->out' receives the output ofbundle (if any), > - * and 'mc->out_vlan' receives the output VLAN (if any). > + * and 'mc->out_vlan' receives the output VLAN (if any). In cases where the > + * mirror has a filter configured 'mc->filter_flow' and 'mc->filter_mask' > + * receives the flow and mask that this mirror should collect. > * > * Everything returned here is assumed to be RCU protected. > */ > @@ -423,6 +508,7 @@ mirror_get(struct mbridge *mbridge, int index, > struct mirror_config *mc) > { > struct mirror *mirror; > + struct filtermask *fm; > > if (!mc || !mbridge) { > return false; > @@ -440,6 +526,14 @@ mirror_get(struct mbridge *mbridge, int index, > mc->out_bundle = mirror->out ? mirror->out->ofbundle : NULL; > mc->out_vlan = mirror->out_vlan; > mc->snaplen = mirror->snaplen; > + fm = ovsrcu_get(struct filtermask *, &mirror->filter_mask); > + if (fm) { > + mc->filter_flow = fm->flow; > + mc->filter_mask = fm->mask; > + } else { > + mc->filter_flow = NULL; > + mc->filter_mask = NULL; > + } > return true; > } > > diff --git a/ofproto/ofproto-dpif-mirror.h b/ofproto/ofproto-dpif-mirror.h > index 37d57463c..90331ede5 100644 > --- a/ofproto/ofproto-dpif-mirror.h > +++ b/ofproto/ofproto-dpif-mirror.h > @@ -23,8 +23,8 @@ > typedef uint32_t mirror_mask_t; > > struct ofproto_mirror_settings; > -struct ofproto_dpif; > struct ofbundle; > +struct ofproto; > > struct mirror_bundles { > struct ofbundle **srcs; > @@ -43,6 +43,11 @@ struct mirror_config { > /* VLANs of packets to select for mirroring. */ > unsigned long *vlans; /* vlan_bitmap, NULL selects all VLANs. */ > > + /* The flow if a filter is used, or NULL. */ > + struct miniflow *filter_flow; > + /* The filter's flow mask, or NULL. */ > + struct minimask *filter_mask; > + > /* Output (mutually exclusive). */ > struct ofbundle *out_bundle; /* A registered ofbundle handle or NULL. */ > uint16_t out_vlan; /* Output VLAN, not used if out_bundle is > @@ -76,7 +81,7 @@ bool mbridge_need_revalidate(struct mbridge *); > void mbridge_register_bundle(struct mbridge *, struct ofbundle *); > void mbridge_unregister_bundle(struct mbridge *, struct ofbundle *); > > -int mirror_set(struct mbridge *, void *aux, > +int mirror_set(struct mbridge *, const struct ofproto *, void *aux, > const struct ofproto_mirror_settings *, > const struct mirror_bundles *); > void mirror_destroy(struct mbridge *, void *aux); > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 55846fe98..88015f817 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -2250,7 +2250,8 @@ lookup_input_bundle(const struct xlate_ctx *ctx, > > /* Mirrors the packet represented by 'ctx' to appropriate mirror destinations, > * given the packet is ingressing or egressing on 'xbundle', which has ingress > - * or egress (as appropriate) mirrors 'mirrors'. */ > + * or egress (as appropriate) mirrors 'mirrors'. In cases where a mirror is > + * filtered, the current flows wildcard will be modified. */ > static void > mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle, > mirror_mask_t mirrors) > @@ -2302,6 +2303,18 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle, > continue; > } > > + /* After the VLAN check, apply a flow mask if a filter is specified. */ > + if (ctx->wc && mc.filter_flow) { > + flow_wildcards_union_with_minimask(ctx->wc, mc.filter_mask); > + if (!OVS_UNLIKELY( > + miniflow_equal_flow_in_minimask(mc.filter_flow, > + &ctx->xin->flow, > + mc.filter_mask))) { > + mirrors = zero_rightmost_1bit(mirrors); > + continue; > + } > + } > + > /* We sent a packet to this mirror. */ > used_mirrors |= rightmost_1bit(mirrors); > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 6f5830e40..7a3df1a47 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -3754,7 +3754,17 @@ mirror_set__(struct ofproto *ofproto_, void *aux, > mb.n_dsts = s->n_dsts; > mb.out_bundle = bundle_lookup(ofproto, s->out_bundle); > > - error = mirror_set(ofproto->mbridge, aux, s, &mb); > + error = mirror_set(ofproto->mbridge, ofproto_, aux, s, &mb); > + > + if (!error) { > + ofproto->backer->need_revalidate = REV_RECONFIGURE; > + } else if (error == ECANCELED) { > + /* The user requested a change that is identical to the current state, > + * the reconfiguration is canceled, but don't log an error message > + * about that. */ > + error = 0; > + } > + > free(mb.srcs); > free(mb.dsts); > return error; > diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h > index 1c07df275..655caa14e 100644 > --- a/ofproto/ofproto.h > +++ b/ofproto/ofproto.h > @@ -501,6 +501,9 @@ struct ofproto_mirror_settings { > uint16_t out_vlan; /* Output VLAN, only if out_bundle is NULL. */ > uint16_t snaplen; /* Max packet size of a mirrored packet > in byte, set to 0 equals 65535. */ > + > + /* Output filter. */ > + char *filter; > }; > > int ofproto_mirror_register(struct ofproto *, void *aux, > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index 3eaccb13a..4e7913352 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -5199,6 +5199,171 @@ OVS_VSWITCHD_STOP > AT_CLEANUP > > > +AT_SETUP([ofproto-dpif - mirroring, filter]) > +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="icmp"], [0], [ignore]) > + > +icmp_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=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)" > +tcp_flow1="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)" > +tcp_flow2="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)" > + > +AT_CHECK([ovs-ofctl del-flows br0]) > +AT_CHECK([ovs-ofctl add-flow br0 'actions=normal' ]) > + > +dnl Add non-matching flows, then change the mirror to match one of the flows, > +dnl then add a matching flow. > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 $icmp_flow]) > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 $tcp_flow1]) > +AT_CHECK([ovs-vsctl set mirror mymirror filter="tcp"], [0]) > +AT_CHECK([ovs-appctl revalidator/wait]) > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 $tcp_flow2]) > +AT_CHECK([ovs-appctl dpif/dump-flows --names br0 | strip_ufid | strip_used | sort], [0], [dnl > +recirc_id(0),in_port(p1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),dnl > +eth_type(0x0800),ipv4(proto=1,frag=no), packets:0, bytes:0, used:never, actions:br0,p2 > +recirc_id(0),in_port(p1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),dnl > +eth_type(0x0800),ipv4(proto=6,frag=no), packets:1, bytes:118, used:0.0s, actions:p3,br0,p2 > +]) > +AT_CHECK([ovs-appctl dpctl/dump-flows --names | strip_ufid | strip_used | sort], [0], [dnl > +flow-dump from the main thread: > +recirc_id(0),in_port(p1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),dnl > +eth_type(0x0800),ipv4(proto=1,frag=no), packets:0, bytes:0, used:never, actions:br0,p2 > +recirc_id(0),in_port(p1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),dnl > +eth_type(0x0800),ipv4(proto=6,frag=no), packets:1, bytes:118, used:0.0s, actions:p3,br0,p2 > +]) > + > +AT_CHECK([ovs-ofctl del-flows br0]) > +AT_CHECK([ovs-ofctl add-flow br0 "in_port=1 actions=output:2"]) > +AT_CHECK([ovs-ofctl add-flow br0 "in_port=2 actions=output:1"]) > + > +dnl Add mirrored flow after non-mirrored flow. > +AT_CHECK([ovs-vsctl set mirror mymirror filter="icmp"], [0]) > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 $tcp_flow1]) > +AT_CHECK([ovs-appctl dpif/dump-flows -m br0 | grep -cE "actions:p2$"], [0], [1 > +]) > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 $icmp_flow]) > +AT_CHECK([ovs-appctl dpif/dump-flows -m br0 | grep -cE "actions:p3,p2$"], [0], [1 > +]) > + > +dnl Check one direction, only icmp should mirror. > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$icmp_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),$tcp_flow1"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 2 > +]) > + > +dnl Check other direction, only icmp should mirror. > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$icmp_flow"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 3,1 > +]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$tcp_flow1"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 1 > +]) > + > +dnl Change filter to tcp, only tcp should mirror. > +AT_CHECK([ovs-vsctl set mirror mymirror filter="tcp"], [0]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$icmp_flow"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 2 > +]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$tcp_flow1"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 3,2 > +]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$icmp_flow"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 1 > +]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$tcp_flow1"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 3,1 > +]) > + > +dnl Invalid filter. Nothing should mirror, error should be logged. > +AT_CHECK([ovs-vsctl set mirror mymirror filter="invalid"], [0]) > +dnl Setting an in_port is also invalid. > +AT_CHECK([ovs-vsctl set mirror mymirror filter="\"in_port=p1\""], [0]) > + > +dnl Each of the above two lines should produce two log messages. > +OVS_WAIT_UNTIL([test $(grep -Ec "filter is invalid|mirror mymirror configuration is invalid" ovs-vswitchd.log) -eq 4]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$icmp_flow"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 2 > +]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$tcp_flow1"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 2 > +]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$icmp_flow"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 1 > +]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$tcp_flow1"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 1 > +]) > + > +dnl Check more complex filter cases with partially overlapping default wildcards. > +AT_CHECK([ovs-vsctl set mirror mymirror filter="\"tcp,tcp_dst=80\""], [0]) > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$tcp_flow1"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 2 > +]) > + > +dnl Change port number. > +AT_CHECK([ovs-appctl dpif-dummy/change-port-number ovs-dummy p1 8]) > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(8),$tcp_flow2"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 3,2 > +]) > + > +dnl Empty filter, all traffic should mirror. > +AT_CHECK([ovs-vsctl clear mirror mymirror filter], [0]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(8),$icmp_flow"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 3,2 > +]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(8),$tcp_flow1"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 3,2 > +]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$icmp_flow"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 3,8 > +]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$tcp_flow1"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 3,8 > +]) > + > +OVS_VSWITCHD_STOP(["/filter is invalid: invalid: unknown field invalid/d > +/filter is invalid due to in_port field/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/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in > index eada803bb..07fc435fa 100755 > --- a/utilities/ovs-tcpdump.in > +++ b/utilities/ovs-tcpdump.in > @@ -142,6 +142,7 @@ The following options are available: > --mirror-to The name for the mirror port to use (optional) > Default 'miINTERFACE' > --span If specified, mirror all ports (optional) > + --filter Set an OpenFlow formatted preselection filter > """ % {'prog': sys.argv[0]}) > sys.exit(0) > > @@ -354,7 +355,7 @@ class OVSDB(object): > return result > > def bridge_mirror(self, intf_name, mirror_intf_name, br_name, > - mirror_select_all=False): > + mirror_select_all=False, mirror_filter=None): > > txn = self._start_txn() > mirror = txn.insert(self.get_table('Mirror')) > @@ -362,6 +363,9 @@ class OVSDB(object): > > mirror.select_all = mirror_select_all > > + if mirror_filter is not None: > + mirror.filter = mirror_filter > + > mirrored_port = self._find_row_by_name('Port', intf_name) > > mirror.verify('select_dst_port') > @@ -445,6 +449,7 @@ def main(): > mirror_interface = None > mirror_select_all = False > dump_cmd = 'tcpdump' > + mirror_filter = None > > for cur, nxt in argv_tuples(sys.argv[1:]): > if skip_next: > @@ -474,6 +479,10 @@ def main(): > elif cur in ['--span']: > mirror_select_all = True > continue > + elif cur in ['--filter']: > + mirror_filter = nxt > + skip_next = True > + continue > tcpdargs.append(cur) > > if interface is None: > @@ -526,7 +535,7 @@ def main(): > ovsdb.make_port(mirror_interface, ovsdb.port_bridge(interface)) > ovsdb.bridge_mirror(interface, mirror_interface, > ovsdb.port_bridge(interface), > - mirror_select_all) > + mirror_select_all, mirror_filter=mirror_filter) > except OVSDBException as oe: > print("ERROR: Unable to properly setup the mirror: %s." % str(oe)) > sys.exit(1) > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > index 95a65fcdc..39f8fba49 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -5144,6 +5144,7 @@ mirror_configure(struct mirror *m) > { > const struct ovsrec_mirror *cfg = m->cfg; > struct ofproto_mirror_settings s; > + int ret; > > /* Set name. */ > if (strcmp(cfg->name, m->name)) { > @@ -5212,8 +5213,18 @@ mirror_configure(struct mirror *m) > /* Get VLAN selection. */ > s.src_vlans = vlan_bitmap_from_array(cfg->select_vlan, cfg->n_select_vlan); > > + /* Set the filter, mirror_set() will strdup this pointer. */ > + s.filter = cfg->filter; > + > /* Configure. */ > - ofproto_mirror_register(m->bridge->ofproto, m, &s); > + ret = ofproto_mirror_register(m->bridge->ofproto, m, &s); > + if (ret == EOPNOTSUPP) { > + VLOG_ERR("ofproto %s: does not support mirroring", > + m->bridge->ofproto->name); > + } else if (ret) { > + VLOG_ERR("bridge %s: mirror %s configuration is invalid", > + m->bridge->name, m->name); > + } > > /* Clean up. */ > if (s.srcs != s.dsts) { > diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema > index e2d5e2e85..f42f09121 100644 > --- a/vswitchd/vswitch.ovsschema > +++ b/vswitchd/vswitch.ovsschema > @@ -1,6 +1,6 @@ > {"name": "Open_vSwitch", > - "version": "8.5.0", > - "cksum": "4040946650 27557", > + "version": "8.6.0", > + "cksum": "401789815 27661", > "tables": { > "Open_vSwitch": { > "columns": { > @@ -461,6 +461,9 @@ > "type": {"key": "string", "value": "integer", > "min": 0, "max": "unlimited"}, > "ephemeral": true}, > + "filter": { > + "type": {"key": {"type": "string"}, > + "min": 0, "max": 1}}, > "external_ids": { > "type": {"key": "string", "value": "string", > "min": 0, "max": "unlimited"}}}}, > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > index 8a1b607d7..2aea5c9fd 100644 > --- a/vswitchd/vswitch.xml > +++ b/vswitchd/vswitch.xml > @@ -5256,6 +5256,22 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ > VLANs on which packets are selected for mirroring. An empty set > selects packets on all VLANs. > </column> > + <column name="filter"> > + <p> > + 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, support for the <code> > + in_port</code> field is not supported; <ref > + column="select_src_port"/> should be used to limit the mirror to > + an source port. > + </p> > + <p> > + This filter is applied after <ref column="select_all"/>, <ref > + column="select_dst_port"/>, <ref column="select_src_port"/>, and > + <ref column="select_vlan"/>. > + </p> > + </column> > </group> > > <group title="Mirroring Destination Configuration"> > -- > 2.39.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 1 May 2024, at 16:54, Mike Pattrick wrote: > Currently a bridge mirror will collect all packets and tools like > ovs-tcpdump can apply additional filters after they have already been > duplicated by vswitchd. This can result in inefficient collection. > > This patch adds support to apply pre-selection to bridge mirrors, which > can limit which packets are mirrored based on flow metadata. This > significantly improves overall vswitchd performance during mirroring if > only a subset of traffic is required. > > Signed-off-by: Mike Pattrick <mkp@redhat.com> Hi Mike, Thanks for following up on this patch. See some comments below. //Eelco > --- > v8: > - Corrected code from v7 related to sequence and in_port. Mirrors > reject filters with an in_port set as this could cause confusion. > - Combined ovsrcu pointers into a new struct, minimatch wasn't used > because the minimatch_* functions didn't fit the usage here. > - Added a test to check for modifying filters when partially > overlapping flows already exist. > - Corrected documentation. > v9: > - Explicitly cleared mirror_config.filter* when not set > --- > Documentation/ref/ovs-tcpdump.8.rst | 8 +- > NEWS | 6 + > lib/flow.h | 9 ++ > ofproto/ofproto-dpif-mirror.c | 104 +++++++++++++++++- > ofproto/ofproto-dpif-mirror.h | 9 +- > ofproto/ofproto-dpif-xlate.c | 15 ++- > ofproto/ofproto-dpif.c | 12 +- > ofproto/ofproto.h | 3 + > tests/ofproto-dpif.at | 165 ++++++++++++++++++++++++++++ > utilities/ovs-tcpdump.in | 13 ++- > vswitchd/bridge.c | 13 ++- > vswitchd/vswitch.ovsschema | 7 +- > vswitchd/vswitch.xml | 16 +++ > 13 files changed, 365 insertions(+), 15 deletions(-) > > diff --git a/Documentation/ref/ovs-tcpdump.8.rst b/Documentation/ref/ovs-tcpdump.8.rst > index b9f8cdf6f..e21e61211 100644 > --- a/Documentation/ref/ovs-tcpdump.8.rst > +++ b/Documentation/ref/ovs-tcpdump.8.rst > @@ -61,8 +61,14 @@ Options > > If specified, mirror all ports (optional). > > +* ``--filter <flow>`` > + > + If specified, only mirror flows that match the provided OpenFlow filter. only mirror flows that… > + The available fields are documented in ``ovs-fields(7)``. > + > See Also > ======== > > ``ovs-appctl(8)``, ``ovs-vswitchd(8)``, ``ovs-pcap(1)``, > -``ovs-tcpundump(1)``, ``tcpdump(8)``, ``wireshark(8)``. > +``ovs-fields(7)``, ``ovs-tcpundump(1)``, ``tcpdump(8)``, > +``wireshark(8)``. > diff --git a/NEWS b/NEWS > index b92cec532..f3a4bf076 100644 > --- a/NEWS > +++ b/NEWS > @@ -7,6 +7,12 @@ Post-v3.3.0 > - The primary development branch has been renamed from 'master' to 'main'. > The OVS tree remains hosted on GitHub. > https://github.com/openvswitch/ovs.git > + - ovs-vsctl: > + * Added a new filter column in the Mirror table which can be used to > + apply filters to mirror ports. > + - ovs-tcpdump: > + * Added command line parameter --filter to enable filtering the flows flows -> packets > + that are captured by tcpdump. > > > v3.3.0 - 16 Feb 2024 > diff --git a/lib/flow.h b/lib/flow.h > index 75a9be3c1..60ec4b0d7 100644 > --- a/lib/flow.h > +++ b/lib/flow.h > @@ -939,6 +939,15 @@ flow_union_with_miniflow(struct flow *dst, const struct miniflow *src) > flow_union_with_miniflow_subset(dst, src, src->map); > } > > +/* Perform a bitwise OR of minimask 'src' mask data with the equivalent > + * fields in 'dst', storing the result in 'dst'. */ > +static inline void > +flow_wildcards_union_with_minimask(struct flow_wildcards *dst, > + const struct minimask *src) > +{ > + flow_union_with_miniflow_subset(&dst->masks, &src->masks, src->masks.map); > +} > + > static inline bool is_ct_valid(const struct flow *flow, > const struct flow_wildcards *mask, > struct flow_wildcards *wc) > diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c > index 4967ecc9a..6d89d13a5 100644 > --- a/ofproto/ofproto-dpif-mirror.c > +++ b/ofproto/ofproto-dpif-mirror.c > @@ -21,6 +21,7 @@ > #include "cmap.h" > #include "hmapx.h" > #include "ofproto.h" > +#include "ofproto-dpif-trace.h" > #include "vlan-bitmap.h" > #include "openvswitch/vlog.h" > > @@ -48,6 +49,11 @@ struct mbundle { > mirror_mask_t mirror_out; /* Mirrors that output to this mbundle. */ > }; > > +struct filtermask { > + struct miniflow *flow; > + struct minimask *mask; > +}; > + > struct mirror { > struct mbridge *mbridge; /* Owning ofproto. */ > size_t idx; /* In ofproto's "mirrors" array. */ > @@ -57,6 +63,10 @@ struct mirror { > struct hmapx srcs; /* Contains "struct mbundle*"s. */ > struct hmapx dsts; /* Contains "struct mbundle*"s. */ > > + /* Filter criteria. */ > + OVSRCU_TYPE(struct filtermask *) filter_mask; > + char *filter_str; > + > /* This is accessed by handler threads assuming RCU protection (see > * mirror_get()), but can be manipulated by mirror_set() without any > * explicit synchronization. */ > @@ -83,6 +93,23 @@ static void mbundle_lookup_multiple(const struct mbridge *, struct ofbundle **, > static int mirror_scan(struct mbridge *); > static void mirror_update_dups(struct mbridge *); > > +static void > +filtermask_free(struct filtermask *fm) > +{ > + free(fm->flow); > + free(fm->mask); > + free(fm); > +} > + > +static struct filtermask * > +filtermask_create(struct flow *flow, struct flow_wildcards *wc) > +{ > + struct filtermask *fm = xmalloc(sizeof *fm); I prefer a new line after variable declarations. > + fm->flow = miniflow_create(flow); > + fm->mask = minimask_create(wc); > + return fm; > +} > + > struct mbridge * > mbridge_create(void) > { > @@ -207,8 +234,8 @@ mirror_bundle_dst(struct mbridge *mbridge, struct ofbundle *ofbundle) > } > > int > -mirror_set(struct mbridge *mbridge, void *aux, > - const struct ofproto_mirror_settings *ms, > +mirror_set(struct mbridge *mbridge, const struct ofproto *ofproto, > + void *aux, const struct ofproto_mirror_settings *ms, > const struct mirror_bundles *mb) > { > struct mbundle *mbundle, *out; > @@ -264,11 +291,13 @@ mirror_set(struct mbridge *mbridge, void *aux, > && vlan_bitmap_equal(vlans, ms->src_vlans) > && mirror->out == out > && mirror->out_vlan == out_vlan > - && mirror->snaplen == ms->snaplen) > + && mirror->snaplen == ms->snaplen > + && nullable_string_is_equal(mirror->filter_str, ms->filter) > + && !ms->filter) > { > hmapx_destroy(&srcs_map); > hmapx_destroy(&dsts_map); > - return 0; > + return ECANCELED; > } > > /* XXX: Not sure if these need to be thread safe. */ > @@ -288,6 +317,51 @@ mirror_set(struct mbridge *mbridge, void *aux, > mirror->out_vlan = out_vlan; > mirror->snaplen = ms->snaplen; > > + if (!nullable_string_is_equal(mirror->filter_str, ms->filter) || Preference is to put the || on the new line. > + (mirror->filter_str || ms->filter)) { > + if (mirror->filter_str) { > + ovsrcu_postpone(filtermask_free, > + ovsrcu_get_protected(struct filtermask *, > + &mirror->filter_mask)); What mutex do we rely on to use the _get_protected() variant? I think we do not protect other threads reading this pointer, so we should use the normal variant. Same for all other instances. > + free(mirror->filter_str); > + mirror->filter_str = NULL; > + ovsrcu_set(&mirror->filter_mask, NULL); > + } > + > + if (ms->filter && strlen(ms->filter)) { > + struct ofputil_port_map map = OFPUTIL_PORT_MAP_INITIALIZER(&map); > + struct flow_wildcards wc; > + struct flow flow; > + 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); > + ofputil_port_map_destroy(&map); > + if (err) { > + VLOG_WARN("filter is invalid: %s", err); > + free(err); > + mirror_destroy(mbridge, mirror->aux); > + return EINVAL; > + } > + > + /* If the user wants to filter on in_port, they should use the srcs > + * bundle. Users setting in_port could experience unexpected > + * behavior, and it would be overly complex to detect all possible > + * issues. So instead we attempt to extract the in_port and error > + * if successful. */ > + if (wc.masks.in_port.ofp_port) { > + VLOG_WARN("filter is invalid due to in_port field."); > + mirror_destroy(mbridge, mirror->aux); > + return EINVAL; > + } > + > + mirror->filter_str = xstrdup(ms->filter); > + ovsrcu_set(&mirror->filter_mask, filtermask_create(&flow, &wc)); > + } > + } > + > /* Update mbundles. */ > mirror_bit = MIRROR_MASK_C(1) << mirror->idx; > CMAP_FOR_EACH (mbundle, cmap_node, &mirror->mbridge->mbundles) { > @@ -343,6 +417,15 @@ mirror_destroy(struct mbridge *mbridge, void *aux) > ovsrcu_postpone(free, vlans); > } > > + if (mirror->filter_str) { > + ovsrcu_postpone(filtermask_free, > + ovsrcu_get_protected(struct filtermask *, > + &mirror->filter_mask)); ovsrcu_get_protected? > + free(mirror->filter_str); > + mirror->filter_str = NULL; > + ovsrcu_set(&mirror->filter_mask, NULL); > + } > + > mbridge->mirrors[mirror->idx] = NULL; > /* mirror_get() might have just read the pointer, so we must postpone the > * free. */ > @@ -414,7 +497,9 @@ mirror_update_stats(struct mbridge *mbridge, mirror_mask_t mirrors, > * in which a 1-bit indicates that the mirror includes a particular VLAN, > * 'mc->dup_mirrors' receives a bitmap of mirrors whose output duplicates > * mirror 'index', 'mc->out' receives the output ofbundle (if any), > - * and 'mc->out_vlan' receives the output VLAN (if any). > + * and 'mc->out_vlan' receives the output VLAN (if any). In cases where the > + * mirror has a filter configured 'mc->filter_flow' and 'mc->filter_mask' > + * receives the flow and mask that this mirror should collect. > * > * Everything returned here is assumed to be RCU protected. > */ > @@ -423,6 +508,7 @@ mirror_get(struct mbridge *mbridge, int index, > struct mirror_config *mc) > { > struct mirror *mirror; > + struct filtermask *fm; I would put them in alphabetical order if they are the same length. > > if (!mc || !mbridge) { > return false; > @@ -440,6 +526,14 @@ mirror_get(struct mbridge *mbridge, int index, > mc->out_bundle = mirror->out ? mirror->out->ofbundle : NULL; > mc->out_vlan = mirror->out_vlan; > mc->snaplen = mirror->snaplen; > + fm = ovsrcu_get(struct filtermask *, &mirror->filter_mask); > + if (fm) { > + mc->filter_flow = fm->flow; > + mc->filter_mask = fm->mask; > + } else { > + mc->filter_flow = NULL; > + mc->filter_mask = NULL; > + } > return true; > } > > diff --git a/ofproto/ofproto-dpif-mirror.h b/ofproto/ofproto-dpif-mirror.h > index 37d57463c..90331ede5 100644 > --- a/ofproto/ofproto-dpif-mirror.h > +++ b/ofproto/ofproto-dpif-mirror.h > @@ -23,8 +23,8 @@ > typedef uint32_t mirror_mask_t; > > struct ofproto_mirror_settings; > -struct ofproto_dpif; > struct ofbundle; > +struct ofproto; > > struct mirror_bundles { > struct ofbundle **srcs; > @@ -43,6 +43,11 @@ struct mirror_config { > /* VLANs of packets to select for mirroring. */ > unsigned long *vlans; /* vlan_bitmap, NULL selects all VLANs. */ > > + /* The flow if a filter is used, or NULL. */ > + struct miniflow *filter_flow; > + /* The filter's flow mask, or NULL. */ > + struct minimask *filter_mask; > + Maybe a single comment for both as they are used together? Something like: /* Miniflow and minimask if a filter is configured, else NULL. */ > /* Output (mutually exclusive). */ > struct ofbundle *out_bundle; /* A registered ofbundle handle or NULL. */ > uint16_t out_vlan; /* Output VLAN, not used if out_bundle is > @@ -76,7 +81,7 @@ bool mbridge_need_revalidate(struct mbridge *); > void mbridge_register_bundle(struct mbridge *, struct ofbundle *); > void mbridge_unregister_bundle(struct mbridge *, struct ofbundle *); > > -int mirror_set(struct mbridge *, void *aux, > +int mirror_set(struct mbridge *, const struct ofproto *, void *aux, > const struct ofproto_mirror_settings *, > const struct mirror_bundles *); > void mirror_destroy(struct mbridge *, void *aux); > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 55846fe98..88015f817 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -2250,7 +2250,8 @@ lookup_input_bundle(const struct xlate_ctx *ctx, > > /* Mirrors the packet represented by 'ctx' to appropriate mirror destinations, > * given the packet is ingressing or egressing on 'xbundle', which has ingress > - * or egress (as appropriate) mirrors 'mirrors'. */ > + * or egress (as appropriate) mirrors 'mirrors'. In cases where a mirror is > + * filtered, the current flows wildcard will be modified. */ What about: When a flow filter is configured, the wildcard for the current flow will be modified. > static void > mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle, > mirror_mask_t mirrors) > @@ -2302,6 +2303,18 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle, > continue; > } > > + /* After the VLAN check, apply a flow mask if a filter is specified. */ > + if (ctx->wc && mc.filter_flow) { > + flow_wildcards_union_with_minimask(ctx->wc, mc.filter_mask); > + if (!OVS_UNLIKELY( > + miniflow_equal_flow_in_minimask(mc.filter_flow, > + &ctx->xin->flow, > + mc.filter_mask))) { > + mirrors = zero_rightmost_1bit(mirrors); > + continue; > + } > + } > + > /* We sent a packet to this mirror. */ > used_mirrors |= rightmost_1bit(mirrors); > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 6f5830e40..7a3df1a47 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -3754,7 +3754,17 @@ mirror_set__(struct ofproto *ofproto_, void *aux, > mb.n_dsts = s->n_dsts; > mb.out_bundle = bundle_lookup(ofproto, s->out_bundle); > > - error = mirror_set(ofproto->mbridge, aux, s, &mb); > + error = mirror_set(ofproto->mbridge, ofproto_, aux, s, &mb); > + > + if (!error) { > + ofproto->backer->need_revalidate = REV_RECONFIGURE; > + } else if (error == ECANCELED) { > + /* The user requested a change that is identical to the current state, > + * the reconfiguration is canceled, but don't log an error message > + * about that. */ > + error = 0; > + } > + > free(mb.srcs); > free(mb.dsts); > return error; > diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h > index 1c07df275..655caa14e 100644 > --- a/ofproto/ofproto.h > +++ b/ofproto/ofproto.h > @@ -501,6 +501,9 @@ struct ofproto_mirror_settings { > uint16_t out_vlan; /* Output VLAN, only if out_bundle is NULL. */ > uint16_t snaplen; /* Max packet size of a mirrored packet > in byte, set to 0 equals 65535. */ > + > + /* Output filter. */ > + char *filter; > }; > > int ofproto_mirror_register(struct ofproto *, void *aux, > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index 3eaccb13a..4e7913352 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -5199,6 +5199,171 @@ OVS_VSWITCHD_STOP > AT_CLEANUP > > > +AT_SETUP([ofproto-dpif - mirroring, filter]) > +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="icmp"], [0], [ignore]) > + > +icmp_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=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)" > +tcp_flow1="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)" > +tcp_flow2="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)" > + > +AT_CHECK([ovs-ofctl del-flows br0]) > +AT_CHECK([ovs-ofctl add-flow br0 'actions=normal' ]) > + > +dnl Add non-matching flows, then change the mirror to match one of the flows, > +dnl then add a matching flow. > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 $icmp_flow]) > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 $tcp_flow1]) > +AT_CHECK([ovs-vsctl set mirror mymirror filter="tcp"], [0]) > +AT_CHECK([ovs-appctl revalidator/wait]) > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 $tcp_flow2]) > +AT_CHECK([ovs-appctl dpif/dump-flows --names br0 | strip_ufid | strip_used | sort], [0], [dnl > +recirc_id(0),in_port(p1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),dnl > +eth_type(0x0800),ipv4(proto=1,frag=no), packets:0, bytes:0, used:never, actions:br0,p2 > +recirc_id(0),in_port(p1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),dnl > +eth_type(0x0800),ipv4(proto=6,frag=no), packets:1, bytes:118, used:0.0s, actions:p3,br0,p2 > +]) > +AT_CHECK([ovs-appctl dpctl/dump-flows --names | strip_ufid | strip_used | sort], [0], [dnl > +flow-dump from the main thread: > +recirc_id(0),in_port(p1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),dnl > +eth_type(0x0800),ipv4(proto=1,frag=no), packets:0, bytes:0, used:never, actions:br0,p2 > +recirc_id(0),in_port(p1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),dnl > +eth_type(0x0800),ipv4(proto=6,frag=no), packets:1, bytes:118, used:0.0s, actions:p3,br0,p2 > +]) > + > +AT_CHECK([ovs-ofctl del-flows br0]) > +AT_CHECK([ovs-ofctl add-flow br0 "in_port=1 actions=output:2"]) > +AT_CHECK([ovs-ofctl add-flow br0 "in_port=2 actions=output:1"]) > + > +dnl Add mirrored flow after non-mirrored flow. > +AT_CHECK([ovs-vsctl set mirror mymirror filter="icmp"], [0]) > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 $tcp_flow1]) > +AT_CHECK([ovs-appctl dpif/dump-flows -m br0 | grep -cE "actions:p2$"], [0], [1 > +]) > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 $icmp_flow]) > +AT_CHECK([ovs-appctl dpif/dump-flows -m br0 | grep -cE "actions:p3,p2$"], [0], [1 > +]) Rather than doing two loose greps, I think it would be nicer to just do a single `AT_CHECK([ovs-appctl dpctl/dump-flows --names …` as above. > +dnl Check one direction, only icmp should mirror. > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$icmp_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),$tcp_flow1"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 2 > +]) > + > +dnl Check other direction, only icmp should mirror. > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$icmp_flow"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 3,1 > +]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$tcp_flow1"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 1 > +]) > + > +dnl Change filter to tcp, only tcp should mirror. > +AT_CHECK([ovs-vsctl set mirror mymirror filter="tcp"], [0]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$icmp_flow"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 2 > +]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$tcp_flow1"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 3,2 > +]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$icmp_flow"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 1 > +]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$tcp_flow1"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 3,1 > +]) > + > +dnl Invalid filter. Nothing should mirror, error should be logged. > +AT_CHECK([ovs-vsctl set mirror mymirror filter="invalid"], [0]) > +dnl Setting an in_port is also invalid. > +AT_CHECK([ovs-vsctl set mirror mymirror filter="\"in_port=p1\""], [0]) > + > +dnl Each of the above two lines should produce two log messages. > +OVS_WAIT_UNTIL([test $(grep -Ec "filter is invalid|mirror mymirror configuration is invalid" ovs-vswitchd.log) -eq 4]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$icmp_flow"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 2 > +]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$tcp_flow1"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 2 > +]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$icmp_flow"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 1 > +]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$tcp_flow1"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 1 > +]) > + > +dnl Check more complex filter cases with partially overlapping default wildcards. > +AT_CHECK([ovs-vsctl set mirror mymirror filter="\"tcp,tcp_dst=80\""], [0]) > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$tcp_flow1"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 2 > +]) > + > +dnl Change port number. > +AT_CHECK([ovs-appctl dpif-dummy/change-port-number ovs-dummy p1 8]) > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(8),$tcp_flow2"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 3,2 > +]) > + > +dnl Empty filter, all traffic should mirror. > +AT_CHECK([ovs-vsctl clear mirror mymirror filter], [0]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(8),$icmp_flow"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 3,2 > +]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(8),$tcp_flow1"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 3,2 > +]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$icmp_flow"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 3,8 > +]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$tcp_flow1"], [0], [stdout]) > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], > + [Datapath actions: 3,8 > +]) > + > +OVS_VSWITCHD_STOP(["/filter is invalid: invalid: unknown field invalid/d > +/filter is invalid due to in_port field/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/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in > index eada803bb..07fc435fa 100755 > --- a/utilities/ovs-tcpdump.in > +++ b/utilities/ovs-tcpdump.in > @@ -142,6 +142,7 @@ The following options are available: > --mirror-to The name for the mirror port to use (optional) > Default 'miINTERFACE' > --span If specified, mirror all ports (optional) > + --filter Set an OpenFlow formatted preselection filter > """ % {'prog': sys.argv[0]}) > sys.exit(0) > > @@ -354,7 +355,7 @@ class OVSDB(object): > return result > > def bridge_mirror(self, intf_name, mirror_intf_name, br_name, > - mirror_select_all=False): > + mirror_select_all=False, mirror_filter=None): > > txn = self._start_txn() > mirror = txn.insert(self.get_table('Mirror')) > @@ -362,6 +363,9 @@ class OVSDB(object): > > mirror.select_all = mirror_select_all > > + if mirror_filter is not None: > + mirror.filter = mirror_filter > + > mirrored_port = self._find_row_by_name('Port', intf_name) > > mirror.verify('select_dst_port') > @@ -445,6 +449,7 @@ def main(): > mirror_interface = None > mirror_select_all = False > dump_cmd = 'tcpdump' > + mirror_filter = None > > for cur, nxt in argv_tuples(sys.argv[1:]): > if skip_next: > @@ -474,6 +479,10 @@ def main(): > elif cur in ['--span']: > mirror_select_all = True > continue > + elif cur in ['--filter']: > + mirror_filter = nxt > + skip_next = True > + continue > tcpdargs.append(cur) > > if interface is None: > @@ -526,7 +535,7 @@ def main(): > ovsdb.make_port(mirror_interface, ovsdb.port_bridge(interface)) > ovsdb.bridge_mirror(interface, mirror_interface, > ovsdb.port_bridge(interface), > - mirror_select_all) > + mirror_select_all, mirror_filter=mirror_filter) > except OVSDBException as oe: > print("ERROR: Unable to properly setup the mirror: %s." % str(oe)) > sys.exit(1) > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > index 95a65fcdc..39f8fba49 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -5144,6 +5144,7 @@ mirror_configure(struct mirror *m) > { > const struct ovsrec_mirror *cfg = m->cfg; > struct ofproto_mirror_settings s; > + int ret; > > /* Set name. */ > if (strcmp(cfg->name, m->name)) { > @@ -5212,8 +5213,18 @@ mirror_configure(struct mirror *m) > /* Get VLAN selection. */ > s.src_vlans = vlan_bitmap_from_array(cfg->select_vlan, cfg->n_select_vlan); > > + /* Set the filter, mirror_set() will strdup this pointer. */ > + s.filter = cfg->filter; > + > /* Configure. */ > - ofproto_mirror_register(m->bridge->ofproto, m, &s); > + ret = ofproto_mirror_register(m->bridge->ofproto, m, &s); > + if (ret == EOPNOTSUPP) { > + VLOG_ERR("ofproto %s: does not support mirroring", > + m->bridge->ofproto->name); > + } else if (ret) { > + VLOG_ERR("bridge %s: mirror %s configuration is invalid", > + m->bridge->name, m->name); > + } > > /* Clean up. */ > if (s.srcs != s.dsts) { > diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema > index e2d5e2e85..f42f09121 100644 > --- a/vswitchd/vswitch.ovsschema > +++ b/vswitchd/vswitch.ovsschema > @@ -1,6 +1,6 @@ > {"name": "Open_vSwitch", > - "version": "8.5.0", > - "cksum": "4040946650 27557", > + "version": "8.6.0", > + "cksum": "401789815 27661", > "tables": { > "Open_vSwitch": { > "columns": { > @@ -461,6 +461,9 @@ > "type": {"key": "string", "value": "integer", > "min": 0, "max": "unlimited"}, > "ephemeral": true}, > + "filter": { > + "type": {"key": {"type": "string"}, > + "min": 0, "max": 1}}, > "external_ids": { > "type": {"key": "string", "value": "string", > "min": 0, "max": "unlimited"}}}}, > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > index 8a1b607d7..2aea5c9fd 100644 > --- a/vswitchd/vswitch.xml > +++ b/vswitchd/vswitch.xml > @@ -5256,6 +5256,22 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ > VLANs on which packets are selected for mirroring. An empty set > selects packets on all VLANs. > </column> > + <column name="filter"> > + <p> > + 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, support for the <code> > + in_port</code> field is not supported; <ref > + column="select_src_port"/> should be used to limit the mirror to > + an source port. > + </p> > + <p> > + This filter is applied after <ref column="select_all"/>, <ref > + column="select_dst_port"/>, <ref column="select_src_port"/>, and > + <ref column="select_vlan"/>. > + </p> > + </column> > </group> > > <group title="Mirroring Destination Configuration"> > -- > 2.39.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/Documentation/ref/ovs-tcpdump.8.rst b/Documentation/ref/ovs-tcpdump.8.rst index b9f8cdf6f..e21e61211 100644 --- a/Documentation/ref/ovs-tcpdump.8.rst +++ b/Documentation/ref/ovs-tcpdump.8.rst @@ -61,8 +61,14 @@ Options If specified, mirror all ports (optional). +* ``--filter <flow>`` + + If specified, only mirror flows that match the provided OpenFlow filter. + The available fields are documented in ``ovs-fields(7)``. + See Also ======== ``ovs-appctl(8)``, ``ovs-vswitchd(8)``, ``ovs-pcap(1)``, -``ovs-tcpundump(1)``, ``tcpdump(8)``, ``wireshark(8)``. +``ovs-fields(7)``, ``ovs-tcpundump(1)``, ``tcpdump(8)``, +``wireshark(8)``. diff --git a/NEWS b/NEWS index b92cec532..f3a4bf076 100644 --- a/NEWS +++ b/NEWS @@ -7,6 +7,12 @@ Post-v3.3.0 - The primary development branch has been renamed from 'master' to 'main'. The OVS tree remains hosted on GitHub. https://github.com/openvswitch/ovs.git + - ovs-vsctl: + * Added a new filter column in the Mirror table which can be used to + apply filters to mirror ports. + - ovs-tcpdump: + * Added command line parameter --filter to enable filtering the flows + that are captured by tcpdump. v3.3.0 - 16 Feb 2024 diff --git a/lib/flow.h b/lib/flow.h index 75a9be3c1..60ec4b0d7 100644 --- a/lib/flow.h +++ b/lib/flow.h @@ -939,6 +939,15 @@ flow_union_with_miniflow(struct flow *dst, const struct miniflow *src) flow_union_with_miniflow_subset(dst, src, src->map); } +/* Perform a bitwise OR of minimask 'src' mask data with the equivalent + * fields in 'dst', storing the result in 'dst'. */ +static inline void +flow_wildcards_union_with_minimask(struct flow_wildcards *dst, + const struct minimask *src) +{ + flow_union_with_miniflow_subset(&dst->masks, &src->masks, src->masks.map); +} + static inline bool is_ct_valid(const struct flow *flow, const struct flow_wildcards *mask, struct flow_wildcards *wc) diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c index 4967ecc9a..6d89d13a5 100644 --- a/ofproto/ofproto-dpif-mirror.c +++ b/ofproto/ofproto-dpif-mirror.c @@ -21,6 +21,7 @@ #include "cmap.h" #include "hmapx.h" #include "ofproto.h" +#include "ofproto-dpif-trace.h" #include "vlan-bitmap.h" #include "openvswitch/vlog.h" @@ -48,6 +49,11 @@ struct mbundle { mirror_mask_t mirror_out; /* Mirrors that output to this mbundle. */ }; +struct filtermask { + struct miniflow *flow; + struct minimask *mask; +}; + struct mirror { struct mbridge *mbridge; /* Owning ofproto. */ size_t idx; /* In ofproto's "mirrors" array. */ @@ -57,6 +63,10 @@ struct mirror { struct hmapx srcs; /* Contains "struct mbundle*"s. */ struct hmapx dsts; /* Contains "struct mbundle*"s. */ + /* Filter criteria. */ + OVSRCU_TYPE(struct filtermask *) filter_mask; + char *filter_str; + /* This is accessed by handler threads assuming RCU protection (see * mirror_get()), but can be manipulated by mirror_set() without any * explicit synchronization. */ @@ -83,6 +93,23 @@ static void mbundle_lookup_multiple(const struct mbridge *, struct ofbundle **, static int mirror_scan(struct mbridge *); static void mirror_update_dups(struct mbridge *); +static void +filtermask_free(struct filtermask *fm) +{ + free(fm->flow); + free(fm->mask); + free(fm); +} + +static struct filtermask * +filtermask_create(struct flow *flow, struct flow_wildcards *wc) +{ + struct filtermask *fm = xmalloc(sizeof *fm); + fm->flow = miniflow_create(flow); + fm->mask = minimask_create(wc); + return fm; +} + struct mbridge * mbridge_create(void) { @@ -207,8 +234,8 @@ mirror_bundle_dst(struct mbridge *mbridge, struct ofbundle *ofbundle) } int -mirror_set(struct mbridge *mbridge, void *aux, - const struct ofproto_mirror_settings *ms, +mirror_set(struct mbridge *mbridge, const struct ofproto *ofproto, + void *aux, const struct ofproto_mirror_settings *ms, const struct mirror_bundles *mb) { struct mbundle *mbundle, *out; @@ -264,11 +291,13 @@ mirror_set(struct mbridge *mbridge, void *aux, && vlan_bitmap_equal(vlans, ms->src_vlans) && mirror->out == out && mirror->out_vlan == out_vlan - && mirror->snaplen == ms->snaplen) + && mirror->snaplen == ms->snaplen + && nullable_string_is_equal(mirror->filter_str, ms->filter) + && !ms->filter) { hmapx_destroy(&srcs_map); hmapx_destroy(&dsts_map); - return 0; + return ECANCELED; } /* XXX: Not sure if these need to be thread safe. */ @@ -288,6 +317,51 @@ mirror_set(struct mbridge *mbridge, void *aux, mirror->out_vlan = out_vlan; mirror->snaplen = ms->snaplen; + if (!nullable_string_is_equal(mirror->filter_str, ms->filter) || + (mirror->filter_str || ms->filter)) { + if (mirror->filter_str) { + ovsrcu_postpone(filtermask_free, + ovsrcu_get_protected(struct filtermask *, + &mirror->filter_mask)); + free(mirror->filter_str); + mirror->filter_str = NULL; + ovsrcu_set(&mirror->filter_mask, NULL); + } + + if (ms->filter && strlen(ms->filter)) { + struct ofputil_port_map map = OFPUTIL_PORT_MAP_INITIALIZER(&map); + struct flow_wildcards wc; + struct flow flow; + 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); + ofputil_port_map_destroy(&map); + if (err) { + VLOG_WARN("filter is invalid: %s", err); + free(err); + mirror_destroy(mbridge, mirror->aux); + return EINVAL; + } + + /* If the user wants to filter on in_port, they should use the srcs + * bundle. Users setting in_port could experience unexpected + * behavior, and it would be overly complex to detect all possible + * issues. So instead we attempt to extract the in_port and error + * if successful. */ + if (wc.masks.in_port.ofp_port) { + VLOG_WARN("filter is invalid due to in_port field."); + mirror_destroy(mbridge, mirror->aux); + return EINVAL; + } + + mirror->filter_str = xstrdup(ms->filter); + ovsrcu_set(&mirror->filter_mask, filtermask_create(&flow, &wc)); + } + } + /* Update mbundles. */ mirror_bit = MIRROR_MASK_C(1) << mirror->idx; CMAP_FOR_EACH (mbundle, cmap_node, &mirror->mbridge->mbundles) { @@ -343,6 +417,15 @@ mirror_destroy(struct mbridge *mbridge, void *aux) ovsrcu_postpone(free, vlans); } + if (mirror->filter_str) { + ovsrcu_postpone(filtermask_free, + ovsrcu_get_protected(struct filtermask *, + &mirror->filter_mask)); + free(mirror->filter_str); + mirror->filter_str = NULL; + ovsrcu_set(&mirror->filter_mask, NULL); + } + mbridge->mirrors[mirror->idx] = NULL; /* mirror_get() might have just read the pointer, so we must postpone the * free. */ @@ -414,7 +497,9 @@ mirror_update_stats(struct mbridge *mbridge, mirror_mask_t mirrors, * in which a 1-bit indicates that the mirror includes a particular VLAN, * 'mc->dup_mirrors' receives a bitmap of mirrors whose output duplicates * mirror 'index', 'mc->out' receives the output ofbundle (if any), - * and 'mc->out_vlan' receives the output VLAN (if any). + * and 'mc->out_vlan' receives the output VLAN (if any). In cases where the + * mirror has a filter configured 'mc->filter_flow' and 'mc->filter_mask' + * receives the flow and mask that this mirror should collect. * * Everything returned here is assumed to be RCU protected. */ @@ -423,6 +508,7 @@ mirror_get(struct mbridge *mbridge, int index, struct mirror_config *mc) { struct mirror *mirror; + struct filtermask *fm; if (!mc || !mbridge) { return false; @@ -440,6 +526,14 @@ mirror_get(struct mbridge *mbridge, int index, mc->out_bundle = mirror->out ? mirror->out->ofbundle : NULL; mc->out_vlan = mirror->out_vlan; mc->snaplen = mirror->snaplen; + fm = ovsrcu_get(struct filtermask *, &mirror->filter_mask); + if (fm) { + mc->filter_flow = fm->flow; + mc->filter_mask = fm->mask; + } else { + mc->filter_flow = NULL; + mc->filter_mask = NULL; + } return true; } diff --git a/ofproto/ofproto-dpif-mirror.h b/ofproto/ofproto-dpif-mirror.h index 37d57463c..90331ede5 100644 --- a/ofproto/ofproto-dpif-mirror.h +++ b/ofproto/ofproto-dpif-mirror.h @@ -23,8 +23,8 @@ typedef uint32_t mirror_mask_t; struct ofproto_mirror_settings; -struct ofproto_dpif; struct ofbundle; +struct ofproto; struct mirror_bundles { struct ofbundle **srcs; @@ -43,6 +43,11 @@ struct mirror_config { /* VLANs of packets to select for mirroring. */ unsigned long *vlans; /* vlan_bitmap, NULL selects all VLANs. */ + /* The flow if a filter is used, or NULL. */ + struct miniflow *filter_flow; + /* The filter's flow mask, or NULL. */ + struct minimask *filter_mask; + /* Output (mutually exclusive). */ struct ofbundle *out_bundle; /* A registered ofbundle handle or NULL. */ uint16_t out_vlan; /* Output VLAN, not used if out_bundle is @@ -76,7 +81,7 @@ bool mbridge_need_revalidate(struct mbridge *); void mbridge_register_bundle(struct mbridge *, struct ofbundle *); void mbridge_unregister_bundle(struct mbridge *, struct ofbundle *); -int mirror_set(struct mbridge *, void *aux, +int mirror_set(struct mbridge *, const struct ofproto *, void *aux, const struct ofproto_mirror_settings *, const struct mirror_bundles *); void mirror_destroy(struct mbridge *, void *aux); diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 55846fe98..88015f817 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -2250,7 +2250,8 @@ lookup_input_bundle(const struct xlate_ctx *ctx, /* Mirrors the packet represented by 'ctx' to appropriate mirror destinations, * given the packet is ingressing or egressing on 'xbundle', which has ingress - * or egress (as appropriate) mirrors 'mirrors'. */ + * or egress (as appropriate) mirrors 'mirrors'. In cases where a mirror is + * filtered, the current flows wildcard will be modified. */ static void mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle, mirror_mask_t mirrors) @@ -2302,6 +2303,18 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle, continue; } + /* After the VLAN check, apply a flow mask if a filter is specified. */ + if (ctx->wc && mc.filter_flow) { + flow_wildcards_union_with_minimask(ctx->wc, mc.filter_mask); + if (!OVS_UNLIKELY( + miniflow_equal_flow_in_minimask(mc.filter_flow, + &ctx->xin->flow, + mc.filter_mask))) { + mirrors = zero_rightmost_1bit(mirrors); + continue; + } + } + /* We sent a packet to this mirror. */ used_mirrors |= rightmost_1bit(mirrors); diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 6f5830e40..7a3df1a47 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -3754,7 +3754,17 @@ mirror_set__(struct ofproto *ofproto_, void *aux, mb.n_dsts = s->n_dsts; mb.out_bundle = bundle_lookup(ofproto, s->out_bundle); - error = mirror_set(ofproto->mbridge, aux, s, &mb); + error = mirror_set(ofproto->mbridge, ofproto_, aux, s, &mb); + + if (!error) { + ofproto->backer->need_revalidate = REV_RECONFIGURE; + } else if (error == ECANCELED) { + /* The user requested a change that is identical to the current state, + * the reconfiguration is canceled, but don't log an error message + * about that. */ + error = 0; + } + free(mb.srcs); free(mb.dsts); return error; diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h index 1c07df275..655caa14e 100644 --- a/ofproto/ofproto.h +++ b/ofproto/ofproto.h @@ -501,6 +501,9 @@ struct ofproto_mirror_settings { uint16_t out_vlan; /* Output VLAN, only if out_bundle is NULL. */ uint16_t snaplen; /* Max packet size of a mirrored packet in byte, set to 0 equals 65535. */ + + /* Output filter. */ + char *filter; }; int ofproto_mirror_register(struct ofproto *, void *aux, diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 3eaccb13a..4e7913352 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -5199,6 +5199,171 @@ OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([ofproto-dpif - mirroring, filter]) +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="icmp"], [0], [ignore]) + +icmp_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=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)" +tcp_flow1="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)" +tcp_flow2="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)" + +AT_CHECK([ovs-ofctl del-flows br0]) +AT_CHECK([ovs-ofctl add-flow br0 'actions=normal' ]) + +dnl Add non-matching flows, then change the mirror to match one of the flows, +dnl then add a matching flow. +AT_CHECK([ovs-appctl netdev-dummy/receive p1 $icmp_flow]) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 $tcp_flow1]) +AT_CHECK([ovs-vsctl set mirror mymirror filter="tcp"], [0]) +AT_CHECK([ovs-appctl revalidator/wait]) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 $tcp_flow2]) +AT_CHECK([ovs-appctl dpif/dump-flows --names br0 | strip_ufid | strip_used | sort], [0], [dnl +recirc_id(0),in_port(p1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),dnl +eth_type(0x0800),ipv4(proto=1,frag=no), packets:0, bytes:0, used:never, actions:br0,p2 +recirc_id(0),in_port(p1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),dnl +eth_type(0x0800),ipv4(proto=6,frag=no), packets:1, bytes:118, used:0.0s, actions:p3,br0,p2 +]) +AT_CHECK([ovs-appctl dpctl/dump-flows --names | strip_ufid | strip_used | sort], [0], [dnl +flow-dump from the main thread: +recirc_id(0),in_port(p1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),dnl +eth_type(0x0800),ipv4(proto=1,frag=no), packets:0, bytes:0, used:never, actions:br0,p2 +recirc_id(0),in_port(p1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),dnl +eth_type(0x0800),ipv4(proto=6,frag=no), packets:1, bytes:118, used:0.0s, actions:p3,br0,p2 +]) + +AT_CHECK([ovs-ofctl del-flows br0]) +AT_CHECK([ovs-ofctl add-flow br0 "in_port=1 actions=output:2"]) +AT_CHECK([ovs-ofctl add-flow br0 "in_port=2 actions=output:1"]) + +dnl Add mirrored flow after non-mirrored flow. +AT_CHECK([ovs-vsctl set mirror mymirror filter="icmp"], [0]) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 $tcp_flow1]) +AT_CHECK([ovs-appctl dpif/dump-flows -m br0 | grep -cE "actions:p2$"], [0], [1 +]) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 $icmp_flow]) +AT_CHECK([ovs-appctl dpif/dump-flows -m br0 | grep -cE "actions:p3,p2$"], [0], [1 +]) + +dnl Check one direction, only icmp should mirror. +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$icmp_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),$tcp_flow1"], [0], [stdout]) +AT_CHECK_UNQUOTED([tail -1 stdout], [0], + [Datapath actions: 2 +]) + +dnl Check other direction, only icmp should mirror. +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$icmp_flow"], [0], [stdout]) +AT_CHECK_UNQUOTED([tail -1 stdout], [0], + [Datapath actions: 3,1 +]) + +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$tcp_flow1"], [0], [stdout]) +AT_CHECK_UNQUOTED([tail -1 stdout], [0], + [Datapath actions: 1 +]) + +dnl Change filter to tcp, only tcp should mirror. +AT_CHECK([ovs-vsctl set mirror mymirror filter="tcp"], [0]) + +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$icmp_flow"], [0], [stdout]) +AT_CHECK_UNQUOTED([tail -1 stdout], [0], + [Datapath actions: 2 +]) + +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$tcp_flow1"], [0], [stdout]) +AT_CHECK_UNQUOTED([tail -1 stdout], [0], + [Datapath actions: 3,2 +]) + +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$icmp_flow"], [0], [stdout]) +AT_CHECK_UNQUOTED([tail -1 stdout], [0], + [Datapath actions: 1 +]) + +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$tcp_flow1"], [0], [stdout]) +AT_CHECK_UNQUOTED([tail -1 stdout], [0], + [Datapath actions: 3,1 +]) + +dnl Invalid filter. Nothing should mirror, error should be logged. +AT_CHECK([ovs-vsctl set mirror mymirror filter="invalid"], [0]) +dnl Setting an in_port is also invalid. +AT_CHECK([ovs-vsctl set mirror mymirror filter="\"in_port=p1\""], [0]) + +dnl Each of the above two lines should produce two log messages. +OVS_WAIT_UNTIL([test $(grep -Ec "filter is invalid|mirror mymirror configuration is invalid" ovs-vswitchd.log) -eq 4]) + +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$icmp_flow"], [0], [stdout]) +AT_CHECK_UNQUOTED([tail -1 stdout], [0], + [Datapath actions: 2 +]) + +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$tcp_flow1"], [0], [stdout]) +AT_CHECK_UNQUOTED([tail -1 stdout], [0], + [Datapath actions: 2 +]) + +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$icmp_flow"], [0], [stdout]) +AT_CHECK_UNQUOTED([tail -1 stdout], [0], + [Datapath actions: 1 +]) + +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$tcp_flow1"], [0], [stdout]) +AT_CHECK_UNQUOTED([tail -1 stdout], [0], + [Datapath actions: 1 +]) + +dnl Check more complex filter cases with partially overlapping default wildcards. +AT_CHECK([ovs-vsctl set mirror mymirror filter="\"tcp,tcp_dst=80\""], [0]) +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$tcp_flow1"], [0], [stdout]) +AT_CHECK_UNQUOTED([tail -1 stdout], [0], + [Datapath actions: 2 +]) + +dnl Change port number. +AT_CHECK([ovs-appctl dpif-dummy/change-port-number ovs-dummy p1 8]) +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(8),$tcp_flow2"], [0], [stdout]) +AT_CHECK_UNQUOTED([tail -1 stdout], [0], + [Datapath actions: 3,2 +]) + +dnl Empty filter, all traffic should mirror. +AT_CHECK([ovs-vsctl clear mirror mymirror filter], [0]) + +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(8),$icmp_flow"], [0], [stdout]) +AT_CHECK_UNQUOTED([tail -1 stdout], [0], + [Datapath actions: 3,2 +]) + +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(8),$tcp_flow1"], [0], [stdout]) +AT_CHECK_UNQUOTED([tail -1 stdout], [0], + [Datapath actions: 3,2 +]) + +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$icmp_flow"], [0], [stdout]) +AT_CHECK_UNQUOTED([tail -1 stdout], [0], + [Datapath actions: 3,8 +]) + +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$tcp_flow1"], [0], [stdout]) +AT_CHECK_UNQUOTED([tail -1 stdout], [0], + [Datapath actions: 3,8 +]) + +OVS_VSWITCHD_STOP(["/filter is invalid: invalid: unknown field invalid/d +/filter is invalid due to in_port field/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/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in index eada803bb..07fc435fa 100755 --- a/utilities/ovs-tcpdump.in +++ b/utilities/ovs-tcpdump.in @@ -142,6 +142,7 @@ The following options are available: --mirror-to The name for the mirror port to use (optional) Default 'miINTERFACE' --span If specified, mirror all ports (optional) + --filter Set an OpenFlow formatted preselection filter """ % {'prog': sys.argv[0]}) sys.exit(0) @@ -354,7 +355,7 @@ class OVSDB(object): return result def bridge_mirror(self, intf_name, mirror_intf_name, br_name, - mirror_select_all=False): + mirror_select_all=False, mirror_filter=None): txn = self._start_txn() mirror = txn.insert(self.get_table('Mirror')) @@ -362,6 +363,9 @@ class OVSDB(object): mirror.select_all = mirror_select_all + if mirror_filter is not None: + mirror.filter = mirror_filter + mirrored_port = self._find_row_by_name('Port', intf_name) mirror.verify('select_dst_port') @@ -445,6 +449,7 @@ def main(): mirror_interface = None mirror_select_all = False dump_cmd = 'tcpdump' + mirror_filter = None for cur, nxt in argv_tuples(sys.argv[1:]): if skip_next: @@ -474,6 +479,10 @@ def main(): elif cur in ['--span']: mirror_select_all = True continue + elif cur in ['--filter']: + mirror_filter = nxt + skip_next = True + continue tcpdargs.append(cur) if interface is None: @@ -526,7 +535,7 @@ def main(): ovsdb.make_port(mirror_interface, ovsdb.port_bridge(interface)) ovsdb.bridge_mirror(interface, mirror_interface, ovsdb.port_bridge(interface), - mirror_select_all) + mirror_select_all, mirror_filter=mirror_filter) except OVSDBException as oe: print("ERROR: Unable to properly setup the mirror: %s." % str(oe)) sys.exit(1) diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index 95a65fcdc..39f8fba49 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -5144,6 +5144,7 @@ mirror_configure(struct mirror *m) { const struct ovsrec_mirror *cfg = m->cfg; struct ofproto_mirror_settings s; + int ret; /* Set name. */ if (strcmp(cfg->name, m->name)) { @@ -5212,8 +5213,18 @@ mirror_configure(struct mirror *m) /* Get VLAN selection. */ s.src_vlans = vlan_bitmap_from_array(cfg->select_vlan, cfg->n_select_vlan); + /* Set the filter, mirror_set() will strdup this pointer. */ + s.filter = cfg->filter; + /* Configure. */ - ofproto_mirror_register(m->bridge->ofproto, m, &s); + ret = ofproto_mirror_register(m->bridge->ofproto, m, &s); + if (ret == EOPNOTSUPP) { + VLOG_ERR("ofproto %s: does not support mirroring", + m->bridge->ofproto->name); + } else if (ret) { + VLOG_ERR("bridge %s: mirror %s configuration is invalid", + m->bridge->name, m->name); + } /* Clean up. */ if (s.srcs != s.dsts) { diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema index e2d5e2e85..f42f09121 100644 --- a/vswitchd/vswitch.ovsschema +++ b/vswitchd/vswitch.ovsschema @@ -1,6 +1,6 @@ {"name": "Open_vSwitch", - "version": "8.5.0", - "cksum": "4040946650 27557", + "version": "8.6.0", + "cksum": "401789815 27661", "tables": { "Open_vSwitch": { "columns": { @@ -461,6 +461,9 @@ "type": {"key": "string", "value": "integer", "min": 0, "max": "unlimited"}, "ephemeral": true}, + "filter": { + "type": {"key": {"type": "string"}, + "min": 0, "max": 1}}, "external_ids": { "type": {"key": "string", "value": "string", "min": 0, "max": "unlimited"}}}}, diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index 8a1b607d7..2aea5c9fd 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -5256,6 +5256,22 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ VLANs on which packets are selected for mirroring. An empty set selects packets on all VLANs. </column> + <column name="filter"> + <p> + 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, support for the <code> + in_port</code> field is not supported; <ref + column="select_src_port"/> should be used to limit the mirror to + an source port. + </p> + <p> + This filter is applied after <ref column="select_all"/>, <ref + column="select_dst_port"/>, <ref column="select_src_port"/>, and + <ref column="select_vlan"/>. + </p> + </column> </group> <group title="Mirroring Destination Configuration">
Currently a bridge mirror will collect all packets and tools like ovs-tcpdump can apply additional filters after they have already been duplicated by vswitchd. This can result in inefficient collection. This patch adds support to apply pre-selection to bridge mirrors, which can limit which packets are mirrored based on flow metadata. This significantly improves overall vswitchd performance during mirroring if only a subset of traffic is required. Signed-off-by: Mike Pattrick <mkp@redhat.com> --- v8: - Corrected code from v7 related to sequence and in_port. Mirrors reject filters with an in_port set as this could cause confusion. - Combined ovsrcu pointers into a new struct, minimatch wasn't used because the minimatch_* functions didn't fit the usage here. - Added a test to check for modifying filters when partially overlapping flows already exist. - Corrected documentation. v9: - Explicitly cleared mirror_config.filter* when not set --- Documentation/ref/ovs-tcpdump.8.rst | 8 +- NEWS | 6 + lib/flow.h | 9 ++ ofproto/ofproto-dpif-mirror.c | 104 +++++++++++++++++- ofproto/ofproto-dpif-mirror.h | 9 +- ofproto/ofproto-dpif-xlate.c | 15 ++- ofproto/ofproto-dpif.c | 12 +- ofproto/ofproto.h | 3 + tests/ofproto-dpif.at | 165 ++++++++++++++++++++++++++++ utilities/ovs-tcpdump.in | 13 ++- vswitchd/bridge.c | 13 ++- vswitchd/vswitch.ovsschema | 7 +- vswitchd/vswitch.xml | 16 +++ 13 files changed, 365 insertions(+), 15 deletions(-)