Re: [PATCH net-next 2/8] net: sched: cls_api: handle generic cls errors

[David Ahern <dsahern@gmail.com>]

On 1/16/18 9:20 AM, Alexander Aring wrote:
> This patch adds extack support for generic cls handling. The extack
> will be set deeper to each called function which is not part of netdev
> core api.
> 
> Cc: David Ahern <dsahern@gmail.com>
> Signed-off-by: Alexander Aring <aring@mojatatu.com>
> ---
>  net/sched/cls_api.c | 55 +++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 43 insertions(+), 12 deletions(-)
> 
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 01d09055707d..c25a9b4bcb4b 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -122,7 +122,8 @@ static inline u32 tcf_auto_prio(struct tcf_proto *tp)
>  
>  static struct tcf_proto *tcf_proto_create(const char *kind, u32 protocol,
>  					  u32 prio, u32 parent, struct Qdisc *q,
> -					  struct tcf_chain *chain)
> +					  struct tcf_chain *chain,
> +					  struct netlink_ext_ack *extack)
>  {
>  	struct tcf_proto *tp;
>  	int err;
> @@ -148,6 +149,7 @@ static struct tcf_proto *tcf_proto_create(const char *kind, u32 protocol,
>  			module_put(tp->ops->owner);
>  			err = -EAGAIN;
>  		} else {
> +			NL_SET_ERR_MSG(extack, "TC classifier not found");
>  			err = -ENOENT;
>  		}
>  		goto errout;
> @@ -662,7 +664,8 @@ static int tfilter_notify(struct net *net, struct sk_buff *oskb,
>  static int tfilter_del_notify(struct net *net, struct sk_buff *oskb,
>  			      struct nlmsghdr *n, struct tcf_proto *tp,
>  			      struct Qdisc *q, u32 parent,
> -			      void *fh, bool unicast, bool *last)
> +			      void *fh, bool unicast, bool *last,
> +			      struct netlink_ext_ack *extack)
>  {
>  	struct sk_buff *skb;
>  	u32 portid = oskb ? NETLINK_CB(oskb).portid : 0;
> @@ -674,6 +677,7 @@ static int tfilter_del_notify(struct net *net, struct sk_buff *oskb,
>  
>  	if (tcf_fill_node(net, skb, tp, q, parent, fh, portid, n->nlmsg_seq,
>  			  n->nlmsg_flags, RTM_DELTFILTER) <= 0) {
> +		NL_SET_ERR_MSG(extack, "Failed to build del event notification");
>  		kfree_skb(skb);
>  		return -EINVAL;
>  	}
> @@ -687,8 +691,11 @@ static int tfilter_del_notify(struct net *net, struct sk_buff *oskb,
>  	if (unicast)
>  		return netlink_unicast(net->rtnl, skb, portid, MSG_DONTWAIT);
>  
> -	return rtnetlink_send(skb, net, portid, RTNLGRP_TC,
> -			      n->nlmsg_flags & NLM_F_ECHO);
> +	err = rtnetlink_send(skb, net, portid, RTNLGRP_TC,
> +			     n->nlmsg_flags & NLM_F_ECHO);
> +	if (err < 0)
> +		NL_SET_ERR_MSG(extack, "Failed to send filter delete notification");

not sure we want to do this -- extack for internal failures like this
one or below in tc_ctl_tfilter.


> +	return err;
>  }
>  
>  static void tfilter_notify_chain(struct net *net, struct sk_buff *oskb,
> @@ -749,8 +756,10 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>  	if (prio == 0) {
>  		switch (n->nlmsg_type) {
>  		case RTM_DELTFILTER:
> -			if (protocol || t->tcm_handle || tca[TCA_KIND])
> +			if (protocol || t->tcm_handle || tca[TCA_KIND]) {
> +				NL_SET_ERR_MSG(extack, "Cannot flush filters with protocol, handle or kind set");
>  				return -ENOENT;
> +			}
>  			break;
>  		case RTM_NEWTFILTER:
>  			/* If no priority is provided by the user,
> @@ -763,6 +772,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>  			}
>  			/* fall-through */
>  		default:
> +			NL_SET_ERR_MSG(extack, "Invalid filter command with priority of zero");
>  			return -ENOENT;
>  		}
>  	}
> @@ -780,23 +790,31 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>  		parent = q->handle;
>  	} else {
>  		q = qdisc_lookup(dev, TC_H_MAJ(t->tcm_parent));
> -		if (!q)
> +		if (!q) {
> +			NL_SET_ERR_MSG(extack, "Parent Qdisc doesn't exists");

Messages should avoid contractions; spell out 'does not'. Please check
all of the patches.

Also, it should be 'exist' (no 's' on the end).


>  			return -EINVAL;
> +		}
>  	}
>  
>  	/* Is it classful? */
>  	cops = q->ops->cl_ops;
> -	if (!cops)
> +	if (!cops) {
> +		NL_SET_ERR_MSG(extack, "Qdisc not classful");
>  		return -EINVAL;
> +	}
>  
> -	if (!cops->tcf_block)
> +	if (!cops->tcf_block) {
> +		NL_SET_ERR_MSG(extack, "Class doesn't support blocks");
>  		return -EOPNOTSUPP;
> +	}
>  
>  	/* Do we search for filter, attached to class? */
>  	if (TC_H_MIN(parent)) {
>  		cl = cops->find(q, parent);
> -		if (cl == 0)
> +		if (cl == 0) {
> +			NL_SET_ERR_MSG(extack, "Specified Class doesn't exist");

s/Class/class/

>  			return -ENOENT;
> +		}
>  	}
>  
>  	/* And the last stroke */
> @@ -808,12 +826,14 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>  
>  	chain_index = tca[TCA_CHAIN] ? nla_get_u32(tca[TCA_CHAIN]) : 0;
>  	if (chain_index > TC_ACT_EXT_VAL_MASK) {
> +		NL_SET_ERR_MSG(extack, "Specified chain index exceeds upper limit");
>  		err = -EINVAL;
>  		goto errout;
>  	}
>  	chain = tcf_chain_get(block, chain_index,
>  			      n->nlmsg_type == RTM_NEWTFILTER);
>  	if (!chain) {
> +		NL_SET_ERR_MSG(extack, "Cannot find specified filter chain");
>  		err = n->nlmsg_type == RTM_NEWTFILTER ? -ENOMEM : -EINVAL;
>  		goto errout;
>  	}
> @@ -829,6 +849,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>  	tp = tcf_chain_tp_find(chain, &chain_info, protocol,
>  			       prio, prio_allocate);
>  	if (IS_ERR(tp)) {
> +		NL_SET_ERR_MSG(extack, "Filter with specified priority/protocol not found");
>  		err = PTR_ERR(tp);
>  		goto errout;
>  	}
> @@ -837,12 +858,14 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>  		/* Proto-tcf does not exist, create new one */
>  
>  		if (tca[TCA_KIND] == NULL || !protocol) {
> +			NL_SET_ERR_MSG(extack, "Filter kind and protocol must be specified");
>  			err = -EINVAL;
>  			goto errout;
>  		}
>  
>  		if (n->nlmsg_type != RTM_NEWTFILTER ||
>  		    !(n->nlmsg_flags & NLM_F_CREATE)) {
> +			NL_SET_ERR_MSG(extack, "Need both RTM_NEWTFILTER and NLM_F_CREATE to create a new filter");

that does not seem the right message. tc_ctl_tfilter is overloaded for
new, delete and get so the response here needs to reflect that. I
believe in this case the user did not specify a valid chain.

Also, the messages are targeted at users not developers, so no code
jargon / API references.


>  			err = -ENOENT;
>  			goto errout;
>  		}
> @@ -851,13 +874,14 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>  			prio = tcf_auto_prio(tcf_chain_tp_prev(&chain_info));
>  
>  		tp = tcf_proto_create(nla_data(tca[TCA_KIND]),
> -				      protocol, prio, parent, q, chain);
> +				      protocol, prio, parent, q, chain, extack);
>  		if (IS_ERR(tp)) {
>  			err = PTR_ERR(tp);
>  			goto errout;
>  		}
>  		tp_created = 1;
>  	} else if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], tp->ops->kind)) {
> +		NL_SET_ERR_MSG(extack, "Specified filter kind does not match existing one");
>  		err = -EINVAL;
>  		goto errout;
>  	}
> @@ -876,6 +900,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>  
>  		if (n->nlmsg_type != RTM_NEWTFILTER ||
>  		    !(n->nlmsg_flags & NLM_F_CREATE)) {
> +			NL_SET_ERR_MSG(extack, "Need both RTM_NEWTFILTER and NLM_F_CREATE to create a new filter");

same here.


>  			err = -ENOENT;
>  			goto errout;
>  		}
> @@ -887,13 +912,14 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>  			if (n->nlmsg_flags & NLM_F_EXCL) {
>  				if (tp_created)
>  					tcf_proto_destroy(tp);
> +				NL_SET_ERR_MSG(extack, "Exclusivity check success. Filter already exists");

Just "Filter already exists"


>  				err = -EEXIST;
>  				goto errout;
>  			}
>  			break;
>  		case RTM_DELTFILTER:
>  			err = tfilter_del_notify(net, skb, n, tp, q, parent,
> -						 fh, false, &last);
> +						 fh, false, &last, extack);
>  			if (err)
>  				goto errout;
>  			if (last) {
> @@ -904,8 +930,11 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>  		case RTM_GETTFILTER:
>  			err = tfilter_notify(net, skb, n, tp, q, parent, fh,
>  					     RTM_NEWTFILTER, true);
> +			if (err < 0)
> +				NL_SET_ERR_MSG(extack, "Failed to send filter notify message");


>  			goto errout;
>  		default:
> +			NL_SET_ERR_MSG(extack, "Invalid netlink message type");
>  			err = -EINVAL;
>  			goto errout;
>  		}
> @@ -1117,8 +1146,10 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
>  	}
>  #else
>  	if ((exts->action && tb[exts->action]) ||
> -	    (exts->police && tb[exts->police]))
> +	    (exts->police && tb[exts->police])) {
> +		NL_SET_ERR_MSG(extack, "Actions are not supported. Check compile options");

To Cong's comment, perhaps "Classifier actions are not supported per
compile options"


>  		return -EOPNOTSUPP;
> +	}
>  #endif
>  
>  	return 0;
>