On 16/10/2023 19:27, Mike Pattrick wrote:
> On Mon, Oct 16, 2023 at 4:08 AM Roi Dayan via dev
> <ovs-dev@openvswitch.org> 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.
>
> If I follow this correctly, that behaviour is the intended
> functionality. Each mirror port must receive one unique copy of every
> packet that ingresses or egresses a selected interface in OVS. I don't
> believe we want to mirror packets when they are in the middle of being
> modified but have not egressed yet.
>
>>
>> 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.
>
> Is this because part of the packet modification happens outside of OVS?
>
>
> Thanks,
> M
>
hi,
No, everything is in ovs.
I looked at it like if we have a bridge with port1 and a tunnel port2.
ovs recv/send packets on port1 which intended to go through tunnel port2.
So packets from port1 being added a tunnel header before going to tunnel port2
and reply packets from tunnel port2 , tunnel being removed before passing back
to port1.
So port1 recv/send are the normal packets and recv/send packets on tunnel port2
are wrapper
with tunnel headers.
But adding a second mirror after adding the tunnel on port1 makes it so we see
packets
on port1 with tunnel header. for one side. not the other.
So we get 2 mirror packets one before modify and one after modify but also with
tunnnel header.
So even in the case you mentioned you want to see OVN ping reply also and not
just ping request. you will get a normal ping request but the ping reply packet
will
be with a tunnel header instead of the "inner" packet of the ping reply only,
without the header.
like doing tcpdump on tunnel port or the inner port.
tcpdump on tunnel port see send/recv packets with tunnel header.
tcpdump on inner port seeing the inner packet. no tunnel headers.
>>
>> also what happens now is that all packets with modify header being mirrored
>> again anyway?
>>
>> _______________________________________________
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
