| Message ID | 20230719162109.2481323-1-mkp@redhat.com |
|---|---|
| State | Accepted, archived |
| Commit | feed7f6775056b3dd55249596a7e587bc9c5fd4a |
| Headers | show |
| Series | [ovs-dev,v6] ofproto-dpif-upcall: Mirror packets that are modified | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 19 Jul 2023, at 18:21, 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 received. > > 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 receive 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 <mkp@redhat.com> > > --- > 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() > v5: Added prereq check for set-field > v6: Removed wc modification from pre-req check > > Signed-off-by: Mike Pattrick <mkp@redhat.com> Thanks for making the additional changes. This patch looks good to me. Acked-by: Eelco Chaudron <echaudro@redhat.com>
Mike Pattrick <mkp@redhat.com> writes: > 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 received. > > 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 receive 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 <mkp@redhat.com> > > --- Acked-by: Aaron Conole <aconole@redhat.com>
On 7/20/23 16:46, Aaron Conole wrote: > Mike Pattrick <mkp@redhat.com> writes: > >> 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 received. >> >> 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 receive 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 <mkp@redhat.com> >> >> --- > > Acked-by: Aaron Conole <aconole@redhat.com> Thanks, Mike, Eelco and Aaron! Applied and backported down to 2.17. Best regards, Ilya Maximets.
On Fri, Jul 21, 2023 at 12:16 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 7/20/23 16:46, Aaron Conole wrote: > > Mike Pattrick <mkp@redhat.com> writes: > > > >> 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 received. > >> > >> 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 receive 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 <mkp@redhat.com> > >> > >> --- > > > > Acked-by: Aaron Conole <aconole@redhat.com> > > Thanks, Mike, Eelco and Aaron! > > Applied and backported down to 2.17. Hello Ilya, Thank you for backporting this, is it possible to apply it back to 2.13 for OSP 16.1? > > Best regards, Ilya Maximets. >
On Fri, Jul 21, 2023 at 12:50 PM Mike Pattrick <mkp@redhat.com> wrote: > > On Fri, Jul 21, 2023 at 12:16 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > > > On 7/20/23 16:46, Aaron Conole wrote: > > > Mike Pattrick <mkp@redhat.com> writes: > > > > > >> 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 received. > > >> > > >> 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 receive 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 <mkp@redhat.com> > > >> > > >> --- > > > > > > Acked-by: Aaron Conole <aconole@redhat.com> > > > > Thanks, Mike, Eelco and Aaron! > > > > Applied and backported down to 2.17. > > Hello Ilya, > > Thank you for backporting this, is it possible to apply it back to > 2.13 for OSP 16.1? Sorry, disregard this email, I had misread the patch. Cheers, M > > > > > > Best regards, Ilya Maximets. > >
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 4928ea99c..47ea0f47e 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -7046,6 +7046,107 @@ 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, + const struct ofpact *a) +{ + switch (a->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_VLAN_VID: + case OFPACT_SET_VLAN_PCP: + case OFPACT_ENCAP: + case OFPACT_DECAP: + case OFPACT_NAT: + ctx->mirrors = 0; + return; + + case OFPACT_SET_FIELD: { + const struct ofpact_set_field *set_field; + const struct mf_field *mf; + + set_field = ofpact_get_SET_FIELD(a); + mf = set_field->field; + if (mf_are_prereqs_ok(mf, flow, NULL)) { + 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 xlate_trace(struct xlate_ctx *ctx, const struct ofpact *a) { @@ -7112,6 +7213,8 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, break; } + reset_mirror_ctx(ctx, flow, a); + if (OVS_UNLIKELY(ctx->xin->trace)) { xlate_trace(ctx, a); } 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]) flow="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)" AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout]) AT_CHECK_UNQUOTED([tail -1 stdout], [0], - [Datapath actions: 3,push_vlan(vid=17,pcp=0),2 + [Datapath actions: 3,push_vlan(vid=17,pcp=0),2,3 ]) flow="in_port(2),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)" @@ -5388,7 +5388,7 @@ flow="in_port(2),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x080 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout]) actual=`tail -1 stdout | sed 's/Datapath actions: //'` -expected="push_vlan(vid=17,pcp=0),1,pop_vlan,push_vlan(vid=12,pcp=0),1,2,100" +expected="push_vlan(vid=12,pcp=0),100,2,1,pop_vlan,push_vlan(vid=17,pcp=0),1,pop_vlan,push_vlan(vid=12,pcp=0),100,2,1" AT_CHECK([ovs-dpctl normalize-actions "$flow" "$expected"], [0], [stdout]) mv stdout expout AT_CHECK([ovs-dpctl normalize-actions "$flow" "$actual"], [0], [expout]) @@ -5656,7 +5656,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) flow="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)" AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout]) AT_CHECK_UNQUOTED([tail -1 stdout], [0], - [Datapath actions: trunc(100),3,push_vlan(vid=17,pcp=0),2 + [Datapath actions: trunc(100),3,push_vlan(vid=17,pcp=0),2,trunc(100),3 ]) flow="in_port(2),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"