[PATCH net-next 0/8] net: sched: cls: add extack support

[PATCH net-next 0/8] net: sched: cls: add extack support

[Alexander Aring]

Hi,

this patch adds extack support for TC classifier subsystem. The first
patch fixes some code style issues for this patch series pointed out
by checkpatch. The other patches until the last one prepares extack
handling for the TC classifier subsystem and handle generic extack
errors.

The last patch is an example for u32 classifier to add extack support
inside the callbacks delete and change. There exists a init callback as
well, but most classifier implementation run a kalloc() once to allocate
something. Not necessary _yet_ to add extack support now.

I know there are patches around which makes changes to these files.
I will rebase my stuff on Jiri's patches if they get in before mine.

- Alex

Cc: David Ahern <dsahern@gmail.com>

Alexander Aring (8):
  net: sched: cls: fix code style issues
  net: sched: cls_api: handle generic cls errors
  net: sched: cls: add extack support for change callback
  net: sched: cls: add extack support for tcf_exts_validate
  net: sched: cls: add extack support for delete callback
  net: sched: cls: add extack support for tcf_change_indev
  net: sched: cls: add extack support for tc_setup_cb_call
  net: sched: cls_u32: add extack support

 include/net/pkt_cls.h     |  13 ++++--
 include/net/sch_generic.h |   7 +++-
 net/sched/cls_api.c       |  74 +++++++++++++++++++++++++--------
 net/sched/cls_basic.c     |  14 ++++---
 net/sched/cls_bpf.c       |  32 ++++++++------
 net/sched/cls_cgroup.c    |   9 ++--
 net/sched/cls_flow.c      |   8 ++--
 net/sched/cls_flower.c    |  31 ++++++++------
 net/sched/cls_fw.c        |  17 ++++----
 net/sched/cls_matchall.c  |  24 +++++++----
 net/sched/cls_route.c     |  12 +++---
 net/sched/cls_rsvp.h      |   7 ++--
 net/sched/cls_tcindex.c   |  14 ++++---
 net/sched/cls_u32.c       | 104 ++++++++++++++++++++++++++++++++--------------
 14 files changed, 243 insertions(+), 123 deletions(-)

-- 
2.11.0

[PATCH net-next 1/8] net: sched: cls: fix code style issues

[Alexander Aring]

This patch changes some code style issues pointed out by checkpatch
inside the TC cls subsystem.

Signed-off-by: Alexander Aring <aring@mojatatu.com>
---
 include/net/sch_generic.h | 3 ++-
 net/sched/cls_api.c       | 2 +-
 net/sched/cls_matchall.c  | 2 +-
 net/sched/cls_u32.c       | 8 ++++----
 4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index ac029d5d88e4..b6ca86a8caeb 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -233,7 +233,8 @@ struct tcf_proto_ops {
 					struct tcf_proto*, unsigned long,
 					u32 handle, struct nlattr **,
 					void **, bool);
-	int			(*delete)(struct tcf_proto*, void *, bool*);
+	int			(*delete)(struct tcf_proto *tp, void *arg,
+					  bool *last);
 	void			(*walk)(struct tcf_proto*, struct tcf_walker *arg);
 	void			(*bind_class)(void *, u32, unsigned long);
 
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 6708b6953bfa..01d09055707d 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -780,7 +780,7 @@ 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 == NULL)
+		if (!q)
 			return -EINVAL;
 	}
 
diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index 66d4e0099158..634114111adf 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -202,7 +202,7 @@ static int mall_change(struct net *net, struct sk_buff *in_skb,
 		goto err_set_parms;
 
 	if (!tc_skip_hw(new->flags)) {
-		err = mall_replace_hw_filter(tp, new, (unsigned long) new);
+		err = mall_replace_hw_filter(tp, new, (unsigned long)new);
 		if (err)
 			goto err_replace_hw_filter;
 	}
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 507859cdd1cb..2e1b4580f798 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -782,7 +782,7 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp,
 		if (handle) {
 			ht_down = u32_lookup_ht(ht->tp_c, handle);
 
-			if (ht_down == NULL)
+			if (!ht_down)
 				return -EINVAL;
 			ht_down->refcnt++;
 		}
@@ -906,7 +906,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 	size_t size;
 #endif
 
-	if (opt == NULL)
+	if (!opt)
 		return handle ? -EINVAL : 0;
 
 	err = nla_parse_nested(tb, TCA_U32_MAX, opt, u32_policy, NULL);
@@ -1010,7 +1010,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 			htid = ht->handle;
 		} else {
 			ht = u32_lookup_ht(tp->data, TC_U32_HTID(htid));
-			if (ht == NULL)
+			if (!ht)
 				return -EINVAL;
 		}
 	} else {
@@ -1022,7 +1022,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 		return -EINVAL;
 
 	if (handle) {
-		if (TC_U32_HTID(handle) && TC_U32_HTID(handle^htid))
+		if (TC_U32_HTID(handle) && TC_U32_HTID(handle ^ htid))
 			return -EINVAL;
 		handle = htid | TC_U32_NODE(handle);
 		err = idr_alloc_ext(&ht->handle_idr, NULL, NULL,
-- 
2.11.0

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

[Alexander Aring]

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");
+	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");
 			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");
 			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");
 			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");
 			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");
 				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");
 		return -EOPNOTSUPP;
+	}
 #endif
 
 	return 0;
-- 
2.11.0

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

[Cong Wang]

On Tue, Jan 16, 2018 at 9:20 AM, Alexander Aring <aring@mojatatu.com> wrote:
> @@ -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");
>                 return -EOPNOTSUPP;
> +       }
>  #endif

"Check compile options" is confusing, it is clearer if we can just
say we need to enable CONFIG_NET_CLS_ACT here.

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

[David Ahern]

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;
> 

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

[Jamal Hadi Salim]

On 18-01-16 06:58 PM, David Ahern wrote:
> On 1/16/18 9:20 AM, Alexander Aring wrote:


>>   		}
>>   
>>   		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.
> 

Are you sure you are looking at the correct code?
It is a create message that is at stake here.
A create has to have RTM_NEWTFILTER and NLM_F_CREATE

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

Generally true, but should this rule really be scripture?
The main user here is tc in  user space and it doesnt make mistakes
in this case i.e we will  never see this error with tc because a
create will always have those two set correctly; OTOH, a developer
writing some new app is more likely to make this mistake (in which
case this message is very helpful).

cheers,
jamal

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

[David Ahern]

On 1/16/18 4:19 PM, Jamal Hadi Salim wrote:
> On 18-01-16 06:58 PM, David Ahern wrote:
>> On 1/16/18 9:20 AM, Alexander Aring wrote:
> 
> 
>>>           }
>>>             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.
>>
> 
> Are you sure you are looking at the correct code?

        tp = tcf_chain_tp_find(chain, &chain_info, protocol,
                               prio, prio_allocate);
        if (IS_ERR(tp)) {
                err = PTR_ERR(tp);
                goto errout;
        }

        if (tp == NULL) {
                /* Proto-tcf does not exist, create new one */

                if (tca[TCA_KIND] == NULL || !protocol) {
                        err = -EINVAL;
                        goto errout;
                }

                if (n->nlmsg_type != RTM_NEWTFILTER ||
                    !(n->nlmsg_flags & NLM_F_CREATE)) {
                        err = -ENOENT;
                        goto errout;
                }

Seems like that code path is run for other than RTM_NEWTFILTER. Even the
check there says != is ok -- just error out with an ENOENT.


> It is a create message that is at stake here.
> A create has to have RTM_NEWTFILTER and NLM_F_CREATE
> 
>> Also, the messages are targeted at users not developers, so no code
>> jargon / API references.
> 
> Generally true, but should this rule really be scripture?
> The main user here is tc in  user space and it doesnt make mistakes
> in this case i.e we will  never see this error with tc because a
> create will always have those two set correctly; OTOH, a developer
> writing some new app is more likely to make this mistake (in which
> case this message is very helpful).

argumentative. I have focused on adding specific error messages that
help a user understand why a command failed. It can be done with
referencing API names.

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

[Jamal Hadi Salim]

On 18-01-16 10:22 PM, David Ahern wrote:

>          tp = tcf_chain_tp_find(chain, &chain_info, protocol,
>                                 prio, prio_allocate);
>          if (IS_ERR(tp)) {
>                  err = PTR_ERR(tp);
>                  goto errout;
>          }
> 
 >
>          if (tp == NULL) {
>                  /* Proto-tcf does not exist, create new one */
>  >
>                  if (tca[TCA_KIND] == NULL || !protocol) {
>                          err = -EINVAL;
>                          goto errout;
>                  }
> 
>                  if (n->nlmsg_type != RTM_NEWTFILTER ||
>                      !(n->nlmsg_flags & NLM_F_CREATE)) {
>                          err = -ENOENT;
>                          goto errout;
>                  }
> 


The assumption is if we got that far in the code a create
path(per the comment) is the sane thing to do. A create
requires tca[TCA_KIND], protocol, RTM_NEWTFILTER and
NLM_F_CREATE

> Seems like that code path is run for other than RTM_NEWTFILTER. Even the
> check there says != is ok -- just error out with an ENOENT.
> 

To be honest, I am not fond of those checks either, I find myself
thinking sometimes about what would happen given a specific message.
A lot of these control paths in tc kernel parts sometimes are too
clever. That code needs refactoring to separate all 3 operations
for readability/maintainability. I would say the same for sch_api
We could fix it up (but those are separate patches); then we can
have much better error messages as well.

>> Generally true, but should this rule really be scripture?
>> The main user here is tc in  user space and it doesnt make mistakes
>> in this case i.e we will  never see this error with tc because a
>> create will always have those two set correctly; OTOH, a developer
>> writing some new app is more likely to make this mistake (in which
>> case this message is very helpful).
> 
> argumentative.
We are having a discussion.

>I have focused on adding specific error messages that
> help a user understand why a command failed. It can be done with
> referencing API names.


That is one use case which is sensible. It is not like the
message is saying "consult IBM manual X for error code 1234"
IMO:
The kernel should be helpful to the user - but that user is not just
the user of an app within iproute2. It could be a robot, a human
using another CLI app, a developer etc. I think we need to look
at specific cases and make those calls. My view is that we should
never fail from tc for this specific case unless someone is fixing
up tc.

If you want to say it is always a human that needs to interpret
and react - then we need rules well stated somewhere
(eg you stated ENOMEM should not have extack, it was sufficiently
expressive etc). This will help reduce the time spent reviewing
patches or for people to respin.

The kernel should - when it makes sense - even return an extack for
a _successful message_ (I can give you several use cases in tc today)
or binary data via the cookie; and hopefully we can avoid collission
when we start sending such patches by having the discussion now.

cheers,
jamal

[PATCH net-next 3/8] net: sched: cls: add extack support for change callback

[Alexander Aring]

This patch adds extack support for classifier change callback api. This
prepares to handle extack support inside each specific classifier
implementation.

Cc: David Ahern <dsahern@gmail.com>
Signed-off-by: Alexander Aring <aring@mojatatu.com>
---
 include/net/sch_generic.h | 3 ++-
 net/sched/cls_api.c       | 3 ++-
 net/sched/cls_basic.c     | 3 ++-
 net/sched/cls_bpf.c       | 2 +-
 net/sched/cls_cgroup.c    | 3 ++-
 net/sched/cls_flow.c      | 2 +-
 net/sched/cls_flower.c    | 2 +-
 net/sched/cls_fw.c        | 2 +-
 net/sched/cls_matchall.c  | 2 +-
 net/sched/cls_route.c     | 3 ++-
 net/sched/cls_rsvp.h      | 2 +-
 net/sched/cls_tcindex.c   | 3 ++-
 net/sched/cls_u32.c       | 3 ++-
 13 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index b6ca86a8caeb..f999ee6bac2e 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -232,7 +232,8 @@ struct tcf_proto_ops {
 	int			(*change)(struct net *net, struct sk_buff *,
 					struct tcf_proto*, unsigned long,
 					u32 handle, struct nlattr **,
-					void **, bool);
+					void **, bool,
+					struct netlink_ext_ack *);
 	int			(*delete)(struct tcf_proto *tp, void *arg,
 					  bool *last);
 	void			(*walk)(struct tcf_proto*, struct tcf_walker *arg);
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index c25a9b4bcb4b..520b5deec7af 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -941,7 +941,8 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
 	}
 
 	err = tp->ops->change(net, skb, tp, cl, t->tcm_handle, tca, &fh,
-			      n->nlmsg_flags & NLM_F_CREATE ? TCA_ACT_NOREPLACE : TCA_ACT_REPLACE);
+			      n->nlmsg_flags & NLM_F_CREATE ? TCA_ACT_NOREPLACE : TCA_ACT_REPLACE,
+			      extack);
 	if (err == 0) {
 		if (tp_created)
 			tcf_chain_tp_insert(chain, &chain_info, tp);
diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
index 5f169ded347e..2cc38cd71938 100644
--- a/net/sched/cls_basic.c
+++ b/net/sched/cls_basic.c
@@ -175,7 +175,8 @@ static int basic_set_parms(struct net *net, struct tcf_proto *tp,
 
 static int basic_change(struct net *net, struct sk_buff *in_skb,
 			struct tcf_proto *tp, unsigned long base, u32 handle,
-			struct nlattr **tca, void **arg, bool ovr)
+			struct nlattr **tca, void **arg, bool ovr,
+			struct netlink_ext_ack *extack)
 {
 	int err;
 	struct basic_head *head = rtnl_dereference(tp->root);
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 8d78e7f4ecc3..fcb831b3917e 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -449,7 +449,7 @@ static int cls_bpf_set_parms(struct net *net, struct tcf_proto *tp,
 static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
 			  struct tcf_proto *tp, unsigned long base,
 			  u32 handle, struct nlattr **tca,
-			  void **arg, bool ovr)
+			  void **arg, bool ovr, struct netlink_ext_ack *extack)
 {
 	struct cls_bpf_head *head = rtnl_dereference(tp->root);
 	struct cls_bpf_prog *oldprog = *arg;
diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index 309d5899265f..b74af0b55820 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -91,7 +91,8 @@ static void cls_cgroup_destroy_rcu(struct rcu_head *root)
 static int cls_cgroup_change(struct net *net, struct sk_buff *in_skb,
 			     struct tcf_proto *tp, unsigned long base,
 			     u32 handle, struct nlattr **tca,
-			     void **arg, bool ovr)
+			     void **arg, bool ovr,
+			     struct netlink_ext_ack *extack)
 {
 	struct nlattr *tb[TCA_CGROUP_MAX + 1];
 	struct cls_cgroup_head *head = rtnl_dereference(tp->root);
diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index 25c2a888e1f0..e944f01d5394 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -401,7 +401,7 @@ static void flow_destroy_filter(struct rcu_head *head)
 static int flow_change(struct net *net, struct sk_buff *in_skb,
 		       struct tcf_proto *tp, unsigned long base,
 		       u32 handle, struct nlattr **tca,
-		       void **arg, bool ovr)
+		       void **arg, bool ovr, struct netlink_ext_ack *extack)
 {
 	struct flow_head *head = rtnl_dereference(tp->root);
 	struct flow_filter *fold, *fnew;
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 6132a7317efa..998ee4faf934 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -852,7 +852,7 @@ static int fl_set_parms(struct net *net, struct tcf_proto *tp,
 static int fl_change(struct net *net, struct sk_buff *in_skb,
 		     struct tcf_proto *tp, unsigned long base,
 		     u32 handle, struct nlattr **tca,
-		     void **arg, bool ovr)
+		     void **arg, bool ovr, struct netlink_ext_ack *extack)
 {
 	struct cls_fl_head *head = rtnl_dereference(tp->root);
 	struct cls_fl_filter *fold = *arg;
diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
index 20f0de1a960a..72784491ce20 100644
--- a/net/sched/cls_fw.c
+++ b/net/sched/cls_fw.c
@@ -257,7 +257,7 @@ static int fw_set_parms(struct net *net, struct tcf_proto *tp,
 static int fw_change(struct net *net, struct sk_buff *in_skb,
 		     struct tcf_proto *tp, unsigned long base,
 		     u32 handle, struct nlattr **tca, void **arg,
-		     bool ovr)
+		     bool ovr, struct netlink_ext_ack *extack)
 {
 	struct fw_head *head = rtnl_dereference(tp->root);
 	struct fw_filter *f = *arg;
diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index 634114111adf..4cfd293dbbd0 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -159,7 +159,7 @@ static int mall_set_parms(struct net *net, struct tcf_proto *tp,
 static int mall_change(struct net *net, struct sk_buff *in_skb,
 		       struct tcf_proto *tp, unsigned long base,
 		       u32 handle, struct nlattr **tca,
-		       void **arg, bool ovr)
+		       void **arg, bool ovr, struct netlink_ext_ack *extack)
 {
 	struct cls_mall_head *head = rtnl_dereference(tp->root);
 	struct nlattr *tb[TCA_MATCHALL_MAX + 1];
diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
index ac9a5b8825b9..bf305db65de0 100644
--- a/net/sched/cls_route.c
+++ b/net/sched/cls_route.c
@@ -471,7 +471,8 @@ static int route4_set_parms(struct net *net, struct tcf_proto *tp,
 
 static int route4_change(struct net *net, struct sk_buff *in_skb,
 			 struct tcf_proto *tp, unsigned long base, u32 handle,
-			 struct nlattr **tca, void **arg, bool ovr)
+			 struct nlattr **tca, void **arg, bool ovr,
+			 struct netlink_ext_ack *extack)
 {
 	struct route4_head *head = rtnl_dereference(tp->root);
 	struct route4_filter __rcu **fp;
diff --git a/net/sched/cls_rsvp.h b/net/sched/cls_rsvp.h
index cf325625c99d..d1f67529c01d 100644
--- a/net/sched/cls_rsvp.h
+++ b/net/sched/cls_rsvp.h
@@ -486,7 +486,7 @@ static int rsvp_change(struct net *net, struct sk_buff *in_skb,
 		       struct tcf_proto *tp, unsigned long base,
 		       u32 handle,
 		       struct nlattr **tca,
-		       void **arg, bool ovr)
+		       void **arg, bool ovr, struct netlink_ext_ack *extack)
 {
 	struct rsvp_head *data = rtnl_dereference(tp->root);
 	struct rsvp_filter *f, *nfp;
diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index 67467ae24c97..0ec84cf2d6b7 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -520,7 +520,8 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 static int
 tcindex_change(struct net *net, struct sk_buff *in_skb,
 	       struct tcf_proto *tp, unsigned long base, u32 handle,
-	       struct nlattr **tca, void **arg, bool ovr)
+	       struct nlattr **tca, void **arg, bool ovr,
+	       struct netlink_ext_ack *extack)
 {
 	struct nlattr *opt = tca[TCA_OPTIONS];
 	struct nlattr *tb[TCA_TCINDEX_MAX + 1];
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 2e1b4580f798..25a014499afe 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -892,7 +892,8 @@ static struct tc_u_knode *u32_init_knode(struct tcf_proto *tp,
 
 static int u32_change(struct net *net, struct sk_buff *in_skb,
 		      struct tcf_proto *tp, unsigned long base, u32 handle,
-		      struct nlattr **tca, void **arg, bool ovr)
+		      struct nlattr **tca, void **arg, bool ovr,
+		      struct netlink_ext_ack *extack)
 {
 	struct tc_u_common *tp_c = tp->data;
 	struct tc_u_hnode *ht;
-- 
2.11.0

[PATCH net-next 4/8] net: sched: cls: add extack support for tcf_exts_validate

[Alexander Aring]

The tcf_exts_validate function calls the act api change callback. For
preparing extack support for act api, this patch adds the extack as
parameter for this function which is common used in cls implementations.

Furthermore the tcf_exts_validate will call action init callback which
prepares the TC action subsystem for extack support.

Cc: David Ahern <dsahern@gmail.com>
Signed-off-by: Alexander Aring <aring@mojatatu.com>
---
 include/net/pkt_cls.h    |  3 ++-
 net/sched/cls_api.c      |  3 ++-
 net/sched/cls_basic.c    |  8 +++++---
 net/sched/cls_bpf.c      |  8 +++++---
 net/sched/cls_cgroup.c   |  3 ++-
 net/sched/cls_flow.c     |  3 ++-
 net/sched/cls_flower.c   |  8 +++++---
 net/sched/cls_fw.c       | 10 ++++++----
 net/sched/cls_matchall.c |  8 +++++---
 net/sched/cls_route.c    |  6 +++---
 net/sched/cls_rsvp.h     |  2 +-
 net/sched/cls_tcindex.c  |  6 +++---
 net/sched/cls_u32.c      | 10 ++++++----
 13 files changed, 47 insertions(+), 31 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 9c341f003091..0286d42d08ef 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -368,7 +368,8 @@ tcf_exts_exec(struct sk_buff *skb, struct tcf_exts *exts,
 
 int tcf_exts_validate(struct net *net, struct tcf_proto *tp,
 		      struct nlattr **tb, struct nlattr *rate_tlv,
-		      struct tcf_exts *exts, bool ovr);
+		      struct tcf_exts *exts, bool ovr,
+		      struct netlink_ext_ack *extack);
 void tcf_exts_destroy(struct tcf_exts *exts);
 void tcf_exts_change(struct tcf_exts *dst, struct tcf_exts *src);
 int tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts);
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 520b5deec7af..011fe66c82b5 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -1114,7 +1114,8 @@ void tcf_exts_destroy(struct tcf_exts *exts)
 EXPORT_SYMBOL(tcf_exts_destroy);
 
 int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
-		      struct nlattr *rate_tlv, struct tcf_exts *exts, bool ovr)
+		      struct nlattr *rate_tlv, struct tcf_exts *exts, bool ovr,
+		      struct netlink_ext_ack *extack)
 {
 #ifdef CONFIG_NET_CLS_ACT
 	{
diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
index 2cc38cd71938..b7bcf67641bf 100644
--- a/net/sched/cls_basic.c
+++ b/net/sched/cls_basic.c
@@ -152,11 +152,12 @@ static const struct nla_policy basic_policy[TCA_BASIC_MAX + 1] = {
 static int basic_set_parms(struct net *net, struct tcf_proto *tp,
 			   struct basic_filter *f, unsigned long base,
 			   struct nlattr **tb,
-			   struct nlattr *est, bool ovr)
+			   struct nlattr *est, bool ovr,
+			   struct netlink_ext_ack *extack)
 {
 	int err;
 
-	err = tcf_exts_validate(net, tp, tb, est, &f->exts, ovr);
+	err = tcf_exts_validate(net, tp, tb, est, &f->exts, ovr, extack);
 	if (err < 0)
 		return err;
 
@@ -222,7 +223,8 @@ static int basic_change(struct net *net, struct sk_buff *in_skb,
 		fnew->handle = idr_index;
 	}
 
-	err = basic_set_parms(net, tp, fnew, base, tb, tca[TCA_RATE], ovr);
+	err = basic_set_parms(net, tp, fnew, base, tb, tca[TCA_RATE], ovr,
+			      extack);
 	if (err < 0) {
 		if (!fold)
 			idr_remove_ext(&head->handle_idr, fnew->handle);
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index fcb831b3917e..50abd71f99ed 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -400,7 +400,8 @@ static int cls_bpf_prog_from_efd(struct nlattr **tb, struct cls_bpf_prog *prog,
 
 static int cls_bpf_set_parms(struct net *net, struct tcf_proto *tp,
 			     struct cls_bpf_prog *prog, unsigned long base,
-			     struct nlattr **tb, struct nlattr *est, bool ovr)
+			     struct nlattr **tb, struct nlattr *est, bool ovr,
+			     struct netlink_ext_ack *extack)
 {
 	bool is_bpf, is_ebpf, have_exts = false;
 	u32 gen_flags = 0;
@@ -411,7 +412,7 @@ static int cls_bpf_set_parms(struct net *net, struct tcf_proto *tp,
 	if ((!is_bpf && !is_ebpf) || (is_bpf && is_ebpf))
 		return -EINVAL;
 
-	ret = tcf_exts_validate(net, tp, tb, est, &prog->exts, ovr);
+	ret = tcf_exts_validate(net, tp, tb, est, &prog->exts, ovr, extack);
 	if (ret < 0)
 		return ret;
 
@@ -497,7 +498,8 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
 		prog->handle = handle;
 	}
 
-	ret = cls_bpf_set_parms(net, tp, prog, base, tb, tca[TCA_RATE], ovr);
+	ret = cls_bpf_set_parms(net, tp, prog, base, tb, tca[TCA_RATE], ovr,
+				extack);
 	if (ret < 0)
 		goto errout_idr;
 
diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index b74af0b55820..aaafcf6965f7 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -122,7 +122,8 @@ static int cls_cgroup_change(struct net *net, struct sk_buff *in_skb,
 	if (err < 0)
 		goto errout;
 
-	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &new->exts, ovr);
+	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &new->exts, ovr,
+				extack);
 	if (err < 0)
 		goto errout;
 
diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index e944f01d5394..5bc702f66bb5 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -454,7 +454,8 @@ static int flow_change(struct net *net, struct sk_buff *in_skb,
 	if (err < 0)
 		goto err2;
 
-	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &fnew->exts, ovr);
+	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &fnew->exts, ovr,
+				extack);
 	if (err < 0)
 		goto err2;
 
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 998ee4faf934..b9664e1df8f0 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -826,11 +826,12 @@ static int fl_check_assign_mask(struct cls_fl_head *head,
 static int fl_set_parms(struct net *net, struct tcf_proto *tp,
 			struct cls_fl_filter *f, struct fl_flow_mask *mask,
 			unsigned long base, struct nlattr **tb,
-			struct nlattr *est, bool ovr)
+			struct nlattr *est, bool ovr,
+			struct netlink_ext_ack *extack)
 {
 	int err;
 
-	err = tcf_exts_validate(net, tp, tb, est, &f->exts, ovr);
+	err = tcf_exts_validate(net, tp, tb, est, &f->exts, ovr, extack);
 	if (err < 0)
 		return err;
 
@@ -915,7 +916,8 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
 		}
 	}
 
-	err = fl_set_parms(net, tp, fnew, &mask, base, tb, tca[TCA_RATE], ovr);
+	err = fl_set_parms(net, tp, fnew, &mask, base, tb, tca[TCA_RATE], ovr,
+			   extack);
 	if (err)
 		goto errout_idr;
 
diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
index 72784491ce20..72a924a38753 100644
--- a/net/sched/cls_fw.c
+++ b/net/sched/cls_fw.c
@@ -218,13 +218,15 @@ static const struct nla_policy fw_policy[TCA_FW_MAX + 1] = {
 
 static int fw_set_parms(struct net *net, struct tcf_proto *tp,
 			struct fw_filter *f, struct nlattr **tb,
-			struct nlattr **tca, unsigned long base, bool ovr)
+			struct nlattr **tca, unsigned long base, bool ovr,
+			struct netlink_ext_ack *extack)
 {
 	struct fw_head *head = rtnl_dereference(tp->root);
 	u32 mask;
 	int err;
 
-	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &f->exts, ovr);
+	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &f->exts, ovr,
+				extack);
 	if (err < 0)
 		return err;
 
@@ -296,7 +298,7 @@ static int fw_change(struct net *net, struct sk_buff *in_skb,
 			return err;
 		}
 
-		err = fw_set_parms(net, tp, fnew, tb, tca, base, ovr);
+		err = fw_set_parms(net, tp, fnew, tb, tca, base, ovr, extack);
 		if (err < 0) {
 			tcf_exts_destroy(&fnew->exts);
 			kfree(fnew);
@@ -345,7 +347,7 @@ static int fw_change(struct net *net, struct sk_buff *in_skb,
 	f->id = handle;
 	f->tp = tp;
 
-	err = fw_set_parms(net, tp, f, tb, tca, base, ovr);
+	err = fw_set_parms(net, tp, f, tb, tca, base, ovr, extack);
 	if (err < 0)
 		goto errout;
 
diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index 4cfd293dbbd0..cd15f05d170e 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -141,11 +141,12 @@ static const struct nla_policy mall_policy[TCA_MATCHALL_MAX + 1] = {
 static int mall_set_parms(struct net *net, struct tcf_proto *tp,
 			  struct cls_mall_head *head,
 			  unsigned long base, struct nlattr **tb,
-			  struct nlattr *est, bool ovr)
+			  struct nlattr *est, bool ovr,
+			  struct netlink_ext_ack *extack)
 {
 	int err;
 
-	err = tcf_exts_validate(net, tp, tb, est, &head->exts, ovr);
+	err = tcf_exts_validate(net, tp, tb, est, &head->exts, ovr, extack);
 	if (err < 0)
 		return err;
 
@@ -197,7 +198,8 @@ static int mall_change(struct net *net, struct sk_buff *in_skb,
 	new->handle = handle;
 	new->flags = flags;
 
-	err = mall_set_parms(net, tp, new, base, tb, tca[TCA_RATE], ovr);
+	err = mall_set_parms(net, tp, new, base, tb, tca[TCA_RATE], ovr,
+			     extack);
 	if (err)
 		goto err_set_parms;
 
diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
index bf305db65de0..d5a5e0bff136 100644
--- a/net/sched/cls_route.c
+++ b/net/sched/cls_route.c
@@ -389,7 +389,7 @@ static int route4_set_parms(struct net *net, struct tcf_proto *tp,
 			    unsigned long base, struct route4_filter *f,
 			    u32 handle, struct route4_head *head,
 			    struct nlattr **tb, struct nlattr *est, int new,
-			    bool ovr)
+			    bool ovr, struct netlink_ext_ack *extack)
 {
 	u32 id = 0, to = 0, nhandle = 0x8000;
 	struct route4_filter *fp;
@@ -397,7 +397,7 @@ static int route4_set_parms(struct net *net, struct tcf_proto *tp,
 	struct route4_bucket *b;
 	int err;
 
-	err = tcf_exts_validate(net, tp, tb, est, &f->exts, ovr);
+	err = tcf_exts_validate(net, tp, tb, est, &f->exts, ovr, extack);
 	if (err < 0)
 		return err;
 
@@ -516,7 +516,7 @@ static int route4_change(struct net *net, struct sk_buff *in_skb,
 	}
 
 	err = route4_set_parms(net, tp, base, f, handle, head, tb,
-			       tca[TCA_RATE], new, ovr);
+			       tca[TCA_RATE], new, ovr, extack);
 	if (err < 0)
 		goto errout;
 
diff --git a/net/sched/cls_rsvp.h b/net/sched/cls_rsvp.h
index d1f67529c01d..c27d23694002 100644
--- a/net/sched/cls_rsvp.h
+++ b/net/sched/cls_rsvp.h
@@ -511,7 +511,7 @@ static int rsvp_change(struct net *net, struct sk_buff *in_skb,
 	err = tcf_exts_init(&e, TCA_RSVP_ACT, TCA_RSVP_POLICE);
 	if (err < 0)
 		return err;
-	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &e, ovr);
+	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &e, ovr, extack);
 	if (err < 0)
 		goto errout2;
 
diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index 0ec84cf2d6b7..9d6621caa92f 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -322,7 +322,7 @@ static int
 tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 		  u32 handle, struct tcindex_data *p,
 		  struct tcindex_filter_result *r, struct nlattr **tb,
-		  struct nlattr *est, bool ovr)
+		  struct nlattr *est, bool ovr, struct netlink_ext_ack *extack)
 {
 	struct tcindex_filter_result new_filter_result, *old_r = r;
 	struct tcindex_filter_result cr;
@@ -334,7 +334,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 	err = tcf_exts_init(&e, TCA_TCINDEX_ACT, TCA_TCINDEX_POLICE);
 	if (err < 0)
 		return err;
-	err = tcf_exts_validate(net, tp, tb, est, &e, ovr);
+	err = tcf_exts_validate(net, tp, tb, est, &e, ovr, extack);
 	if (err < 0)
 		goto errout;
 
@@ -541,7 +541,7 @@ tcindex_change(struct net *net, struct sk_buff *in_skb,
 		return err;
 
 	return tcindex_set_parms(net, tp, base, handle, p, r, tb,
-				 tca[TCA_RATE], ovr);
+				 tca[TCA_RATE], ovr, extack);
 }
 
 static void tcindex_walk(struct tcf_proto *tp, struct tcf_walker *walker)
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 25a014499afe..6b25bc741daf 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -764,11 +764,12 @@ static const struct nla_policy u32_policy[TCA_U32_MAX + 1] = {
 static int u32_set_parms(struct net *net, struct tcf_proto *tp,
 			 unsigned long base, struct tc_u_hnode *ht,
 			 struct tc_u_knode *n, struct nlattr **tb,
-			 struct nlattr *est, bool ovr)
+			 struct nlattr *est, bool ovr,
+			 struct netlink_ext_ack *extack)
 {
 	int err;
 
-	err = tcf_exts_validate(net, tp, tb, est, &n->exts, ovr);
+	err = tcf_exts_validate(net, tp, tb, est, &n->exts, ovr, extack);
 	if (err < 0)
 		return err;
 
@@ -936,7 +937,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 
 		err = u32_set_parms(net, tp, base,
 				    rtnl_dereference(n->ht_up), new, tb,
-				    tca[TCA_RATE], ovr);
+				    tca[TCA_RATE], ovr, extack);
 
 		if (err) {
 			u32_destroy_key(tp, new, false);
@@ -1083,7 +1084,8 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 	}
 #endif
 
-	err = u32_set_parms(net, tp, base, ht, n, tb, tca[TCA_RATE], ovr);
+	err = u32_set_parms(net, tp, base, ht, n, tb, tca[TCA_RATE], ovr,
+			    extack);
 	if (err == 0) {
 		struct tc_u_knode __rcu **ins;
 		struct tc_u_knode *pins;
-- 
2.11.0

[PATCH net-next 5/8] net: sched: cls: add extack support for delete callback

[Alexander Aring]

This patch adds extack support for classifier delete callback api. This
prepares to handle extack support inside each specific classifier
implementation.

Cc: David Ahern <dsahern@gmail.com>
Signed-off-by: Alexander Aring <aring@mojatatu.com>
---
 include/net/sch_generic.h | 3 ++-
 net/sched/cls_api.c       | 2 +-
 net/sched/cls_basic.c     | 3 ++-
 net/sched/cls_bpf.c       | 3 ++-
 net/sched/cls_cgroup.c    | 3 ++-
 net/sched/cls_flow.c      | 3 ++-
 net/sched/cls_flower.c    | 3 ++-
 net/sched/cls_fw.c        | 3 ++-
 net/sched/cls_matchall.c  | 3 ++-
 net/sched/cls_route.c     | 3 ++-
 net/sched/cls_rsvp.h      | 3 ++-
 net/sched/cls_tcindex.c   | 5 +++--
 net/sched/cls_u32.c       | 3 ++-
 13 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index f999ee6bac2e..01227fcaa09a 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -235,7 +235,8 @@ struct tcf_proto_ops {
 					void **, bool,
 					struct netlink_ext_ack *);
 	int			(*delete)(struct tcf_proto *tp, void *arg,
-					  bool *last);
+					  bool *last,
+					  struct netlink_ext_ack *);
 	void			(*walk)(struct tcf_proto*, struct tcf_walker *arg);
 	void			(*bind_class)(void *, u32, unsigned long);
 
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 011fe66c82b5..6f9fe99ebe2b 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -682,7 +682,7 @@ static int tfilter_del_notify(struct net *net, struct sk_buff *oskb,
 		return -EINVAL;
 	}
 
-	err = tp->ops->delete(tp, fh, last);
+	err = tp->ops->delete(tp, fh, last, extack);
 	if (err) {
 		kfree_skb(skb);
 		return err;
diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
index b7bcf67641bf..6088be65d167 100644
--- a/net/sched/cls_basic.c
+++ b/net/sched/cls_basic.c
@@ -130,7 +130,8 @@ static void basic_destroy(struct tcf_proto *tp)
 	kfree_rcu(head, rcu);
 }
 
-static int basic_delete(struct tcf_proto *tp, void *arg, bool *last)
+static int basic_delete(struct tcf_proto *tp, void *arg, bool *last,
+			struct netlink_ext_ack *extack)
 {
 	struct basic_head *head = rtnl_dereference(tp->root);
 	struct basic_filter *f = arg;
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 50abd71f99ed..4e30863723d9 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -292,7 +292,8 @@ static void __cls_bpf_delete(struct tcf_proto *tp, struct cls_bpf_prog *prog)
 		__cls_bpf_delete_prog(prog);
 }
 
-static int cls_bpf_delete(struct tcf_proto *tp, void *arg, bool *last)
+static int cls_bpf_delete(struct tcf_proto *tp, void *arg, bool *last,
+			  struct netlink_ext_ack *extack)
 {
 	struct cls_bpf_head *head = rtnl_dereference(tp->root);
 
diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index aaafcf6965f7..1b54fbfca414 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -156,7 +156,8 @@ static void cls_cgroup_destroy(struct tcf_proto *tp)
 	}
 }
 
-static int cls_cgroup_delete(struct tcf_proto *tp, void *arg, bool *last)
+static int cls_cgroup_delete(struct tcf_proto *tp, void *arg, bool *last,
+			     struct netlink_ext_ack *extack)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index 5bc702f66bb5..c37c0415c40c 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -575,7 +575,8 @@ static int flow_change(struct net *net, struct sk_buff *in_skb,
 	return err;
 }
 
-static int flow_delete(struct tcf_proto *tp, void *arg, bool *last)
+static int flow_delete(struct tcf_proto *tp, void *arg, bool *last,
+		       struct netlink_ext_ack *extack)
 {
 	struct flow_head *head = rtnl_dereference(tp->root);
 	struct flow_filter *f = arg;
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index b9664e1df8f0..a3ad5d24b0b9 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -984,7 +984,8 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
 	return err;
 }
 
-static int fl_delete(struct tcf_proto *tp, void *arg, bool *last)
+static int fl_delete(struct tcf_proto *tp, void *arg, bool *last,
+		     struct netlink_ext_ack *extack)
 {
 	struct cls_fl_head *head = rtnl_dereference(tp->root);
 	struct cls_fl_filter *f = arg;
diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
index 72a924a38753..bd21ed83eb07 100644
--- a/net/sched/cls_fw.c
+++ b/net/sched/cls_fw.c
@@ -172,7 +172,8 @@ static void fw_destroy(struct tcf_proto *tp)
 	kfree_rcu(head, rcu);
 }
 
-static int fw_delete(struct tcf_proto *tp, void *arg, bool *last)
+static int fw_delete(struct tcf_proto *tp, void *arg, bool *last,
+		     struct netlink_ext_ack *extack)
 {
 	struct fw_head *head = rtnl_dereference(tp->root);
 	struct fw_filter *f = arg;
diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index cd15f05d170e..375b6252f29b 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -224,7 +224,8 @@ static int mall_change(struct net *net, struct sk_buff *in_skb,
 	return err;
 }
 
-static int mall_delete(struct tcf_proto *tp, void *arg, bool *last)
+static int mall_delete(struct tcf_proto *tp, void *arg, bool *last,
+		       struct netlink_ext_ack *extack)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
index d5a5e0bff136..a7836eb613c0 100644
--- a/net/sched/cls_route.c
+++ b/net/sched/cls_route.c
@@ -316,7 +316,8 @@ static void route4_destroy(struct tcf_proto *tp)
 	kfree_rcu(head, rcu);
 }
 
-static int route4_delete(struct tcf_proto *tp, void *arg, bool *last)
+static int route4_delete(struct tcf_proto *tp, void *arg, bool *last,
+			 struct netlink_ext_ack *extack)
 {
 	struct route4_head *head = rtnl_dereference(tp->root);
 	struct route4_filter *f = arg;
diff --git a/net/sched/cls_rsvp.h b/net/sched/cls_rsvp.h
index c27d23694002..5cc0df690cff 100644
--- a/net/sched/cls_rsvp.h
+++ b/net/sched/cls_rsvp.h
@@ -350,7 +350,8 @@ static void rsvp_destroy(struct tcf_proto *tp)
 	kfree_rcu(data, rcu);
 }
 
-static int rsvp_delete(struct tcf_proto *tp, void *arg, bool *last)
+static int rsvp_delete(struct tcf_proto *tp, void *arg, bool *last,
+		       struct netlink_ext_ack *extack)
 {
 	struct rsvp_head *head = rtnl_dereference(tp->root);
 	struct rsvp_filter *nfp, *f = arg;
diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index 9d6621caa92f..01a163e0b6aa 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -193,7 +193,8 @@ static void tcindex_destroy_fexts(struct rcu_head *head)
 	tcf_queue_work(&f->work);
 }
 
-static int tcindex_delete(struct tcf_proto *tp, void *arg, bool *last)
+static int tcindex_delete(struct tcf_proto *tp, void *arg, bool *last,
+			  struct netlink_ext_ack *extack)
 {
 	struct tcindex_data *p = rtnl_dereference(tp->root);
 	struct tcindex_filter_result *r = arg;
@@ -246,7 +247,7 @@ static int tcindex_destroy_element(struct tcf_proto *tp,
 {
 	bool last;
 
-	return tcindex_delete(tp, arg, &last);
+	return tcindex_delete(tp, arg, &last, NULL);
 }
 
 static void __tcindex_destroy(struct rcu_head *head)
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 6b25bc741daf..b2c83490cc73 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -671,7 +671,8 @@ static void u32_destroy(struct tcf_proto *tp)
 	tp->data = NULL;
 }
 
-static int u32_delete(struct tcf_proto *tp, void *arg, bool *last)
+static int u32_delete(struct tcf_proto *tp, void *arg, bool *last,
+		      struct netlink_ext_ack *extack)
 {
 	struct tc_u_hnode *ht = arg;
 	struct tc_u_hnode *root_ht = rtnl_dereference(tp->root);
-- 
2.11.0

[PATCH net-next 6/8] net: sched: cls: add extack support for tcf_change_indev

[Alexander Aring]

This patch adds extack handling for the tcf_change_indev function which
is common used by TC classifier implementations.

Cc: David Ahern <dsahern@gmail.com>
Signed-off-by: Alexander Aring <aring@mojatatu.com>
---
 include/net/pkt_cls.h  | 7 +++++--
 net/sched/cls_flower.c | 7 ++++---
 net/sched/cls_fw.c     | 2 +-
 net/sched/cls_u32.c    | 2 +-
 4 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 0286d42d08ef..03f62be4e57e 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -549,13 +549,16 @@ static inline int tcf_valid_offset(const struct sk_buff *skb,
 #include <net/net_namespace.h>
 
 static inline int
-tcf_change_indev(struct net *net, struct nlattr *indev_tlv)
+tcf_change_indev(struct net *net, struct nlattr *indev_tlv,
+		 struct netlink_ext_ack *extack)
 {
 	char indev[IFNAMSIZ];
 	struct net_device *dev;
 
-	if (nla_strlcpy(indev, indev_tlv, IFNAMSIZ) >= IFNAMSIZ)
+	if (nla_strlcpy(indev, indev_tlv, IFNAMSIZ) >= IFNAMSIZ) {
+		NL_SET_ERR_MSG(extack, "Interface name too long");
 		return -EINVAL;
+	}
 	dev = __dev_get_by_name(net, indev);
 	if (!dev)
 		return -ENODEV;
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index a3ad5d24b0b9..9eb552257890 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -525,13 +525,14 @@ static void fl_set_key_ip(struct nlattr **tb,
 }
 
 static int fl_set_key(struct net *net, struct nlattr **tb,
-		      struct fl_flow_key *key, struct fl_flow_key *mask)
+		      struct fl_flow_key *key, struct fl_flow_key *mask,
+		      struct netlink_ext_ack *extack)
 {
 	__be16 ethertype;
 	int ret = 0;
 #ifdef CONFIG_NET_CLS_IND
 	if (tb[TCA_FLOWER_INDEV]) {
-		int err = tcf_change_indev(net, tb[TCA_FLOWER_INDEV]);
+		int err = tcf_change_indev(net, tb[TCA_FLOWER_INDEV], extack);
 		if (err < 0)
 			return err;
 		key->indev_ifindex = err;
@@ -840,7 +841,7 @@ static int fl_set_parms(struct net *net, struct tcf_proto *tp,
 		tcf_bind_filter(tp, &f->res, base);
 	}
 
-	err = fl_set_key(net, tb, &f->key, &mask->key);
+	err = fl_set_key(net, tb, &f->key, &mask->key, extack);
 	if (err)
 		return err;
 
diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
index bd21ed83eb07..94d159a8869a 100644
--- a/net/sched/cls_fw.c
+++ b/net/sched/cls_fw.c
@@ -239,7 +239,7 @@ static int fw_set_parms(struct net *net, struct tcf_proto *tp,
 #ifdef CONFIG_NET_CLS_IND
 	if (tb[TCA_FW_INDEV]) {
 		int ret;
-		ret = tcf_change_indev(net, tb[TCA_FW_INDEV]);
+		ret = tcf_change_indev(net, tb[TCA_FW_INDEV], extack);
 		if (ret < 0)
 			return ret;
 		f->ifindex = ret;
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index b2c83490cc73..78d0a320e7fa 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -803,7 +803,7 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp,
 #ifdef CONFIG_NET_CLS_IND
 	if (tb[TCA_U32_INDEV]) {
 		int ret;
-		ret = tcf_change_indev(net, tb[TCA_U32_INDEV]);
+		ret = tcf_change_indev(net, tb[TCA_U32_INDEV], extack);
 		if (ret < 0)
 			return -EINVAL;
 		n->ifindex = ret;
-- 
2.11.0

[PATCH net-next 7/8] net: sched: cls: add extack support for tc_setup_cb_call

[Alexander Aring]

This patch adds extack handling for the tc_setup_cb_call function which
is common used by TC classifier implementations.

Cc: David Ahern <dsahern@gmail.com>
Signed-off-by: Alexander Aring <aring@mojatatu.com>
---
 include/net/pkt_cls.h    |  3 ++-
 net/sched/cls_api.c      | 11 ++++++++---
 net/sched/cls_bpf.c      | 19 +++++++++++--------
 net/sched/cls_flower.c   | 11 ++++++-----
 net/sched/cls_matchall.c | 11 +++++++----
 net/sched/cls_u32.c      | 20 +++++++++++---------
 6 files changed, 45 insertions(+), 30 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 03f62be4e57e..991f5011510b 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -577,7 +577,8 @@ tcf_match_indev(struct sk_buff *skb, int ifindex)
 #endif /* CONFIG_NET_CLS_IND */
 
 int tc_setup_cb_call(struct tcf_block *block, struct tcf_exts *exts,
-		     enum tc_setup_type type, void *type_data, bool err_stop);
+		     enum tc_setup_type type, void *type_data, bool err_stop,
+		     struct netlink_ext_ack *extack);
 
 enum tc_block_command {
 	TC_BLOCK_BIND,
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 6f9fe99ebe2b..c4484a8341cd 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -1264,21 +1264,26 @@ static int tc_exts_setup_cb_egdev_call(struct tcf_exts *exts,
 }
 
 int tc_setup_cb_call(struct tcf_block *block, struct tcf_exts *exts,
-		     enum tc_setup_type type, void *type_data, bool err_stop)
+		     enum tc_setup_type type, void *type_data, bool err_stop,
+		     struct netlink_ext_ack *extack)
 {
 	int ok_count;
 	int ret;
 
 	ret = tcf_block_cb_call(block, type, type_data, err_stop);
-	if (ret < 0)
+	if (ret < 0) {
+		NL_SET_ERR_MSG(extack, "Failed to inialize tcf block");
 		return ret;
+	}
 	ok_count = ret;
 
 	if (!exts)
 		return ok_count;
 	ret = tc_exts_setup_cb_egdev_call(exts, type, type_data, err_stop);
-	if (ret < 0)
+	if (ret < 0) {
+		NL_SET_ERR_MSG(extack, "Failed to inialize tcf block extensions");
 		return ret;
+	}
 	ok_count += ret;
 
 	return ok_count;
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 4e30863723d9..79a07ae405f7 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -147,7 +147,8 @@ static bool cls_bpf_is_ebpf(const struct cls_bpf_prog *prog)
 }
 
 static int cls_bpf_offload_cmd(struct tcf_proto *tp, struct cls_bpf_prog *prog,
-			       struct cls_bpf_prog *oldprog)
+			       struct cls_bpf_prog *oldprog,
+			       struct netlink_ext_ack *extack)
 {
 	struct tcf_block *block = tp->chain->block;
 	struct tc_cls_bpf_offload cls_bpf = {};
@@ -167,10 +168,11 @@ static int cls_bpf_offload_cmd(struct tcf_proto *tp, struct cls_bpf_prog *prog,
 	cls_bpf.exts_integrated = obj->exts_integrated;
 	cls_bpf.gen_flags = obj->gen_flags;
 
-	err = tc_setup_cb_call(block, NULL, TC_SETUP_CLSBPF, &cls_bpf, skip_sw);
+	err = tc_setup_cb_call(block, NULL, TC_SETUP_CLSBPF, &cls_bpf, skip_sw,
+			       extack);
 	if (prog) {
 		if (err < 0) {
-			cls_bpf_offload_cmd(tp, oldprog, prog);
+			cls_bpf_offload_cmd(tp, oldprog, prog, NULL);
 			return err;
 		} else if (err > 0) {
 			prog->gen_flags |= TCA_CLS_FLAGS_IN_HW;
@@ -184,7 +186,8 @@ static int cls_bpf_offload_cmd(struct tcf_proto *tp, struct cls_bpf_prog *prog,
 }
 
 static int cls_bpf_offload(struct tcf_proto *tp, struct cls_bpf_prog *prog,
-			   struct cls_bpf_prog *oldprog)
+			   struct cls_bpf_prog *oldprog,
+			   struct netlink_ext_ack *extack)
 {
 	if (prog && oldprog && prog->gen_flags != oldprog->gen_flags)
 		return -EINVAL;
@@ -196,7 +199,7 @@ static int cls_bpf_offload(struct tcf_proto *tp, struct cls_bpf_prog *prog,
 	if (!prog && !oldprog)
 		return 0;
 
-	return cls_bpf_offload_cmd(tp, prog, oldprog);
+	return cls_bpf_offload_cmd(tp, prog, oldprog, extack);
 }
 
 static void cls_bpf_stop_offload(struct tcf_proto *tp,
@@ -204,7 +207,7 @@ static void cls_bpf_stop_offload(struct tcf_proto *tp,
 {
 	int err;
 
-	err = cls_bpf_offload_cmd(tp, NULL, prog);
+	err = cls_bpf_offload_cmd(tp, NULL, prog, NULL);
 	if (err)
 		pr_err("Stopping hardware offload failed: %d\n", err);
 }
@@ -223,7 +226,7 @@ static void cls_bpf_offload_update_stats(struct tcf_proto *tp,
 	cls_bpf.exts_integrated = prog->exts_integrated;
 	cls_bpf.gen_flags = prog->gen_flags;
 
-	tc_setup_cb_call(block, NULL, TC_SETUP_CLSBPF, &cls_bpf, false);
+	tc_setup_cb_call(block, NULL, TC_SETUP_CLSBPF, &cls_bpf, false, NULL);
 }
 
 static int cls_bpf_init(struct tcf_proto *tp)
@@ -504,7 +507,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
 	if (ret < 0)
 		goto errout_idr;
 
-	ret = cls_bpf_offload(tp, prog, oldprog);
+	ret = cls_bpf_offload(tp, prog, oldprog, extack);
 	if (ret)
 		goto errout_parms;
 
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 9eb552257890..a06a1156b07c 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -228,13 +228,14 @@ static void fl_hw_destroy_filter(struct tcf_proto *tp, struct cls_fl_filter *f)
 	cls_flower.cookie = (unsigned long) f;
 
 	tc_setup_cb_call(block, &f->exts, TC_SETUP_CLSFLOWER,
-			 &cls_flower, false);
+			 &cls_flower, false, NULL);
 }
 
 static int fl_hw_replace_filter(struct tcf_proto *tp,
 				struct flow_dissector *dissector,
 				struct fl_flow_key *mask,
-				struct cls_fl_filter *f)
+				struct cls_fl_filter *f,
+				struct netlink_ext_ack *extack)
 {
 	struct tc_cls_flower_offload cls_flower = {};
 	struct tcf_block *block = tp->chain->block;
@@ -251,7 +252,7 @@ static int fl_hw_replace_filter(struct tcf_proto *tp,
 	cls_flower.classid = f->res.classid;
 
 	err = tc_setup_cb_call(block, &f->exts, TC_SETUP_CLSFLOWER,
-			       &cls_flower, skip_sw);
+			       &cls_flower, skip_sw, extack);
 	if (err < 0) {
 		fl_hw_destroy_filter(tp, f);
 		return err;
@@ -277,7 +278,7 @@ static void fl_hw_update_stats(struct tcf_proto *tp, struct cls_fl_filter *f)
 	cls_flower.classid = f->res.classid;
 
 	tc_setup_cb_call(block, &f->exts, TC_SETUP_CLSFLOWER,
-			 &cls_flower, false);
+			 &cls_flower, false, NULL);
 }
 
 static void __fl_delete(struct tcf_proto *tp, struct cls_fl_filter *f)
@@ -942,7 +943,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
 		err = fl_hw_replace_filter(tp,
 					   &head->dissector,
 					   &mask.key,
-					   fnew);
+					   fnew, extack);
 		if (err)
 			goto errout_idr;
 	}
diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index 375b6252f29b..2b3e407cc2c7 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -80,12 +80,14 @@ static void mall_destroy_hw_filter(struct tcf_proto *tp,
 	cls_mall.command = TC_CLSMATCHALL_DESTROY;
 	cls_mall.cookie = cookie;
 
-	tc_setup_cb_call(block, NULL, TC_SETUP_CLSMATCHALL, &cls_mall, false);
+	tc_setup_cb_call(block, NULL, TC_SETUP_CLSMATCHALL, &cls_mall, false,
+			 NULL);
 }
 
 static int mall_replace_hw_filter(struct tcf_proto *tp,
 				  struct cls_mall_head *head,
-				  unsigned long cookie)
+				  unsigned long cookie,
+				  struct netlink_ext_ack *extack)
 {
 	struct tc_cls_matchall_offload cls_mall = {};
 	struct tcf_block *block = tp->chain->block;
@@ -98,7 +100,7 @@ static int mall_replace_hw_filter(struct tcf_proto *tp,
 	cls_mall.cookie = cookie;
 
 	err = tc_setup_cb_call(block, NULL, TC_SETUP_CLSMATCHALL,
-			       &cls_mall, skip_sw);
+			       &cls_mall, skip_sw, extack);
 	if (err < 0) {
 		mall_destroy_hw_filter(tp, head, cookie);
 		return err;
@@ -204,7 +206,8 @@ static int mall_change(struct net *net, struct sk_buff *in_skb,
 		goto err_set_parms;
 
 	if (!tc_skip_hw(new->flags)) {
-		err = mall_replace_hw_filter(tp, new, (unsigned long)new);
+		err = mall_replace_hw_filter(tp, new, (unsigned long)new,
+					     extack);
 		if (err)
 			goto err_replace_hw_filter;
 	}
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 78d0a320e7fa..03dc08ab3cf1 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -497,11 +497,11 @@ static void u32_clear_hw_hnode(struct tcf_proto *tp, struct tc_u_hnode *h)
 	cls_u32.hnode.handle = h->handle;
 	cls_u32.hnode.prio = h->prio;
 
-	tc_setup_cb_call(block, NULL, TC_SETUP_CLSU32, &cls_u32, false);
+	tc_setup_cb_call(block, NULL, TC_SETUP_CLSU32, &cls_u32, false, NULL);
 }
 
 static int u32_replace_hw_hnode(struct tcf_proto *tp, struct tc_u_hnode *h,
-				u32 flags)
+				u32 flags, struct netlink_ext_ack *extack)
 {
 	struct tcf_block *block = tp->chain->block;
 	struct tc_cls_u32_offload cls_u32 = {};
@@ -515,7 +515,8 @@ static int u32_replace_hw_hnode(struct tcf_proto *tp, struct tc_u_hnode *h,
 	cls_u32.hnode.handle = h->handle;
 	cls_u32.hnode.prio = h->prio;
 
-	err = tc_setup_cb_call(block, NULL, TC_SETUP_CLSU32, &cls_u32, skip_sw);
+	err = tc_setup_cb_call(block, NULL, TC_SETUP_CLSU32, &cls_u32, skip_sw,
+			       extack);
 	if (err < 0) {
 		u32_clear_hw_hnode(tp, h);
 		return err;
@@ -538,11 +539,11 @@ static void u32_remove_hw_knode(struct tcf_proto *tp, u32 handle)
 	cls_u32.command = TC_CLSU32_DELETE_KNODE;
 	cls_u32.knode.handle = handle;
 
-	tc_setup_cb_call(block, NULL, TC_SETUP_CLSU32, &cls_u32, false);
+	tc_setup_cb_call(block, NULL, TC_SETUP_CLSU32, &cls_u32, false, NULL);
 }
 
 static int u32_replace_hw_knode(struct tcf_proto *tp, struct tc_u_knode *n,
-				u32 flags)
+				u32 flags, struct netlink_ext_ack *extack)
 {
 	struct tcf_block *block = tp->chain->block;
 	struct tc_cls_u32_offload cls_u32 = {};
@@ -565,7 +566,8 @@ static int u32_replace_hw_knode(struct tcf_proto *tp, struct tc_u_knode *n,
 	if (n->ht_down)
 		cls_u32.knode.link_handle = n->ht_down->handle;
 
-	err = tc_setup_cb_call(block, NULL, TC_SETUP_CLSU32, &cls_u32, skip_sw);
+	err = tc_setup_cb_call(block, NULL, TC_SETUP_CLSU32, &cls_u32, skip_sw,
+			       extack);
 	if (err < 0) {
 		u32_remove_hw_knode(tp, n->handle);
 		return err;
@@ -945,7 +947,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 			return err;
 		}
 
-		err = u32_replace_hw_knode(tp, new, flags);
+		err = u32_replace_hw_knode(tp, new, flags, extack);
 		if (err) {
 			u32_destroy_key(tp, new, false);
 			return err;
@@ -992,7 +994,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 		ht->prio = tp->prio;
 		idr_init(&ht->handle_idr);
 
-		err = u32_replace_hw_hnode(tp, ht, flags);
+		err = u32_replace_hw_hnode(tp, ht, flags, extack);
 		if (err) {
 			idr_remove_ext(&tp_c->handle_idr, handle);
 			kfree(ht);
@@ -1091,7 +1093,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 		struct tc_u_knode __rcu **ins;
 		struct tc_u_knode *pins;
 
-		err = u32_replace_hw_knode(tp, n, flags);
+		err = u32_replace_hw_knode(tp, n, flags, extack);
 		if (err)
 			goto errhw;
 
-- 
2.11.0

Re: [PATCH net-next 7/8] net: sched: cls: add extack support for tc_setup_cb_call

[Cong Wang]

On Tue, Jan 16, 2018 at 9:20 AM, Alexander Aring <aring@mojatatu.com> wrote:
>  int tc_setup_cb_call(struct tcf_block *block, struct tcf_exts *exts,
> -                    enum tc_setup_type type, void *type_data, bool err_stop)
> +                    enum tc_setup_type type, void *type_data, bool err_stop,
> +                    struct netlink_ext_ack *extack)
>  {
>         int ok_count;
>         int ret;
>
>         ret = tcf_block_cb_call(block, type, type_data, err_stop);
> -       if (ret < 0)
> +       if (ret < 0) {
> +               NL_SET_ERR_MSG(extack, "Failed to inialize tcf block");


s/inialize/initialize/

>                 return ret;
> +       }
>         ok_count = ret;
>
>         if (!exts)
>                 return ok_count;
>         ret = tc_exts_setup_cb_egdev_call(exts, type, type_data, err_stop);
> -       if (ret < 0)
> +       if (ret < 0) {
> +               NL_SET_ERR_MSG(extack, "Failed to inialize tcf block extensions");

Ditto.

[PATCH net-next 8/8] net: sched: cls_u32: add extack support

[Alexander Aring]

This patch adds extack support for the u32 classifier as example for
delete and init callback.

Cc: David Ahern <dsahern@gmail.com>
Signed-off-by: Alexander Aring <aring@mojatatu.com>
---
 net/sched/cls_u32.c | 66 ++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 50 insertions(+), 16 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 03dc08ab3cf1..69c2fbafd0a3 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -524,8 +524,10 @@ static int u32_replace_hw_hnode(struct tcf_proto *tp, struct tc_u_hnode *h,
 		offloaded = true;
 	}
 
-	if (skip_sw && !offloaded)
+	if (skip_sw && !offloaded) {
+		NL_SET_ERR_MSG(extack, "Failed to offload filter requested with skip sw");
 		return -EINVAL;
+	}
 
 	return 0;
 }
@@ -575,8 +577,10 @@ static int u32_replace_hw_knode(struct tcf_proto *tp, struct tc_u_knode *n,
 		n->flags |= TCA_CLS_FLAGS_IN_HW;
 	}
 
-	if (skip_sw && !(n->flags & TCA_CLS_FLAGS_IN_HW))
+	if (skip_sw && !(n->flags & TCA_CLS_FLAGS_IN_HW)) {
+		NL_SET_ERR_MSG(extack, "Failed to offload filter requested with skip sw");
 		return -EINVAL;
+	}
 
 	return 0;
 }
@@ -690,13 +694,16 @@ static int u32_delete(struct tcf_proto *tp, void *arg, bool *last,
 		goto out;
 	}
 
-	if (root_ht == ht)
+	if (root_ht == ht) {
+		NL_SET_ERR_MSG(extack, "Not allowd to delete root node");
 		return -EINVAL;
+	}
 
 	if (ht->refcnt == 1) {
 		ht->refcnt--;
 		u32_destroy_hnode(tp, ht);
 	} else {
+		NL_SET_ERR_MSG(extack, "Can not delete in-use filter");
 		return -EBUSY;
 	}
 
@@ -780,14 +787,18 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp,
 		u32 handle = nla_get_u32(tb[TCA_U32_LINK]);
 		struct tc_u_hnode *ht_down = NULL, *ht_old;
 
-		if (TC_U32_KEY(handle))
+		if (TC_U32_KEY(handle)) {
+			NL_SET_ERR_MSG(extack, "u32 Link handle must be a hash table");
 			return -EINVAL;
+		}
 
 		if (handle) {
 			ht_down = u32_lookup_ht(ht->tp_c, handle);
 
-			if (!ht_down)
+			if (!ht_down) {
+				NL_SET_ERR_MSG(extack, "Link hash table not found");
 				return -EINVAL;
+			}
 			ht_down->refcnt++;
 		}
 
@@ -911,28 +922,40 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 	size_t size;
 #endif
 
-	if (!opt)
-		return handle ? -EINVAL : 0;
+	if (!opt) {
+		if (handle) {
+			NL_SET_ERR_MSG(extack, "Filter handle requires options");
+			return -EINVAL;
+		} else {
+			return 0;
+		}
+	}
 
-	err = nla_parse_nested(tb, TCA_U32_MAX, opt, u32_policy, NULL);
+	err = nla_parse_nested(tb, TCA_U32_MAX, opt, u32_policy, extack);
 	if (err < 0)
 		return err;
 
 	if (tb[TCA_U32_FLAGS]) {
 		flags = nla_get_u32(tb[TCA_U32_FLAGS]);
-		if (!tc_flags_valid(flags))
+		if (!tc_flags_valid(flags)) {
+			NL_SET_ERR_MSG(extack, "Invalid filter flags");
 			return -EINVAL;
+		}
 	}
 
 	n = *arg;
 	if (n) {
 		struct tc_u_knode *new;
 
-		if (TC_U32_KEY(n->handle) == 0)
+		if (TC_U32_KEY(n->handle) == 0) {
+			NL_SET_ERR_MSG(extack, "Key node id cannot be zero");
 			return -EINVAL;
+		}
 
-		if (n->flags != flags)
+		if (n->flags != flags) {
+			NL_SET_ERR_MSG(extack, "Key node flags do not match passed flags");
 			return -EINVAL;
+		}
 
 		new = u32_init_knode(tp, n);
 		if (!new)
@@ -966,10 +989,14 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 	if (tb[TCA_U32_DIVISOR]) {
 		unsigned int divisor = nla_get_u32(tb[TCA_U32_DIVISOR]);
 
-		if (--divisor > 0x100)
+		if (--divisor > 0x100) {
+			NL_SET_ERR_MSG(extack, "Exceeded maximum 256 hash buckets");
 			return -EINVAL;
-		if (TC_U32_KEY(handle))
+		}
+		if (TC_U32_KEY(handle)) {
+			NL_SET_ERR_MSG(extack, "Divisor can only be used on a hash table");
 			return -EINVAL;
+		}
 		ht = kzalloc(sizeof(*ht) + divisor*sizeof(void *), GFP_KERNEL);
 		if (ht == NULL)
 			return -ENOBUFS;
@@ -1015,20 +1042,26 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 			htid = ht->handle;
 		} else {
 			ht = u32_lookup_ht(tp->data, TC_U32_HTID(htid));
-			if (!ht)
+			if (!ht) {
+				NL_SET_ERR_MSG(extack, "Specified hash table not found");
 				return -EINVAL;
+			}
 		}
 	} else {
 		ht = rtnl_dereference(tp->root);
 		htid = ht->handle;
 	}
 
-	if (ht->divisor < TC_U32_HASH(htid))
+	if (ht->divisor < TC_U32_HASH(htid)) {
+		NL_SET_ERR_MSG(extack, "Specified hash table buckets exceed configured value");
 		return -EINVAL;
+	}
 
 	if (handle) {
-		if (TC_U32_HTID(handle) && TC_U32_HTID(handle ^ htid))
+		if (TC_U32_HTID(handle) && TC_U32_HTID(handle ^ htid)) {
+			NL_SET_ERR_MSG(extack, "Handle specified hash table address mismatch");
 			return -EINVAL;
+		}
 		handle = htid | TC_U32_NODE(handle);
 		err = idr_alloc_ext(&ht->handle_idr, NULL, NULL,
 				    handle, handle + 1,
@@ -1039,6 +1072,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 		handle = gen_new_kid(ht, htid);
 
 	if (tb[TCA_U32_SEL] == NULL) {
+		NL_SET_ERR_MSG(extack, "Selector not specified");
 		err = -EINVAL;
 		goto erridr;
 	}
-- 
2.11.0

Re: [PATCH net-next 8/8] net: sched: cls_u32: add extack support

[Jakub Kicinski]

On Tue, 16 Jan 2018 12:20:27 -0500, Alexander Aring wrote:
> @@ -780,14 +787,18 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp,
>  		u32 handle = nla_get_u32(tb[TCA_U32_LINK]);
>  		struct tc_u_hnode *ht_down = NULL, *ht_old;
>  
> -		if (TC_U32_KEY(handle))
> +		if (TC_U32_KEY(handle)) {
> +			NL_SET_ERR_MSG(extack, "u32 Link handle must be a hash table");
>  			return -EINVAL;
> +		}

Since classifiers are commonly built as modules would it make more
sense to use NL_SET_ERR_MSG_MOD()?

Re: [PATCH net-next 8/8] net: sched: cls_u32: add extack support

[Cong Wang]

On Tue, Jan 16, 2018 at 9:20 AM, Alexander Aring <aring@mojatatu.com> wrote:
> -       if (root_ht == ht)
> +       if (root_ht == ht) {
> +               NL_SET_ERR_MSG(extack, "Not allowd to delete root node");

s/allowd/allowed/

Re: [PATCH net-next 0/8] net: sched: cls: add extack support

[Jakub Kicinski]

On Tue, 16 Jan 2018 12:20:19 -0500, Alexander Aring wrote:
> Hi,
> 
> this patch adds extack support for TC classifier subsystem. The first
> patch fixes some code style issues for this patch series pointed out
> by checkpatch. The other patches until the last one prepares extack
> handling for the TC classifier subsystem and handle generic extack
> errors.
> 
> The last patch is an example for u32 classifier to add extack support
> inside the callbacks delete and change. There exists a init callback as
> well, but most classifier implementation run a kalloc() once to allocate
> something. Not necessary _yet_ to add extack support now.
> 
> I know there are patches around which makes changes to these files.
> I will rebase my stuff on Jiri's patches if they get in before mine.

Ugh, this is going to conflict with our series too :(  (and I CCed you
on ours)

Would it be OK for you to hold off until Jiri's code gets merged and
ours comes down via bpf-next?  That shouldn't take long at all.  The
conflicts between bpf/bpf-next/net-next are really taking their toll 
on us this release cycles, I would really appreciate if we could make
some progress on this relatively simple series at least...

Re: [PATCH net-next 0/8] net: sched: cls: add extack support

[Jamal Hadi Salim]

On 18-01-16 04:46 PM, Jakub Kicinski wrote:
> On Tue, 16 Jan 2018 12:20:19 -0500, Alexander Aring wrote:

[..]

> Ugh, this is going to conflict with our series too :(  (and I CCed you
> on ours)
> 
> Would it be OK for you to hold off until Jiri's code gets merged and
> ours comes down via bpf-next?  That shouldn't take long at all.  The
> conflicts between bpf/bpf-next/net-next are really taking their toll
> on us this release cycles, I would really appreciate if we could make
> some progress on this relatively simple series at least...
> 

I would say precedence should be Jiri's patches, Alex's patches
and then yours:
Alex's patches fix the core (cls_api.c) area with proper extack
for the core and then he has one patch to cover a specific
use case of the u32 classifier extack. Yours is only concerned
with one use case - bpf which depend on the core (that is in Alex's
patches)

cheers,
jamal

Re: [PATCH net-next 0/8] net: sched: cls: add extack support

[Jakub Kicinski]

On Tue, 16 Jan 2018 17:12:57 -0500, Jamal Hadi Salim wrote:
> On 18-01-16 04:46 PM, Jakub Kicinski wrote:
> > On Tue, 16 Jan 2018 12:20:19 -0500, Alexander Aring wrote:  
> 
> [..]
> 
> > Ugh, this is going to conflict with our series too :(  (and I CCed you
> > on ours)
> > 
> > Would it be OK for you to hold off until Jiri's code gets merged and
> > ours comes down via bpf-next?  That shouldn't take long at all.  The
> > conflicts between bpf/bpf-next/net-next are really taking their toll
> > on us this release cycles, I would really appreciate if we could make
> > some progress on this relatively simple series at least...
> >   
> 
> I would say precedence should be Jiri's patches, Alex's patches
> and then yours:
> Alex's patches fix the core (cls_api.c) area with proper extack
> for the core and then he has one patch to cover a specific
> use case of the u32 classifier extack. Yours is only concerned
> with one use case - bpf which depend on the core (that is in Alex's
> patches)

Our patches are concerned with propagating the extack to drivers, 
and nfp (and netdevsim) make use of it.

I'm miffed by the fact that you jumped out with this conflicting series
*after* we posted ours, and we got shot down on white space.

Re: [PATCH net-next 0/8] net: sched: cls: add extack support

[Jamal Hadi Salim]

On 18-01-16 05:41 PM, Jakub Kicinski wrote:
> On Tue, 16 Jan 2018 17:12:57 -0500, Jamal Hadi Salim wrote:
>> On 18-01-16 04:46 PM, Jakub Kicinski wrote:
>>> On Tue, 16 Jan 2018 12:20:19 -0500, Alexander Aring wrote:
>>
>> [..]
>>

>> I would say precedence should be Jiri's patches, Alex's patches
>> and then yours:
>> Alex's patches fix the core (cls_api.c) area with proper extack
>> for the core and then he has one patch to cover a specific
>> use case of the u32 classifier extack. Yours is only concerned
>> with one use case - bpf which depend on the core (that is in Alex's
>> patches)
> 
> Our patches are concerned with propagating the extack to drivers,
> and nfp (and netdevsim) make use of it.
> 
> I'm miffed by the fact that you jumped out with this conflicting series
> *after* we posted ours, and we got shot down on white space.

I totally empathize with the general frustration.
The general rule is we fix the core first then add users (classifiers in
this case). Note:
Alex has a _lot_ of patches that he has been trying to send for the
last little while and this one is certainly not a new set (I actually
had reviewed this set). There are others. And the rule of "fix core
first then add users" has been imposed on him as well.

cheers,
jamal

Re: [PATCH net-next 0/8] net: sched: cls: add extack support

[Daniel Borkmann]

Hey David, and others, [+Alexei]

On 01/17/2018 12:27 AM, Jamal Hadi Salim wrote:
> On 18-01-16 05:41 PM, Jakub Kicinski wrote:
>> On Tue, 16 Jan 2018 17:12:57 -0500, Jamal Hadi Salim wrote:
>>> On 18-01-16 04:46 PM, Jakub Kicinski wrote:
>>>> On Tue, 16 Jan 2018 12:20:19 -0500, Alexander Aring wrote:
>>>
>>> [..]
>>>
>>> I would say precedence should be Jiri's patches, Alex's patches
>>> and then yours:
>>> Alex's patches fix the core (cls_api.c) area with proper extack
>>> for the core and then he has one patch to cover a specific
>>> use case of the u32 classifier extack. Yours is only concerned
>>> with one use case - bpf which depend on the core (that is in Alex's
>>> patches)
>>
>> Our patches are concerned with propagating the extack to drivers,
>> and nfp (and netdevsim) make use of it.
>>
>> I'm miffed by the fact that you jumped out with this conflicting series
>> *after* we posted ours, and we got shot down on white space.

So I've been looking over Quentin's series just now that sits in my
bucket and it looks fine to me, but merge with this one would probably
end up badly for David. Therefore I'm proposing the following that
should hopefully be fine and work out for Alexander and Jakub/Quentin
as a consensus:

I'm getting the current bpf-next stuff as PR out in a few minutes, so
David can pull this in and therefore net-next will also have the
dependency on nfp for Quentin's series. Then, given this one here
needs another respin anyway, I would suggest to combine the missing
patches from Alexander's series, and get it all out in a single patch
series directly for net-next w/o any interdependency hassle.

Thanks,
Daniel

Re: [PATCH net-next 0/8] net: sched: cls: add extack support

[Daniel Borkmann]

On 01/17/2018 01:08 AM, Daniel Borkmann wrote:
> Hey David, and others, [+Alexei]
> 
> On 01/17/2018 12:27 AM, Jamal Hadi Salim wrote:
>> On 18-01-16 05:41 PM, Jakub Kicinski wrote:
>>> On Tue, 16 Jan 2018 17:12:57 -0500, Jamal Hadi Salim wrote:
>>>> On 18-01-16 04:46 PM, Jakub Kicinski wrote:
>>>>> On Tue, 16 Jan 2018 12:20:19 -0500, Alexander Aring wrote:
>>>>
>>>> [..]
>>>>
>>>> I would say precedence should be Jiri's patches, Alex's patches
>>>> and then yours:
>>>> Alex's patches fix the core (cls_api.c) area with proper extack
>>>> for the core and then he has one patch to cover a specific
>>>> use case of the u32 classifier extack. Yours is only concerned
>>>> with one use case - bpf which depend on the core (that is in Alex's
>>>> patches)
>>>
>>> Our patches are concerned with propagating the extack to drivers,
>>> and nfp (and netdevsim) make use of it.
>>>
>>> I'm miffed by the fact that you jumped out with this conflicting series
>>> *after* we posted ours, and we got shot down on white space.
> 
> So I've been looking over Quentin's series just now that sits in my
> bucket and it looks fine to me, but merge with this one would probably
> end up badly for David. Therefore I'm proposing the following that
> should hopefully be fine and work out for Alexander and Jakub/Quentin
> as a consensus:
> 
> I'm getting the current bpf-next stuff as PR out in a few minutes, so
> David can pull this in and therefore net-next will also have the
> dependency on nfp for Quentin's series. Then, given this one here
> needs another respin anyway, I would suggest to combine the missing
> patches from Alexander's series, and get it all out in a single patch
> series directly for net-next w/o any interdependency hassle.

Ok, bpf-next PR with the nfp dependencies is now out, so all this can
make progress here. I've therefore purged Jakub's extack series from
bpf queue, so a combined series can target net-next directly then.

Thanks,
Daniel