[ovs-dev] [PATCH ovn v3] ovn-controller: Avoid infinite replying for TCP/ICMP connection reset messages

Mohammad Heib mheib at redhat.com
Tue Nov 30 09:42:47 UTC 2021


On 11/29/21 8:45 PM, Numan Siddique wrote:
> On Tue, Oct 12, 2021 at 8:18 AM <mheib at redhat.com> wrote:
>> From: Mohammad Heib <mheib at redhat.com>
>>
>> When the ovn controller receives an ip packet that targets a lport that has ACL
>> rule to reject ip packets, the controller will reply with TCP_RST or icmp4/6 unreachable packet
>> to notify the sender that the destination is not available.
>>
>> In turn, the receiver host will receive the notification packet and handle it as a normal IP packet
>> and if the receiver host is part of the same logical-switch/port-group or has IP reject ACL rule
>> it will send TCP_RST or icmp4/6 unreachable packet replying to the TCP_RST or icmp4/6 unreachable
>> packet we received and here we will enter to an infinity loop of replying about replying which
>> will consume high CPU.
>>
>> To avoid such scenarios this patch proposes to drop/ignore TCP_RST or icmp4/6 unreachable packets
>> that received on lport that has  IP reject ACL rules.
>>
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1934011
>> Fixes: 64f8c9e9f ("actions: Add a new OVN action - reject {}.")
>> Signed-off-by: Mohammad Heib <mheib at redhat.com>

Hi Numan,

thank you for your review :).


> Hi Mohammad,
>
> Sorry for the very late response to this patch.
>
> The patch LGTM.   I just have a few comments in the test.
>
>
>> ---
>>   controller/pinctrl.c |  29 +++++++++++
>>   tests/ovn-northd.at  | 111 +++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 140 insertions(+)
>>
>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
>> index cc3edaaf4..eff599d2b 100644
>> --- a/controller/pinctrl.c
>> +++ b/controller/pinctrl.c
>> @@ -1934,11 +1934,40 @@ pinctrl_handle_sctp_abort(struct rconn *swconn, const struct flow *ip_flow,
>>       dp_packet_uninit(&packet);
>>   }
>>
>> +static bool
>> +pinctrl_handle_reject_ignore_pkt(const struct flow *ip_flow,
>> +                                 struct dp_packet *pkt_in)
>> +{
>> +    if (ip_flow->nw_proto == IPPROTO_TCP) {
>> +        struct tcp_header *th = dp_packet_l4(pkt_in);
>> +        if (!th || (TCP_FLAGS(th->tcp_ctl) & TCP_RST)) {
>> +            return true;
>> +        }
>> +    } else {
>> +        if (is_icmpv4(ip_flow, NULL)) {
>> +            struct icmp_header *ih = dp_packet_l4(pkt_in);
>> +            if (!ih || (ih->icmp_type == ICMP4_DST_UNREACH)) {
>> +                return true;
>> +            }
>> +        } else if (is_icmpv6(ip_flow, NULL)) {
>> +            struct icmp6_data_header *ih = dp_packet_l4(pkt_in);
>> +            if (!ih || (ih->icmp6_base.icmp6_type == ICMP6_DST_UNREACH)) {
>> +                return true;
>> +            }
>> +        }
>> +    }
>> +    return false;
>> +}
>> +
>>   static void
>>   pinctrl_handle_reject(struct rconn *swconn, const struct flow *ip_flow,
>>                         struct dp_packet *pkt_in,
>>                         const struct match *md, struct ofpbuf *userdata)
>>   {
>> +    if (pinctrl_handle_reject_ignore_pkt(ip_flow, pkt_in)) {
>> +        return;
>> +    }
>> +
>>       if (ip_flow->nw_proto == IPPROTO_TCP) {
>>           pinctrl_handle_tcp_reset(swconn, ip_flow, pkt_in, md, userdata, true);
>>       } else if (ip_flow->nw_proto == IPPROTO_SCTP) {
>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>> index 8b9049899..4e4ed7b5f 100644
>> --- a/tests/ovn-northd.at
>> +++ b/tests/ovn-northd.at
>> @@ -2062,6 +2062,117 @@ sw1flows3:  table=4 (ls_out_acl         ), priority=2003 , match=((reg0[[9]] ==
>>   AT_CLEANUP
>>   ])
>>
> I think this test case should be added in ovn.at and not in
> ovn-northd.at.  Please move it to ovn.at
yes, i was actually confused about where to add the test case but
definitely ovn.at is a more appropriate plac.
>
>
>> +OVN_FOR_EACH_NORTHD([
>> +AT_SETUP([ACL Reject ping pong])
>> +AT_KEYWORDS([ACL Reject ping pong])
>> +ovn_start
>> +
>> +send_icmp6_packet() {
>> +    local inport=$1 hv=$2 eth_src=$3 eth_dst=$4 ipv6_src=$5 ipv6_dst=$6
>> +
>> +    local ip6_hdr=6000000000083aff${ipv6_src}${ipv6_dst}
>> +    local packet=${eth_dst}${eth_src}86dd${ip6_hdr}8000dcb662f00001
>> +
>> +    as hv$hv ovs-appctl netdev-dummy/receive hv${hv}-vif$inport $packet
>> +}
>> +
>> +send_icmp_packet() {
>> +    local inport=$1 hv=$2 eth_src=$3 eth_dst=$4 ipv4_src=$5 ipv4_dst=$6 ip_chksum=$7 data=$8
>> +    shift 8
>> +
>> +    local ip_ttl=ff
>> +    local ip_len=001c
>> +    local packet=${eth_dst}${eth_src}08004500${ip_len}00004000${ip_ttl}01${ip_chksum}${ipv4_src}${ipv4_dst}${data}
>> +    as hv$hv ovs-appctl netdev-dummy/receive hv${hv}-vif$inport $packet
>> +}
>> +
>> +net_add n1
>> +
>> +sim_add hv1
>> +as hv1
>> +ovs-vsctl add-br br-phys
>> +ovn_attach n1 br-phys 192.168.0.1
>> +ovs-vsctl -- add-port br-int hv1-vif1 -- \
>> +    set interface hv1-vif1 external-ids:iface-id=sw0-p1 \
>> +    options:tx_pcap=hv1/vif1-tx.pcap \
>> +    options:rxq_pcap=hv1/vif1-rx.pcap \
>> +    ofport-request=1
>> +
>> +sim_add hv2
>> +as hv2
>> +ovs-vsctl add-br br-phys
>> +ovn_attach n1 br-phys 192.168.0.2
>> +ovs-vsctl -- add-port br-int hv2-vif1 -- \
>> +    set interface hv2-vif1 external-ids:iface-id=sw0-p2 \
>> +    options:tx_pcap=hv2/vif1-tx.pcap \
>> +    options:rxq_pcap=hv2/vif1-rx.pcap \
>> +    ofport-request=1
>> +
>> +ovn-nbctl ls-add sw0
>> +
>> +check ovn-nbctl lsp-add sw0 sw0-p1
>> +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3 1000::3"
>> +check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3 1000::3"
>> +
>> +check ovn-nbctl lsp-add sw0 sw0-p2
>> +check ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4 1000::4"
>> +check ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4 1000::4"
>> +
>> +check ovn-nbctl acl-add sw0 to-lport 1002 ip reject
>> +
>> +OVN_POPULATE_ARP
>> +
>> +wait_for_ports_up
>> +ovn-nbctl --wait=hv sync
>> +
>> +ovn-sbctl dump-flows sw0 > sw0-flows
>> +AT_CAPTURE_FILE([sw0-flows])
>> +
>> +AT_CHECK([grep -E 'ls_(in|out)_acl' sw0-flows |grep reject| sed 's/table=../table=??/' | sort], [0], [dnl
>> +  table=??(ls_out_acl         ), priority=2002 , match=(ip), action=(reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=ingress,table=22); };)
>> +])
>> +
>> +
>> +eth_src=505400000003
>> +eth_dst=505400000004
>> +ip_src=$(ip_to_hex 10 0 0 3)
>> +ip_dst=$(ip_to_hex 10 0 0 4)
>> +ipv6_src=10000000000000000000000000000003
>> +ipv6_dst=10000000000000000000000000000004
>> +
>> +send_icmp_packet 1 1 $eth_src $eth_dst $ip_src $ip_dst c4c9 0000000000000000000000
>> +send_icmp6_packet 1 1 $eth_src $eth_dst $ipv6_src $ipv6_dst
>> +sleep 2
>> +
>> +# Get total number of ipv4 packets that received on ovs
>> +
>> +# sender side
>> +flow=$(as hv1 ovs-ofctl dump-flows br-int table=44 | grep priority=2002|grep ip,metadata=0x1)
>> +n_pkts="$(echo $flow|awk -F',' '{ print $4 }'|awk -F'=' '{ print $2 }')"
>> +check test $n_pkts -lt 30
> Can you please tell me why the pkt counter is checked for lt 30 ?
> Can't we check for a specific counter ?
actually, yes we can i really don't remember why I implemented it like 
that.

but you are right we must only get one reject pkt on each side.

changed in v4 
<https://patchwork.ozlabs.org/project/ovn/patch/20211130092314.131387-1-mheib@redhat.com/>

>
> Thanks
> Numan
>
Thanks,


>
>> +
>> +# receiver side
>> +flow=$(as hv2 ovs-ofctl dump-flows br-int table=44 | grep priority=2002|grep ip,metadata=0x1)
>> +n_pkts="$(echo $flow|awk -F',' '{ print $4 }'|awk -F'=' '{ print $2 }')"
>> +check test $n_pkts -lt 30
>> +
>> +# Get total number of ipv6 packets that received on ovs
>> +
>> +# sender side
>> +flow=$(as hv1 ovs-ofctl dump-flows br-int table=44 | grep priority=2002|grep ipv6,metadata=0x1)
>> +n_pkts="$(echo $flow|awk -F',' '{ print $4 }'|awk -F'=' '{ print $2 }')"
>> +check test $n_pkts -lt 30
>> +
>> +
>> +# receiver side
>> +flow=$(as hv2 ovs-ofctl dump-flows br-int table=44 | grep priority=2002|grep ipv6,metadata=0x1)
>> +n_pkts="$(echo $flow|awk -F',' '{ print $4 }'|awk -F'=' '{ print $2 }')"
>> +check test $n_pkts -lt 30
>> +
>> +OVN_CLEANUP([hv1], [hv2])
>> +AT_CLEANUP
>> +])
>> +
>>   OVN_FOR_EACH_NORTHD([
>>   AT_SETUP([ACL fair Meters])
>>   AT_KEYWORDS([acl log meter fair])
>> --
>> 2.27.0
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>


More information about the dev mailing list