Re: [ovs-dev] [PATCH v4] ofproto-dpif-upcall: Mirror packets that are modified
On 7/17/23 06:06, Mike Pattrick wrote:
> On Fri, Jul 14, 2023 at 7:31 AM Ilya Maximets wrote:
>>
>> On 7/11/23 14:46, Eelco Chaudron wrote:
>>>
>>>
>>> On 10 Jul 2023, at 17:34, 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
>>>
>>> Thanks Mike for fixing my v3 comments. This patch looks good to me.
>>>
>>> Acked-by: Eelco Chaudron
>>
>> Hi, Mike and Eelco. The patch seems fine in general, but there are
>> some inconsistencies, see below.
>>
>>>
---
v2: Refactored all code into a single function
v3: Cleaned up a code change that was incorrectly retained in v2 but
not needed
v4: Removed the default case from reset_mirror_ctx()
---
ofproto/ofproto-dpif-xlate.c | 86
tests/ofproto-dpif.at| 6 +--
2 files changed, 89 insertions(+), 3 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 29f4daa63..aba0cdb6e 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -6989,6 +6989,90 @@ 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)
+{
+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:
>>
>> We clear the mask unconditionally here. For other similar actions
>> below we check prerequisites, but not for this one. Seems a bit
>> inconsistent.
>
> We could add the set field prereq check here.
OK. Could you, please, do that and send a new version?
Explanation below seems fine to me, so no need to change anything there.
Best regards, Ilya Maximets.
>
>> Also, existence of the OF action doesn't actually mean that packet
>> will be changed. Either due to prerequisite checks or due to the
>> value being changed to the same value. The actual packet changes
>> can only be detected after commit_odp_actions().
>>
>> Maybe that's OK to mirror duplicates in this case, I'm not sure.
>
> Since the mirror action only activates on output action, there are a
> limited number of scenarios where this could happen. Specifically when
> the user configures a mirror on multiple interfaces and has one of
> these redundant actions, or a mirror on a single interface but
> redundant modification actions result in sending duplicate packets out
> on it multiple times. It's debatable whether we should mirror a
> duplicate packet in that case, failure to could be interpreted as an
> error if the user who may be expecting to receive a copy of all
> ingress and egress packets even if packet modifications have no
> effect.
>
> For example, broken OF flows may be inadvertently sending multiple
> copies of a packet out an interface that should be modified. The user
> in this case would see those packets on the remote end, but may be
> confused as to why they only see one copy of the packet on the local
> end when using a tool like ovs-tcpdump.
>
>>
+case OFPACT_SET_VLAN_VID:
+case OFPACT_SET_VLAN_PCP:
+case OFPACT_ENCAP:
+case OFPACT_DECAP:
+case OFPACT_NAT:
>>
>> The NAT is tricky, because on it's own it doesn't change the packet.
>> NAT is applied during the CT action and CT action always forks the
>> pipeline. So, the packet will remain unchanged in the current and
>> it will be modified in the forked pipeline. But we're clearing the
>> list of mirrors
Re: [ovs-dev] [PATCH v4] ofproto-dpif-upcall: Mirror packets that are modified
On Fri, Jul 14, 2023 at 7:31 AM Ilya Maximets wrote:
>
> On 7/11/23 14:46, Eelco Chaudron wrote:
> >
> >
> > On 10 Jul 2023, at 17:34, 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
> >
> > Thanks Mike for fixing my v3 comments. This patch looks good to me.
> >
> > Acked-by: Eelco Chaudron
>
> Hi, Mike and Eelco. The patch seems fine in general, but there are
> some inconsistencies, see below.
>
> >
> >> ---
> >> v2: Refactored all code into a single function
> >> v3: Cleaned up a code change that was incorrectly retained in v2 but
> >> not needed
> >> v4: Removed the default case from reset_mirror_ctx()
> >> ---
> >> ofproto/ofproto-dpif-xlate.c | 86
> >> tests/ofproto-dpif.at| 6 +--
> >> 2 files changed, 89 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> >> index 29f4daa63..aba0cdb6e 100644
> >> --- a/ofproto/ofproto-dpif-xlate.c
> >> +++ b/ofproto/ofproto-dpif-xlate.c
> >> @@ -6989,6 +6989,90 @@ 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)
> >> +{
> >> +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:
>
> We clear the mask unconditionally here. For other similar actions
> below we check prerequisites, but not for this one. Seems a bit
> inconsistent.
We could add the set field prereq check here.
> Also, existence of the OF action doesn't actually mean that packet
> will be changed. Either due to prerequisite checks or due to the
> value being changed to the same value. The actual packet changes
> can only be detected after commit_odp_actions().
>
> Maybe that's OK to mirror duplicates in this case, I'm not sure.
Since the mirror action only activates on output action, there are a
limited number of scenarios where this could happen. Specifically when
the user configures a mirror on multiple interfaces and has one of
these redundant actions, or a mirror on a single interface but
redundant modification actions result in sending duplicate packets out
on it multiple times. It's debatable whether we should mirror a
duplicate packet in that case, failure to could be interpreted as an
error if the user who may be expecting to receive a copy of all
ingress and egress packets even if packet modifications have no
effect.
For example, broken OF flows may be inadvertently sending multiple
copies of a packet out an interface that should be modified. The user
in this case would see those packets on the remote end, but may be
confused as to why they only see one copy of the packet on the local
end when using a tool like ovs-tcpdump.
>
> >> +case OFPACT_SET_VLAN_VID:
> >> +case OFPACT_SET_VLAN_PCP:
> >> +case OFPACT_ENCAP:
> >> +case OFPACT_DECAP:
> >> +case OFPACT_NAT:
>
> The NAT is tricky, because on it's own it doesn't change the packet.
> NAT is applied during the CT action and CT action always forks the
> pipeline. So, the packet will remain unchanged in the current and
> it will be modified in the forked pipeline. But we're clearing the
> list of mirrors for both here.
From my understanding, the NAT action is evaluated inside the
recursion, but it is applied outside of the recursion and the table
action also takes effect outside of the recursion. I may be completely
wrong here, but if that's the case, then this acts
Re: [ovs-dev] [PATCH v4] ofproto-dpif-upcall: Mirror packets that are modified
On 7/11/23 14:46, Eelco Chaudron wrote:
>
>
> On 10 Jul 2023, at 17:34, 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
>
> Thanks Mike for fixing my v3 comments. This patch looks good to me.
>
> Acked-by: Eelco Chaudron
Hi, Mike and Eelco. The patch seems fine in general, but there are
some inconsistencies, see below.
>
>> ---
>> v2: Refactored all code into a single function
>> v3: Cleaned up a code change that was incorrectly retained in v2 but
>> not needed
>> v4: Removed the default case from reset_mirror_ctx()
>> ---
>> ofproto/ofproto-dpif-xlate.c | 86
>> tests/ofproto-dpif.at| 6 +--
>> 2 files changed, 89 insertions(+), 3 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index 29f4daa63..aba0cdb6e 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -6989,6 +6989,90 @@ 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)
>> +{
>> +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:
We clear the mask unconditionally here. For other similar actions
below we check prerequisites, but not for this one. Seems a bit
inconsistent.
Also, existence of the OF action doesn't actually mean that packet
will be changed. Either due to prerequisite checks or due to the
value being changed to the same value. The actual packet changes
can only be detected after commit_odp_actions().
Maybe that's OK to mirror duplicates in this case, I'm not sure.
>> +case OFPACT_SET_VLAN_VID:
>> +case OFPACT_SET_VLAN_PCP:
>> +case OFPACT_ENCAP:
>> +case OFPACT_DECAP:
>> +case OFPACT_NAT:
The NAT is tricky, because on it's own it doesn't change the packet.
NAT is applied during the CT action and CT action always forks the
pipeline. So, the packet will remain unchanged in the current and
it will be modified in the forked pipeline. But we're clearing the
list of mirrors for both here.
Again, not sure how bad it is to send duplicates to a mirror in
this case.
Thoughts?
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4] ofproto-dpif-upcall: Mirror packets that are modified
On 10 Jul 2023, at 17:34, 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
Thanks Mike for fixing my v3 comments. This patch looks good to me.
Acked-by: Eelco Chaudron
> ---
> v2: Refactored all code into a single function
> v3: Cleaned up a code change that was incorrectly retained in v2 but
> not needed
> v4: Removed the default case from reset_mirror_ctx()
> ---
> ofproto/ofproto-dpif-xlate.c | 86
> tests/ofproto-dpif.at| 6 +--
> 2 files changed, 89 insertions(+), 3 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 29f4daa63..aba0cdb6e 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -6989,6 +6989,90 @@ 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)
> +{
> +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;
> +return;
> +case OFPACT_SET_IPV4_SRC:
> +case OFPACT_SET_IPV4_DST:
> +if (flow->dl_type == htons(ETH_TYPE_IP)) {
> +ctx->mirrors = 0;
> +}
> +return;
> +case OFPACT_SET_IP_DSCP:
> +case OFPACT_SET_IP_ECN:
> +case OFPACT_SET_IP_TTL:
> +if (is_ip_any(flow)) {
> +ctx->mirrors = 0;
> +}
> +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;
> +}
> +return;
> +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 OFPACT_ENQUEUE:
> +case OFPACT_SET_QUEUE:
> +case OFPACT_POP_QUEUE:
> +case OFPACT_MULTIPATH:
> +case OFPACT_BUNDLE:
> +case OFPACT_EXIT:
> +case OFPACT_UNROLL_XLATE:
> +case OFPACT_FIN_TIMEOUT:
> +case OFPACT_CLEAR_ACTIONS:
> +case OFPACT_WRITE_ACTIONS:
> +case OFPACT_METER:
> +case OFPACT_SAMPLE:
> +case OFPACT_CLONE:
> +case OFPACT_DEBUG_RECIRC:
> +case OFPACT_DEBUG_SLOW:
> +case OFPACT_CT:
> +case OFPACT_CT_CLEAR:
> +case OFPACT_CHECK_PKT_LARGER:
> +case OFPACT_DELETE_FIELD:
> +case OFPACT_NOTE:
> +case OFPACT_CONJUNCTION:
> +return;
> +}
> +OVS_NOT_REACHED();
> +}
> +
> static void
> do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
> struct xlate_ctx *ctx, bool is_last_action,
> @@ -7030,6 +7114,8 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
> ofpacts_len,
> break;
> }
>
> +reset_mirror_ctx(ctx, flow, a->type);
> +
> if (OVS_UNLIKELY(ctx->xin->trace)) {
> struct ofputil_port_map map = OFPUTIL_PORT_MAP_INITIALIZER();
>
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 6824ce0bb..f242f77f3 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -5349,7 +5349,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>
>
