netdev - Re: [Patch net-next] net_sched: add reverse binding for tc class

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:   Thu, 31 Aug 2017 00:45:16 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Cong Wang <xiyou.wangcong@...il.com>
CC:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jamal Hadi Salim <jhs@...atatu.com>
Subject: Re: [Patch net-next] net_sched: add reverse binding for tc class

On 08/31/2017 12:22 AM, Daniel Borkmann wrote:
> On 08/31/2017 12:01 AM, Cong Wang wrote:
>> On Wed, Aug 30, 2017 at 2:48 PM, Daniel Borkmann <daniel@...earbox.net> wrote:
>>> On 08/30/2017 11:30 PM, Cong Wang wrote:
>>> [...]
>>>>
>>>> Note, we still can NOT totally get rid of those class lookup in
>>>> ->enqueue() because cgroup and flow filters have no way to determine
>>>> the classid at setup time, they still have to go through dynamic lookup.
>>>
>>> [...]
>>>>
>>>> ---
>>>>    include/net/sch_generic.h |  1 +
>>>>    net/sched/cls_basic.c     |  9 +++++++
>>>>    net/sched/cls_bpf.c       |  9 +++++++
>>>
>>> Same is for cls_bpf as well, so bind_class wouldn't work there
>>> either as we could return dynamic classids. bind_class cannot
>>> be added here, too.
>>
>> I think you are probably right, but the following code is
>> misleading there:
>>
>>          if (tb[TCA_BPF_CLASSID]) {
>>                  prog->res.classid = nla_get_u32(tb[TCA_BPF_CLASSID]);
>>                  tcf_bind_filter(tp, &prog->res, base);
>>          }
>>
>> If the classid is dynamic, why this tb[TCA_BPF_CLASSID]?
>
> The prog->res.classid is the default one, but can be overridden
> later depending on the specified program. cls_bpf_classify() does
> after prog return (filter_res holds return code):
>
>      [...]
>          if (filter_res == 0)
>              continue;
>          if (filter_res != -1) {
>              res->class   = 0;
>              res->classid = filter_res;
>          } else {
>              *res = prog->res;
>          }
>      [...]
>
> Meaning in case of a match (-1), we use the default bound one,
> but prog may as well return an alternative found classid if it
> wants to. So both versions are possible.

But even for that case your patch looks fine to me actually, since
for dynamic classid we set class to 0. No objections from my side
then.