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 <dsah...@gmail.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 <dsah...@gmail.com>
> Signed-off-by: Alexander Aring <ar...@mojatatu.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();
