[ovs-dev] [PATCH ovn] pinctrl: fix IP buffering with connection-tracking

[Numan Siddique]

On Wed, Feb 19, 2020 at 4:29 PM Dumitru Ceara <dceara at redhat.com> wrote:
>
> On 2/19/20 11:02 AM, Lorenzo Bianconi wrote:
> > On Feb 19, Dumitru Ceara wrote:
> >> On 2/13/20 5:49 PM, Lorenzo Bianconi wrote:
> >>> Whenever we need to reinject an IP packet buffered during L2 address
> >>> resolution we need to preserve ovs ofport in order to let ovs
> >>> connection tracking to properly SNAT/DNAT the packet.
> >>> Do not overwrite the MFF_IN_PORT in consider_port_binding routine
> >>>
> >>> Signed-off-by: Numan Siddique <numans at ovn.org>
> >>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
> >>
> >> Hi Lorenzo,
> >
> > Hi Dumitru,
> >
> > thanks for the review :)
> >
> >>
> >> The code looks good to me.
> >>
> >> I couldn't find any side effects due to not resetting MFF_IN_PORT when
> >> entering the router pipeline but I'm wondering if it's possible to add a
> >> test case for the issue you're fixing.
> >
> > I guess test-cases are already in system-ovn.at:
> >
> > - ovn -- DNAT and SNAT on distributed router - N/S
> > - ovn -- DNAT and SNAT on distributed router - N/S - IPv6
> > - ovn -- DNAT and SNAT on distributed router - E/W
> > - ovn -- DNAT and SNAT on distributed router - E/W - IPv6
> >
> > but they actually run a single device.
> >
> > Regards,
> > Lorenzo
>
> I see, thanks.
>
> Acked-by: Dumitru Ceara <dceara at redhat.com>

Thanks for the review Dumitru.

I applied this patch to master and branch-20.03

Thanks
Numan

>
> >
> >>
> >> This way, if we did miss a case when the now non-zero in_port is causing
> >> problems in the router pipeline we at least know that zeroing it out
> >> will trigger the new test to fail.
> >>
> >> Thanks,
> >> Dumitru
> >>
> >>> ---
> >>>  controller/physical.c | 1 -
> >>>  controller/pinctrl.c  | 5 ++++-
> >>>  2 files changed, 4 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/controller/physical.c b/controller/physical.c
> >>> index a7edaddac..144aeb7bd 100644
> >>> --- a/controller/physical.c
> >>> +++ b/controller/physical.c
> >>> @@ -905,7 +905,6 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
> >>>          for (int i = 0; i < MFF_N_LOG_REGS; i++) {
> >>>              put_load(0, MFF_LOG_REG0 + i, 0, 32, ofpacts_p);
> >>>          }
> >>> -        put_load(0, MFF_IN_PORT, 0, 16, ofpacts_p);
> >>>          put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p);
> >>>          clone = ofpbuf_at_assert(ofpacts_p, clone_ofs, sizeof *clone);
> >>>          ofpacts_p->header = clone;
> >>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> >>> index 09b0f7bf7..d06915a65 100644
> >>> --- a/controller/pinctrl.c
> >>> +++ b/controller/pinctrl.c
> >>> @@ -559,6 +559,7 @@ set_actions_and_enqueue_msg(struct rconn *swconn,
> >>>
> >>>  struct buffer_info {
> >>>      struct ofpbuf ofpacts;
> >>> +    ofp_port_t ofp_port;
> >>>      struct dp_packet *p;
> >>>  };
> >>>
> >>> @@ -629,6 +630,8 @@ buffered_push_packet(struct buffered_packets *bp,
> >>>      ofpbuf_init(&bi->ofpacts, 4096);
> >>>
> >>>      reload_metadata(&bi->ofpacts, md);
> >>> +    bi->ofp_port = md->flow.in_port.ofp_port;
> >>> +
> >>>      struct ofpact_resubmit *resubmit = ofpact_put_RESUBMIT(&bi->ofpacts);
> >>>      resubmit->in_port = OFPP_CONTROLLER;
> >>>      resubmit->table_id = OFTABLE_REMOTE_OUTPUT;
> >>> @@ -663,7 +666,7 @@ buffered_send_packets(struct rconn *swconn, struct buffered_packets *bp,
> >>>              .ofpacts = bi->ofpacts.data,
> >>>              .ofpacts_len = bi->ofpacts.size,
> >>>          };
> >>> -        match_set_in_port(&po.flow_metadata, OFPP_CONTROLLER);
> >>> +        match_set_in_port(&po.flow_metadata, bi->ofp_port);
> >>>          queue_msg(swconn, ofputil_encode_packet_out(&po, proto));
> >>>
> >>>          ofpbuf_uninit(&bi->ofpacts);
> >>>
> >>
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>