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
>
> 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
