Message ID | 20190513160335.24128-1-mcmahon@arista.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | getneigh: add nondump to retrieve single entry | expand |
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);
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.
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
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);