netdev - Re: [patch net-next RFC 0/4] net: sched: allow qdiscs to share filter block instances

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: <1b890dd0-6c33-1d69-ab02-30622736dd3d@mojatatu.com>
Date:   Fri, 14 Jul 2017 03:33:35 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, davem@...emloft.net,
        xiyou.wangcong@...il.com, edumazet@...gle.com,
        stephen@...workplumber.org, jbenc@...hat.com, mlxsw@...lanox.com,
        andrew@...n.ch, vivien.didelot@...oirfairelinux.com,
        f.fainelli@...il.com, john.fastabend@...il.com,
        alexander.h.duyck@...el.com, daniel@...earbox.net,
        ogerlitz@...lanox.com, mrv@...atatu.com
Subject: Re: [patch net-next RFC 0/4] net: sched: allow qdiscs to share filter
 block instances

On 17-07-11 08:34 AM, Jiri Pirko wrote:
> Tue, Jul 11, 2017 at 02:15:27PM CEST, jhs@...atatu.com wrote:
>> Hi Jiri,
>>

[..]
>>> Now we can add filter to any of qdiscs sharing the same block:
>>>
>>> $ tc filter add dev ens7 parent ffff: protocol ip pref 25 flower dst_ip 192.168.0.0/16 action drop
>>>
>>
>> So for backward compat - this also makes sense. But:
>> it does make sense to create new syntax for adding
>> filters and actions:
>>
>> tc filter add block 22 protocol ip pref 25 flower \
>>   dst_ip 192.168.0.0/16 action drop
> 
> Was thinking about that. Decided to pass on this now. This should be
> addressed by follow-up anyway.
> 

Ok, thanks. I feel it is very important.

> 
>>
>> Coordinates of the filter block before were:
>>
>> <ifindex>, <parent>, [handle]
>>
>> You should be able to abuse struct tcmsg ifindex to represent block #
>> as long as you set parent to be something meaningful that is
>> identified "block coordinate" via TC_H_XXX (pick something safe not
>> in use by ingress or egress; look at: uapi/linux/pkt_sched.h)
> 
> Not sure about this. I have take closer look. In general, I don't like
> to abuse anything :)
> 

I should have said it without "ab" i.e "use" ;->
The purpose of those coordinates is to describe the "location"
of what you call the "block" now.
We used to have egress, then ingress "blocks" tied to a port.
then Daniel added clsact above but still ingress + egress
And it used to be tied to a port and you just changed it to be
port independent etc.
It is a natural evolution.


>>> We will see the same output if we list filters for ens7 and ens8, including stats:

[..]
>>> $ tc -s filter show dev ens8 root
>>> filter dev ens7 parent ffff: protocol ip pref 25 flower
>>> filter dev ens7 parent ffff: protocol ip pref 25 flower handle 0x1
>>>     eth_type ipv4
>>>     dst_ip 192.168.1.0/24
>>>           action order 1: gact action drop
>>>            random type none pass val 0
>>>            index 3 ref 1 bind 1 installed 10202 sec used 10152 sec
>>>           Action statistics:
>>>           Sent 4200 bytes 50 pkt (dropped 50, overlimits 0 requeues 0)
>>>           backlog 0b 0p requeues 0
>>>
>>>
>>> Issues:
>>> - tp->q is set by the device used to add the filter. That has to be resolved.
>>>     Impacts the dump (as you can see above)
>>>
>>
>> I think you have more problems if the dump above is reality;->
>> You added to ingress and this is showing egress.
> 
> Howcome? I only don't see "dev x" on ens7. That is the only difference,
> 

I meant you are displaying egress qdisc filters(root) but you added them
to ingress (ffff:). I would have expected the output you showed had you
said:
tc -s filter show dev ens8 parent ffff:


cheers,
jamal