On 7/17/23 06:06, Mike Pattrick wrote: > On Fri, Jul 14, 2023 at 7:31 AM Ilya Maximets <i.maxim...@ovn.org> 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 <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. > > 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 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 as intended. > >> Again, not sure how bad it is to send duplicates to a mirror in >> this case. > > I don't think the mere possibility of sending duplicate packets to a > mirror is such an issue, as the behaviour is consistent. A certain > group of actions will cause the packet to mirror again, and those > actions are associated with packet modifications. The alternative > behaviour of only mirroring the packet when it actually changes could > result in unforeseen confusion. > > > Thanks, > M > >> >> Thoughts? >> >> Best regards, Ilya Maximets. >> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
