| Message ID | 20200615094727.3133392-1-numans@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,ovn] Fix ovn-controller generated packets from getting dropped for reject ACL action. | expand |
Looks good to me. Acked-by: Mark Michelson <mmichels@redhat.com> On 6/15/20 5:47 AM, numans@ovn.org wrote: > From: Numan Siddique <numans@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@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]) >
On Tue, Jun 16, 2020 at 6:34 PM Mark Michelson <mmichels@redhat.com> wrote: > Looks good to me. > > Acked-by: Mark Michelson <mmichels@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@ovn.org wrote: > > From: Numan Siddique <numans@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@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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
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])