Message-ID: <1518693270.2606.70.camel@redhat.com>
Date:   Thu, 15 Feb 2018 12:14:30 +0100
From:   Davide Caratti <dcaratti@...hat.com>
To:     Alexander Aring <aring@...atatu.com>, davem@...emloft.net
Cc:     jhs@...atatu.com, xiyou.wangcong@...il.com, jiri@...nulli.us,
        netdev@...r.kernel.org, kernel@...atatu.com,
        David Ahern <dsahern@...il.com>
Subject: Re: [PATCHv2 net-next 3/8] net: sched: act: handle generic action
 errors

On Wed, 2018-02-14 at 17:13 -0500, Alexander Aring wrote:
> This patch adds extack support for generic act handling. The extack
> will be set deeper to each called function which is not part of netdev
> core api.
> 
> Based on work by David Ahern <dsahern@...il.com>

hello Alexander,

after looking at the code more closely, I think there is margin for some
more improvement here (i.e make the message contents easier to grep from a
log). Please let me know if the comments below make sense for you.

thank you in advance!

-- 
davide

> Cc: David Ahern <dsahern@...il.com>
> Signed-off-by: Alexander Aring <aring@...atatu.com>
> ---
>  net/sched/act_api.c | 93 +++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 61 insertions(+), 32 deletions(-)
> 
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 8d89b026414f..a5138ae026a1 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -617,31 +617,40 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
>  	int err;
>  
>  	if (name == NULL) {
> -		err = nla_parse_nested(tb, TCA_ACT_MAX, nla, NULL, NULL);
> +		err = nla_parse_nested(tb, TCA_ACT_MAX, nla, NULL, extack);
>  		if (err < 0)
>  			goto err_out;
>  		err = -EINVAL;
>  		kind = tb[TCA_ACT_KIND];
> -		if (!kind)
> +		if (!kind) {
> +			NL_SET_ERR_MSG(extack, "TC action kind must be specified");
>  			goto err_out;
> -		if (nla_strlcpy(act_name, kind, IFNAMSIZ) >= IFNAMSIZ)
> +		}
> +		if (nla_strlcpy(act_name, kind, IFNAMSIZ) >= IFNAMSIZ) {
> +			NL_SET_ERR_MSG(extack, "TC action name too long");
>  			goto err_out;
> +		}
>  		if (tb[TCA_ACT_COOKIE]) {
>  			int cklen = nla_len(tb[TCA_ACT_COOKIE]);
>  
> -			if (cklen > TC_COOKIE_MAX_SIZE)
> +			if (cklen > TC_COOKIE_MAX_SIZE) {
> +				NL_SET_ERR_MSG(extack, "TC cookie size above the maximum");
>  				goto err_out;
> +			}
>  
>  			cookie = nla_memdup_cookie(tb);
>  			if (!cookie) {
> +				NL_SET_ERR_MSG(extack, "No memory to generate TC cookie");
>  				err = -ENOMEM;
>  				goto err_out;
>  			}
>  		}
>  	} else {
> -		err = -EINVAL;
> -		if (strlcpy(act_name, name, IFNAMSIZ) >= IFNAMSIZ)
> +		if (strlcpy(act_name, name, IFNAMSIZ) >= IFNAMSIZ) {
> +			NL_SET_ERR_MSG(extack, "TC action name too long");
> +			err = -EINVAL;
>  			goto err_out;
> +		}
>  	}
>  
>  	a_o = tc_lookup_action_n(act_name);
> @@ -664,6 +673,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
>  			goto err_mod;
>  		}
>  #endif
> +		NL_SET_ERR_MSG(extack, "Failed to load TC action module");
>  		err = -ENOENT;
>  		goto err_out;
>  	}
> @@ -698,6 +708,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
>  
>  			list_add_tail(&a->list, &actions);
>  			tcf_action_destroy(&actions, bind);
> +			NL_SET_ERR_MSG(extack, "Failed to init action chain");

most of the times, the word 'action' is prepended by the word 'TC'.
Proposal:

"Failed to init TC action chain" 

>  			return ERR_PTR(err);
>  		}
>  	}
> @@ -734,7 +745,7 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
>  	int err;
>  	int i;
>  
> -	err = nla_parse_nested(tb, TCA_ACT_MAX_PRIO, nla, NULL, NULL);
> +	err = nla_parse_nested(tb, TCA_ACT_MAX_PRIO, nla, NULL, extack);
>  	if (err < 0)
>  		return err;
>  
> @@ -842,7 +853,8 @@ static int tca_get_fill(struct sk_buff *skb, struct list_head *actions,
>  
>  static int
>  tcf_get_notify(struct net *net, u32 portid, struct nlmsghdr *n,
> -	       struct list_head *actions, int event)
> +	       struct list_head *actions, int event,
> +	       struct netlink_ext_ack *extack)
>  {
>  	struct sk_buff *skb;
>  
> @@ -851,6 +863,7 @@ tcf_get_notify(struct net *net, u32 portid, struct nlmsghdr *n,
>  		return -ENOBUFS;
>  	if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, 0, event,
>  			 0, 0) <= 0) {
> +		NL_SET_ERR_MSG(extack, "Failed to fill netlink tc action attributes");

see also @@ -975,6 +1000,7 @@ and @@ -975,6 +1000,7 @@:

this is the same message you get in case tcf_add_notify() fails, and it's
very similar to what you get when tcf_del_notify() fails: the only
difference is uppercase/lowercase 'tc' word.

Proposal:

"Failed to fill netlink attributes while getting TC action"

>  		kfree_skb(skb);
>  		return -EINVAL;
>  	}
> @@ -859,7 +872,8 @@ tcf_get_notify(struct net *net, u32 portid, struct nlmsghdr *n,
>  }
>  
>  static struct tc_action *tcf_action_get_1(struct net *net, struct nlattr *nla,
> -					  struct nlmsghdr *n, u32 portid)
> +					  struct nlmsghdr *n, u32 portid,
> +					  struct netlink_ext_ack *extack)
>  {
>  	struct nlattr *tb[TCA_ACT_MAX + 1];
>  	const struct tc_action_ops *ops;
> @@ -867,20 +881,24 @@ static struct tc_action *tcf_action_get_1(struct net *net, struct nlattr *nla,
>  	int index;
>  	int err;
>  
> -	err = nla_parse_nested(tb, TCA_ACT_MAX, nla, NULL, NULL);
> +	err = nla_parse_nested(tb, TCA_ACT_MAX, nla, NULL, extack);
>  	if (err < 0)
>  		goto err_out;
>  
>  	err = -EINVAL;
>  	if (tb[TCA_ACT_INDEX] == NULL ||
> -	    nla_len(tb[TCA_ACT_INDEX]) < sizeof(index))
> +	    nla_len(tb[TCA_ACT_INDEX]) < sizeof(index)) {
> +		NL_SET_ERR_MSG(extack, "Invalid TC action index value");
>  		goto err_out;
> +	}
>  	index = nla_get_u32(tb[TCA_ACT_INDEX]);
>  
>  	err = -EINVAL;
>  	ops = tc_lookup_action(tb[TCA_ACT_KIND]);
> -	if (!ops) /* could happen in batch of actions */
> +	if (!ops) { /* could happen in batch of actions */
> +		NL_SET_ERR_MSG(extack, "Specified TC action not found");
>  		goto err_out;
> +	}
>  	err = -ENOENT;
>  	if (ops->lookup(net, &a, index) == 0)
>  		goto err_mod;
> @@ -895,7 +913,8 @@ static struct tc_action *tcf_action_get_1(struct net *net, struct nlattr *nla,
>  }
>  
>  static int tca_action_flush(struct net *net, struct nlattr *nla,
> -			    struct nlmsghdr *n, u32 portid)
> +			    struct nlmsghdr *n, u32 portid,
> +			    struct netlink_ext_ack *extack)
>  {
>  	struct sk_buff *skb;
>  	unsigned char *b;
> @@ -909,35 +928,39 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
>  	int err = -ENOMEM;
>  
>  	skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
> -	if (!skb) {
> -		pr_debug("tca_action_flush: failed skb alloc\n");
> +	if (!skb)
>  		return err;
> -	}
>  
>  	b = skb_tail_pointer(skb);
>  
> -	err = nla_parse_nested(tb, TCA_ACT_MAX, nla, NULL, NULL);
> +	err = nla_parse_nested(tb, TCA_ACT_MAX, nla, NULL, extack);
>  	if (err < 0)
>  		goto err_out;
>  
>  	err = -EINVAL;
>  	kind = tb[TCA_ACT_KIND];
>  	ops = tc_lookup_action(kind);
> -	if (!ops) /*some idjot trying to flush unknown action */
> +	if (!ops) { /*some idjot trying to flush unknown action */

the above comment was already there before your patch. I think that it can
be removed, now that we have an error message for this situation; it's
also funny (and harmless) to leave it - but then maybe it becomes more
clear if the word 'idiot' is spelled correctly :)

> +		NL_SET_ERR_MSG(extack, "Aborted Flush. Cannot flush unknown TC action");

Also the tests below result in an aborted flush when they fail. So,
'Aborted Flush' can probably be omitted. Proposal:

"Cannot flush unknown TC action"

>  		goto err_out;
> +	}
>  
>  	nlh = nlmsg_put(skb, portid, n->nlmsg_seq, RTM_DELACTION,
>  			sizeof(*t), 0);
> -	if (!nlh)
> +	if (!nlh) {
> +		NL_SET_ERR_MSG(extack, "Aborted Flush. Failed to create flush netlink notification");
Proposal:

"Failed to create netlink notification while flushing TC action". But I'm
not sure that these kind of errors deserve an extended ACK (see below).

>  		goto out_module_put;
> +	}
>  	t = nlmsg_data(nlh);
>  	t->tca_family = AF_UNSPEC;
>  	t->tca__pad1 = 0;
>  	t->tca__pad2 = 0;
>  
>  	nest = nla_nest_start(skb, TCA_ACT_TAB);
> -	if (!nest)
> +	if (!nest) {
> +		NL_SET_ERR_MSG(extack, "Failed to add new netlink message");

"Failed to set nested attribute while flushing TC action". But I'm not
sure that this kind of errors deserve an extended ACK. Probaly a good
compromise is to do a single message in the error path of the function
(i.e. below the "out_module_put" label).

>  		goto out_module_put;
> +	}
>  
>  	err = ops->walk(net, skb, &dcb, RTM_DELACTION, ops);
>  	if (err <= 0)

here the action flush is aborted because walk() failed. Is it worth adding
a message here? (I'm also ok if there is a single error below
"out_module_put" label).

(and, for the record, I think we miss a nla_nest_cancel() here. I will
post a patch targeting 'net' today.)

> @@ -952,6 +975,8 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
>  			     n->nlmsg_flags & NLM_F_ECHO);
>  	if (err > 0)
>  		return 0;
> +	if (err < 0)
> +		NL_SET_ERR_MSG(extack, "Failed to send flush notification");

Proposal:

"Failed to send TC action flush notification"

>  
>  	return err;
>  
> @@ -964,7 +989,7 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
>  
>  static int
>  tcf_del_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions,
> -	       u32 portid)
> +	       u32 portid, struct netlink_ext_ack *extack)
>  {
>  	int ret;
>  	struct sk_buff *skb;
> @@ -975,6 +1000,7 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions,
>  
>  	if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, 0, RTM_DELACTION,
>  			 0, 1) <= 0) {
> +		NL_SET_ERR_MSG(extack, "Failed to fill netlink tc action attributes");

see also @@ -1039,7 +1067,7 @@:

this error message is almost the same as the one we get in case of failing
tca_get_fill(..., RTM_NEWACTION, ...) in tcf_add_notify(). The only
difference is the word 'tc' lowercase here, and uppercase word 'TC' in
tcf_add_notify(). So, if these message must be different, it's better to
specify better what went wrong _ something like:

"Failed to fill netlink attributes while deleting TC action"

>  		kfree_skb(skb);
>  		return -EINVAL;
>  	}
> @@ -982,6 +1008,7 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions,
>  	/* now do the delete */
>  	ret = tcf_action_destroy(actions, 0);
>  	if (ret < 0) {
> +		NL_SET_ERR_MSG(extack, "Failed to delete TC action");
>  		kfree_skb(skb);
>  		return ret;
>  	}
> @@ -995,26 +1022,27 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions,
>  
>  static int
>  tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
> -	      u32 portid, int event)
> +	      u32 portid, int event, struct netlink_ext_ack *extack)
>  {
>  	int i, ret;
>  	struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
>  	struct tc_action *act;
>  	LIST_HEAD(actions);
>  
> -	ret = nla_parse_nested(tb, TCA_ACT_MAX_PRIO, nla, NULL, NULL);
> +	ret = nla_parse_nested(tb, TCA_ACT_MAX_PRIO, nla, NULL, extack);
>  	if (ret < 0)
>  		return ret;
>  
>  	if (event == RTM_DELACTION && n->nlmsg_flags & NLM_F_ROOT) {
>  		if (tb[1])
> -			return tca_action_flush(net, tb[1], n, portid);
> +			return tca_action_flush(net, tb[1], n, portid, extack);
>  
> +		NL_SET_ERR_MSG(extack, "Invalid netlink attributes to flush actions");

"Invalid netlink attributes while flushing TC action"
>  		return -EINVAL;
>  	}
>  
>  	for (i = 1; i <= TCA_ACT_MAX_PRIO && tb[i]; i++) {
> -		act = tcf_action_get_1(net, tb[i], n, portid);
> +		act = tcf_action_get_1(net, tb[i], n, portid, extack);
>  		if (IS_ERR(act)) {
>  			ret = PTR_ERR(act);
>  			goto err;
> @@ -1024,9 +1052,9 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
>  	}
>  
>  	if (event == RTM_GETACTION)
> -		ret = tcf_get_notify(net, portid, n, &actions, event);
> +		ret = tcf_get_notify(net, portid, n, &actions, event, extack);
>  	else { /* delete */
> -		ret = tcf_del_notify(net, n, &actions, portid);
> +		ret = tcf_del_notify(net, n, &actions, portid, extack);
>  		if (ret)
>  			goto err;
>  		return ret;
> @@ -1039,7 +1067,7 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
>  
>  static int
>  tcf_add_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions,
> -	       u32 portid)
> +	       u32 portid, struct netlink_ext_ack *extack)
>  {
>  	struct sk_buff *skb;
>  	int err = 0;
> @@ -1050,6 +1078,7 @@ tcf_add_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions,
>  
>  	if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, n->nlmsg_flags,
>  			 RTM_NEWACTION, 0, 0) <= 0) {
> +		NL_SET_ERR_MSG(extack, "Failed to fill netlink tc action attributes");

see comment at @@ -975,6 +1000,7 @@

"Failed to fill netlink attributes while adding TC action"

>  		kfree_skb(skb);
>  		return -EINVAL;
>  	}
> @@ -1073,7 +1102,7 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
>  	if (ret)
>  		return ret;
>  
> -	return tcf_add_notify(net, n, &actions, portid);
> +	return tcf_add_notify(net, n, &actions, portid, extack);
>  }
>  
>  static u32 tcaa_root_flags_allowed = TCA_FLAG_LARGE_DUMP_ON;
> @@ -1101,7 +1130,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
>  		return ret;
>  
>  	if (tca[TCA_ACT_TAB] == NULL) {
> -		pr_notice("tc_ctl_action: received NO action attribs\n");
> +		NL_SET_ERR_MSG(extack, "Netlink Action attributes missing");

"action" is lowercase in almost all messages, and tipically prepended by
'TC' keyword:

"Missing netlink attributes for TC action"

>  		return -EINVAL;
>  	}
>  
> @@ -1124,11 +1153,11 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
>  		break;
>  	case RTM_DELACTION:
>  		ret = tca_action_gd(net, tca[TCA_ACT_TAB], n,
> -				    portid, RTM_DELACTION);
> +				    portid, RTM_DELACTION, extack);
>  		break;
>  	case RTM_GETACTION:
>  		ret = tca_action_gd(net, tca[TCA_ACT_TAB], n,
> -				    portid, RTM_GETACTION);
> +				    portid, RTM_GETACTION, extack);
>  		break;
>  	default:
>  		BUG();

Powered by blists - more mailing lists