[ovs-dev] [PATCH ovn] Fix ovn-controller generated packets from getting dropped for reject ACL action.

Numan Siddique numans at ovn.org
Wed Jun 17 08:33:45 UTC 2020


On Tue, Jun 16, 2020 at 6:34 PM Mark Michelson <mmichels at redhat.com> wrote:

> Looks good to me.
>
> Acked-by: Mark Michelson <mmichels at redhat.com>
>

Thanks for the review. I applied this patch to master, branch-20.06 and
branch-20.03.

Thanks
Numan



>
> On 6/15/20 5:47 AM, numans at ovn.org wrote:
> > From: Numan Siddique <numans at ovn.org>
> >
> > TCP reset/ICMP packet generated by ovn-controller for the ACL reject
> action
> > gets dropped by ovs-vswithd with the below messages in ovs-vswitchd log
> > even though ovn-controller sets the in_port as OFPP_CONTROLLER.
> >
> > ----
> > ofproto_dpif_upcall(handler1)|INFO|received packet on unassociated
> datapath port 4294967295
> > ofproto_dpif_upcall(revalidator37)|WARN|Failed to acquire udpif_key
> corresponding to
> > unexpected flow (Invalid argument):
> ufid:0daac824-bda7-44d8-ad38-cdd9c5f0fc97
> > ----
> >
> > ovs-vswitchd drops the packet because the in_port is 0.
> >
> > The below OF flow sets the in_port to 0 if 'MLF_ALLOW_LOOPBACK_BIT' is
> set in the REG0
> > in table 64.
> >
> > priority=100,reg10=0x1/0x1,reg15=0x2,metadata=0x2
> actions=push:NXM_OF_IN_PORT[],load:0->NXM_OF_IN_PORT[],resubmit(,65),pop:NXM_OF_IN_PORT[]
> >
> > This patch fixes this issue by setting the in_port to OFPP_NONE so that
> ovs-vswitchd
> > doesn't drop the packet.
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1832176
> > Signed-off-by: Numan Siddique <numans at ovn.org>
> > ---
> >   controller/physical.c | 18 ++++++++++++------
> >   tests/system-ovn.at   | 32 ++++++++++++++++++++++++++++++++
> >   2 files changed, 44 insertions(+), 6 deletions(-)
> >
> > diff --git a/controller/physical.c b/controller/physical.c
> > index f06313b9d..d8e312426 100644
> > --- a/controller/physical.c
> > +++ b/controller/physical.c
> > @@ -765,12 +765,18 @@ put_local_common_flows(uint32_t dp_key, uint32_t
> port_key,
> >        *   - or if the destination is a nested container
> >        *   - or if "nested_container" flag is set and the destination is
> the
> >        *     parent port,
> > -     * temporarily set the in_port to zero, resubmit to
> > +     * temporarily set the in_port to OFPP_NONE, resubmit to
> >        * table 65 for logical-to-physical translation, then restore
> >        * the port number.
> >        *
> >        * If 'parent_port_key' is set, then the 'port_key' represents a
> nested
> > -     * container. */
> > +     * container.
> > +     *
> > +     * Note:We can set in_port to 0 too. But if recirculation happens
> > +     * later (eg. clone action to enter peer pipeline and a subsequent
> > +     * ct action), ovs-vswitchd will drop the packet if the frozen
> metadata
> > +     * in_port is 0.
> > +     * */
> >
> >       bool nested_container = parent_port_key ? true: false;
> >       match_init_catchall(&match);
> > @@ -783,7 +789,7 @@ put_local_common_flows(uint32_t dp_key, uint32_t
> port_key,
> >       }
> >
> >       put_stack(MFF_IN_PORT, ofpact_put_STACK_PUSH(ofpacts_p));
> > -    put_load(0, MFF_IN_PORT, 0, 16, ofpacts_p);
> > +    put_load(ofp_to_u16(OFPP_NONE), MFF_IN_PORT, 0, 16, ofpacts_p);
> >       put_resubmit(OFTABLE_LOG_TO_PHY, ofpacts_p);
> >       put_stack(MFF_IN_PORT, ofpact_put_STACK_POP(ofpacts_p));
> >       ofctrl_add_flow(flow_table, OFTABLE_SAVE_INPORT, 100, 0,
> > @@ -792,8 +798,8 @@ put_local_common_flows(uint32_t dp_key, uint32_t
> port_key,
> >       if (nested_container) {
> >           /* It's a nested container and when the packet from the nested
> >            * container is to be sent to the parent port,
> "nested_container"
> > -         * flag will be set. We need to temporarily set the in_port to
> zero
> > -         * as mentioned in the comment above.
> > +         * flag will be set. We need to temporarily set the in_port to
> > +         * OFPP_NONE as mentioned in the comment above.
> >            *
> >            * If a parent port has multiple child ports, then this if
> condition
> >            * will be hit multiple times, but we want to add only one
> flow.
> > @@ -814,7 +820,7 @@ put_local_common_flows(uint32_t dp_key, uint32_t
> port_key,
> >                                MLF_NESTED_CONTAINER,
> MLF_NESTED_CONTAINER);
> >
> >           put_stack(MFF_IN_PORT, ofpact_put_STACK_PUSH(ofpacts_p));
> > -        put_load(0, MFF_IN_PORT, 0, 16, ofpacts_p);
> > +        put_load(ofp_to_u16(OFPP_NONE), MFF_IN_PORT, 0, 16, ofpacts_p);
> >           put_resubmit(OFTABLE_LOG_TO_PHY, ofpacts_p);
> >           put_stack(MFF_IN_PORT, ofpact_put_STACK_POP(ofpacts_p));
> >           ofctrl_check_and_add_flow(flow_table, OFTABLE_SAVE_INPORT,
> 100, 0,
> > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > index 9dfe6a4ad..52f05f07e 100644
> > --- a/tests/system-ovn.at
> > +++ b/tests/system-ovn.at
> > @@ -3927,6 +3927,24 @@ ovn-nbctl acl-add pg0 to-lport 1002 "outport ==
> @pg0 && ip4 && ip4.src == 0.0.0.
> >   ovn-nbctl --log acl-add pg0 to-lport 1004 "inport == @pg0 && ip && tcp
> && tcp.dst == 84" reject
> >   ovn-nbctl --log acl-add pg0 to-lport 1004 "inport == @pg0 && ip && udp
> && udp.dst == 94" reject
> >
> > +ovn-nbctl ls-add sw1
> > +ovn-nbctl lsp-add sw1 sw1-p1-rej
> > +ovn-nbctl lsp-set-addresses sw1-p1-rej "40:54:00:00:00:03 20.0.0.3"
> > +ovn-nbctl lsp-set-port-security sw1-p1-rej "40:54:00:00:00:03 20.0.0.3"
> > +
> > +ovn-nbctl lr-add lr0
> > +ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24
> > +ovn-nbctl lsp-add sw0 sw0-lr0
> > +ovn-nbctl lsp-set-type sw0-lr0 router
> > +ovn-nbctl lsp-set-addresses sw0-lr0 router
> > +ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0
> > +
> > +ovn-nbctl lrp-add lr0 lr0-sw1 00:00:00:00:ff:02 20.0.0.1/24
> > +ovn-nbctl lsp-add sw1 sw1-lr0
> > +ovn-nbctl lsp-set-type sw1-lr0 router
> > +ovn-nbctl lsp-set-addresses sw1-lr0 router
> > +ovn-nbctl lsp-set-options sw1-lr0 router-port=lr0-sw1
> > +
> >   OVN_POPULATE_ARP
> >   ovn-nbctl --wait=hv sync
> >
> > @@ -3941,6 +3959,10 @@ ADD_VETH(sw0-p2-rej, sw0-p2-rej, br-int, "
> 10.0.0.4/24", "50:54:00:00:00:04", \
> >   NS_CHECK_EXEC([sw0-p1-rej], [ip a a aef0::3/64 dev sw0-p1-rej], [0])
> >   NS_CHECK_EXEC([sw0-p2-rej], [ip a a aef0::4/64 dev sw0-p2-rej], [0])
> >
> > +ADD_NAMESPACES(sw1-p1-rej)
> > +ADD_VETH(sw1-p1-rej, sw1-p1-rej, br-int, "20.0.0.3/24",
> "40:54:00:00:00:03", \
> > +         "20.0.0.1")
> > +
> >   sleep 1
> >
> >   # Capture packets in sw0-p1-rej.
> > @@ -3993,6 +4015,16 @@ OVS_WAIT_UNTIL([
> >       test "${total}" = "2"
> >   ])
> >
> > +ovn-nbctl acl-add sw1 from-lport 1004 "ip" allow-related
> > +ovn-nbctl acl-add sw1 to-lport 1004 "ip" allow-related
> > +ovn-nbctl --log acl-add pg0 to-lport 1004 "outport == @pg0 && ip && tcp
> && tcp.dst == 84" reject
> > +
> > +OVS_WAIT_UNTIL([
> > +    ip netns exec sw1-p1-rej nc  10.0.0.4 84 2> r
> > +    res=$(cat r)
> > +    test "$res" = "Ncat: Connection refused."
> > +])
> > +
> >   # Now test for IPv4 UDP.
> >   NS_CHECK_EXEC([sw0-p1-rej], [tcpdump -n -c 1 -i sw0-p1-rej udp port 90
> > sw0-p1-rej-udp.pcap &], [0])
> >   NS_CHECK_EXEC([sw0-p1-rej], [tcpdump -n -c 1 -i sw0-p1-rej icmp >
> sw0-p1-rej-icmp.pcap &], [0])
> >
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>


More information about the dev mailing list