Re: [ovs-dev] [PATCH v3] ofproto-dpif-upcall: Mirror packets that are modified
On Thu, Jun 29, 2023 at 10:06 AM Eelco Chaudron wrote:
>
>
>
> On 26 Jun 2023, at 15:43, Mike Pattrick wrote:
>
> > Currently OVS keeps track of which mirrors that each packet has been
> > sent to for the purpose of deduplication. However, this doesn't consider
> > that openflow rules can make significant changes to packets after
> > ingress.
> >
> > For example, OVN can create OpenFlow rules that turn an echo request
> > into an echo response by flipping source/destination addresses and
> > setting the ICMP type to Reply. When a mirror is configured, only the
> > request gets mirrored even though a response is recieved.
> >
> > This can cause a false impression of the actual traffic on wire if
> > someone inspects the mirror and doesn't see an echo reply even though
> > one has been sent.
> >
> > This patch resets the mirrors every time a packet is modified, so
> > mirrors will recieve every copy of a packet that is sent for output.
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2155579
> > Signed-off-by: Mike Pattrick
> > ---
> > v2: Refactored all code into a single function
> > v3: Cleaned up a code change that was incorrectly retained in v2 but
> > not needed
> > ---
> > ofproto/ofproto-dpif-xlate.c | 87
> > tests/ofproto-dpif.at| 6 +--
> > 2 files changed, 90 insertions(+), 3 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index 29f4daa63..5c2c6c792 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -6989,6 +6989,91 @@ xlate_ofpact_unroll_xlate(struct xlate_ctx *ctx,
> > "cookie=%#"PRIx64, a->rule_table_id, a->rule_cookie);
> > }
> >
> > +/* Reset the mirror context if we modify the packet and would like to
> > mirror
> > + * the new copy. */
> > +static void
> > +reset_mirror_ctx(struct xlate_ctx *ctx, const struct flow *flow, uint8_t
> > type)
> > +{
>
> How about the change below? This will give a compile error if new actions get
> defined.
Seems reasonable to me!
I'll resubmit with that change.
>
> The rest looks good.
>
>
> //Eelco
>
>
> static void
> -reset_mirror_ctx(struct xlate_ctx *ctx, const struct flow *flow, uint8_t
> type)
> +reset_mirror_ctx(struct xlate_ctx *ctx, const struct flow *flow,
> + enum ofpact_type type)
> {
> switch (type) {
> case OFPACT_STRIP_VLAN:
> @@ -7014,26 +7015,26 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const struct
> flow *flow, uint8_t type)
> case OFPACT_DECAP:
> case OFPACT_NAT:
> ctx->mirrors = 0;
> -break;
> +return;
> case OFPACT_SET_IPV4_SRC:
> case OFPACT_SET_IPV4_DST:
> if (flow->dl_type == htons(ETH_TYPE_IP)) {
> ctx->mirrors = 0;
> }
> -break;
> +return;
> case OFPACT_SET_IP_DSCP:
> case OFPACT_SET_IP_ECN:
> case OFPACT_SET_IP_TTL:
> if (is_ip_any(flow)) {
> ctx->mirrors = 0;
> }
> -break;
> +return;
> case OFPACT_SET_L4_SRC_PORT:
> case OFPACT_SET_L4_DST_PORT:
> if (is_ip_any(flow) && !(flow->nw_frag & FLOW_NW_FRAG_LATER)) {
> ctx->mirrors = 0;
> }
> -break;
> +return;
> case OFPACT_OUTPUT_REG:
> case OFPACT_OUTPUT_TRUNC:
> case OFPACT_GROUP:
> @@ -7068,10 +7069,9 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const struct
> flow *flow, uint8_t type)
> case OFPACT_DELETE_FIELD:
> case OFPACT_NOTE:
> case OFPACT_CONJUNCTION:
> -break;
> -default:
> -OVS_NOT_REACHED();
> +return;
> }
> +OVS_NOT_REACHED();
> }
>
> > +switch (type) {
> > +case OFPACT_STRIP_VLAN:
> > +case OFPACT_PUSH_VLAN:
> > +case OFPACT_SET_ETH_SRC:
> > +case OFPACT_SET_ETH_DST:
> > +case OFPACT_PUSH_MPLS:
> > +case OFPACT_POP_MPLS:
> > +case OFPACT_SET_MPLS_LABEL:
> > +case OFPACT_SET_MPLS_TC:
> > +case OFPACT_SET_MPLS_TTL:
> > +case OFPACT_DEC_MPLS_TTL:
> > +case OFPACT_DEC_NSH_TTL:
> > +case OFPACT_DEC_TTL:
> > +case OFPACT_SET_FIELD:
> > +case OFPACT_SET_VLAN_VID:
> > +case OFPACT_SET_VLAN_PCP:
> > +case OFPACT_ENCAP:
> > +case OFPACT_DECAP:
> > +case OFPACT_NAT:
> > +ctx->mirrors = 0;
> > +break;
> > +case OFPACT_SET_IPV4_SRC:
> > +case OFPACT_SET_IPV4_DST:
> > +if (flow->dl_type == htons(ETH_TYPE_IP)) {
> > +ctx->mirrors = 0;
> > +}
> > +break;
> > +case OFPACT_SET_IP_DSCP:
> > +case OFPACT_SET_IP_ECN:
> > +case OFPACT_SET_IP_TTL:
> > +if (is_ip_any(flow)) {
> > +ctx->mirrors = 0;
> > +}
> > +break;
> > +case OFPACT_SET_L4_SRC_PORT:
> > +case OFPACT_SET_L4_DST_PORT:
> > +if (is_ip_any(flow) && !(flow->nw_frag & FLOW_NW_FRAG_LATER)) {
> > +ctx->mirrors = 0;
> > +}
> > +
Re: [ovs-dev] [PATCH v3] ofproto-dpif-upcall: Mirror packets that are modified
On 26 Jun 2023, at 15:43, Mike Pattrick wrote:
> Currently OVS keeps track of which mirrors that each packet has been
> sent to for the purpose of deduplication. However, this doesn't consider
> that openflow rules can make significant changes to packets after
> ingress.
>
> For example, OVN can create OpenFlow rules that turn an echo request
> into an echo response by flipping source/destination addresses and
> setting the ICMP type to Reply. When a mirror is configured, only the
> request gets mirrored even though a response is recieved.
>
> This can cause a false impression of the actual traffic on wire if
> someone inspects the mirror and doesn't see an echo reply even though
> one has been sent.
>
> This patch resets the mirrors every time a packet is modified, so
> mirrors will recieve every copy of a packet that is sent for output.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2155579
> Signed-off-by: Mike Pattrick
> ---
> v2: Refactored all code into a single function
> v3: Cleaned up a code change that was incorrectly retained in v2 but
> not needed
> ---
> ofproto/ofproto-dpif-xlate.c | 87
> tests/ofproto-dpif.at| 6 +--
> 2 files changed, 90 insertions(+), 3 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 29f4daa63..5c2c6c792 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -6989,6 +6989,91 @@ xlate_ofpact_unroll_xlate(struct xlate_ctx *ctx,
> "cookie=%#"PRIx64, a->rule_table_id, a->rule_cookie);
> }
>
> +/* Reset the mirror context if we modify the packet and would like to mirror
> + * the new copy. */
> +static void
> +reset_mirror_ctx(struct xlate_ctx *ctx, const struct flow *flow, uint8_t
> type)
> +{
How about the change below? This will give a compile error if new actions get
defined.
The rest looks good.
//Eelco
static void
-reset_mirror_ctx(struct xlate_ctx *ctx, const struct flow *flow, uint8_t type)
+reset_mirror_ctx(struct xlate_ctx *ctx, const struct flow *flow,
+ enum ofpact_type type)
{
switch (type) {
case OFPACT_STRIP_VLAN:
@@ -7014,26 +7015,26 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const struct
flow *flow, uint8_t type)
case OFPACT_DECAP:
case OFPACT_NAT:
ctx->mirrors = 0;
-break;
+return;
case OFPACT_SET_IPV4_SRC:
case OFPACT_SET_IPV4_DST:
if (flow->dl_type == htons(ETH_TYPE_IP)) {
ctx->mirrors = 0;
}
-break;
+return;
case OFPACT_SET_IP_DSCP:
case OFPACT_SET_IP_ECN:
case OFPACT_SET_IP_TTL:
if (is_ip_any(flow)) {
ctx->mirrors = 0;
}
-break;
+return;
case OFPACT_SET_L4_SRC_PORT:
case OFPACT_SET_L4_DST_PORT:
if (is_ip_any(flow) && !(flow->nw_frag & FLOW_NW_FRAG_LATER)) {
ctx->mirrors = 0;
}
-break;
+return;
case OFPACT_OUTPUT_REG:
case OFPACT_OUTPUT_TRUNC:
case OFPACT_GROUP:
@@ -7068,10 +7069,9 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const struct
flow *flow, uint8_t type)
case OFPACT_DELETE_FIELD:
case OFPACT_NOTE:
case OFPACT_CONJUNCTION:
-break;
-default:
-OVS_NOT_REACHED();
+return;
}
+OVS_NOT_REACHED();
}
> +switch (type) {
> +case OFPACT_STRIP_VLAN:
> +case OFPACT_PUSH_VLAN:
> +case OFPACT_SET_ETH_SRC:
> +case OFPACT_SET_ETH_DST:
> +case OFPACT_PUSH_MPLS:
> +case OFPACT_POP_MPLS:
> +case OFPACT_SET_MPLS_LABEL:
> +case OFPACT_SET_MPLS_TC:
> +case OFPACT_SET_MPLS_TTL:
> +case OFPACT_DEC_MPLS_TTL:
> +case OFPACT_DEC_NSH_TTL:
> +case OFPACT_DEC_TTL:
> +case OFPACT_SET_FIELD:
> +case OFPACT_SET_VLAN_VID:
> +case OFPACT_SET_VLAN_PCP:
> +case OFPACT_ENCAP:
> +case OFPACT_DECAP:
> +case OFPACT_NAT:
> +ctx->mirrors = 0;
> +break;
> +case OFPACT_SET_IPV4_SRC:
> +case OFPACT_SET_IPV4_DST:
> +if (flow->dl_type == htons(ETH_TYPE_IP)) {
> +ctx->mirrors = 0;
> +}
> +break;
> +case OFPACT_SET_IP_DSCP:
> +case OFPACT_SET_IP_ECN:
> +case OFPACT_SET_IP_TTL:
> +if (is_ip_any(flow)) {
> +ctx->mirrors = 0;
> +}
> +break;
> +case OFPACT_SET_L4_SRC_PORT:
> +case OFPACT_SET_L4_DST_PORT:
> +if (is_ip_any(flow) && !(flow->nw_frag & FLOW_NW_FRAG_LATER)) {
> +ctx->mirrors = 0;
> +}
> +break;
> +case OFPACT_OUTPUT_REG:
> +case OFPACT_OUTPUT_TRUNC:
> +case OFPACT_GROUP:
> +case OFPACT_OUTPUT:
> +case OFPACT_CONTROLLER:
> +case OFPACT_RESUBMIT:
> +case OFPACT_GOTO_TABLE:
> +case OFPACT_WRITE_METADATA:
> +case OFPACT_SET_TUNNEL:
> +case OFPACT_REG_MOVE:
> +case OFPACT_STACK_PUSH:
> +case OFPACT_STACK_POP:
> +case OFPACT_LEARN:
> +case
