* getneigh: add nondump to retrieve single entry
@ 2019-05-13 16:03 mcmahon
2019-05-13 16:28 ` Stephen Hemminger
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: mcmahon @ 2019-05-13 16:03 UTC (permalink / raw)
To: davem, dsahern, roopa, christian, khlebnikov, lzgrablic, netdev,
linux-kernel, mowat, dmia
Cc: Ben McMahon
From: Leonard Zgrablic <lzgrablic@arista.com>
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 <lzgrablic@arista.com>
Signed-off-by: Ben McMahon <mcmahon@arista.com>
---
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 */
+}
+
+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);
+ 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);
+
+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);
+
+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);
+
+ 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;
+}
+
+
+
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 */
}
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);
rtnl_register(PF_UNSPEC, RTM_GETNEIGHTBL, NULL, neightbl_dump_info,
0);
--
2.21.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: getneigh: add nondump to retrieve single entry
2019-05-13 16:03 getneigh: add nondump to retrieve single entry mcmahon
@ 2019-05-13 16:28 ` Stephen Hemminger
2019-05-13 22:01 ` David Ahern
2019-05-14 15:40 ` Roopa Prabhu
2 siblings, 0 replies; 4+ messages in thread
From: Stephen Hemminger @ 2019-05-13 16:28 UTC (permalink / raw)
To: mcmahon
Cc: davem, dsahern, roopa, christian, khlebnikov, lzgrablic, netdev,
mowat, dmia
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 <lzgrablic@arista.com>
>
> 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 <lzgrablic@arista.com>
> Signed-off-by: Ben McMahon <mcmahon@arista.com>
> ---
> 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);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: getneigh: add nondump to retrieve single entry
2019-05-13 16:03 getneigh: add nondump to retrieve single entry mcmahon
2019-05-13 16:28 ` Stephen Hemminger
@ 2019-05-13 22:01 ` David Ahern
2019-05-14 15:40 ` Roopa Prabhu
2 siblings, 0 replies; 4+ messages in thread
From: David Ahern @ 2019-05-13 22:01 UTC (permalink / raw)
To: mcmahon, davem, roopa, christian, khlebnikov, lzgrablic, netdev,
linux-kernel, mowat, dmia
On 5/13/19 10:03 AM, mcmahon@arista.com wrote:
> 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
> +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;
reverse xmas tree ordering
...
> +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);
this patch is clearly for a MUCH older kernel than 5.2 (like 3.18
maybe?) as that lock no longer exists.
> + for (tbl = neigh_tables; tbl; tbl = tbl->next) {
> + struct sk_buff *nskb;
> + int err;
> +
> + if (tbl->family != ndm->ndm_family)
> + continue;
Use neigh_find_table.
You need to update the patch to top of net-next tree and re-work the
locking. Run tests with RCU and lock debugging enabled to make sure you
have it right.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: getneigh: add nondump to retrieve single entry
2019-05-13 16:03 getneigh: add nondump to retrieve single entry mcmahon
2019-05-13 16:28 ` Stephen Hemminger
2019-05-13 22:01 ` David Ahern
@ 2019-05-14 15:40 ` Roopa Prabhu
2 siblings, 0 replies; 4+ messages in thread
From: Roopa Prabhu @ 2019-05-14 15:40 UTC (permalink / raw)
To: mcmahon
Cc: David Miller, David Ahern, christian, khlebnikov, lzgrablic,
netdev, Linux Kernel Mailing List, mowat, dmia
On Mon, May 13, 2019 at 9:04 AM <mcmahon@arista.com> wrote:
>
> From: Leonard Zgrablic <lzgrablic@arista.com>
>
> 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 <lzgrablic@arista.com>
> Signed-off-by: Ben McMahon <mcmahon@arista.com>
> ---
I am a bit confused here. How is this different from the below commit
already in the tree ?
commit 82cbb5c631a07b3aa6df6eab644d55da9de5a645
Author: Roopa Prabhu <roopa@cumulusnetworks.com>
Date: Wed Dec 19 12:51:38 2018 -0800
neighbour: register rtnl doit handler
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-05-14 15:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-13 16:03 getneigh: add nondump to retrieve single entry mcmahon
2019-05-13 16:28 ` Stephen Hemminger
2019-05-13 22:01 ` David Ahern
2019-05-14 15:40 ` Roopa Prabhu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.