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 for Android: free password hash cracker in your pocket

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

Date:   Thu, 7 Dec 2017 10:52:01 -0700
From:   David Ahern <dsahern@...il.com>
To:     Jamal Hadi Salim <jhs@...atatu.com>,
        Alexander Aring <aring@...atatu.com>, davem@...emloft.net
Cc:     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/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.