Re: [PATCH net-next v2 4/4] net/sched: cls_route: make netlink errors meaningful

[Jakub Kicinski <kuba@kernel.org>]

On Fri, 25 Aug 2023 12:51:48 -0300 Pedro Tammela wrote:
> Use netlink extended ack and parsing policies to return more meaningful
> errors instead of the relying solely on errnos.

Hm, I don't see the changes to the policy, or anything meaningful 
in the existing one.

> diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
> index 1e20bbd687f1..b34cf02c6c51 100644
> --- a/net/sched/cls_route.c
> +++ b/net/sched/cls_route.c
> @@ -400,30 +400,32 @@ static int route4_set_parms(struct net *net, struct tcf_proto *tp,
>  		if (new && handle & 0x8000)
>  			return -EINVAL;
>  		to = nla_get_u32(tb[TCA_ROUTE4_TO]);
> -		if (to > 0xFF)
> -			return -EINVAL;
>  		nhandle = to;
>  	}
>  
> +	if (tb[TCA_ROUTE4_FROM] && tb[TCA_ROUTE4_IIF]) {
> +		NL_SET_ERR_MSG(extack,
> +			       "'from' and 'fromif' are mutually exclusive");

Let's point at one of them? NL_SET_ERR_MSG_ATTR() ?

> +		return -EINVAL;
> +	}
> +
>  	if (tb[TCA_ROUTE4_FROM]) {
> -		if (tb[TCA_ROUTE4_IIF])
> -			return -EINVAL;
>  		id = nla_get_u32(tb[TCA_ROUTE4_FROM]);
> -		if (id > 0xFF)
> -			return -EINVAL;
>  		nhandle |= id << 16;
>  	} else if (tb[TCA_ROUTE4_IIF]) {
>  		id = nla_get_u32(tb[TCA_ROUTE4_IIF]);
> -		if (id > 0x7FFF)
> -			return -EINVAL;
>  		nhandle |= (id | 0x8000) << 16;
>  	} else
>  		nhandle |= 0xFFFF << 16;
>  
>  	if (handle && new) {
>  		nhandle |= handle & 0x7F00;
> -		if (nhandle != handle)
> +		if (nhandle != handle) {
> +			NL_SET_ERR_MSG_FMT(extack,
> +					   "Unexpected handle %x (expected %x)",
> +					   handle, nhandle);

How about: Handle mismatch constructed: %x (expected: %x)?

>  			return -EINVAL;
> +		}
>  	}
>  
>  	if (!nhandle) {
> @@ -478,7 +480,6 @@ static int route4_change(struct net *net, struct sk_buff *in_skb,
>  	struct route4_filter __rcu **fp;
>  	struct route4_filter *fold, *f1, *pfp, *f = NULL;
>  	struct route4_bucket *b;
> -	struct nlattr *opt = tca[TCA_OPTIONS];
>  	struct nlattr *tb[TCA_ROUTE4_MAX + 1];
>  	unsigned int h, th;
>  	int err;
> @@ -489,10 +490,12 @@ static int route4_change(struct net *net, struct sk_buff *in_skb,
>  		return -EINVAL;
>  	}
>  
> -	if (opt == NULL)
> +	if (NL_REQ_ATTR_CHECK(extack, NULL, tca, TCA_OPTIONS)) {
> +		NL_SET_ERR_MSG_MOD(extack, "missing options");
>  		return -EINVAL;
> +	}
>  
> -	err = nla_parse_nested_deprecated(tb, TCA_ROUTE4_MAX, opt,
> +	err = nla_parse_nested_deprecated(tb, TCA_ROUTE4_MAX, tca[TCA_OPTIONS],
>  					  route4_policy, NULL);
>  	if (err < 0)
>  		return err;
-- 
pw-bot: cr