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 <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
>>>
>>
>>
>
>
> # 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=0x0000,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=0x0000,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=0xffff,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.
I’m not following this, but I think the goal of the original patch is to mirror
again if the packet gets modified. This happens in your case, because dec_ttl
modifies the packet, so it will be mirrored.
Or are you saying that the packet mirrored is not modified, and it’s still the
original packet? Adding Mike to this thread, as he did the original patch.
> If we look in the original commit purpose is to have a mirror in case rule
> modify the packet and return it back so
> i would expect the second mirror to be before adding encap.
> So in case of OVN like explained in the original commit
> if not tunnel. you can get ping and ping-reply.
> with tunnel. you can get ping and ping-reply-with-tunnel-header.
> but a real mirror on rx port should have been after decap so without tunnel
> header.
Your example is for encap, but for decap I would assume the original packet,
and the decap’ed packet get mirrored, depending on how the decap is
implemented. If it’s the kernel doing the decap you would not see it I guess.
But an ofproto trace might tell you more.
> also what happens now is that all packets with modify header being mirrored
> again anyway?
If the packet is actually modified I assume this is what the patch was trying
to accomplish, Mike?
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
