Re: getneigh: add nondump to retrieve single entry

[Stephen Hemminger <stephen@networkplumber.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 <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);