Re: [ovs-dev] [PATCH v3 1/3] ofproto-dpif-upcall: Fix redundant mirror on geneve tunnel options.
On 16/10/2023 19:27, Mike Pattrick wrote:
> On Mon, Oct 16, 2023 at 4:08 AM Roi Dayan via dev
> wrote:
>>
>>
>> On 16/10/2023 11:00, Roi Dayan wrote:
>>>
>>> On 16/10/2023 10:42, Eelco Chaudron wrote:
On 16 Oct 2023, at 9:09, Roi Dayan wrote:
> On 09/10/2023 15:05, Roi Dayan wrote:
>> The cited commit fixed missing mirror packets by reset mirror when
>> packets are modified but setting geneve options was also treated as
>> a modified packet but should be treated as a part of set_tunnel
>> which doesn't reset mirror.
>>
>> Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are
>> modified.")
>> Signed-off-by: Roi Dayan
>> Acked-by: Simon Horman
>> Acked-by: Eelco Chaudron
>> ---
>>
>> Notes:
>> v2:
>> - user correct sha in fixes line.
>>
>> ofproto/ofproto-dpif-xlate.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index be4bd6657688..e243773307b7 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -7097,7 +7097,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const
>> struct flow *flow,
>>
>> set_field = ofpact_get_SET_FIELD(a);
>> mf = set_field->field;
>> -if (mf_are_prereqs_ok(mf, flow, NULL)) {
>> +if (mf_are_prereqs_ok(mf, flow, NULL) &&
>> !mf_is_tun_metadata(mf)) {
>> ctx->mirrors = 0;
>> }
>> return;
>
>
> Hi,
>
> I would like to consult another related issue to the original commit.
>
> feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
>
> Now with geneve options the redundant mirror is removed but if there will
> be
> a real modification there will still be a mirror output but in an
> incorrect place.
>
> For example adding dec_ttl, the action list will be like this:
>
> actions:enp8s0f0_1,set(tunnel(tun_id=0x2a,src=7.7.7.7,dst=7.7.7.8,ttl=64,tp_dst=6081,geneve({class=0x,type=0x80,len=4,0x1234}),flags(df|key))),set(ipv4(ttl=63)),genev_sys_6081,enp8s0f0_1
>
> A mirror output enp8s0f0_1 added at the end but the second mirror is with
> the tunnel header already.
>
> When not using tunnels the mirror is fine and the action list will look
> like this:
>
> actions:port1,dec_ttl,port2,port1
>
> So with tunnel the second mirror shouldn't have been somehow with the
> dec_ttl action but without the tunnel header?
>
> Should the actions list somehow be like this
>
> actions:enp8s0f0_1,set(ipv4(ttl=63),enp8s0f0_1,set(tunnel(...),genev_sys_6081
Not sure I follow you, but do you think the dect ttl and set tunnel should
have been swapped? I guess this would depend on your OpenFlow rule. Can
you show an ofproto trace on your example, which might help clarify OVS’s
reasoning?
//Eelco
>>>
>>> yes. but not just dec_ttl. any header modification beside encap.
>>> after all original commit explains users could reverse the packet with
>>> rules so
>>> there won't be a real packet coming so add mirror after modification.
>>> but mirror is usually before encap or on recv after decap.
>>>
>>> I expected action list to be, maybe: mirrorPort, header mod like replace
>>> src/dst and ping type, mirrorPort, encap, tunnelPort.
>>>
>>> i'll check about the ofproto trace
>>>
>
> Am I looking at this wrong? What do you think?
>
> Thanks,
> Roi
>>>
>>>
>>
>>
>> # ovs-ofctl dump-flows br-ovs
>> cookie=0x0, duration=317.989s, table=0, n_packets=8, n_bytes=376, arp
>> actions=NORMAL
>> cookie=0x0, duration=317.982s, table=0, n_packets=21, n_bytes=2058,
>> ip,in_port=geneve1
>> actions=set_field:0x1234->tun_metadata0,dec_ttl,output:"enp8s0f0_0"
>> cookie=0x0, duration=317.975s, table=0, n_packets=21, n_bytes=2058,
>> ip,in_port="enp8s0f0_0"
>> actions=set_field:0x1234->tun_metadata0,dec_ttl,output:geneve1
>> cookie=0x0, duration=318.117s, table=0, n_packets=11, n_bytes=846,
>> priority=0 actions=NORMAL
>>
>>
>>
>> # ovs-appctl ofproto/trace br-ovs
>> in_port=enp8s0f0_0,tcp,nw_ttl=64,nw_src=1.1.1.7,tcp_dst=22
>> Flow:
>> tcp,in_port=1,vlan_tci=0x,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,nw_src=1.1.1.7,nw_dst=0.0.0.0,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=22,tcp_flags=0
>>
>> bridge("br-ovs")
>>
>> 0. ip,in_port=1, priority 32768
>> set_field:0x1234->tun_metadata0
>> dec_ttl
>> output:3
>> -> output to kernel tunnel
>>
>> Final flow:
>> tcp,in_port=1,vlan_tci=0x,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,nw_src=1.1.1.7,nw_dst=0.0.0.0,nw_tos=0,nw_ecn=0,nw_ttl=63,nw_frag=no,tp_src=0,tp_dst=22,tcp_flags=0
>> Megaflow: recirc_id=0,eth,tcp,in_port=1,nw_ecn=
Re: [ovs-dev] [PATCH v3 1/3] ofproto-dpif-upcall: Fix redundant mirror on geneve tunnel options.
On Mon, Oct 16, 2023 at 4:08 AM Roi Dayan via dev
wrote:
>
>
> On 16/10/2023 11:00, Roi Dayan wrote:
> >
> > On 16/10/2023 10:42, Eelco Chaudron wrote:
> >>
> >>
> >> On 16 Oct 2023, at 9:09, Roi Dayan wrote:
> >>
> >>> On 09/10/2023 15:05, Roi Dayan wrote:
> The cited commit fixed missing mirror packets by reset mirror when
> packets are modified but setting geneve options was also treated as
> a modified packet but should be treated as a part of set_tunnel
> which doesn't reset mirror.
>
> Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are
> modified.")
> Signed-off-by: Roi Dayan
> Acked-by: Simon Horman
> Acked-by: Eelco Chaudron
> ---
>
> Notes:
> v2:
> - user correct sha in fixes line.
>
> ofproto/ofproto-dpif-xlate.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index be4bd6657688..e243773307b7 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -7097,7 +7097,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const
> struct flow *flow,
>
> set_field = ofpact_get_SET_FIELD(a);
> mf = set_field->field;
> -if (mf_are_prereqs_ok(mf, flow, NULL)) {
> +if (mf_are_prereqs_ok(mf, flow, NULL) &&
> !mf_is_tun_metadata(mf)) {
> ctx->mirrors = 0;
> }
> return;
> >>>
> >>>
> >>> Hi,
> >>>
> >>> I would like to consult another related issue to the original commit.
> >>>
> >>> feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
> >>>
> >>> Now with geneve options the redundant mirror is removed but if there will
> >>> be
> >>> a real modification there will still be a mirror output but in an
> >>> incorrect place.
> >>>
> >>> For example adding dec_ttl, the action list will be like this:
> >>>
> >>> actions:enp8s0f0_1,set(tunnel(tun_id=0x2a,src=7.7.7.7,dst=7.7.7.8,ttl=64,tp_dst=6081,geneve({class=0x,type=0x80,len=4,0x1234}),flags(df|key))),set(ipv4(ttl=63)),genev_sys_6081,enp8s0f0_1
> >>>
> >>> A mirror output enp8s0f0_1 added at the end but the second mirror is with
> >>> the tunnel header already.
> >>>
> >>> When not using tunnels the mirror is fine and the action list will look
> >>> like this:
> >>>
> >>> actions:port1,dec_ttl,port2,port1
> >>>
> >>> So with tunnel the second mirror shouldn't have been somehow with the
> >>> dec_ttl action but without the tunnel header?
> >>>
> >>> Should the actions list somehow be like this
> >>>
> >>> actions:enp8s0f0_1,set(ipv4(ttl=63),enp8s0f0_1,set(tunnel(...),genev_sys_6081
> >>
> >> Not sure I follow you, but do you think the dect ttl and set tunnel should
> >> have been swapped? I guess this would depend on your OpenFlow rule. Can
> >> you show an ofproto trace on your example, which might help clarify OVS’s
> >> reasoning?
> >>
> >> //Eelco
> >>
> >
> > yes. but not just dec_ttl. any header modification beside encap.
> > after all original commit explains users could reverse the packet with
> > rules so
> > there won't be a real packet coming so add mirror after modification.
> > but mirror is usually before encap or on recv after decap.
> >
> > I expected action list to be, maybe: mirrorPort, header mod like replace
> > src/dst and ping type, mirrorPort, encap, tunnelPort.
> >
> > i'll check about the ofproto trace
> >
> >>
> >>>
> >>> Am I looking at this wrong? What do you think?
> >>>
> >>> Thanks,
> >>> Roi
> >>
> >
> >
>
>
> # ovs-ofctl dump-flows br-ovs
> cookie=0x0, duration=317.989s, table=0, n_packets=8, n_bytes=376, arp
> actions=NORMAL
> cookie=0x0, duration=317.982s, table=0, n_packets=21, n_bytes=2058,
> ip,in_port=geneve1
> actions=set_field:0x1234->tun_metadata0,dec_ttl,output:"enp8s0f0_0"
> cookie=0x0, duration=317.975s, table=0, n_packets=21, n_bytes=2058,
> ip,in_port="enp8s0f0_0"
> actions=set_field:0x1234->tun_metadata0,dec_ttl,output:geneve1
> cookie=0x0, duration=318.117s, table=0, n_packets=11, n_bytes=846,
> priority=0 actions=NORMAL
>
>
>
> # ovs-appctl ofproto/trace br-ovs
> in_port=enp8s0f0_0,tcp,nw_ttl=64,nw_src=1.1.1.7,tcp_dst=22
> Flow:
> tcp,in_port=1,vlan_tci=0x,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,nw_src=1.1.1.7,nw_dst=0.0.0.0,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=22,tcp_flags=0
>
> bridge("br-ovs")
>
> 0. ip,in_port=1, priority 32768
> set_field:0x1234->tun_metadata0
> dec_ttl
> output:3
> -> output to kernel tunnel
>
> Final flow:
> tcp,in_port=1,vlan_tci=0x,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,nw_src=1.1.1.7,nw_dst=0.0.0.0,nw_tos=0,nw_ecn=0,nw_ttl=63,nw_frag=no,tp_src=0,tp_dst=22,tcp_flags=0
> Megaflow: recirc_id=0,eth,tcp,in_port=1,nw_ecn=0,nw_ttl=64,nw_frag=no
> Datapath actions:
> 3,set(tunnel(tun_id=0x2a,src=7.7.7.7
Re: [ovs-dev] [PATCH v3 1/3] ofproto-dpif-upcall: Fix redundant mirror on geneve tunnel options.
On 16/10/2023 14:02, Eelco Chaudron wrote:
>
>
> On 16 Oct 2023, at 12:52, Roi Dayan wrote:
>
>> On 16/10/2023 11:31, Eelco Chaudron wrote:
>>>
>>>
>>> On 16 Oct 2023, at 10:07, Roi Dayan wrote:
>>>
On 16/10/2023 11:00, Roi Dayan wrote:
>
> On 16/10/2023 10:42, Eelco Chaudron wrote:
>>
>>
>> On 16 Oct 2023, at 9:09, Roi Dayan wrote:
>>
>>> On 09/10/2023 15:05, Roi Dayan wrote:
The cited commit fixed missing mirror packets by reset mirror when
packets are modified but setting geneve options was also treated as
a modified packet but should be treated as a part of set_tunnel
which doesn't reset mirror.
Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are
modified.")
Signed-off-by: Roi Dayan
Acked-by: Simon Horman
Acked-by: Eelco Chaudron
---
Notes:
v2:
- user correct sha in fixes line.
ofproto/ofproto-dpif-xlate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ofproto/ofproto-dpif-xlate.c
b/ofproto/ofproto-dpif-xlate.c
index be4bd6657688..e243773307b7 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -7097,7 +7097,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const
struct flow *flow,
set_field = ofpact_get_SET_FIELD(a);
mf = set_field->field;
-if (mf_are_prereqs_ok(mf, flow, NULL)) {
+if (mf_are_prereqs_ok(mf, flow, NULL) &&
!mf_is_tun_metadata(mf)) {
ctx->mirrors = 0;
}
return;
>>>
>>>
>>> Hi,
>>>
>>> I would like to consult another related issue to the original commit.
>>>
>>> feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
>>>
>>> Now with geneve options the redundant mirror is removed but if there
>>> will be
>>> a real modification there will still be a mirror output but in an
>>> incorrect place.
>>>
>>> For example adding dec_ttl, the action list will be like this:
>>>
>>> actions:enp8s0f0_1,set(tunnel(tun_id=0x2a,src=7.7.7.7,dst=7.7.7.8,ttl=64,tp_dst=6081,geneve({class=0x,type=0x80,len=4,0x1234}),flags(df|key))),set(ipv4(ttl=63)),genev_sys_6081,enp8s0f0_1
>>>
>>> A mirror output enp8s0f0_1 added at the end but the second mirror is
>>> with the tunnel header already.
>>>
>>> When not using tunnels the mirror is fine and the action list will look
>>> like this:
>>>
>>> actions:port1,dec_ttl,port2,port1
>>>
>>> So with tunnel the second mirror shouldn't have been somehow with the
>>> dec_ttl action but without the tunnel header?
>>>
>>> Should the actions list somehow be like this
>>>
>>> actions:enp8s0f0_1,set(ipv4(ttl=63),enp8s0f0_1,set(tunnel(...),genev_sys_6081
>>
>> Not sure I follow you, but do you think the dect ttl and set tunnel
>> should have been swapped? I guess this would depend on your OpenFlow
>> rule. Can you show an ofproto trace on your example, which might help
>> clarify OVS’s reasoning?
>>
>> //Eelco
>>
>
> yes. but not just dec_ttl. any header modification beside encap.
> after all original commit explains users could reverse the packet with
> rules so
> there won't be a real packet coming so add mirror after modification.
> but mirror is usually before encap or on recv after decap.
>
> I expected action list to be, maybe: mirrorPort, header mod like replace
> src/dst and ping type, mirrorPort, encap, tunnelPort.
>
> i'll check about the ofproto trace
>
>>
>>>
>>> Am I looking at this wrong? What do you think?
>>>
>>> Thanks,
>>> Roi
>>
>
>
# ovs-ofctl dump-flows br-ovs
cookie=0x0, duration=317.989s, table=0, n_packets=8, n_bytes=376, arp
actions=NORMAL
cookie=0x0, duration=317.982s, table=0, n_packets=21, n_bytes=2058,
ip,in_port=geneve1
actions=set_field:0x1234->tun_metadata0,dec_ttl,output:"enp8s0f0_0"
cookie=0x0, duration=317.975s, table=0, n_packets=21, n_bytes=2058,
ip,in_port="enp8s0f0_0"
actions=set_field:0x1234->tun_metadata0,dec_ttl,output:geneve1
cookie=0x0, duration=318.117s, table=0, n_packets=11, n_bytes=846,
priority=0 actions=NORMAL
# ovs-appctl ofproto/trace br-ovs
in_port=enp8s0f0_0,tcp,nw_ttl=64,nw_src=1.1.1.7,tcp_dst=22
Flow:
tcp,in_port=1,vlan_tci=0x,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,nw_src=1.1.1.7,nw_dst=0.0.0.0,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=22,tcp_flags=0
bridge("br-ovs")
0. ip,in_port=1, priority 32768
>>>
Re: [ovs-dev] [PATCH v3 1/3] ofproto-dpif-upcall: Fix redundant mirror on geneve tunnel options.
On 16 Oct 2023, at 12:52, Roi Dayan wrote:
> On 16/10/2023 11:31, Eelco Chaudron wrote:
>>
>>
>> On 16 Oct 2023, at 10:07, Roi Dayan wrote:
>>
>>> On 16/10/2023 11:00, Roi Dayan wrote:
On 16/10/2023 10:42, Eelco Chaudron wrote:
>
>
> On 16 Oct 2023, at 9:09, Roi Dayan wrote:
>
>> On 09/10/2023 15:05, Roi Dayan wrote:
>>> The cited commit fixed missing mirror packets by reset mirror when
>>> packets are modified but setting geneve options was also treated as
>>> a modified packet but should be treated as a part of set_tunnel
>>> which doesn't reset mirror.
>>>
>>> Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are
>>> modified.")
>>> Signed-off-by: Roi Dayan
>>> Acked-by: Simon Horman
>>> Acked-by: Eelco Chaudron
>>> ---
>>>
>>> Notes:
>>> v2:
>>> - user correct sha in fixes line.
>>>
>>> ofproto/ofproto-dpif-xlate.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>>> index be4bd6657688..e243773307b7 100644
>>> --- a/ofproto/ofproto-dpif-xlate.c
>>> +++ b/ofproto/ofproto-dpif-xlate.c
>>> @@ -7097,7 +7097,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const
>>> struct flow *flow,
>>>
>>> set_field = ofpact_get_SET_FIELD(a);
>>> mf = set_field->field;
>>> -if (mf_are_prereqs_ok(mf, flow, NULL)) {
>>> +if (mf_are_prereqs_ok(mf, flow, NULL) &&
>>> !mf_is_tun_metadata(mf)) {
>>> ctx->mirrors = 0;
>>> }
>>> return;
>>
>>
>> Hi,
>>
>> I would like to consult another related issue to the original commit.
>>
>> feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
>>
>> Now with geneve options the redundant mirror is removed but if there
>> will be
>> a real modification there will still be a mirror output but in an
>> incorrect place.
>>
>> For example adding dec_ttl, the action list will be like this:
>>
>> actions:enp8s0f0_1,set(tunnel(tun_id=0x2a,src=7.7.7.7,dst=7.7.7.8,ttl=64,tp_dst=6081,geneve({class=0x,type=0x80,len=4,0x1234}),flags(df|key))),set(ipv4(ttl=63)),genev_sys_6081,enp8s0f0_1
>>
>> A mirror output enp8s0f0_1 added at the end but the second mirror is
>> with the tunnel header already.
>>
>> When not using tunnels the mirror is fine and the action list will look
>> like this:
>>
>> actions:port1,dec_ttl,port2,port1
>>
>> So with tunnel the second mirror shouldn't have been somehow with the
>> dec_ttl action but without the tunnel header?
>>
>> Should the actions list somehow be like this
>>
>> actions:enp8s0f0_1,set(ipv4(ttl=63),enp8s0f0_1,set(tunnel(...),genev_sys_6081
>
> Not sure I follow you, but do you think the dect ttl and set tunnel
> should have been swapped? I guess this would depend on your OpenFlow
> rule. Can you show an ofproto trace on your example, which might help
> clarify OVS’s reasoning?
>
> //Eelco
>
yes. but not just dec_ttl. any header modification beside encap.
after all original commit explains users could reverse the packet with
rules so
there won't be a real packet coming so add mirror after modification.
but mirror is usually before encap or on recv after decap.
I expected action list to be, maybe: mirrorPort, header mod like replace
src/dst and ping type, mirrorPort, encap, tunnelPort.
i'll check about the ofproto trace
>
>>
>> Am I looking at this wrong? What do you think?
>>
>> Thanks,
>> Roi
>
>>>
>>>
>>> # ovs-ofctl dump-flows br-ovs
>>> cookie=0x0, duration=317.989s, table=0, n_packets=8, n_bytes=376, arp
>>> actions=NORMAL
>>> cookie=0x0, duration=317.982s, table=0, n_packets=21, n_bytes=2058,
>>> ip,in_port=geneve1
>>> actions=set_field:0x1234->tun_metadata0,dec_ttl,output:"enp8s0f0_0"
>>> cookie=0x0, duration=317.975s, table=0, n_packets=21, n_bytes=2058,
>>> ip,in_port="enp8s0f0_0"
>>> actions=set_field:0x1234->tun_metadata0,dec_ttl,output:geneve1
>>> cookie=0x0, duration=318.117s, table=0, n_packets=11, n_bytes=846,
>>> priority=0 actions=NORMAL
>>>
>>>
>>>
>>> # ovs-appctl ofproto/trace br-ovs
>>> in_port=enp8s0f0_0,tcp,nw_ttl=64,nw_src=1.1.1.7,tcp_dst=22
>>> Flow:
>>> tcp,in_port=1,vlan_tci=0x,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,nw_src=1.1.1.7,nw_dst=0.0.0.0,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=22,tcp_flags=0
>>>
>>> bridge("br-ovs")
>>>
>>> 0. ip,in_port=1, priority 32768
>>> set_field:0x1234->tun_metadata0
>>> dec_ttl
>>> output:3
>>> -> output to kernel tunnel
>>>
>>> Final flow:
>>> tcp,in_port=1,vlan_tci=0x,dl_src=00:00:00:00:00:00,dl_dst=
Re: [ovs-dev] [PATCH v3 1/3] ofproto-dpif-upcall: Fix redundant mirror on geneve tunnel options.
On 16/10/2023 11:31, Eelco Chaudron wrote:
>
>
> On 16 Oct 2023, at 10:07, Roi Dayan wrote:
>
>> On 16/10/2023 11:00, Roi Dayan wrote:
>>>
>>> On 16/10/2023 10:42, Eelco Chaudron wrote:
On 16 Oct 2023, at 9:09, Roi Dayan wrote:
> On 09/10/2023 15:05, Roi Dayan wrote:
>> The cited commit fixed missing mirror packets by reset mirror when
>> packets are modified but setting geneve options was also treated as
>> a modified packet but should be treated as a part of set_tunnel
>> which doesn't reset mirror.
>>
>> Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are
>> modified.")
>> Signed-off-by: Roi Dayan
>> Acked-by: Simon Horman
>> Acked-by: Eelco Chaudron
>> ---
>>
>> Notes:
>> v2:
>> - user correct sha in fixes line.
>>
>> ofproto/ofproto-dpif-xlate.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index be4bd6657688..e243773307b7 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -7097,7 +7097,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const
>> struct flow *flow,
>>
>> set_field = ofpact_get_SET_FIELD(a);
>> mf = set_field->field;
>> -if (mf_are_prereqs_ok(mf, flow, NULL)) {
>> +if (mf_are_prereqs_ok(mf, flow, NULL) &&
>> !mf_is_tun_metadata(mf)) {
>> ctx->mirrors = 0;
>> }
>> return;
>
>
> Hi,
>
> I would like to consult another related issue to the original commit.
>
> feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
>
> Now with geneve options the redundant mirror is removed but if there will
> be
> a real modification there will still be a mirror output but in an
> incorrect place.
>
> For example adding dec_ttl, the action list will be like this:
>
> actions:enp8s0f0_1,set(tunnel(tun_id=0x2a,src=7.7.7.7,dst=7.7.7.8,ttl=64,tp_dst=6081,geneve({class=0x,type=0x80,len=4,0x1234}),flags(df|key))),set(ipv4(ttl=63)),genev_sys_6081,enp8s0f0_1
>
> A mirror output enp8s0f0_1 added at the end but the second mirror is with
> the tunnel header already.
>
> When not using tunnels the mirror is fine and the action list will look
> like this:
>
> actions:port1,dec_ttl,port2,port1
>
> So with tunnel the second mirror shouldn't have been somehow with the
> dec_ttl action but without the tunnel header?
>
> Should the actions list somehow be like this
>
> actions:enp8s0f0_1,set(ipv4(ttl=63),enp8s0f0_1,set(tunnel(...),genev_sys_6081
Not sure I follow you, but do you think the dect ttl and set tunnel should
have been swapped? I guess this would depend on your OpenFlow rule. Can
you show an ofproto trace on your example, which might help clarify OVS’s
reasoning?
//Eelco
>>>
>>> yes. but not just dec_ttl. any header modification beside encap.
>>> after all original commit explains users could reverse the packet with
>>> rules so
>>> there won't be a real packet coming so add mirror after modification.
>>> but mirror is usually before encap or on recv after decap.
>>>
>>> I expected action list to be, maybe: mirrorPort, header mod like replace
>>> src/dst and ping type, mirrorPort, encap, tunnelPort.
>>>
>>> i'll check about the ofproto trace
>>>
>
> Am I looking at this wrong? What do you think?
>
> Thanks,
> Roi
>>>
>>>
>>
>>
>> # ovs-ofctl dump-flows br-ovs
>> cookie=0x0, duration=317.989s, table=0, n_packets=8, n_bytes=376, arp
>> actions=NORMAL
>> cookie=0x0, duration=317.982s, table=0, n_packets=21, n_bytes=2058,
>> ip,in_port=geneve1
>> actions=set_field:0x1234->tun_metadata0,dec_ttl,output:"enp8s0f0_0"
>> cookie=0x0, duration=317.975s, table=0, n_packets=21, n_bytes=2058,
>> ip,in_port="enp8s0f0_0"
>> actions=set_field:0x1234->tun_metadata0,dec_ttl,output:geneve1
>> cookie=0x0, duration=318.117s, table=0, n_packets=11, n_bytes=846,
>> priority=0 actions=NORMAL
>>
>>
>>
>> # ovs-appctl ofproto/trace br-ovs
>> in_port=enp8s0f0_0,tcp,nw_ttl=64,nw_src=1.1.1.7,tcp_dst=22
>> Flow:
>> tcp,in_port=1,vlan_tci=0x,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,nw_src=1.1.1.7,nw_dst=0.0.0.0,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=22,tcp_flags=0
>>
>> bridge("br-ovs")
>>
>> 0. ip,in_port=1, priority 32768
>> set_field:0x1234->tun_metadata0
>> dec_ttl
>> output:3
>> -> output to kernel tunnel
>>
>> Final flow:
>> tcp,in_port=1,vlan_tci=0x,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,nw_src=1.1.1.7,nw_dst=0.0.0.0,nw_tos=0,nw_ecn=0,nw_ttl=63,nw_frag=no,tp_src=0,tp_dst=22,tcp_flags=0
>> Megaflow: recirc_id=0,eth,tcp,in_port=1,nw_ecn=0,nw_ttl=64,n
Re: [ovs-dev] [PATCH v3 1/3] ofproto-dpif-upcall: Fix redundant mirror on geneve tunnel options.
On 16 Oct 2023, at 10:07, Roi Dayan wrote:
> On 16/10/2023 11:00, Roi Dayan wrote:
>>
>> On 16/10/2023 10:42, Eelco Chaudron wrote:
>>>
>>>
>>> On 16 Oct 2023, at 9:09, Roi Dayan wrote:
>>>
On 09/10/2023 15:05, Roi Dayan wrote:
> The cited commit fixed missing mirror packets by reset mirror when
> packets are modified but setting geneve options was also treated as
> a modified packet but should be treated as a part of set_tunnel
> which doesn't reset mirror.
>
> Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are
> modified.")
> Signed-off-by: Roi Dayan
> Acked-by: Simon Horman
> Acked-by: Eelco Chaudron
> ---
>
> Notes:
> v2:
> - user correct sha in fixes line.
>
> ofproto/ofproto-dpif-xlate.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index be4bd6657688..e243773307b7 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -7097,7 +7097,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const
> struct flow *flow,
>
> set_field = ofpact_get_SET_FIELD(a);
> mf = set_field->field;
> -if (mf_are_prereqs_ok(mf, flow, NULL)) {
> +if (mf_are_prereqs_ok(mf, flow, NULL) &&
> !mf_is_tun_metadata(mf)) {
> ctx->mirrors = 0;
> }
> return;
Hi,
I would like to consult another related issue to the original commit.
feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
Now with geneve options the redundant mirror is removed but if there will
be
a real modification there will still be a mirror output but in an
incorrect place.
For example adding dec_ttl, the action list will be like this:
actions:enp8s0f0_1,set(tunnel(tun_id=0x2a,src=7.7.7.7,dst=7.7.7.8,ttl=64,tp_dst=6081,geneve({class=0x,type=0x80,len=4,0x1234}),flags(df|key))),set(ipv4(ttl=63)),genev_sys_6081,enp8s0f0_1
A mirror output enp8s0f0_1 added at the end but the second mirror is with
the tunnel header already.
When not using tunnels the mirror is fine and the action list will look
like this:
actions:port1,dec_ttl,port2,port1
So with tunnel the second mirror shouldn't have been somehow with the
dec_ttl action but without the tunnel header?
Should the actions list somehow be like this
actions:enp8s0f0_1,set(ipv4(ttl=63),enp8s0f0_1,set(tunnel(...),genev_sys_6081
>>>
>>> Not sure I follow you, but do you think the dect ttl and set tunnel should
>>> have been swapped? I guess this would depend on your OpenFlow rule. Can you
>>> show an ofproto trace on your example, which might help clarify OVS’s
>>> reasoning?
>>>
>>> //Eelco
>>>
>>
>> yes. but not just dec_ttl. any header modification beside encap.
>> after all original commit explains users could reverse the packet with rules
>> so
>> there won't be a real packet coming so add mirror after modification.
>> but mirror is usually before encap or on recv after decap.
>>
>> I expected action list to be, maybe: mirrorPort, header mod like replace
>> src/dst and ping type, mirrorPort, encap, tunnelPort.
>>
>> i'll check about the ofproto trace
>>
>>>
Am I looking at this wrong? What do you think?
Thanks,
Roi
>>>
>>
>>
>
>
> # ovs-ofctl dump-flows br-ovs
> cookie=0x0, duration=317.989s, table=0, n_packets=8, n_bytes=376, arp
> actions=NORMAL
> cookie=0x0, duration=317.982s, table=0, n_packets=21, n_bytes=2058,
> ip,in_port=geneve1
> actions=set_field:0x1234->tun_metadata0,dec_ttl,output:"enp8s0f0_0"
> cookie=0x0, duration=317.975s, table=0, n_packets=21, n_bytes=2058,
> ip,in_port="enp8s0f0_0"
> actions=set_field:0x1234->tun_metadata0,dec_ttl,output:geneve1
> cookie=0x0, duration=318.117s, table=0, n_packets=11, n_bytes=846,
> priority=0 actions=NORMAL
>
>
>
> # ovs-appctl ofproto/trace br-ovs
> in_port=enp8s0f0_0,tcp,nw_ttl=64,nw_src=1.1.1.7,tcp_dst=22
> Flow:
> tcp,in_port=1,vlan_tci=0x,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,nw_src=1.1.1.7,nw_dst=0.0.0.0,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=22,tcp_flags=0
>
> bridge("br-ovs")
>
> 0. ip,in_port=1, priority 32768
> set_field:0x1234->tun_metadata0
> dec_ttl
> output:3
> -> output to kernel tunnel
>
> Final flow:
> tcp,in_port=1,vlan_tci=0x,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,nw_src=1.1.1.7,nw_dst=0.0.0.0,nw_tos=0,nw_ecn=0,nw_ttl=63,nw_frag=no,tp_src=0,tp_dst=22,tcp_flags=0
> Megaflow: recirc_id=0,eth,tcp,in_port=1,nw_ecn=0,nw_ttl=64,nw_frag=no
> Datapath actions:
> 3,set(tunnel(tun_id=0x2a,src=7.7.7.7,dst=7.7.7.8,ttl=64,tp_dst=6081,geneve({class=0x,type=0x80,len=4,0x1234}),flags(df|key))),set(ipv4(ttl=63)),6,3
>
Re: [ovs-dev] [PATCH v3 1/3] ofproto-dpif-upcall: Fix redundant mirror on geneve tunnel options.
On 16/10/2023 11:00, Roi Dayan wrote:
>
> On 16/10/2023 10:42, Eelco Chaudron wrote:
>>
>>
>> On 16 Oct 2023, at 9:09, Roi Dayan wrote:
>>
>>> On 09/10/2023 15:05, Roi Dayan wrote:
The cited commit fixed missing mirror packets by reset mirror when
packets are modified but setting geneve options was also treated as
a modified packet but should be treated as a part of set_tunnel
which doesn't reset mirror.
Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are
modified.")
Signed-off-by: Roi Dayan
Acked-by: Simon Horman
Acked-by: Eelco Chaudron
---
Notes:
v2:
- user correct sha in fixes line.
ofproto/ofproto-dpif-xlate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index be4bd6657688..e243773307b7 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -7097,7 +7097,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const struct
flow *flow,
set_field = ofpact_get_SET_FIELD(a);
mf = set_field->field;
-if (mf_are_prereqs_ok(mf, flow, NULL)) {
+if (mf_are_prereqs_ok(mf, flow, NULL) && !mf_is_tun_metadata(mf))
{
ctx->mirrors = 0;
}
return;
>>>
>>>
>>> Hi,
>>>
>>> I would like to consult another related issue to the original commit.
>>>
>>> feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
>>>
>>> Now with geneve options the redundant mirror is removed but if there will be
>>> a real modification there will still be a mirror output but in an incorrect
>>> place.
>>>
>>> For example adding dec_ttl, the action list will be like this:
>>>
>>> actions:enp8s0f0_1,set(tunnel(tun_id=0x2a,src=7.7.7.7,dst=7.7.7.8,ttl=64,tp_dst=6081,geneve({class=0x,type=0x80,len=4,0x1234}),flags(df|key))),set(ipv4(ttl=63)),genev_sys_6081,enp8s0f0_1
>>>
>>> A mirror output enp8s0f0_1 added at the end but the second mirror is with
>>> the tunnel header already.
>>>
>>> When not using tunnels the mirror is fine and the action list will look
>>> like this:
>>>
>>> actions:port1,dec_ttl,port2,port1
>>>
>>> So with tunnel the second mirror shouldn't have been somehow with the
>>> dec_ttl action but without the tunnel header?
>>>
>>> Should the actions list somehow be like this
>>>
>>> actions:enp8s0f0_1,set(ipv4(ttl=63),enp8s0f0_1,set(tunnel(...),genev_sys_6081
>>
>> Not sure I follow you, but do you think the dect ttl and set tunnel should
>> have been swapped? I guess this would depend on your OpenFlow rule. Can you
>> show an ofproto trace on your example, which might help clarify OVS’s
>> reasoning?
>>
>> //Eelco
>>
>
> yes. but not just dec_ttl. any header modification beside encap.
> after all original commit explains users could reverse the packet with rules
> so
> there won't be a real packet coming so add mirror after modification.
> but mirror is usually before encap or on recv after decap.
>
> I expected action list to be, maybe: mirrorPort, header mod like replace
> src/dst and ping type, mirrorPort, encap, tunnelPort.
>
> i'll check about the ofproto trace
>
>>
>>>
>>> Am I looking at this wrong? What do you think?
>>>
>>> Thanks,
>>> Roi
>>
>
>
# ovs-ofctl dump-flows br-ovs
cookie=0x0, duration=317.989s, table=0, n_packets=8, n_bytes=376, arp
actions=NORMAL
cookie=0x0, duration=317.982s, table=0, n_packets=21, n_bytes=2058,
ip,in_port=geneve1
actions=set_field:0x1234->tun_metadata0,dec_ttl,output:"enp8s0f0_0"
cookie=0x0, duration=317.975s, table=0, n_packets=21, n_bytes=2058,
ip,in_port="enp8s0f0_0"
actions=set_field:0x1234->tun_metadata0,dec_ttl,output:geneve1
cookie=0x0, duration=318.117s, table=0, n_packets=11, n_bytes=846, priority=0
actions=NORMAL
# ovs-appctl ofproto/trace br-ovs
in_port=enp8s0f0_0,tcp,nw_ttl=64,nw_src=1.1.1.7,tcp_dst=22
Flow:
tcp,in_port=1,vlan_tci=0x,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,nw_src=1.1.1.7,nw_dst=0.0.0.0,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=22,tcp_flags=0
bridge("br-ovs")
0. ip,in_port=1, priority 32768
set_field:0x1234->tun_metadata0
dec_ttl
output:3
-> output to kernel tunnel
Final flow:
tcp,in_port=1,vlan_tci=0x,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,nw_src=1.1.1.7,nw_dst=0.0.0.0,nw_tos=0,nw_ecn=0,nw_ttl=63,nw_frag=no,tp_src=0,tp_dst=22,tcp_flags=0
Megaflow: recirc_id=0,eth,tcp,in_port=1,nw_ecn=0,nw_ttl=64,nw_frag=no
Datapath actions:
3,set(tunnel(tun_id=0x2a,src=7.7.7.7,dst=7.7.7.8,ttl=64,tp_dst=6081,geneve({class=0x,type=0x80,len=4,0x1234}),flags(df|key))),set(ipv4(ttl=63)),6,3
so mirror port 3 is mirroring the tx packet and then after tunnel and dec ttl
the final packet
but we get packet with the encap header.
If we look in the original commit purpose is to have a mirror in cas
Re: [ovs-dev] [PATCH v3 1/3] ofproto-dpif-upcall: Fix redundant mirror on geneve tunnel options.
On 16/10/2023 10:42, Eelco Chaudron wrote:
>
>
> On 16 Oct 2023, at 9:09, Roi Dayan wrote:
>
>> On 09/10/2023 15:05, Roi Dayan wrote:
>>> The cited commit fixed missing mirror packets by reset mirror when
>>> packets are modified but setting geneve options was also treated as
>>> a modified packet but should be treated as a part of set_tunnel
>>> which doesn't reset mirror.
>>>
>>> Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are
>>> modified.")
>>> Signed-off-by: Roi Dayan
>>> Acked-by: Simon Horman
>>> Acked-by: Eelco Chaudron
>>> ---
>>>
>>> Notes:
>>> v2:
>>> - user correct sha in fixes line.
>>>
>>> ofproto/ofproto-dpif-xlate.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>>> index be4bd6657688..e243773307b7 100644
>>> --- a/ofproto/ofproto-dpif-xlate.c
>>> +++ b/ofproto/ofproto-dpif-xlate.c
>>> @@ -7097,7 +7097,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const struct
>>> flow *flow,
>>>
>>> set_field = ofpact_get_SET_FIELD(a);
>>> mf = set_field->field;
>>> -if (mf_are_prereqs_ok(mf, flow, NULL)) {
>>> +if (mf_are_prereqs_ok(mf, flow, NULL) && !mf_is_tun_metadata(mf)) {
>>> ctx->mirrors = 0;
>>> }
>>> return;
>>
>>
>> Hi,
>>
>> I would like to consult another related issue to the original commit.
>>
>> feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
>>
>> Now with geneve options the redundant mirror is removed but if there will be
>> a real modification there will still be a mirror output but in an incorrect
>> place.
>>
>> For example adding dec_ttl, the action list will be like this:
>>
>> actions:enp8s0f0_1,set(tunnel(tun_id=0x2a,src=7.7.7.7,dst=7.7.7.8,ttl=64,tp_dst=6081,geneve({class=0x,type=0x80,len=4,0x1234}),flags(df|key))),set(ipv4(ttl=63)),genev_sys_6081,enp8s0f0_1
>>
>> A mirror output enp8s0f0_1 added at the end but the second mirror is with
>> the tunnel header already.
>>
>> When not using tunnels the mirror is fine and the action list will look like
>> this:
>>
>> actions:port1,dec_ttl,port2,port1
>>
>> So with tunnel the second mirror shouldn't have been somehow with the
>> dec_ttl action but without the tunnel header?
>>
>> Should the actions list somehow be like this
>>
>> actions:enp8s0f0_1,set(ipv4(ttl=63),enp8s0f0_1,set(tunnel(...),genev_sys_6081
>
> Not sure I follow you, but do you think the dect ttl and set tunnel should
> have been swapped? I guess this would depend on your OpenFlow rule. Can you
> show an ofproto trace on your example, which might help clarify OVS’s
> reasoning?
>
> //Eelco
>
yes. but not just dec_ttl. any header modification beside encap.
after all original commit explains users could reverse the packet with rules so
there won't be a real packet coming so add mirror after modification.
but mirror is usually before encap or on recv after decap.
I expected action list to be, maybe: mirrorPort, header mod like replace
src/dst and ping type, mirrorPort, encap, tunnelPort.
i'll check about the ofproto trace
>
>>
>> Am I looking at this wrong? What do you think?
>>
>> Thanks,
>> Roi
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 1/3] ofproto-dpif-upcall: Fix redundant mirror on geneve tunnel options.
On 16 Oct 2023, at 9:09, Roi Dayan wrote:
> On 09/10/2023 15:05, Roi Dayan wrote:
>> The cited commit fixed missing mirror packets by reset mirror when
>> packets are modified but setting geneve options was also treated as
>> a modified packet but should be treated as a part of set_tunnel
>> which doesn't reset mirror.
>>
>> Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are
>> modified.")
>> Signed-off-by: Roi Dayan
>> Acked-by: Simon Horman
>> Acked-by: Eelco Chaudron
>> ---
>>
>> Notes:
>> v2:
>> - user correct sha in fixes line.
>>
>> ofproto/ofproto-dpif-xlate.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index be4bd6657688..e243773307b7 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -7097,7 +7097,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const struct
>> flow *flow,
>>
>> set_field = ofpact_get_SET_FIELD(a);
>> mf = set_field->field;
>> -if (mf_are_prereqs_ok(mf, flow, NULL)) {
>> +if (mf_are_prereqs_ok(mf, flow, NULL) && !mf_is_tun_metadata(mf)) {
>> ctx->mirrors = 0;
>> }
>> return;
>
>
> Hi,
>
> I would like to consult another related issue to the original commit.
>
> feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
>
> Now with geneve options the redundant mirror is removed but if there will be
> a real modification there will still be a mirror output but in an incorrect
> place.
>
> For example adding dec_ttl, the action list will be like this:
>
> actions:enp8s0f0_1,set(tunnel(tun_id=0x2a,src=7.7.7.7,dst=7.7.7.8,ttl=64,tp_dst=6081,geneve({class=0x,type=0x80,len=4,0x1234}),flags(df|key))),set(ipv4(ttl=63)),genev_sys_6081,enp8s0f0_1
>
> A mirror output enp8s0f0_1 added at the end but the second mirror is with the
> tunnel header already.
>
> When not using tunnels the mirror is fine and the action list will look like
> this:
>
> actions:port1,dec_ttl,port2,port1
>
> So with tunnel the second mirror shouldn't have been somehow with the dec_ttl
> action but without the tunnel header?
>
> Should the actions list somehow be like this
>
> actions:enp8s0f0_1,set(ipv4(ttl=63),enp8s0f0_1,set(tunnel(...),genev_sys_6081
Not sure I follow you, but do you think the dect ttl and set tunnel should have
been swapped? I guess this would depend on your OpenFlow rule. Can you show an
ofproto trace on your example, which might help clarify OVS’s reasoning?
//Eelco
>
> Am I looking at this wrong? What do you think?
>
> Thanks,
> Roi
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 1/3] ofproto-dpif-upcall: Fix redundant mirror on geneve tunnel options.
On 09/10/2023 15:05, Roi Dayan wrote:
> The cited commit fixed missing mirror packets by reset mirror when
> packets are modified but setting geneve options was also treated as
> a modified packet but should be treated as a part of set_tunnel
> which doesn't reset mirror.
>
> Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
> Signed-off-by: Roi Dayan
> Acked-by: Simon Horman
> Acked-by: Eelco Chaudron
> ---
>
> Notes:
> v2:
> - user correct sha in fixes line.
>
> ofproto/ofproto-dpif-xlate.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index be4bd6657688..e243773307b7 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -7097,7 +7097,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const struct
> flow *flow,
>
> set_field = ofpact_get_SET_FIELD(a);
> mf = set_field->field;
> -if (mf_are_prereqs_ok(mf, flow, NULL)) {
> +if (mf_are_prereqs_ok(mf, flow, NULL) && !mf_is_tun_metadata(mf)) {
> ctx->mirrors = 0;
> }
> return;
Hi,
I would like to consult another related issue to the original commit.
feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
Now with geneve options the redundant mirror is removed but if there will be
a real modification there will still be a mirror output but in an incorrect
place.
For example adding dec_ttl, the action list will be like this:
actions:enp8s0f0_1,set(tunnel(tun_id=0x2a,src=7.7.7.7,dst=7.7.7.8,ttl=64,tp_dst=6081,geneve({class=0x,type=0x80,len=4,0x1234}),flags(df|key))),set(ipv4(ttl=63)),genev_sys_6081,enp8s0f0_1
A mirror output enp8s0f0_1 added at the end but the second mirror is with the
tunnel header already.
When not using tunnels the mirror is fine and the action list will look like
this:
actions:port1,dec_ttl,port2,port1
So with tunnel the second mirror shouldn't have been somehow with the dec_ttl
action but without the tunnel header?
Should the actions list somehow be like this
actions:enp8s0f0_1,set(ipv4(ttl=63),enp8s0f0_1,set(tunnel(...),genev_sys_6081
Am I looking at this wrong? What do you think?
Thanks,
Roi
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 1/3] ofproto-dpif-upcall: Fix redundant mirror on geneve tunnel options.
The cited commit fixed missing mirror packets by reset mirror when
packets are modified but setting geneve options was also treated as
a modified packet but should be treated as a part of set_tunnel
which doesn't reset mirror.
Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
Signed-off-by: Roi Dayan
Acked-by: Simon Horman
Acked-by: Eelco Chaudron
---
Notes:
v2:
- user correct sha in fixes line.
ofproto/ofproto-dpif-xlate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index be4bd6657688..e243773307b7 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -7097,7 +7097,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const struct flow
*flow,
set_field = ofpact_get_SET_FIELD(a);
mf = set_field->field;
-if (mf_are_prereqs_ok(mf, flow, NULL)) {
+if (mf_are_prereqs_ok(mf, flow, NULL) && !mf_is_tun_metadata(mf)) {
ctx->mirrors = 0;
}
return;
--
2.38.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
