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. > 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
