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] [day] [month] [year] [list]

Message-ID: <1886f57b-d9b1-02d7-6c94-721ee5f34e59@mojatatu.com>
Date:   Wed, 17 Jan 2018 09:32:47 -0500
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     David Ahern <dsahern@...il.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 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