Re: [ovs-dev] [PATCH] ofproto-dpif-upcall: Mirror packets that are modified

[Simon Horman]

On Tue, Feb 21, 2023 at 10:11:27AM -0500, Mike Pattrick wrote:
> On Tue, Feb 21, 2023 at 5:35 AM Simon Horman  
> wrote:
> >
> > On Thu, Feb 16, 2023 at 03:05:08PM -0500, 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.
> >
> > nit: I think 'received' at the end of the paragraph is a bit confusing.
> >  Received by who? (Yes, I know who :)
> >
> > > 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 
> > > ---
> > >  ofproto/ofproto-dpif-xlate.c | 37 +++-
> > >  tests/ofproto-dpif.at|  6 +++---
> > >  2 files changed, 35 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > > index a9cf3cbee..34b4b4018 100644
> > > --- a/ofproto/ofproto-dpif-xlate.c
> > > +++ b/ofproto/ofproto-dpif-xlate.c
> > > @@ -7078,6 +7078,7 @@ do_xlate_actions(const struct ofpact *ofpacts, 
> > > size_t ofpacts_len,
> > >  break;
> > >
> > >  case OFPACT_SET_VLAN_VID:
> > > +ctx->mirrors = 0;
> >
> > The pattern of adding some housekeeping to many actions to support a
> > specific case reminds me of MPLS. My recollection is that although
> > I used such an approach when adding MPLS support it was subsequently
> > factored out into (what is now) recirc_for_mpls().
> >
> > As I recall the main concern was about maintainability. And I think that
> > concern applies here. F.e. if a new action that modifies packets
> > is added, will ctx->mirrors = 0 get forgotten?
> 
> That's a good point. I can wrap this in a switch statement, and will
> investigate if xlate_commit_actions is an appropriate location.

Thanks.

Just to clarify: my suggestion regarding xlate_commit_actions()
was not just that it might be a good location. But also
that you may be able to get it, or something it calls,
to tell you if any packet modifications were made.

> 
> 
> Thanks
> M
> 
> >
> > So I wonder if we can try to handle this a different way.
> > One idea I had - but have not thought through very thoroughly -
> > is to make use of the fact that xlate_commit_actions() is called
> > to append ODP actions corresponding to any packet modifications.
> >
> > Such an approach may also ensure that the logic only executes
> > when packets are really modified - though there may be duplication if
> > there is a modify-output-restore-output sequence.
> >
> > ...
> >
> > > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> > > index 222415ac0..b027e8c2c 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
> > >  ])
> >
> > Test results look nice :)
> >
> > ...
> >
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ofproto-dpif-upcall: Mirror packets that are modified

[Mike Pattrick]

On Tue, Feb 21, 2023 at 5:35 AM Simon Horman  wrote:
>
> On Thu, Feb 16, 2023 at 03:05:08PM -0500, 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.
>
> nit: I think 'received' at the end of the paragraph is a bit confusing.
>  Received by who? (Yes, I know who :)
>
> > 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 
> > ---
> >  ofproto/ofproto-dpif-xlate.c | 37 +++-
> >  tests/ofproto-dpif.at|  6 +++---
> >  2 files changed, 35 insertions(+), 8 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index a9cf3cbee..34b4b4018 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -7078,6 +7078,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t 
> > ofpacts_len,
> >  break;
> >
> >  case OFPACT_SET_VLAN_VID:
> > +ctx->mirrors = 0;
>
> The pattern of adding some housekeeping to many actions to support a
> specific case reminds me of MPLS. My recollection is that although
> I used such an approach when adding MPLS support it was subsequently
> factored out into (what is now) recirc_for_mpls().
>
> As I recall the main concern was about maintainability. And I think that
> concern applies here. F.e. if a new action that modifies packets
> is added, will ctx->mirrors = 0 get forgotten?

That's a good point. I can wrap this in a switch statement, and will
investigate if xlate_commit_actions is an appropriate location.


Thanks
M

>
> So I wonder if we can try to handle this a different way.
> One idea I had - but have not thought through very thoroughly -
> is to make use of the fact that xlate_commit_actions() is called
> to append ODP actions corresponding to any packet modifications.
>
> Such an approach may also ensure that the logic only executes
> when packets are really modified - though there may be duplication if
> there is a modify-output-restore-output sequence.
>
> ...
>
> > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> > index 222415ac0..b027e8c2c 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
> >  ])
>
> Test results look nice :)
>
> ...
>

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ofproto-dpif-upcall: Mirror packets that are modified

[Simon Horman]

On Thu, Feb 16, 2023 at 03:05:08PM -0500, 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.

nit: I think 'received' at the end of the paragraph is a bit confusing.
 Received by who? (Yes, I know who :)

> 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 
> ---
>  ofproto/ofproto-dpif-xlate.c | 37 +++-
>  tests/ofproto-dpif.at|  6 +++---
>  2 files changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index a9cf3cbee..34b4b4018 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -7078,6 +7078,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t 
> ofpacts_len,
>  break;
>  
>  case OFPACT_SET_VLAN_VID:
> +ctx->mirrors = 0;

The pattern of adding some housekeeping to many actions to support a
specific case reminds me of MPLS. My recollection is that although
I used such an approach when adding MPLS support it was subsequently
factored out into (what is now) recirc_for_mpls().

As I recall the main concern was about maintainability. And I think that
concern applies here. F.e. if a new action that modifies packets
is added, will ctx->mirrors = 0 get forgotten?

So I wonder if we can try to handle this a different way.
One idea I had - but have not thought through very thoroughly -
is to make use of the fact that xlate_commit_actions() is called
to append ODP actions corresponding to any packet modifications.

Such an approach may also ensure that the logic only executes
when packets are really modified - though there may be duplication if
there is a modify-output-restore-output sequence.

...

> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 222415ac0..b027e8c2c 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
>  ])

Test results look nice :)

...
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ofproto-dpif-upcall: Mirror packets that are modified

[Abhiram RN]

Hi Mike,

Thanks for the patch!.
I took your patch and tried running the OVN unit test for the 'both'
direction packets mirroring check using the ovsmirrortest.diff attached in
the BZ (2155579 )
It passed with your patch.

Thanks & Regards,
Abhiram R N

On Fri, Feb 17, 2023 at 1:37 AM 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 
> ---
>  ofproto/ofproto-dpif-xlate.c | 37 +++-
>  tests/ofproto-dpif.at|  6 +++---
>  2 files changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index a9cf3cbee..34b4b4018 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -7078,6 +7078,7 @@ do_xlate_actions(const struct ofpact *ofpacts,
> size_t ofpacts_len,
>  break;
>
>  case OFPACT_SET_VLAN_VID:
> +ctx->mirrors = 0;
>  wc->masks.vlans[0].tci |= htons(VLAN_VID_MASK | VLAN_CFI);
>  if (flow->vlans[0].tci & htons(VLAN_CFI) ||
>  ofpact_get_SET_VLAN_VID(a)->push_vlan_if_needed) {
> @@ -7092,6 +7093,7 @@ do_xlate_actions(const struct ofpact *ofpacts,
> size_t ofpacts_len,
>  break;
>
>  case OFPACT_SET_VLAN_PCP:
> +ctx->mirrors = 0;
>  wc->masks.vlans[0].tci |= htons(VLAN_PCP_MASK | VLAN_CFI);
>  if (flow->vlans[0].tci & htons(VLAN_CFI) ||
>  ofpact_get_SET_VLAN_PCP(a)->push_vlan_if_needed) {
> @@ -7106,27 +7108,32 @@ do_xlate_actions(const struct ofpact *ofpacts,
> size_t ofpacts_len,
>  break;
>
>  case OFPACT_STRIP_VLAN:
> +ctx->mirrors = 0;
>  flow_pop_vlan(flow, wc);
>  break;
>
>  case OFPACT_PUSH_VLAN:
> +ctx->mirrors = 0;
>  flow_push_vlan_uninit(flow, wc);
>  flow->vlans[0].tpid = ofpact_get_PUSH_VLAN(a)->ethertype;
>  flow->vlans[0].tci = htons(VLAN_CFI);
>  break;
>
>  case OFPACT_SET_ETH_SRC:
> +ctx->mirrors = 0;
>  WC_MASK_FIELD(wc, dl_src);
>  flow->dl_src = ofpact_get_SET_ETH_SRC(a)->mac;
>  break;
>
>  case OFPACT_SET_ETH_DST:
> +ctx->mirrors = 0;
>  WC_MASK_FIELD(wc, dl_dst);
>  flow->dl_dst = ofpact_get_SET_ETH_DST(a)->mac;
>  break;
>
>  case OFPACT_SET_IPV4_SRC:
>  if (flow->dl_type == htons(ETH_TYPE_IP)) {
> +ctx->mirrors = 0;
>  memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src);
>  flow->nw_src = ofpact_get_SET_IPV4_SRC(a)->ipv4;
>  }
> @@ -7134,6 +7141,7 @@ do_xlate_actions(const struct ofpact *ofpacts,
> size_t ofpacts_len,
>
>  case OFPACT_SET_IPV4_DST:
>  if (flow->dl_type == htons(ETH_TYPE_IP)) {
> +ctx->mirrors = 0;
>  memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
>  flow->nw_dst = ofpact_get_SET_IPV4_DST(a)->ipv4;
>  }
> @@ -7141,6 +7149,7 @@ do_xlate_actions(const struct ofpact *ofpacts,
> size_t ofpacts_len,
>
>  case OFPACT_SET_IP_DSCP:
>  if (is_ip_any(flow)) {
> +ctx->mirrors = 0;
>  wc->masks.nw_tos |= IP_DSCP_MASK;
>  flow->nw_tos &= ~IP_DSCP_MASK;
>  flow->nw_tos |= ofpact_get_SET_IP_DSCP(a)->dscp;
> @@ -7149,6 +7158,7 @@ do_xlate_actions(const struct ofpact *ofpacts,
> size_t ofpacts_len,
>
>  case OFPACT_SET_IP_ECN:
>  if (is_ip_any(flow)) {
> +ctx->mirrors = 0;
>  wc->masks.nw_tos |= IP_ECN_MASK;
>  flow->nw_tos &= ~IP_ECN_MASK;
>  flow->nw_tos |= ofpact_get_SET_IP_ECN(a)->ecn;
> @@ -7157,6 +7167,7 @@ do_xlate_actions(const struct ofpact *ofpacts,
> size_t ofpacts_len,
>
>  case OFPACT_SET_IP_TTL:
>  if (is_ip_any(flow)) {
> +ctx->mirrors = 0;
>  wc->masks.nw_ttl = 0xf

[ovs-dev] [PATCH] ofproto-dpif-upcall: Mirror packets that are modified

[Mike Pattrick]

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 
---
 ofproto/ofproto-dpif-xlate.c | 37 +++-
 tests/ofproto-dpif.at|  6 +++---
 2 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index a9cf3cbee..34b4b4018 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -7078,6 +7078,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t 
ofpacts_len,
 break;
 
 case OFPACT_SET_VLAN_VID:
+ctx->mirrors = 0;
 wc->masks.vlans[0].tci |= htons(VLAN_VID_MASK | VLAN_CFI);
 if (flow->vlans[0].tci & htons(VLAN_CFI) ||
 ofpact_get_SET_VLAN_VID(a)->push_vlan_if_needed) {
@@ -7092,6 +7093,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t 
ofpacts_len,
 break;
 
 case OFPACT_SET_VLAN_PCP:
+ctx->mirrors = 0;
 wc->masks.vlans[0].tci |= htons(VLAN_PCP_MASK | VLAN_CFI);
 if (flow->vlans[0].tci & htons(VLAN_CFI) ||
 ofpact_get_SET_VLAN_PCP(a)->push_vlan_if_needed) {
@@ -7106,27 +7108,32 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t 
ofpacts_len,
 break;
 
 case OFPACT_STRIP_VLAN:
+ctx->mirrors = 0;
 flow_pop_vlan(flow, wc);
 break;
 
 case OFPACT_PUSH_VLAN:
+ctx->mirrors = 0;
 flow_push_vlan_uninit(flow, wc);
 flow->vlans[0].tpid = ofpact_get_PUSH_VLAN(a)->ethertype;
 flow->vlans[0].tci = htons(VLAN_CFI);
 break;
 
 case OFPACT_SET_ETH_SRC:
+ctx->mirrors = 0;
 WC_MASK_FIELD(wc, dl_src);
 flow->dl_src = ofpact_get_SET_ETH_SRC(a)->mac;
 break;
 
 case OFPACT_SET_ETH_DST:
+ctx->mirrors = 0;
 WC_MASK_FIELD(wc, dl_dst);
 flow->dl_dst = ofpact_get_SET_ETH_DST(a)->mac;
 break;
 
 case OFPACT_SET_IPV4_SRC:
 if (flow->dl_type == htons(ETH_TYPE_IP)) {
+ctx->mirrors = 0;
 memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src);
 flow->nw_src = ofpact_get_SET_IPV4_SRC(a)->ipv4;
 }
@@ -7134,6 +7141,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t 
ofpacts_len,
 
 case OFPACT_SET_IPV4_DST:
 if (flow->dl_type == htons(ETH_TYPE_IP)) {
+ctx->mirrors = 0;
 memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
 flow->nw_dst = ofpact_get_SET_IPV4_DST(a)->ipv4;
 }
@@ -7141,6 +7149,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t 
ofpacts_len,
 
 case OFPACT_SET_IP_DSCP:
 if (is_ip_any(flow)) {
+ctx->mirrors = 0;
 wc->masks.nw_tos |= IP_DSCP_MASK;
 flow->nw_tos &= ~IP_DSCP_MASK;
 flow->nw_tos |= ofpact_get_SET_IP_DSCP(a)->dscp;
@@ -7149,6 +7158,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t 
ofpacts_len,
 
 case OFPACT_SET_IP_ECN:
 if (is_ip_any(flow)) {
+ctx->mirrors = 0;
 wc->masks.nw_tos |= IP_ECN_MASK;
 flow->nw_tos &= ~IP_ECN_MASK;
 flow->nw_tos |= ofpact_get_SET_IP_ECN(a)->ecn;
@@ -7157,6 +7167,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t 
ofpacts_len,
 
 case OFPACT_SET_IP_TTL:
 if (is_ip_any(flow)) {
+ctx->mirrors = 0;
 wc->masks.nw_ttl = 0xff;
 flow->nw_ttl = ofpact_get_SET_IP_TTL(a)->ttl;
 }
@@ -7164,6 +7175,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t 
ofpacts_len,
 
 case OFPACT_SET_L4_SRC_PORT:
 if (is_ip_any(flow) && !(flow->nw_frag & FLOW_NW_FRAG_LATER)) {
+ctx->mirrors = 0;
 memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
 memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src);
 flow->tp_src = htons(ofpact_get_SET_L4_SRC_PORT(a)->port);
@@