netdev - Re: [PATCH net-next 1/6] net: sched: sch_api: handle generic qdisc errors

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <75db5547-08bb-c3cb-ef7b-f3f73f66709d@gmail.com>
Date:   Wed, 6 Dec 2017 22:28:59 -0700
From:   David Ahern <dsahern@...il.com>
To:     Alexander Aring <aring@...atatu.com>, davem@...emloft.net
Cc:     jhs@...atatu.com, xiyou.wangcong@...il.com, jiri@...nulli.us,
        netdev@...r.kernel.org, kernel@...atatu.com
Subject: Re: [PATCH net-next 1/6] net: sched: sch_api: handle generic qdisc
 errors

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.