Re: [PATCH v2 net-next 02/15] neighbour: Move two validations from neigh_get() to neigh_valid_get_req().

[Jakub Kicinski <kuba@kernel.org>]

On Sat, 12 Jul 2025 20:34:11 +0000 Kuniyuki Iwashima wrote:
>  	err = nlmsg_parse_deprecated_strict(nlh, sizeof(struct ndmsg), tb,
>  					    NDA_MAX, nda_policy, extack);
>  	if (err < 0)
> @@ -2951,11 +2957,14 @@ static struct ndmsg *neigh_valid_get_req(const struct nlmsghdr *nlh,
>  	}
>  
>  	for (i = 0; i <= NDA_MAX; ++i) {
> -		if (!tb[i])
> -			continue;
> -
>  		switch (i) {
>  		case NDA_DST:
> +			if (!tb[i]) {
> +				NL_SET_ERR_MSG(extack, "Network address not specified");
> +				err = -EINVAL;
> +				goto err;
> +			}

Any idea why this attr validation is coded up so weirdly?

NDA_DST is 1, so we could make this whole thing:

const struct nla_policy nda_get_policy[] = {
	[NDA_DST]		= { .type = NLA_BINARY, .len = MAX_ADDR_LEN },
};

	err = nlmsg_parse(nlh, sizeof(struct ndmsg), tb,
			  ARRAY_SIZE(nda_get_policy) - 1,
		  	  nda_get_policy, extack);

and then no looping over the attributes would be necessary.

I'd also be tempted to replace the extack string with
	NL_SET_ERR_ATTR_MISS(extack, NULL, NDA_DST);
but I'm biased towards YNL :)