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 <r...@nvidia.com>
>>> Acked-by: Simon Horman <ho...@ovn.org>
>>> Acked-by: Eelco Chaudron <echau...@redhat.com>
>>> ---
>>>
>>> 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=0xffff,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
