Re: [PATCH net-next RFC] ipv4 neigh: match add/del dev lookup behaviour to SIOCSARP/SIOCDARP

[roopa <roopa@cumulusnetworks.com>]

On 2/24/15, 6:01 AM, Rosen, Rami wrote:
> Hi,
>
> @@ -1692,24 +1703,31 @@ static int neigh_add(struct sk_buff *skb, struct nlmsghdr *nlh)
>   		goto out;
>   
>   	ndm = nlmsg_data(nlh);
> +	tbl = neigh_find_table(ndm->ndm_family);
> +	if (tbl == NULL)
> +		return -EAFNOSUPPORT;
> +
> +	if (nla_len(tb[NDA_DST]) < tbl->key_len)
> +		goto out;
> +
> +	dst = nla_data(tb[NDA_DST]);
>   	if (ndm->ndm_ifindex) {
>   		dev = __dev_get_by_index(net, ndm->ndm_ifindex);
>   		if (dev == NULL) {
>   			err = -ENODEV;
>   			goto out;
>   		}
>
> If ip_route_output() fails, then the neigh_add() method will return -EINVAL (as it was initialized thus);
I was intentionally trying not to return any special error code if 
ip_route_output() fails. So the patch tries to make the ip_route_output 
failure same as the case where the user did not specify a device.

> or, in cases when the NTF_PROXY flag is set, it can return 0 or ENOBUFS, according to the result of the pneigh_lookup(), which is called immediately subsequently in this path. But in fact __ip_route_output_key(), which is invoked by ip_route_output(), can return different error codes, like -EINVAL,-ENODEV, -ENETUNREACH and more, via ERR_PTR.
>
>   So maybe better is (in order to reflect the cause of the error):
> +		if (!IS_ERR(rt)) {
> +			dev = rt->dst.dev;
> +			ip_rt_put(rt);
> +		} else
>                                                 return PTR_ERR(rt);

I intentionally don't return here ..because from the original code, the 
NTF_PROXY case seemed to be ok with the device being NULL.  The device 
NULL check was after the NTF_PROXY code.

>
> We can exit the method at this point with "return PTR_ERR(rt)" because we are assured at this point that dev is NULL.
>
> Error handling in neigh_delete() method maybe also be better changed accordingly.

I intend to do some testing for the NTF_PROXY case, however, AFAICT, 
this patch should not have introduced any functional changes in that path.

Thanks!.