netdev - Re: [PATCH net-next 2/8] net: sched: cls

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]

Date:   Tue, 16 Jan 2018 19:22:42 -0800
From:   David Ahern <dsahern@...il.com>
To:     Jamal Hadi Salim <jhs@...atatu.com>,
        Alexander Aring <aring@...atatu.com>
Cc:     xiyou.wangcong@...il.com, jiri@...nulli.us, davem@...emloft.net,
        netdev@...r.kernel.org, kernel@...atatu.com
Subject: Re: [PATCH net-next 2/8] net: sched: cls_api: handle generic cls
 errors

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.