> On Aug 18, 2016, at 8:46 AM, nickcooper-zhangtonghao
> <nickcooper-zhangtong...@opencloud.tech> wrote:
This patch series is a bit odd. I think there are three patches here, but
they're formatted like there are two. It looks like this should have been
patch 1 of 3. Normally, if patch 0 is supplied, it's a cover letter describing
the rest of the commits, and not a commit itself.
I think the title is a bit exaggerated and, regardless, it's not very
descriptive. How about "ovn: Don't learn from unrequested ARP replies." or
something similar?
> The the logical routers check only the "arp.op == 2" for ARP replies
There are two "the"s at the beginning of the sentence.
> and then use ARP replies to populate the logical router's ARP table.
> If we continue to send ARP replies, which have different "arp.spa" and
> "arp.sha", to logical router, the MAC_Binding table will continue to
> increase. That may reduce system performance and cause instability and
> crashes.
>
> So,
> 1. When logical router receive an ARP reply packet, we should check the
> packet's "arp.tpa" and "arp.spa".
> 2. The logical router uses a cache to store the ARP request. Only when
> logical routers send an ARP request packet, and get a corresponding
> ARP reply, will the 'ovn-controller' update MAC_Binding table of SB
> database.
According to Section 4.6 in RFC 5944, we should update the MAC address of
existing entries in the case of gratuitous ARP. Also, the new code only
updates the ARP cache for replies with outstanding ARP requests, but I think it
should also update the cache for any existing entry. This could be necessary
for gateway failover, for example.
Here are a few other things, I'd also look at:
> Signed-off-by: nickcooper-zhangtonghao
> <nickcooper-zhangtong...@opencloud.tech>
> ---
> ovn/controller/pinctrl.c | 152 ++++++++++++++++++++++++++++++++++++++++++++++-
> ovn/northd/ovn-northd.c | 22 ++++---
> 2 files changed, 166 insertions(+), 8 deletions(-)
>
> diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> index 8a759cd..bba7b29 100644
> --- a/ovn/controller/pinctrl.c
> +++ b/ovn/controller/pinctrl.c
> @@ -185,6 +192,17 @@ pinctrl_handle_arp(const struct flow *ip_flow, const
> struct match *md,
> enum ofputil_protocol proto = ofputil_protocol_from_ofp_version(version);
> queue_msg(ofputil_encode_packet_out(&po, proto));
>
> + /* The logical router uses a cache to store the ARP request.
> + * Only when logical routers send an ARP request packet, and get a
> + * corresponding ARP reply, will the 'ovn-controller' update
> + * mac_binding table of SB database. */
> + uint32_t dp_key = ntohll(md->flow.metadata);
> + uint32_t port_key = md->flow.regs[MFF_LOG_OUTPORT - MFF_REG0];
> + char ip_s[INET_ADDRSTRLEN];
Is there a reason this is INET_ADDRSTRLEN, but the similar member in
arp_request_cache is "INET6_ADDRSTRLEN + 1"?
> + inet_ntop(AF_INET, &ip_flow->nw_dst, ip_s, sizeof(ip_s));
As described in CodingStyle.md, you shouldn't use parentheses around the
"sizeof" argument.
> static void
> init_put_mac_bindings(void)
> @@ -862,6 +895,100 @@ pinctrl_find_put_mac_binding(uint32_t dp_key, uint32_t
> port_key,
> }
>
> static void
> +init_arp_request_caches(void)
> +{
> + hmap_init(&arp_request_caches);
> +}
> +
> +static void
> +destroy_arp_request_caches(void)
> +{
> + flush_arp_request_caches();
This is the only caller to flush_arp_request_caches(), and that function is
trivial, so I'd just pull it into here.
> +static bool
> +pinctrl_find_delete_arp_request_cache(uint32_t dp_key, uint32_t port_key,
> + const char *ip_s)
> +{
> + struct arp_request_cache *arc, *next;
> + HMAP_FOR_EACH_SAFE (arc, next, hmap_node, &arp_request_caches) {
> + if (arc->dp_key == dp_key
> + && arc->port_key == port_key
> + && !strcmp(arc->ip_s, ip_s)) {
> + hmap_remove(&arp_request_caches, &arc->hmap_node);
> + free(arc);
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +
> +static struct arp_request_cache *
> +pinctrl_find_arp_request_cache(uint32_t dp_key, uint32_t port_key,
> + const char *ip_s, uint32_t hash)
> +{
> + struct arp_request_cache *arc;
> + HMAP_FOR_EACH_WITH_HASH (arc, hmap_node, hash, &arp_request_caches) {
> + if (arc->dp_key == dp_key
> + && arc->port_key == port_key
> + && !strcmp(arc->ip_s, ip_s)) {
> + return arc;
> + }
> + }
> + return NULL;
> +}
The find_delete call contains the same lookup code as the plain find call. Why
not just have find_delete call it?
Somewhat minor, but the names are very similar, but have different return
values; the find_delete one seems misnamed.
> +static void
> +pinctrl_insert_arp_request_cache(uint32_t dp_key, uint32_t port_key,
> + const char *ip_s, uint32_t hash)
> +{
> + if (hmap_count(&arp_request_caches) >= 1000) {
> + VLOG_INFO("The ARP request cache is full. Request will be dropped");
> + return;
> + }
It seems like it would be safer to limit the cache per port, not for the whole
system, since that seems like its own potential DoS issue with a badly behaving
port.
> +static void
> +run_update_arp_request_cache(void)
> +{
> + struct arp_request_cache *arc, *next;
> + HMAP_FOR_EACH_SAFE (arc, next, hmap_node, &arp_request_caches) {
> + /* If the ARP request has been send 10s, and the reply is not
> received,
> + * remove the ARP request from the cache.*/
> + if (time_msec() > arc->timestamp + 10000) {
Rather than use a magic number, how about using a #define earlier in file?
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index ac0d8f7..02be5e6 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -3143,11 +3143,6 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
> "ip4.dst == 0.0.0.0/8",
> "drop;");
>
> - /* ARP reply handling. Use ARP replies to populate the logical
> - * router's ARP table. */
> - ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 90, "arp.op == 2",
> - "put_arp(inport, arp.spa, arp.sha);");
> -
> /* Drop Ethernet local broadcast. By definition this traffic should
> * not be forwarded.*/
> ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 50,
> @@ -3215,9 +3210,22 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
> ds_cstr(&match), ds_cstr(&actions));
> }
>
> - /* ARP reply. These flows reply to ARP requests for the router's own
> - * IP address. */
> for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> + /* ARP reply handling. Use ARP replies to populate the logical
> + * router's ARP table. The logical router doesn't support proxy
> ARP */
The last sentence should end with a period.
> + ds_clear(&match);
> + ds_put_format(&match,
> + "inport == %s && arp.op == 2 && arp.tpa == %s "
> + "&& arp.spa == %s/%d",
> + op->json_key,
> op->lrp_networks.ipv4_addrs[i].addr_s,
> + op->lrp_networks.ipv4_addrs[i].network_s,
> + op->lrp_networks.ipv4_addrs[i].plen);
> +
> + ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 90,
> + ds_cstr(&match), "put_arp(inport, arp.spa,
> arp.sha);");
You should update the flow descriptions in the ovn-northd man page.
Thanks,
--Justin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
