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 <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?
>>>
>>
>> so yes. I modify the packet and the purpose of the original packet is to
>> mirror modified packets.
>> but the goal was to catch "recv" packet for packets being modified and put
>> back to the port
>> and the example was OVN modify icmp packets src/dst and type to reply.
>> so the mirror gets gets request+reply.
>> this works ok for ports without tunnel.
>> but if a port has a tunnel then the mirror of the reply is with encap header
>> because the 2nd
>> mirror is added at the end while I think it should have been without the
>> encap header.
>
> I guess this is because of your change, i.e. the encap is no longer being
> marked as a modification to the packet.
> This might be due to the fact the kernel uses a specific interface to do
> this, rather than an action.
It's not because of my change. from the original patch set_tunnel is set as not
to reset mirrors.
My change is the additon that modify of tunnel options is not a normal modify
but consider is as part
of set_tunnel and also not to reset mirrors.
So my change is "fixing" tunnel options. but every other modify packet still
here.
>
> Wondering if without your fix this would work fine for the userspace
> datapath, as this actually does modify the packet inline.
>
> I’ll let Mike take it from here ;)
>
Sure.
My suggestion for current, quick, "fix" (or workaround) is maybe avoid reset
mirrors if MF
is a set_tunnel flow then not reset mirrors, even if packet being modified.
This will keep the behavior of adding the mirror for OVN correctly when not
using tunnel and avoid
adding the mirror for MF with tunnels which looks incorrect but no one maybe
hit it yet.
>
> //Eelco
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
