diff mbox series

[ovs-dev,v7,2/2] ofproto-dpif-mirror: Add support for pre-selection filter.

Message ID 20240226211649.553204-2-mkp@redhat.com
State Changes Requested
Headers show
Series [ovs-dev,v7,1/2] ofproto-dpif-mirror: Reduce number of function parameters. | expand

Checks

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

Commit Message

Mike Pattrick Feb. 26, 2024, 9:16 p.m. UTC 
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>
---
v7:
 - Make sure filter mask is added to masks of non-matching flows.
 - Added additional tests.
---
 Documentation/ref/ovs-tcpdump.8.rst |   8 +-
 NEWS                                |   3 +
 lib/flow.c                          |  21 +++-
 lib/flow.h                          |  12 +++
 ofproto/ofproto-dpif-mirror.c       |  78 ++++++++++++++-
 ofproto/ofproto-dpif-mirror.h       |  12 ++-
 ofproto/ofproto-dpif-xlate.c        |  26 ++++-
 ofproto/ofproto-dpif.c              |   9 +-
 ofproto/ofproto-dpif.h              |   6 ++
 ofproto/ofproto.c                   |   4 +-
 ofproto/ofproto.h                   |   3 +
 tests/ofproto-dpif.at               | 142 ++++++++++++++++++++++++++++
 utilities/ovs-tcpdump.in            |  13 ++-
 vswitchd/bridge.c                   |  13 ++-
 vswitchd/vswitch.ovsschema          |   5 +-
 vswitchd/vswitch.xml                |  13 +++
 16 files changed, 343 insertions(+), 25 deletions(-)

Comments

Eelco Chaudron March 1, 2024, 4:30 p.m. UTC | #1 
On 26 Feb 2024, at 22:16, 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 the patch revision. Some small comments below.

Cheers,

Eelco

> ---
> v7:
>  - Make sure filter mask is added to masks of non-matching flows.
>  - Added additional tests.
> ---
>  Documentation/ref/ovs-tcpdump.8.rst |   8 +-
>  NEWS                                |   3 +
>  lib/flow.c                          |  21 +++-
>  lib/flow.h                          |  12 +++
>  ofproto/ofproto-dpif-mirror.c       |  78 ++++++++++++++-
>  ofproto/ofproto-dpif-mirror.h       |  12 ++-
>  ofproto/ofproto-dpif-xlate.c        |  26 ++++-
>  ofproto/ofproto-dpif.c              |   9 +-
>  ofproto/ofproto-dpif.h              |   6 ++
>  ofproto/ofproto.c                   |   4 +-
>  ofproto/ofproto.h                   |   3 +
>  tests/ofproto-dpif.at               | 142 ++++++++++++++++++++++++++++
>  utilities/ovs-tcpdump.in            |  13 ++-
>  vswitchd/bridge.c                   |  13 ++-
>  vswitchd/vswitch.ovsschema          |   5 +-
>  vswitchd/vswitch.xml                |  13 +++
>  16 files changed, 343 insertions(+), 25 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 c9e4064e6..35f7eb0c7 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -4,6 +4,9 @@ Post-v3.3.0
>       * Conntrack now supports 'random' flag for selecting ports in a range
>         while natting and 'persistent' flag for selection of the IP address
>         from a range.
> +   - OVSDB:
> +     * Added a new filter column in the Mirror table which can be used to
> +       apply filters to mirror ports.
>
>
>  v3.3.0 - 16 Feb 2024
> diff --git a/lib/flow.c b/lib/flow.c
> index 8e3402388..a088bdc86 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -3569,7 +3569,7 @@ miniflow_equal_in_minimask(const struct miniflow *a, const struct miniflow *b,
>      return true;
>  }
>
> -/* Returns true if 'a' and 'b' are equal at the places where there are 1-bits
> +/* Returns true if 'a' and 'b' are equal at the places where there are 0-bits

I think the logic is that only the differences on the masked data will return false. So the original text is right.

>   * in 'mask', false if they differ. */
>  bool
>  miniflow_equal_flow_in_minimask(const struct miniflow *a, const struct flow *b,
> @@ -3587,6 +3587,25 @@ miniflow_equal_flow_in_minimask(const struct miniflow *a, const struct flow *b,
>      return true;
>  }
>
> +/* Returns false if 'a' and 'b' differ in places where there are 1-bits in
> + * 'wc', true otherwise. */

Maybe wright this the same as above to avoid confusion, i.e.

Returns true if 'a' and 'b' are equal at the places where there are 1-bits in ‘wc’, false if they differ [at the places where there are 1-bits in ‘wc’].

> +bool
> +miniflow_equal_flow_in_flow_wc(const struct miniflow *a, const struct flow *b,
> +                               const struct flow_wildcards *wc)
> +{
> +    const struct flow *wc_masks = &wc->masks;
> +    size_t idx;
> +
> +    FLOWMAP_FOR_EACH_INDEX (idx, a->map) {
> +        if ((miniflow_get(a, idx) ^ flow_u64_value(b, idx)) &
> +                flow_u64_value(wc_masks, idx)) {
> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}
> +
>  
>  void
>  minimask_init(struct minimask *mask, const struct flow_wildcards *wc)
> diff --git a/lib/flow.h b/lib/flow.h
> index 75a9be3c1..a644be39d 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -748,6 +748,9 @@ bool miniflow_equal_in_minimask(const struct miniflow *a,
>  bool miniflow_equal_flow_in_minimask(const struct miniflow *a,
>                                       const struct flow *b,
>                                       const struct minimask *);
> +bool miniflow_equal_flow_in_flow_wc(const struct miniflow *a,
> +                                    const struct flow *b,
> +                                    const struct flow_wildcards *);
>  uint32_t miniflow_hash_5tuple(const struct miniflow *flow, uint32_t basis);
>
>  
> @@ -939,6 +942,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 a84c843b3..38e8d1fca 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"
>
> @@ -50,6 +51,7 @@ struct mbundle {
>
>  struct mirror {
>      struct mbridge *mbridge;    /* Owning ofproto. */
> +    uint64_t change_seq;        /* Change sequence of owning ofproto. */
>      size_t idx;                 /* In ofproto's "mirrors" array. */
>      void *aux;                  /* Key supplied by ofproto's client. */
>
> @@ -57,6 +59,11 @@ struct mirror {
>      struct hmapx srcs;          /* Contains "struct mbundle*"s. */
>      struct hmapx dsts;          /* Contains "struct mbundle*"s. */
>
> +    /* Filter criteria. */
> +    OVSRCU_TYPE(struct miniflow *) filter;
> +    OVSRCU_TYPE(struct minimask *) 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. */
> @@ -207,8 +214,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)
>
>  {
> @@ -265,7 +272,11 @@ 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)
> +        /* If there is a filter, the datapath ports may have changed which
> +         * could affect how the filter performs. Check change_seq if . */

Was some comment text deleted?

> +        && (!ms->filter || ofproto->change_seq == mirror->change_seq))
>      {
>          hmapx_destroy(&srcs_map);
>          hmapx_destroy(&dsts_map);
> @@ -289,6 +300,50 @@ 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)) {

Wasn’t the goal to also re-configure in the case of a seq change?

> +        if (mirror->filter_str) {
> +            ovsrcu_postpone(free, ovsrcu_get_protected(struct miniflow *,
> +                                                       &mirror->filter));
> +            ovsrcu_postpone(free, ovsrcu_get_protected(struct minimask *,
> +                                                       &mirror->mask));
> +            free(mirror->filter_str);
> +            mirror->filter_str = NULL;
> +            ovsrcu_set(&mirror->filter, NULL);
> +            ovsrcu_set(&mirror->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;
> +            }
> +
> +            /* Do not allow a filter on in_port. */
> +            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->mask, minimask_create(&wc));
> +            ovsrcu_set(&mirror->filter, miniflow_create(&flow));
> +            mirror->change_seq = ofproto->change_seq;
> +        }
> +    }
> +
>      /* Update mbundles. */
>      mirror_bit = MIRROR_MASK_C(1) << mirror->idx;
>      CMAP_FOR_EACH (mbundle, cmap_node, &mirror->mbridge->mbundles) {
> @@ -344,6 +399,17 @@ mirror_destroy(struct mbridge *mbridge, void *aux)
>          ovsrcu_postpone(free, vlans);
>      }
>
> +    if (mirror->filter_str) {
> +        ovsrcu_postpone(free, ovsrcu_get_protected(struct miniflow *,
> +                                                   &mirror->filter));
> +        ovsrcu_postpone(free, ovsrcu_get_protected(struct minimask *,
> +                                                   &mirror->mask));
> +        free(mirror->filter_str);
> +        mirror->filter_str = NULL;
> +        ovsrcu_set(&mirror->filter, NULL);
> +        ovsrcu_set(&mirror->mask, NULL);
> +    }
> +
>      mbridge->mirrors[mirror->idx] = NULL;
>      /* mirror_get() might have just read the pointer, so we must postpone the
>       * free. */
> @@ -415,7 +481,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.
>   */
> @@ -441,6 +509,8 @@ 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;
> +    mc->filter_flow = ovsrcu_get(struct miniflow *, &mirror->filter);
> +    mc->filter_mask = ovsrcu_get(struct minimask *, &mirror->mask);
>      return true;
>  }
>  
> diff --git a/ofproto/ofproto-dpif-mirror.h b/ofproto/ofproto-dpif-mirror.h
> index a983b3876..7d40f7ec2 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
> @@ -53,7 +58,6 @@ struct mirror_config {
>      uint16_t snaplen;
>  };
>
> -

I think you need to fix this in patch 1.

>  /* The following functions are used by handler threads without any locking,
>   * assuming RCU protection. */
>
> @@ -78,8 +82,8 @@ 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 *mbridge, void *aux,
> -               const struct ofproto_mirror_settings *ms,
> +int mirror_set(struct mbridge *mbridge, const struct ofproto *ofproto,
> +               void *aux, const struct ofproto_mirror_settings *ms,
>                 const struct mirror_bundles *mb);
>  void mirror_destroy(struct mbridge *, void *aux);
>  int mirror_get_stats(struct mbridge *, void *aux, uint64_t *packets,
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index aa2a514f9..58d89b04e 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,29 @@ 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) {
> +            if (OVS_UNLIKELY(
> +                miniflow_equal_flow_in_minimask(mc.filter_flow,
> +                                                &ctx->xin->flow,
> +                                                mc.filter_mask))) {
> +                flow_wildcards_union_with_minimask(ctx->wc, mc.filter_mask);
> +            } else if (!miniflow_equal_flow_in_minimask(&mc.filter_mask->masks,
> +                                                        &ctx->wc->masks,
> +                                                        mc.filter_mask)) {
> +                /* If flow isn't in the mirror filter but flow's wildcard mask
> +                 * doesn't overlap with the filter mask then add the filter
> +                 * mask to the flow's wildcard mask. This prevents mirrored
> +                 * traffic from matching on this flow. */
> +                flow_wildcards_union_with_minimask(ctx->wc, mc.filter_mask);
> +                mirrors = zero_rightmost_1bit(mirrors);
> +                continue;
> +            } else {
> +                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 4f31f5ebb..85fe6c10f 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -222,13 +222,6 @@ static void ct_zone_config_uninit(struct dpif_backer *backer);
>  static void ct_zone_timeout_policy_sweep(struct dpif_backer *backer);
>  static void ct_zone_limits_commit(struct dpif_backer *backer);
>
> -static inline struct ofproto_dpif *
> -ofproto_dpif_cast(const struct ofproto *ofproto)
> -{
> -    ovs_assert(ofproto->ofproto_class == &ofproto_dpif_class);
> -    return CONTAINER_OF(ofproto, struct ofproto_dpif, up);
> -}
> -
>  /* Global variables. */
>  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>
> @@ -3669,7 +3662,7 @@ 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);
>      free(mb.srcs);
>      free(mb.dsts);
>      return error;
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index 92d33aa64..b95682595 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -363,6 +363,12 @@ struct ofproto_dpif {
>
>  struct ofproto_dpif *ofproto_dpif_lookup_by_name(const char *name);
>  struct ofproto_dpif *ofproto_dpif_lookup_by_uuid(const struct uuid *uuid);
> +static inline struct ofproto_dpif *
> +ofproto_dpif_cast(const struct ofproto *ofproto)
> +{
> +    ovs_assert(ofproto->ofproto_class == &ofproto_dpif_class);
> +    return CONTAINER_OF(ofproto, struct ofproto_dpif, up);
> +}
>
>  ovs_version_t ofproto_dpif_get_tables_version(struct ofproto_dpif *);
>
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 122a06f30..06624006a 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -6389,7 +6389,7 @@ handle_flow_mod__(struct ofproto *ofproto, const struct ofputil_flow_mod *fm,
>      error = ofproto_flow_mod_start(ofproto, &ofm);
>      if (!error) {
>          ofproto_bump_tables_version(ofproto);
> -        error = ofproto_flow_mod_finish(ofproto, &ofm, req);
> +        error = ofproto_flow_mod_finish(ofproto, &ofm, req);
>          ofmonitor_flush(ofproto->connmgr);
>      }
>      ovs_mutex_unlock(&ofproto_mutex);
> @@ -8460,7 +8460,7 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags)
>              /* Send error referring to the original message. */
>              ofconn_send_error(ofconn, be->msg, error);
>              error = OFPERR_OFPBFC_MSG_FAILED;
> -
> +
>              /* 2. Revert.  Undo all the changes made above. */
>              LIST_FOR_EACH_REVERSE_CONTINUE(be, node, &bundle->msg_list) {
>                  if (be->type == OFPTYPE_FLOW_MOD) {
> 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 daeea7775..6c5bf048a 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -5182,6 +5182,148 @@ 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])
> +

My OCD probably wants some more aligned indentation, but I see both are used throughout the various test cases.

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])

> +AT_DATA([flows.txt], [dnl
> +in_port=1 actions=output:2
> +in_port=2 actions=output:1
> +])
> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> +
> +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)"
> +
> +dnl Add mirrored flow after non-mirrored flow.
> +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])
> +
> +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 4cbd9a5d3..7483b52ac 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..bdb2c8c0a 100644
> --- a/vswitchd/vswitch.ovsschema
> +++ b/vswitchd/vswitch.ovsschema
> @@ -1,6 +1,6 @@
>  {"name": "Open_vSwitch",
>   "version": "8.5.0",
> - "cksum": "4040946650 27557",
> + "cksum": "2211155747 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 612ba41e3..ec536a08e 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -5256,6 +5256,19 @@ 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>
> +            Filter to apply to mirror. Flows that match <ref column="filter"/>
> +            will flow through this mirror, flows that do not match will be
> +            ignored. The <ref column="filter"/> syntax is described in
> +            <code>ovs-fields</code>(7).
> +        </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
Ilya Maximets March 1, 2024, 7:52 p.m. UTC | #2 
On 2/26/24 22:16, 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>
> ---
> v7:
>  - Make sure filter mask is added to masks of non-matching flows.
>  - Added additional tests.

Hi, Mike.  Thanks for v7!

I see Elco made some comments, so I'll just throw in a few from me.
See inline.

Best regards, Ilya Maixmets.

> ---
>  Documentation/ref/ovs-tcpdump.8.rst |   8 +-
>  NEWS                                |   3 +
>  lib/flow.c                          |  21 +++-
>  lib/flow.h                          |  12 +++
>  ofproto/ofproto-dpif-mirror.c       |  78 ++++++++++++++-
>  ofproto/ofproto-dpif-mirror.h       |  12 ++-
>  ofproto/ofproto-dpif-xlate.c        |  26 ++++-
>  ofproto/ofproto-dpif.c              |   9 +-
>  ofproto/ofproto-dpif.h              |   6 ++
>  ofproto/ofproto.c                   |   4 +-

All the changes for ofproto.c are not related to this patch.

>  ofproto/ofproto.h                   |   3 +
>  tests/ofproto-dpif.at               | 142 ++++++++++++++++++++++++++++
>  utilities/ovs-tcpdump.in            |  13 ++-
>  vswitchd/bridge.c                   |  13 ++-
>  vswitchd/vswitch.ovsschema          |   5 +-
>  vswitchd/vswitch.xml                |  13 +++
>  16 files changed, 343 insertions(+), 25 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 c9e4064e6..35f7eb0c7 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -4,6 +4,9 @@ Post-v3.3.0
>       * Conntrack now supports 'random' flag for selecting ports in a range
>         while natting and 'persistent' flag for selection of the IP address
>         from a range.
> +   - OVSDB:

'OVSDB' section is usually for changes in ovsdb-server itself.
OVS configuration changes can be in the ovs-vsctl section, if
you're describing a new option for a mirror in ovs-vsctl.  Or
just a separate entry outsie of any sections.  ovs-vsctl might
be a better option here, but you may want to rephrase the entry
in terms of ovs-vsctl configuration.

You may also add an entry for ovs-tcpdump since you're changing
it as well.

> +     * Added a new filter column in the Mirror table which can be used to
> +       apply filters to mirror ports.
>  
>  
>  v3.3.0 - 16 Feb 2024
> diff --git a/lib/flow.c b/lib/flow.c
> index 8e3402388..a088bdc86 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -3569,7 +3569,7 @@ miniflow_equal_in_minimask(const struct miniflow *a, const struct miniflow *b,
>      return true;
>  }
>  
> -/* Returns true if 'a' and 'b' are equal at the places where there are 1-bits
> +/* Returns true if 'a' and 'b' are equal at the places where there are 0-bits
>   * in 'mask', false if they differ. */
>  bool
>  miniflow_equal_flow_in_minimask(const struct miniflow *a, const struct flow *b,
> @@ -3587,6 +3587,25 @@ miniflow_equal_flow_in_minimask(const struct miniflow *a, const struct flow *b,
>      return true;
>  }
>  
> +/* Returns false if 'a' and 'b' differ in places where there are 1-bits in
> + * 'wc', true otherwise. */
> +bool
> +miniflow_equal_flow_in_flow_wc(const struct miniflow *a, const struct flow *b,
> +                               const struct flow_wildcards *wc)
> +{
> +    const struct flow *wc_masks = &wc->masks;
> +    size_t idx;
> +
> +    FLOWMAP_FOR_EACH_INDEX (idx, a->map) {
> +        if ((miniflow_get(a, idx) ^ flow_u64_value(b, idx)) &
> +                flow_u64_value(wc_masks, idx)) {
> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}

I don't see this function being used anywhere.  Do we need it?

> +
>  
>  void
>  minimask_init(struct minimask *mask, const struct flow_wildcards *wc)
> diff --git a/lib/flow.h b/lib/flow.h
> index 75a9be3c1..a644be39d 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -748,6 +748,9 @@ bool miniflow_equal_in_minimask(const struct miniflow *a,
>  bool miniflow_equal_flow_in_minimask(const struct miniflow *a,
>                                       const struct flow *b,
>                                       const struct minimask *);
> +bool miniflow_equal_flow_in_flow_wc(const struct miniflow *a,
> +                                    const struct flow *b,
> +                                    const struct flow_wildcards *);
>  uint32_t miniflow_hash_5tuple(const struct miniflow *flow, uint32_t basis);
>  
>  
> @@ -939,6 +942,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 a84c843b3..38e8d1fca 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"
>  
> @@ -50,6 +51,7 @@ struct mbundle {
>  
>  struct mirror {
>      struct mbridge *mbridge;    /* Owning ofproto. */
> +    uint64_t change_seq;        /* Change sequence of owning ofproto. */
>      size_t idx;                 /* In ofproto's "mirrors" array. */
>      void *aux;                  /* Key supplied by ofproto's client. */
>  
> @@ -57,6 +59,11 @@ struct mirror {
>      struct hmapx srcs;          /* Contains "struct mbundle*"s. */
>      struct hmapx dsts;          /* Contains "struct mbundle*"s. */
>  
> +    /* Filter criteria. */
> +    OVSRCU_TYPE(struct miniflow *) filter;
> +    OVSRCU_TYPE(struct minimask *) mask;

These are two separate pointers.  There is a chance that RCU reader
will read an old flow and a new mask that are not releated to each other.
We should probably just use a minimatch structure here.

It might also be possible to reuse some of the minimatch_* functions.

> +    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. */
> @@ -207,8 +214,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)
>  
>  {
> @@ -265,7 +272,11 @@ 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)
> +        /* If there is a filter, the datapath ports may have changed which
> +         * could affect how the filter performs. Check change_seq if . */
> +        && (!ms->filter || ofproto->change_seq == mirror->change_seq))
>      {
>          hmapx_destroy(&srcs_map);
>          hmapx_destroy(&dsts_map);
> @@ -289,6 +300,50 @@ 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)) {
> +        if (mirror->filter_str) {
> +            ovsrcu_postpone(free, ovsrcu_get_protected(struct miniflow *,
> +                                                       &mirror->filter));
> +            ovsrcu_postpone(free, ovsrcu_get_protected(struct minimask *,
> +                                                       &mirror->mask));
> +            free(mirror->filter_str);
> +            mirror->filter_str = NULL;
> +            ovsrcu_set(&mirror->filter, NULL);
> +            ovsrcu_set(&mirror->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;
> +            }
> +
> +            /* Do not allow a filter on in_port. */

Why do we have a port map then?

> +            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->mask, minimask_create(&wc));
> +            ovsrcu_set(&mirror->filter, miniflow_create(&flow));
> +            mirror->change_seq = ofproto->change_seq;
> +        }
> +    }
> +
>      /* Update mbundles. */
>      mirror_bit = MIRROR_MASK_C(1) << mirror->idx;
>      CMAP_FOR_EACH (mbundle, cmap_node, &mirror->mbridge->mbundles) {
> @@ -344,6 +399,17 @@ mirror_destroy(struct mbridge *mbridge, void *aux)
>          ovsrcu_postpone(free, vlans);
>      }
>  
> +    if (mirror->filter_str) {
> +        ovsrcu_postpone(free, ovsrcu_get_protected(struct miniflow *,
> +                                                   &mirror->filter));
> +        ovsrcu_postpone(free, ovsrcu_get_protected(struct minimask *,
> +                                                   &mirror->mask));
> +        free(mirror->filter_str);
> +        mirror->filter_str = NULL;
> +        ovsrcu_set(&mirror->filter, NULL);
> +        ovsrcu_set(&mirror->mask, NULL);
> +    }
> +
>      mbridge->mirrors[mirror->idx] = NULL;
>      /* mirror_get() might have just read the pointer, so we must postpone the
>       * free. */
> @@ -415,7 +481,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

Two spaces.

> + * 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.
>   */
> @@ -441,6 +509,8 @@ 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;
> +    mc->filter_flow = ovsrcu_get(struct miniflow *, &mirror->filter);
> +    mc->filter_mask = ovsrcu_get(struct minimask *, &mirror->mask);

Separate reads.  One can be NULL while the other is not.

>      return true;
>  }
>  
> diff --git a/ofproto/ofproto-dpif-mirror.h b/ofproto/ofproto-dpif-mirror.h
> index a983b3876..7d40f7ec2 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;

Again, maybe a minimatch will fit better?

> +
>      /* 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
> @@ -53,7 +58,6 @@ struct mirror_config {
>      uint16_t snaplen;
>  };
>  
> -
>  /* The following functions are used by handler threads without any locking,
>   * assuming RCU protection. */
>  
> @@ -78,8 +82,8 @@ 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 *mbridge, void *aux,
> -               const struct ofproto_mirror_settings *ms,
> +int mirror_set(struct mbridge *mbridge, const struct ofproto *ofproto,
> +               void *aux, const struct ofproto_mirror_settings *ms,

No need for names for obviously typed arguemnts.

>                 const struct mirror_bundles *mb);
>  void mirror_destroy(struct mbridge *, void *aux);
>  int mirror_get_stats(struct mbridge *, void *aux, uint64_t *packets,
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index aa2a514f9..58d89b04e 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

Two spaces.

> + * 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,29 @@ 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) {
> +            if (OVS_UNLIKELY(
> +                miniflow_equal_flow_in_minimask(mc.filter_flow,
> +                                                &ctx->xin->flow,
> +                                                mc.filter_mask))) {
> +                flow_wildcards_union_with_minimask(ctx->wc, mc.filter_mask);
> +            } else if (!miniflow_equal_flow_in_minimask(&mc.filter_mask->masks,
> +                                                        &ctx->wc->masks,
> +                                                        mc.filter_mask)) {
> +                /* If flow isn't in the mirror filter but flow's wildcard mask
> +                 * doesn't overlap with the filter mask then add the filter
> +                 * mask to the flow's wildcard mask. This prevents mirrored
> +                 * traffic from matching on this flow. */
> +                flow_wildcards_union_with_minimask(ctx->wc, mc.filter_mask);

This is not right, because masks has to be added *before* checking.
If we add masks only to those that match, we will install a too
wide datapath flow that will match packets that should have been
mirrored.

Please, aslo add a test for this case.

> +                mirrors = zero_rightmost_1bit(mirrors);
> +                continue;
> +            } else {
> +                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 4f31f5ebb..85fe6c10f 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -222,13 +222,6 @@ static void ct_zone_config_uninit(struct dpif_backer *backer);
>  static void ct_zone_timeout_policy_sweep(struct dpif_backer *backer);
>  static void ct_zone_limits_commit(struct dpif_backer *backer);
>  
> -static inline struct ofproto_dpif *
> -ofproto_dpif_cast(const struct ofproto *ofproto)
> -{
> -    ovs_assert(ofproto->ofproto_class == &ofproto_dpif_class);
> -    return CONTAINER_OF(ofproto, struct ofproto_dpif, up);
> -}
> -
>  /* Global variables. */
>  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>  
> @@ -3669,7 +3662,7 @@ 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);
>      free(mb.srcs);
>      free(mb.dsts);
>      return error;
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index 92d33aa64..b95682595 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -363,6 +363,12 @@ struct ofproto_dpif {
>  
>  struct ofproto_dpif *ofproto_dpif_lookup_by_name(const char *name);
>  struct ofproto_dpif *ofproto_dpif_lookup_by_uuid(const struct uuid *uuid);
> +static inline struct ofproto_dpif *
> +ofproto_dpif_cast(const struct ofproto *ofproto)
> +{
> +    ovs_assert(ofproto->ofproto_class == &ofproto_dpif_class);
> +    return CONTAINER_OF(ofproto, struct ofproto_dpif, up);
> +}

Why this function is moved?  I don't see it being used.

>  
>  ovs_version_t ofproto_dpif_get_tables_version(struct ofproto_dpif *);
>  
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 122a06f30..06624006a 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -6389,7 +6389,7 @@ handle_flow_mod__(struct ofproto *ofproto, const struct ofputil_flow_mod *fm,
>      error = ofproto_flow_mod_start(ofproto, &ofm);
>      if (!error) {
>          ofproto_bump_tables_version(ofproto);
> -        error = ofproto_flow_mod_finish(ofproto, &ofm, req);        
> +        error = ofproto_flow_mod_finish(ofproto, &ofm, req);
>          ofmonitor_flush(ofproto->connmgr);
>      }
>      ovs_mutex_unlock(&ofproto_mutex);
> @@ -8460,7 +8460,7 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags)
>              /* Send error referring to the original message. */
>              ofconn_send_error(ofconn, be->msg, error);
>              error = OFPERR_OFPBFC_MSG_FAILED;
> - 
> +
>              /* 2. Revert.  Undo all the changes made above. */
>              LIST_FOR_EACH_REVERSE_CONTINUE(be, node, &bundle->msg_list) {
>                  if (be->type == OFPTYPE_FLOW_MOD) {


Unrelated changes.

> 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 daeea7775..6c5bf048a 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -5182,6 +5182,148 @@ 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])
> +
> +AT_DATA([flows.txt], [dnl
> +in_port=1 actions=output:2
> +in_port=2 actions=output:1
> +])
> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> +
> +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)"
> +
> +dnl Add mirrored flow after non-mirrored flow.
> +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])
> +
> +OVS_WAIT_UNTIL([test $(grep -Ec "filter is invalid|mirror mymirror configuration is invalid" ovs-vswitchd.log) -eq 4])

Why 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
> +
> +

One too many empty lines.

We need tests that check the match conditions in datapath flows.

>  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 4cbd9a5d3..7483b52ac 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..bdb2c8c0a 100644
> --- a/vswitchd/vswitch.ovsschema
> +++ b/vswitchd/vswitch.ovsschema
> @@ -1,6 +1,6 @@
>  {"name": "Open_vSwitch",
>   "version": "8.5.0",

Need to increase the version.

> - "cksum": "4040946650 27557",
> + "cksum": "2211155747 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 612ba41e3..ec536a08e 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -5256,6 +5256,19 @@ 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>
> +            Filter to apply to mirror. Flows that match <ref column="filter"/>
> +            will flow through this mirror, flows that do not match will be
> +            ignored. The <ref column="filter"/> syntax is described in
> +            <code>ovs-fields</code>(7).

We're mirroring packets or frames, not flows.
And... double spaces. :)

> +        </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">
diff mbox series

Patch

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 c9e4064e6..35f7eb0c7 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,9 @@  Post-v3.3.0
      * Conntrack now supports 'random' flag for selecting ports in a range
        while natting and 'persistent' flag for selection of the IP address
        from a range.
+   - OVSDB:
+     * Added a new filter column in the Mirror table which can be used to
+       apply filters to mirror ports.
 
 
 v3.3.0 - 16 Feb 2024
diff --git a/lib/flow.c b/lib/flow.c
index 8e3402388..a088bdc86 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -3569,7 +3569,7 @@  miniflow_equal_in_minimask(const struct miniflow *a, const struct miniflow *b,
     return true;
 }
 
-/* Returns true if 'a' and 'b' are equal at the places where there are 1-bits
+/* Returns true if 'a' and 'b' are equal at the places where there are 0-bits
  * in 'mask', false if they differ. */
 bool
 miniflow_equal_flow_in_minimask(const struct miniflow *a, const struct flow *b,
@@ -3587,6 +3587,25 @@  miniflow_equal_flow_in_minimask(const struct miniflow *a, const struct flow *b,
     return true;
 }
 
+/* Returns false if 'a' and 'b' differ in places where there are 1-bits in
+ * 'wc', true otherwise. */
+bool
+miniflow_equal_flow_in_flow_wc(const struct miniflow *a, const struct flow *b,
+                               const struct flow_wildcards *wc)
+{
+    const struct flow *wc_masks = &wc->masks;
+    size_t idx;
+
+    FLOWMAP_FOR_EACH_INDEX (idx, a->map) {
+        if ((miniflow_get(a, idx) ^ flow_u64_value(b, idx)) &
+                flow_u64_value(wc_masks, idx)) {
+            return false;
+        }
+    }
+
+    return true;
+}
+
 
 void
 minimask_init(struct minimask *mask, const struct flow_wildcards *wc)
diff --git a/lib/flow.h b/lib/flow.h
index 75a9be3c1..a644be39d 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -748,6 +748,9 @@  bool miniflow_equal_in_minimask(const struct miniflow *a,
 bool miniflow_equal_flow_in_minimask(const struct miniflow *a,
                                      const struct flow *b,
                                      const struct minimask *);
+bool miniflow_equal_flow_in_flow_wc(const struct miniflow *a,
+                                    const struct flow *b,
+                                    const struct flow_wildcards *);
 uint32_t miniflow_hash_5tuple(const struct miniflow *flow, uint32_t basis);
 
 
@@ -939,6 +942,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 a84c843b3..38e8d1fca 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"
 
@@ -50,6 +51,7 @@  struct mbundle {
 
 struct mirror {
     struct mbridge *mbridge;    /* Owning ofproto. */
+    uint64_t change_seq;        /* Change sequence of owning ofproto. */
     size_t idx;                 /* In ofproto's "mirrors" array. */
     void *aux;                  /* Key supplied by ofproto's client. */
 
@@ -57,6 +59,11 @@  struct mirror {
     struct hmapx srcs;          /* Contains "struct mbundle*"s. */
     struct hmapx dsts;          /* Contains "struct mbundle*"s. */
 
+    /* Filter criteria. */
+    OVSRCU_TYPE(struct miniflow *) filter;
+    OVSRCU_TYPE(struct minimask *) 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. */
@@ -207,8 +214,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)
 
 {
@@ -265,7 +272,11 @@  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)
+        /* If there is a filter, the datapath ports may have changed which
+         * could affect how the filter performs. Check change_seq if . */
+        && (!ms->filter || ofproto->change_seq == mirror->change_seq))
     {
         hmapx_destroy(&srcs_map);
         hmapx_destroy(&dsts_map);
@@ -289,6 +300,50 @@  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)) {
+        if (mirror->filter_str) {
+            ovsrcu_postpone(free, ovsrcu_get_protected(struct miniflow *,
+                                                       &mirror->filter));
+            ovsrcu_postpone(free, ovsrcu_get_protected(struct minimask *,
+                                                       &mirror->mask));
+            free(mirror->filter_str);
+            mirror->filter_str = NULL;
+            ovsrcu_set(&mirror->filter, NULL);
+            ovsrcu_set(&mirror->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;
+            }
+
+            /* Do not allow a filter on in_port. */
+            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->mask, minimask_create(&wc));
+            ovsrcu_set(&mirror->filter, miniflow_create(&flow));
+            mirror->change_seq = ofproto->change_seq;
+        }
+    }
+
     /* Update mbundles. */
     mirror_bit = MIRROR_MASK_C(1) << mirror->idx;
     CMAP_FOR_EACH (mbundle, cmap_node, &mirror->mbridge->mbundles) {
@@ -344,6 +399,17 @@  mirror_destroy(struct mbridge *mbridge, void *aux)
         ovsrcu_postpone(free, vlans);
     }
 
+    if (mirror->filter_str) {
+        ovsrcu_postpone(free, ovsrcu_get_protected(struct miniflow *,
+                                                   &mirror->filter));
+        ovsrcu_postpone(free, ovsrcu_get_protected(struct minimask *,
+                                                   &mirror->mask));
+        free(mirror->filter_str);
+        mirror->filter_str = NULL;
+        ovsrcu_set(&mirror->filter, NULL);
+        ovsrcu_set(&mirror->mask, NULL);
+    }
+
     mbridge->mirrors[mirror->idx] = NULL;
     /* mirror_get() might have just read the pointer, so we must postpone the
      * free. */
@@ -415,7 +481,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.
  */
@@ -441,6 +509,8 @@  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;
+    mc->filter_flow = ovsrcu_get(struct miniflow *, &mirror->filter);
+    mc->filter_mask = ovsrcu_get(struct minimask *, &mirror->mask);
     return true;
 }
 
diff --git a/ofproto/ofproto-dpif-mirror.h b/ofproto/ofproto-dpif-mirror.h
index a983b3876..7d40f7ec2 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
@@ -53,7 +58,6 @@  struct mirror_config {
     uint16_t snaplen;
 };
 
-
 /* The following functions are used by handler threads without any locking,
  * assuming RCU protection. */
 
@@ -78,8 +82,8 @@  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 *mbridge, void *aux,
-               const struct ofproto_mirror_settings *ms,
+int mirror_set(struct mbridge *mbridge, const struct ofproto *ofproto,
+               void *aux, const struct ofproto_mirror_settings *ms,
                const struct mirror_bundles *mb);
 void mirror_destroy(struct mbridge *, void *aux);
 int mirror_get_stats(struct mbridge *, void *aux, uint64_t *packets,
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index aa2a514f9..58d89b04e 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,29 @@  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) {
+            if (OVS_UNLIKELY(
+                miniflow_equal_flow_in_minimask(mc.filter_flow,
+                                                &ctx->xin->flow,
+                                                mc.filter_mask))) {
+                flow_wildcards_union_with_minimask(ctx->wc, mc.filter_mask);
+            } else if (!miniflow_equal_flow_in_minimask(&mc.filter_mask->masks,
+                                                        &ctx->wc->masks,
+                                                        mc.filter_mask)) {
+                /* If flow isn't in the mirror filter but flow's wildcard mask
+                 * doesn't overlap with the filter mask then add the filter
+                 * mask to the flow's wildcard mask. This prevents mirrored
+                 * traffic from matching on this flow. */
+                flow_wildcards_union_with_minimask(ctx->wc, mc.filter_mask);
+                mirrors = zero_rightmost_1bit(mirrors);
+                continue;
+            } else {
+                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 4f31f5ebb..85fe6c10f 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -222,13 +222,6 @@  static void ct_zone_config_uninit(struct dpif_backer *backer);
 static void ct_zone_timeout_policy_sweep(struct dpif_backer *backer);
 static void ct_zone_limits_commit(struct dpif_backer *backer);
 
-static inline struct ofproto_dpif *
-ofproto_dpif_cast(const struct ofproto *ofproto)
-{
-    ovs_assert(ofproto->ofproto_class == &ofproto_dpif_class);
-    return CONTAINER_OF(ofproto, struct ofproto_dpif, up);
-}
-
 /* Global variables. */
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
 
@@ -3669,7 +3662,7 @@  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);
     free(mb.srcs);
     free(mb.dsts);
     return error;
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index 92d33aa64..b95682595 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -363,6 +363,12 @@  struct ofproto_dpif {
 
 struct ofproto_dpif *ofproto_dpif_lookup_by_name(const char *name);
 struct ofproto_dpif *ofproto_dpif_lookup_by_uuid(const struct uuid *uuid);
+static inline struct ofproto_dpif *
+ofproto_dpif_cast(const struct ofproto *ofproto)
+{
+    ovs_assert(ofproto->ofproto_class == &ofproto_dpif_class);
+    return CONTAINER_OF(ofproto, struct ofproto_dpif, up);
+}
 
 ovs_version_t ofproto_dpif_get_tables_version(struct ofproto_dpif *);
 
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 122a06f30..06624006a 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -6389,7 +6389,7 @@  handle_flow_mod__(struct ofproto *ofproto, const struct ofputil_flow_mod *fm,
     error = ofproto_flow_mod_start(ofproto, &ofm);
     if (!error) {
         ofproto_bump_tables_version(ofproto);
-        error = ofproto_flow_mod_finish(ofproto, &ofm, req);        
+        error = ofproto_flow_mod_finish(ofproto, &ofm, req);
         ofmonitor_flush(ofproto->connmgr);
     }
     ovs_mutex_unlock(&ofproto_mutex);
@@ -8460,7 +8460,7 @@  do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags)
             /* Send error referring to the original message. */
             ofconn_send_error(ofconn, be->msg, error);
             error = OFPERR_OFPBFC_MSG_FAILED;
- 
+
             /* 2. Revert.  Undo all the changes made above. */
             LIST_FOR_EACH_REVERSE_CONTINUE(be, node, &bundle->msg_list) {
                 if (be->type == OFPTYPE_FLOW_MOD) {
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 daeea7775..6c5bf048a 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -5182,6 +5182,148 @@  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])
+
+AT_DATA([flows.txt], [dnl
+in_port=1 actions=output:2
+in_port=2 actions=output:1
+])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+
+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)"
+
+dnl Add mirrored flow after non-mirrored flow.
+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])
+
+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 4cbd9a5d3..7483b52ac 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..bdb2c8c0a 100644
--- a/vswitchd/vswitch.ovsschema
+++ b/vswitchd/vswitch.ovsschema
@@ -1,6 +1,6 @@ 
 {"name": "Open_vSwitch",
  "version": "8.5.0",
- "cksum": "4040946650 27557",
+ "cksum": "2211155747 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 612ba41e3..ec536a08e 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -5256,6 +5256,19 @@  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>
+            Filter to apply to mirror. Flows that match <ref column="filter"/>
+            will flow through this mirror, flows that do not match will be
+            ignored. The <ref column="filter"/> syntax is described in
+            <code>ovs-fields</code>(7).
+        </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">