diff mbox series

[net-next,1/6] net: sched: sch_api: handle generic qdisc errors

Message ID 20171206160845.6646-2-aring@mojatatu.com
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series net: sched: sch: introduce extack support | expand

Commit Message

Alexander Aring Dec. 6, 2017, 4:08 p.m. UTC 
This patch adds extack support for generic qdisc handling. The extack
will be set deeper to each called function which is not part of netdev
core api. A future patchset will add per-qdisc specific changes.

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

Comments

Jamal Hadi Salim Dec. 6, 2017, 4:52 p.m. UTC | #1 
On 17-12-06 11:08 AM, Alexander Aring wrote:
> This patch adds extack support for generic qdisc handling. The extack
> will be set deeper to each called function which is not part of netdev
> core api. A future patchset will add per-qdisc specific changes.
> 
> Cc: David Ahern <dsahern@gmail.com>
> Signed-off-by: Alexander Aring <aring@mojatatu.com>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal
David Ahern Dec. 7, 2017, 5:28 a.m. UTC | #2 
On 12/6/17 9:08 AM, Alexander Aring wrote:
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index a48ca41b7ecf..7b52a16d2fea 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -449,7 +449,8 @@ static const struct nla_policy stab_policy[TCA_STAB_MAX + 1] = {
>  	[TCA_STAB_DATA] = { .type = NLA_BINARY },
>  };
>  
> -static struct qdisc_size_table *qdisc_get_stab(struct nlattr *opt)
> +static struct qdisc_size_table *qdisc_get_stab(struct nlattr *opt,
> +					       struct netlink_ext_ack *extack)
>  {
>  	struct nlattr *tb[TCA_STAB_MAX + 1];
>  	struct qdisc_size_table *stab;
> @@ -458,23 +459,31 @@ static struct qdisc_size_table *qdisc_get_stab(struct nlattr *opt)
>  	u16 *tab = NULL;
>  	int err;
>  
> -	err = nla_parse_nested(tb, TCA_STAB_MAX, opt, stab_policy, NULL);
> -	if (err < 0)
> +	err = nla_parse_nested(tb, TCA_STAB_MAX, opt, stab_policy, extack);
> +	if (err < 0) {
> +		NL_SET_ERR_MSG(extack, "Failed to parse stab netlink msg");

you don't want to set extack here; it should be set in nla_parse_nested
since it is passed as an arg.


>  		return ERR_PTR(err);
> -	if (!tb[TCA_STAB_BASE])
> +	}
> +	if (!tb[TCA_STAB_BASE]) {
> +		NL_SET_ERR_MSG(extack, "Stab base is missing");

"stab base" is the name of the netlink attribute, but it does not
describe *what* is missing. The key here is returning a message that
tells an average user what they did wrong.


>  		return ERR_PTR(-EINVAL);
> +	}
>  
>  	s = nla_data(tb[TCA_STAB_BASE]);
>  
>  	if (s->tsize > 0) {
> -		if (!tb[TCA_STAB_DATA])
> +		if (!tb[TCA_STAB_DATA]) {
> +			NL_SET_ERR_MSG(extack, "Stab data is missing");

ditto here.

Looking at the code for the tc command, stab appears to be table size;
perhaps a tc author can elaborate on what STAB really means.


>  			return ERR_PTR(-EINVAL);
> +		}
>  		tab = nla_data(tb[TCA_STAB_DATA]);
>  		tsize = nla_len(tb[TCA_STAB_DATA]) / sizeof(u16);
>  	}
>  
> -	if (tsize != s->tsize || (!tab && tsize > 0))
> +	if (tsize != s->tsize || (!tab && tsize > 0)) {
> +		NL_SET_ERR_MSG(extack, "Invalid data length of stab data");
>  		return ERR_PTR(-EINVAL);
> +	}
>  
>  	list_for_each_entry(stab, &qdisc_stab_list, list) {
>  		if (memcmp(&stab->szopts, s, sizeof(*s)))
> @@ -486,8 +495,10 @@ static struct qdisc_size_table *qdisc_get_stab(struct nlattr *opt)
>  	}
>  
>  	stab = kmalloc(sizeof(*stab) + tsize * sizeof(u16), GFP_KERNEL);
> -	if (!stab)
> +	if (!stab) {
> +		NL_SET_ERR_MSG(extack, "Failed to allocate memory for stab data");

ENOMEM does not need a text message. Which memory allocation failed is
not really relevant.


>  		return ERR_PTR(-ENOMEM);
> +	}
>  
>  	stab->refcnt = 1;
>  	stab->szopts = *s;
> @@ -896,7 +907,8 @@ static void notify_and_destroy(struct net *net, struct sk_buff *skb,
>  
>  static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
>  		       struct sk_buff *skb, struct nlmsghdr *n, u32 classid,
> -		       struct Qdisc *new, struct Qdisc *old)
> +		       struct Qdisc *new, struct Qdisc *old,
> +		       struct netlink_ext_ack *extack)
>  {
>  	struct Qdisc *q = old;
>  	struct net *net = dev_net(dev);
> @@ -911,8 +923,10 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
>  		    (new && new->flags & TCQ_F_INGRESS)) {
>  			num_q = 1;
>  			ingress = 1;
> -			if (!dev_ingress_queue(dev))
> +			if (!dev_ingress_queue(dev)) {
> +				NL_SET_ERR_MSG(extack, "Cannot find ingress queue for specified netdev");

netdev is a kernel term. 'device' is in the tc man page so a relevant
term for the message.


I think the above gives you an idea. Apply those comments to the rest of
this patch and the next ones.
Jamal Hadi Salim Dec. 7, 2017, 12:04 p.m. UTC | #3 
On 17-12-07 12:28 AM, David Ahern wrote:

>>   
>> -	err = nla_parse_nested(tb, TCA_STAB_MAX, opt, stab_policy, NULL);
>> -	if (err < 0)
>> +	err = nla_parse_nested(tb, TCA_STAB_MAX, opt, stab_policy, extack);
>> +	if (err < 0) {
>> +		NL_SET_ERR_MSG(extack, "Failed to parse stab netlink msg");
> 
> you don't want to set extack here; it should be set in nla_parse_nested
> since it is passed as an arg.
> 

Can you really have a generic message in nla_parse(_nested)? What
would it say?
Note: the bad attribute is saved in the bowels of nla_parsing.

-
>>   	stab = kmalloc(sizeof(*stab) + tsize * sizeof(u16), GFP_KERNEL);
>> -	if (!stab)
>> +	if (!stab) {
>> +		NL_SET_ERR_MSG(extack, "Failed to allocate memory for stab data");
> 
> ENOMEM does not need a text message. Which memory allocation failed is
> not really relevant.
> 

YMMV.
On the one hand it is useful to distinguish which allocation
in the code path failed(if there was a bug for example).
On the other hand you could argue an alloc failure is as dramatic
as the "sky is falling" - doesnt matter what part of the globe it is 
falling at. If the cost of sending or not is the same why not include
the message?

cheers,
jamal
David Ahern Dec. 7, 2017, 5:52 p.m. UTC | #4 
On 12/7/17 5:04 AM, Jamal Hadi Salim wrote:
> On 17-12-07 12:28 AM, David Ahern wrote:
> 
>>>   -    err = nla_parse_nested(tb, TCA_STAB_MAX, opt, stab_policy, NULL);
>>> -    if (err < 0)
>>> +    err = nla_parse_nested(tb, TCA_STAB_MAX, opt, stab_policy, extack);
>>> +    if (err < 0) {
>>> +        NL_SET_ERR_MSG(extack, "Failed to parse stab netlink msg");
>>
>> you don't want to set extack here; it should be set in nla_parse_nested
>> since it is passed as an arg.
>>
> 
> Can you really have a generic message in nla_parse(_nested)? What
> would it say?
> Note: the bad attribute is saved in the bowels of nla_parsing.

sure, nla_parse only sets the bad attr arg. If you keep the above, then
it needs to be corrected - it is failing to parse the TCA_STAB nested
attribute.
> 
> -
>>>       stab = kmalloc(sizeof(*stab) + tsize * sizeof(u16), GFP_KERNEL);
>>> -    if (!stab)
>>> +    if (!stab) {
>>> +        NL_SET_ERR_MSG(extack, "Failed to allocate memory for stab
>>> data");
>>
>> ENOMEM does not need a text message. Which memory allocation failed is
>> not really relevant.
>>
> 
> YMMV.
> On the one hand it is useful to distinguish which allocation
> in the code path failed(if there was a bug for example).
> On the other hand you could argue an alloc failure is as dramatic
> as the "sky is falling" - doesnt matter what part of the globe it is
> falling at. If the cost of sending or not is the same why not include
> the message?

What value are the messages providing above and beyond the standard libc
strerror(errno)? In the case of ENOMEM, there is nothing in the user can
do to fix that particular command, so why bloat the code with extraneous
messages? Similarly with other errors like ENODEV -- if there is only 1
device in question AND the user specified the device then you do not
need to augment with an additional error message.

The value of extack is in explaining EINVAL, EOPNOTSUPP, or the
confusing inter-dependencies in command arguments. It is not a matter of
adding an extack message for every single error path; add messages that
provide value in helping the user.
David Miller Dec. 7, 2017, 6:08 p.m. UTC | #5 
From: David Ahern <dsahern@gmail.com>
Date: Thu, 7 Dec 2017 10:52:01 -0700

> What value are the messages providing above and beyond the standard libc
> strerror(errno)? In the case of ENOMEM, there is nothing in the user can
> do to fix that particular command, so why bloat the code with extraneous
> messages? Similarly with other errors like ENODEV -- if there is only 1
> device in question AND the user specified the device then you do not
> need to augment with an additional error message.

Agreed, all of this stuff is bloat and not useful in any way to the user.
diff mbox series

Patch

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index a48ca41b7ecf..7b52a16d2fea 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -449,7 +449,8 @@  static const struct nla_policy stab_policy[TCA_STAB_MAX + 1] = {
 	[TCA_STAB_DATA] = { .type = NLA_BINARY },
 };
 
-static struct qdisc_size_table *qdisc_get_stab(struct nlattr *opt)
+static struct qdisc_size_table *qdisc_get_stab(struct nlattr *opt,
+					       struct netlink_ext_ack *extack)
 {
 	struct nlattr *tb[TCA_STAB_MAX + 1];
 	struct qdisc_size_table *stab;
@@ -458,23 +459,31 @@  static struct qdisc_size_table *qdisc_get_stab(struct nlattr *opt)
 	u16 *tab = NULL;
 	int err;
 
-	err = nla_parse_nested(tb, TCA_STAB_MAX, opt, stab_policy, NULL);
-	if (err < 0)
+	err = nla_parse_nested(tb, TCA_STAB_MAX, opt, stab_policy, extack);
+	if (err < 0) {
+		NL_SET_ERR_MSG(extack, "Failed to parse stab netlink msg");
 		return ERR_PTR(err);
-	if (!tb[TCA_STAB_BASE])
+	}
+	if (!tb[TCA_STAB_BASE]) {
+		NL_SET_ERR_MSG(extack, "Stab base is missing");
 		return ERR_PTR(-EINVAL);
+	}
 
 	s = nla_data(tb[TCA_STAB_BASE]);
 
 	if (s->tsize > 0) {
-		if (!tb[TCA_STAB_DATA])
+		if (!tb[TCA_STAB_DATA]) {
+			NL_SET_ERR_MSG(extack, "Stab data is missing");
 			return ERR_PTR(-EINVAL);
+		}
 		tab = nla_data(tb[TCA_STAB_DATA]);
 		tsize = nla_len(tb[TCA_STAB_DATA]) / sizeof(u16);
 	}
 
-	if (tsize != s->tsize || (!tab && tsize > 0))
+	if (tsize != s->tsize || (!tab && tsize > 0)) {
+		NL_SET_ERR_MSG(extack, "Invalid data length of stab data");
 		return ERR_PTR(-EINVAL);
+	}
 
 	list_for_each_entry(stab, &qdisc_stab_list, list) {
 		if (memcmp(&stab->szopts, s, sizeof(*s)))
@@ -486,8 +495,10 @@  static struct qdisc_size_table *qdisc_get_stab(struct nlattr *opt)
 	}
 
 	stab = kmalloc(sizeof(*stab) + tsize * sizeof(u16), GFP_KERNEL);
-	if (!stab)
+	if (!stab) {
+		NL_SET_ERR_MSG(extack, "Failed to allocate memory for stab data");
 		return ERR_PTR(-ENOMEM);
+	}
 
 	stab->refcnt = 1;
 	stab->szopts = *s;
@@ -896,7 +907,8 @@  static void notify_and_destroy(struct net *net, struct sk_buff *skb,
 
 static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
 		       struct sk_buff *skb, struct nlmsghdr *n, u32 classid,
-		       struct Qdisc *new, struct Qdisc *old)
+		       struct Qdisc *new, struct Qdisc *old,
+		       struct netlink_ext_ack *extack)
 {
 	struct Qdisc *q = old;
 	struct net *net = dev_net(dev);
@@ -911,8 +923,10 @@  static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
 		    (new && new->flags & TCQ_F_INGRESS)) {
 			num_q = 1;
 			ingress = 1;
-			if (!dev_ingress_queue(dev))
+			if (!dev_ingress_queue(dev)) {
+				NL_SET_ERR_MSG(extack, "Cannot find ingress queue for specified netdev");
 				return -ENOENT;
+			}
 		}
 
 		if (dev->flags & IFF_UP)
@@ -958,10 +972,12 @@  static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
 		if (cops && cops->graft) {
 			unsigned long cl = cops->find(parent, classid);
 
-			if (cl)
+			if (cl) {
 				err = cops->graft(parent, cl, new, &old);
-			else
+			} else {
+				NL_SET_ERR_MSG(extack, "Specified class not found");
 				err = -ENOENT;
+			}
 		}
 		if (!err)
 			notify_and_destroy(net, skb, n, classid, old, new);
@@ -982,7 +998,8 @@  static struct lock_class_key qdisc_rx_lock;
 static struct Qdisc *qdisc_create(struct net_device *dev,
 				  struct netdev_queue *dev_queue,
 				  struct Qdisc *p, u32 parent, u32 handle,
-				  struct nlattr **tca, int *errp)
+				  struct nlattr **tca, int *errp,
+				  struct netlink_ext_ack *extack)
 {
 	int err;
 	struct nlattr *kind = tca[TCA_KIND];
@@ -1020,11 +1037,14 @@  static struct Qdisc *qdisc_create(struct net_device *dev,
 #endif
 
 	err = -ENOENT;
-	if (!ops)
+	if (!ops) {
+		NL_SET_ERR_MSG(extack, "Specified qdisc not found");
 		goto err_out;
+	}
 
 	sch = qdisc_alloc(dev_queue, ops);
 	if (IS_ERR(sch)) {
+		NL_SET_ERR_MSG(extack, "Failed to allocate qdisc");
 		err = PTR_ERR(sch);
 		goto err_out2;
 	}
@@ -1039,8 +1059,10 @@  static struct Qdisc *qdisc_create(struct net_device *dev,
 		if (handle == 0) {
 			handle = qdisc_alloc_handle(dev);
 			err = -ENOMEM;
-			if (handle == 0)
+			if (handle == 0) {
+				NL_SET_ERR_MSG(extack, "Failed to allocate qdisc handle");
 				goto err_out3;
+			}
 		}
 		lockdep_set_class(qdisc_lock(sch), &qdisc_tx_lock);
 		if (!netif_is_multiqueue(dev))
@@ -1069,16 +1091,20 @@  static struct Qdisc *qdisc_create(struct net_device *dev,
 	if (qdisc_is_percpu_stats(sch)) {
 		sch->cpu_bstats =
 			netdev_alloc_pcpu_stats(struct gnet_stats_basic_cpu);
-		if (!sch->cpu_bstats)
+		if (!sch->cpu_bstats) {
+			NL_SET_ERR_MSG(extack, "Failed to allocate qdisc per cpu bstats");
 			goto err_out4;
+		}
 
 		sch->cpu_qstats = alloc_percpu(struct gnet_stats_queue);
-		if (!sch->cpu_qstats)
+		if (!sch->cpu_qstats) {
+			NL_SET_ERR_MSG(extack, "Failed to allocate qdisc per cpu qstats");
 			goto err_out4;
+		}
 	}
 
 	if (tca[TCA_STAB]) {
-		stab = qdisc_get_stab(tca[TCA_STAB]);
+		stab = qdisc_get_stab(tca[TCA_STAB], extack);
 		if (IS_ERR(stab)) {
 			err = PTR_ERR(stab);
 			goto err_out4;
@@ -1089,8 +1115,10 @@  static struct Qdisc *qdisc_create(struct net_device *dev,
 		seqcount_t *running;
 
 		err = -EOPNOTSUPP;
-		if (sch->flags & TCQ_F_MQROOT)
+		if (sch->flags & TCQ_F_MQROOT) {
+			NL_SET_ERR_MSG(extack, "Invalid qdisc flag TCQ_F_MQROOT");
 			goto err_out4;
+		}
 
 		if (sch->parent != TC_H_ROOT &&
 		    !(sch->flags & TCQ_F_INGRESS) &&
@@ -1105,8 +1133,10 @@  static struct Qdisc *qdisc_create(struct net_device *dev,
 					NULL,
 					running,
 					tca[TCA_RATE]);
-		if (err)
+		if (err) {
+			NL_SET_ERR_MSG(extack, "Failed to generate new estimator");
 			goto err_out4;
+		}
 	}
 
 	qdisc_hash_add(sch, false);
@@ -1139,21 +1169,24 @@  static struct Qdisc *qdisc_create(struct net_device *dev,
 	goto err_out3;
 }
 
-static int qdisc_change(struct Qdisc *sch, struct nlattr **tca)
+static int qdisc_change(struct Qdisc *sch, struct nlattr **tca,
+			struct netlink_ext_ack *extack)
 {
 	struct qdisc_size_table *ostab, *stab = NULL;
 	int err = 0;
 
 	if (tca[TCA_OPTIONS]) {
-		if (!sch->ops->change)
+		if (!sch->ops->change) {
+			NL_SET_ERR_MSG(extack, "Change operation not supported by qdisc");
 			return -EINVAL;
+		}
 		err = sch->ops->change(sch, tca[TCA_OPTIONS]);
 		if (err)
 			return err;
 	}
 
 	if (tca[TCA_STAB]) {
-		stab = qdisc_get_stab(tca[TCA_STAB]);
+		stab = qdisc_get_stab(tca[TCA_STAB], extack);
 		if (IS_ERR(stab))
 			return PTR_ERR(stab);
 	}
@@ -1235,24 +1268,32 @@  static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
 	int err;
 
 	if ((n->nlmsg_type != RTM_GETQDISC) &&
-	    !netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN))
+	    !netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN)) {
+		NL_SET_ERR_MSG(extack, "Net admin permission required for this operation");
 		return -EPERM;
+	}
 
 	err = nlmsg_parse(n, sizeof(*tcm), tca, TCA_MAX, NULL, extack);
-	if (err < 0)
+	if (err < 0) {
+		NL_SET_ERR_MSG(extack, "Failed to parse tcm netlink message");
 		return err;
+	}
 
 	dev = __dev_get_by_index(net, tcm->tcm_ifindex);
-	if (!dev)
+	if (!dev) {
+		NL_SET_ERR_MSG(extack, "Failed to find specified netdev");
 		return -ENODEV;
+	}
 
 	clid = tcm->tcm_parent;
 	if (clid) {
 		if (clid != TC_H_ROOT) {
 			if (TC_H_MAJ(clid) != TC_H_MAJ(TC_H_INGRESS)) {
 				p = qdisc_lookup(dev, TC_H_MAJ(clid));
-				if (!p)
+				if (!p) {
+					NL_SET_ERR_MSG(extack, "Failed to find qdisc with specified classid");
 					return -ENOENT;
+				}
 				q = qdisc_leaf(p, clid);
 			} else if (dev_ingress_queue(dev)) {
 				q = dev_ingress_queue(dev)->qdisc_sleeping;
@@ -1260,26 +1301,38 @@  static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
 		} else {
 			q = dev->qdisc;
 		}
-		if (!q)
+		if (!q) {
+			NL_SET_ERR_MSG(extack, "Cannot find specified qdisc on specified netdev");
 			return -ENOENT;
+		}
 
-		if (tcm->tcm_handle && q->handle != tcm->tcm_handle)
+		if (tcm->tcm_handle && q->handle != tcm->tcm_handle) {
+			NL_SET_ERR_MSG(extack, "Invalid handle");
 			return -EINVAL;
+		}
 	} else {
 		q = qdisc_lookup(dev, tcm->tcm_handle);
-		if (!q)
+		if (!q) {
+			NL_SET_ERR_MSG(extack, "Failed to find qdisc with specified handle");
 			return -ENOENT;
+		}
 	}
 
-	if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], q->ops->id))
+	if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], q->ops->id)) {
+		NL_SET_ERR_MSG(extack, "Invalid qdisc name");
 		return -EINVAL;
+	}
 
 	if (n->nlmsg_type == RTM_DELQDISC) {
-		if (!clid)
+		if (!clid) {
+			NL_SET_ERR_MSG(extack, "Classid cannot be zero");
 			return -EINVAL;
-		if (q->handle == 0)
+		}
+		if (q->handle == 0) {
+			NL_SET_ERR_MSG(extack, "Cannot delete qdisc with handle of zero");
 			return -ENOENT;
-		err = qdisc_graft(dev, p, skb, n, clid, NULL, q);
+		}
+		err = qdisc_graft(dev, p, skb, n, clid, NULL, q, extack);
 		if (err != 0)
 			return err;
 	} else {
@@ -1303,30 +1356,38 @@  static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
 	struct Qdisc *q, *p;
 	int err;
 
-	if (!netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN))
+	if (!netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN)) {
+		NL_SET_ERR_MSG(extack, "Net admin permission required for this operation");
 		return -EPERM;
+	}
 
 replay:
 	/* Reinit, just in case something touches this. */
 	err = nlmsg_parse(n, sizeof(*tcm), tca, TCA_MAX, NULL, extack);
-	if (err < 0)
+	if (err < 0) {
+		NL_SET_ERR_MSG(extack, "Failed to parse netlink TC attributes");
 		return err;
+	}
 
 	tcm = nlmsg_data(n);
 	clid = tcm->tcm_parent;
 	q = p = NULL;
 
 	dev = __dev_get_by_index(net, tcm->tcm_ifindex);
-	if (!dev)
+	if (!dev) {
+		NL_SET_ERR_MSG(extack, "Failed to find specified netdev");
 		return -ENODEV;
+	}
 
 
 	if (clid) {
 		if (clid != TC_H_ROOT) {
 			if (clid != TC_H_INGRESS) {
 				p = qdisc_lookup(dev, TC_H_MAJ(clid));
-				if (!p)
+				if (!p) {
+					NL_SET_ERR_MSG(extack, "Failed to find specified qdisc");
 					return -ENOENT;
+				}
 				q = qdisc_leaf(p, clid);
 			} else if (dev_ingress_queue_create(dev)) {
 				q = dev_ingress_queue(dev)->qdisc_sleeping;
@@ -1341,21 +1402,33 @@  static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
 
 		if (!q || !tcm->tcm_handle || q->handle != tcm->tcm_handle) {
 			if (tcm->tcm_handle) {
-				if (q && !(n->nlmsg_flags & NLM_F_REPLACE))
+				if (q && !(n->nlmsg_flags & NLM_F_REPLACE)) {
+					NL_SET_ERR_MSG(extack, "NLM_F_REPLACE needed to override");
 					return -EEXIST;
-				if (TC_H_MIN(tcm->tcm_handle))
+				}
+				if (TC_H_MIN(tcm->tcm_handle)) {
+					NL_SET_ERR_MSG(extack, "Invalid minor handle");
 					return -EINVAL;
+				}
 				q = qdisc_lookup(dev, tcm->tcm_handle);
-				if (!q)
+				if (!q) {
+					NL_SET_ERR_MSG(extack, "No qdisc found for specified handle");
 					goto create_n_graft;
-				if (n->nlmsg_flags & NLM_F_EXCL)
+				}
+				if (n->nlmsg_flags & NLM_F_EXCL) {
+					NL_SET_ERR_MSG(extack, "Exclusivity flag on, cannot override");
 					return -EEXIST;
+				}
 				if (tca[TCA_KIND] &&
-				    nla_strcmp(tca[TCA_KIND], q->ops->id))
+				    nla_strcmp(tca[TCA_KIND], q->ops->id)) {
+					NL_SET_ERR_MSG(extack, "Invalid qdisc name");
 					return -EINVAL;
+				}
 				if (q == p ||
-				    (p && check_loop(q, p, 0)))
+				    (p && check_loop(q, p, 0))) {
+					NL_SET_ERR_MSG(extack, "Too many references");
 					return -ELOOP;
+				}
 				qdisc_refcount_inc(q);
 				goto graft;
 			} else {
@@ -1390,33 +1463,45 @@  static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
 			}
 		}
 	} else {
-		if (!tcm->tcm_handle)
+		if (!tcm->tcm_handle) {
+			NL_SET_ERR_MSG(extack, "Handle cannot be zero");
 			return -EINVAL;
+		}
 		q = qdisc_lookup(dev, tcm->tcm_handle);
 	}
 
 	/* Change qdisc parameters */
-	if (!q)
+	if (!q) {
+		NL_SET_ERR_MSG(extack, "No qdisc available");
 		return -ENOENT;
-	if (n->nlmsg_flags & NLM_F_EXCL)
+	}
+	if (n->nlmsg_flags & NLM_F_EXCL) {
+		NL_SET_ERR_MSG(extack, "Exclusivity flag on, cannot modify");
 		return -EEXIST;
-	if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], q->ops->id))
+	}
+	if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], q->ops->id)) {
+		NL_SET_ERR_MSG(extack, "Invalid qdisc name");
 		return -EINVAL;
-	err = qdisc_change(q, tca);
+	}
+	err = qdisc_change(q, tca, extack);
 	if (err == 0)
 		qdisc_notify(net, skb, n, clid, NULL, q);
 	return err;
 
 create_n_graft:
-	if (!(n->nlmsg_flags & NLM_F_CREATE))
+	if (!(n->nlmsg_flags & NLM_F_CREATE)) {
+		NL_SET_ERR_MSG(extack, "Qdisc not found. To create specify NLM_F_CREATE flag");
 		return -ENOENT;
+	}
 	if (clid == TC_H_INGRESS) {
-		if (dev_ingress_queue(dev))
+		if (dev_ingress_queue(dev)) {
 			q = qdisc_create(dev, dev_ingress_queue(dev), p,
 					 tcm->tcm_parent, tcm->tcm_parent,
-					 tca, &err);
-		else
+					 tca, &err, extack);
+		} else {
+			NL_SET_ERR_MSG(extack, "Cannot find ingress queue for specified netdev");
 			err = -ENOENT;
+		}
 	} else {
 		struct netdev_queue *dev_queue;
 
@@ -1429,7 +1514,7 @@  static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
 
 		q = qdisc_create(dev, dev_queue, p,
 				 tcm->tcm_parent, tcm->tcm_handle,
-				 tca, &err);
+				 tca, &err, extack);
 	}
 	if (q == NULL) {
 		if (err == -EAGAIN)
@@ -1438,7 +1523,7 @@  static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
 	}
 
 graft:
-	err = qdisc_graft(dev, p, skb, n, clid, q, NULL);
+	err = qdisc_graft(dev, p, skb, n, clid, q, NULL, extack);
 	if (err) {
 		if (q)
 			qdisc_destroy(q);