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 <m...@redhat.com>
>
> Thanks Mike for fixing my v3 comments. This patch looks good to me.
>
> Acked-by: Eelco Chaudron <echau...@redhat.com>
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
