From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id F3FCEC04AA7 for ; Mon, 13 May 2019 16:28:46 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id A7278208CA for ; Mon, 13 May 2019 16:28:46 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=networkplumber-org.20150623.gappssmtp.com header.i=@networkplumber-org.20150623.gappssmtp.com header.b="vpavDwDl" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730431AbfEMQ2o (ORCPT ); Mon, 13 May 2019 12:28:44 -0400 Received: from mail-pl1-f196.google.com ([209.85.214.196]:34020 "EHLO mail-pl1-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727774AbfEMQ2n (ORCPT ); Mon, 13 May 2019 12:28:43 -0400 Received: by mail-pl1-f196.google.com with SMTP id w7so6753607plz.1 for ; Mon, 13 May 2019 09:28:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=My+kDDL0oryvZjMDBw5iBPRGR4Dr+xP3h+KGQntv3CA=; b=vpavDwDl7NlqTKTBZ2kmZkLM5C6XlpnFpYwP9I5w7s1Ca6WRXnjzv5//xRS/i1Sa+I /h/+hAbYfgjSzM/8UR3q6Si5EpZj0mTfCyjC+sqvDsXAEWWdP9Vuoas44DtBiKl6aqfG BUFicq024q+p1hE9W9zaBvXk735LRNYCDEDkGUdVae5PSRHGxtF1xBPMFxHxctYkxIJr WMTHxNWZoLbDU5PXgZBmDDc43OiM1yeoJ6XHklsNf4v3Ot8RZhvnkfwd+XSZ4NmgikDq Wm0KxursGL2jKAX8QMSYy1WayMDi2S9GCcwZKDev/QXQdOcpARcArHO8MdZTtLs+UMdW ML4A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=My+kDDL0oryvZjMDBw5iBPRGR4Dr+xP3h+KGQntv3CA=; b=XXlzKNejhGhivz9bXSPaYkYbdG2OBnKH7c1gY6z6hP2XfVCt2ufsGqUp0u9QbL4LsI PO+mDAz1gHmoEvIK9WaLtMyo9DR1zIgb1ARRHNixd1kKCL1s4MaPOSen8Rgx+uiEYbf/ ye1rmjjg4BO+QJBcy4kePwTLI5pLLuf5G04b/M5Tg+/NxcC0c63EDLy3MPy3wTHX9Fww UJFY1J2WW/ywB3L/pY/GN6d9Ae6Qqh7pg36ii5+IxYQNJ5YUayAToAQBBXRkdG6szLNi N3BKnklTQqHAaeCsZ0EW2X14lyYSSBlHpzs5necUWjSI7PxSJ9Snwr5s0GdBX10yGVG6 ZSPA== X-Gm-Message-State: APjAAAVyHUstvTlddTh0QF3UEkkYYlEnAh/t3AIxz6VJNsujQOhFl5O+ ++7INmuqYbBLWF2ofCace4/Asw== X-Google-Smtp-Source: APXvYqwjjViTKMBXz9To/fIOEzSllcN7Gs4ZkqQjuhnSv/JjVfi1ow+1pACVQR/6hrHU9vrVqiCxVQ== X-Received: by 2002:a17:902:248:: with SMTP id 66mr22996452plc.185.1557764921522; Mon, 13 May 2019 09:28:41 -0700 (PDT) Received: from hermes.lan (204-195-22-127.wavecable.com. [204.195.22.127]) by smtp.gmail.com with ESMTPSA id i15sm18479418pfr.8.2019.05.13.09.28.41 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 13 May 2019 09:28:41 -0700 (PDT) Date: Mon, 13 May 2019 09:28:34 -0700 From: Stephen Hemminger To: mcmahon@arista.com Cc: davem@davemloft.net, dsahern@gmail.com, roopa@cumulusnetworks.com, christian@brauner.io, khlebnikov@yandex-team.ru, lzgrablic@arista.com, netdev@vger.kernel.org, mowat@arista.com, dmia@arista.com Subject: Re: getneigh: add nondump to retrieve single entry Message-ID: <20190513092834.13fb99b2@hermes.lan> In-Reply-To: <20190513160335.24128-1-mcmahon@arista.com> References: <20190513160335.24128-1-mcmahon@arista.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Functionally this patch looks fine, but it has several style things that need to be fixed. The Subject line of the mail should be: [PATCH net-next] getneigh: add nondump to retrieve single entry Also, your timing is wrong. net-next is still closed. Since there are multiple style errors, learn to use checkpatch. > From: Leonard Zgrablic > > Currently there is only a dump version of RTM_GETNEIGH for PF_UNSPEC in > RTNETLINK that dumps neighbor entries, no non-dump version that can be used to > retrieve a single neighbor entry. > > Add support for the non-dump (doit) version of RTM_GETNEIGH for PF_UNSPEC so > that a single neighbor entry can be retrieved. > > Signed-off-by: Leonard Zgrablic > Signed-off-by: Ben McMahon > --- > net/core/neighbour.c | 160 +++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 147 insertions(+), 13 deletions(-) > > diff --git a/net/core/neighbour.c b/net/core/neighbour.c > index 30f6fd8f68e0..981f1568710b 100644 > --- a/net/core/neighbour.c > +++ b/net/core/neighbour.c > @@ -2733,6 +2733,149 @@ static int neigh_dump_info(struct sk_buff *skb, struct netlink_callback *cb) > return skb->len; > } > > +static inline size_t neigh_nlmsg_size(void) > +{ > + return NLMSG_ALIGN(sizeof(struct ndmsg)) > + + nla_total_size(MAX_ADDR_LEN) /* NDA_DST */ > + + nla_total_size(MAX_ADDR_LEN) /* NDA_LLADDR */ > + + nla_total_size(sizeof(struct nda_cacheinfo)) > + + nla_total_size(4); /* NDA_PROBES */ > +} Why do you need to move this code? Instead just put your new function after the existing reply logic. > +static int neigh_find_fill(struct neigh_table *tbl, const void *pkey, > + struct net_device *dev, struct sk_buff *skb, u32 pid, > + u32 seq) > +{ > + struct neighbour *neigh; > + int key_len = tbl->key_len; > + u32 hash_val; > + struct neigh_hash_table *nht; > + int err; > + > + if (dev == NULL) > + return -EINVAL; > + > + NEIGH_CACHE_STAT_INC(tbl, lookups); > + > + rcu_read_lock_bh(); > + nht = rcu_dereference_bh(tbl->nht); Indentation incorrect. > + hash_val = tbl->hash(pkey, dev, nht->hash_rnd) >> > + (32 - nht->hash_shift); > + > + for (neigh = rcu_dereference_bh(nht->hash_buckets[hash_val]); > + neigh != NULL; > + neigh = rcu_dereference_bh(neigh->next)) { > + if (dev == neigh->dev && > + !memcmp(neigh->primary_key, pkey, key_len)) { > + if (!atomic_read(&neigh->refcnt)) > + neigh = NULL; > + NEIGH_CACHE_STAT_INC(tbl, hits); > + break; > + } > + } > + if (neigh == NULL) { > + err = -ENOENT; > + goto out_rcu_read_unlock; > + } > + > + err = neigh_fill_info(skb, neigh, pid, seq, RTM_NEWNEIGH, 0); > + You could not use a goto by just doing: if (neigh) err = neigh_fill_info(...) else err = -ENOENT > +out_rcu_read_unlock: > + rcu_read_unlock_bh(); > + return err; > +} > + > +static int pneigh_find_fill(struct neigh_table *tbl, const void *pkey, > + struct net_device *dev, struct net *net, > + struct sk_buff *skb, u32 pid, u32 seq) > +{ > + struct pneigh_entry *pneigh; > + int key_len = tbl->key_len; > + u32 hash_val = pneigh_hash(pkey, key_len); > + int err; > + > + read_lock_bh(&tbl->lock); > + > + pneigh = __pneigh_lookup_1(tbl->phash_buckets[hash_val], net, pkey, > + key_len, dev); > + if (pneigh == NULL) { > + err = -ENOENT; > + goto out_read_unlock; > + } > + > + err = pneigh_fill_info(skb, pneigh, pid, seq, RTM_NEWNEIGH, 0, tbl); Another spot you use goto when not necessary > +out_read_unlock: > + read_unlock_bh(&tbl->lock); > + return err; > +} > + > +static int neigh_get(struct sk_buff *skb, struct nlmsghdr *nlh) > +{ > + struct net *net = sock_net(skb->sk); > + struct ndmsg *ndm; > + struct nlattr *dst_attr; > + struct neigh_table *tbl; > + struct net_device *dev = NULL; > + > + ASSERT_RTNL(); > + if (nlmsg_len(nlh) < sizeof(*ndm)) > + return -EINVAL; > + > + dst_attr = nlmsg_find_attr(nlh, sizeof(*ndm), NDA_DST); > + if (dst_attr == NULL) > + return -EINVAL; > + > + ndm = nlmsg_data(nlh); > + if (ndm->ndm_ifindex) { > + dev = __dev_get_by_index(net, ndm->ndm_ifindex); > + if (dev == NULL) > + return -ENODEV; > + } > + > + read_lock(&neigh_tbl_lock); > + for (tbl = neigh_tables; tbl; tbl = tbl->next) { > + struct sk_buff *nskb; > + int err; > + > + if (tbl->family != ndm->ndm_family) > + continue; > + > + read_unlock(&neigh_tbl_lock); I find it cleaner if you can make lookup a separate function rather than having to make sure all paths from here on down in the loop do a return. > + > + if (nla_len(dst_attr) < tbl->key_len) > + return -EINVAL; > + > + nskb = nlmsg_new(neigh_nlmsg_size(), GFP_KERNEL); > + if (nskb == NULL) > + return -ENOBUFS; > + > + if (ndm->ndm_flags & NTF_PROXY) > + err = pneigh_find_fill(tbl, nla_data(dst_attr), dev, > + net, nskb, > + NETLINK_CB(skb).portid, > + nlh->nlmsg_seq); > + else > + err = neigh_find_fill(tbl, nla_data(dst_attr), dev, > + nskb, NETLINK_CB(skb).portid, > + nlh->nlmsg_seq); > + > + if (err < 0) { > + /* -EMSGSIZE implies BUG in neigh_nlmsg_size */ > + WARN_ON(err == -EMSGSIZE); > + kfree_skb(nskb); > + } else { > + err = rtnl_unicast(nskb, net, NETLINK_CB(skb).portid); > + } > + > + return err; > + } > + read_unlock(&neigh_tbl_lock); > + return -EAFNOSUPPORT; > +} > + > + > + One blank line between functions. > static int neigh_valid_get_req(const struct nlmsghdr *nlh, > struct neigh_table **tbl, > void **dst, int *dev_idx, u8 *ndm_flags, > @@ -2793,16 +2936,6 @@ static int neigh_valid_get_req(const struct nlmsghdr *nlh, > return 0; > } > > -static inline size_t neigh_nlmsg_size(void) > -{ > - return NLMSG_ALIGN(sizeof(struct ndmsg)) > - + nla_total_size(MAX_ADDR_LEN) /* NDA_DST */ > - + nla_total_size(MAX_ADDR_LEN) /* NDA_LLADDR */ > - + nla_total_size(sizeof(struct nda_cacheinfo)) > - + nla_total_size(4) /* NDA_PROBES */ > - + nla_total_size(1); /* NDA_PROTOCOL */ > -} > - > static int neigh_get_reply(struct net *net, struct neighbour *neigh, > u32 pid, u32 seq) > { > @@ -2827,8 +2960,8 @@ static int neigh_get_reply(struct net *net, struct neighbour *neigh, > static inline size_t pneigh_nlmsg_size(void) > { > return NLMSG_ALIGN(sizeof(struct ndmsg)) > - + nla_total_size(MAX_ADDR_LEN) /* NDA_DST */ > - + nla_total_size(1); /* NDA_PROTOCOL */ > + + nla_total_size(MAX_ADDR_LEN) /* NDA_DST */ > + + nla_total_size(1); /* NDA_PROTOCOL */ Leave this code alone. > } > > static int pneigh_get_reply(struct net *net, struct pneigh_entry *neigh, > @@ -3703,7 +3836,8 @@ static int __init neigh_init(void) > { > rtnl_register(PF_UNSPEC, RTM_NEWNEIGH, neigh_add, NULL, 0); > rtnl_register(PF_UNSPEC, RTM_DELNEIGH, neigh_delete, NULL, 0); > - rtnl_register(PF_UNSPEC, RTM_GETNEIGH, neigh_get, neigh_dump_info, 0); > + rtnl_register(PF_UNSPEC, RTM_GETNEIGH, neigh_get, neigh_dump_info, > + NULL); This change is incorrect. Last argument to rtnl_register is flags, and was correct already. just drop it. > > rtnl_register(PF_UNSPEC, RTM_GETNEIGHTBL, NULL, neightbl_dump_info, > 0);