netdev - Re: [patch net-next v4 00/10] 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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 4 Jan 2018 11:12:57 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <kubakici@...pl>
Cc:     David Ahern <dsahern@...il.com>, netdev@...r.kernel.org,
        davem@...emloft.net, jhs@...atatu.com, xiyou.wangcong@...il.com,
        mlxsw@...lanox.com, andrew@...n.ch,
        vivien.didelot@...oirfairelinux.com, f.fainelli@...il.com,
        michael.chan@...adcom.com, ganeshgr@...lsio.com,
        saeedm@...lanox.com, matanb@...lanox.com, leonro@...lanox.com,
        idosch@...lanox.com, simon.horman@...ronome.com,
        pieter.jansenvanvuuren@...ronome.com, john.hurley@...ronome.com,
        alexander.h.duyck@...el.com, ogerlitz@...lanox.com,
        john.fastabend@...il.com, daniel@...earbox.net
Subject: Re: [patch net-next v4 00/10] net: sched: allow qdiscs to share
 filter block instances

Thu, Jan 04, 2018 at 08:06:58AM CET, kubakici@...pl wrote:
>On Thu, 4 Jan 2018 07:57:02 +0100, Jiri Pirko wrote:
>> Thu, Jan 04, 2018 at 12:51:52AM CET, kubakici@...pl wrote:
>> >On Wed, 3 Jan 2018 18:22:09 +0100, Jiri Pirko wrote:  
>> >> However I don't agree about breaking the existing filter add and show
>> >> and also imposibility to make not-shared block shared in the runtime
>> >> before defining it first.  
>> >
>> >FWIW I would agree with David that allowing add on a shared block
>> >modify filters on another interface can break existing users.  (No
>> >opinion on dump and lifetime).  
>> 
>> I don't think that David is saying that, but why do you think it would
>> break existing users?
>
>Perhaps I worded is too strongly as "breaking existing users", but it
>certainly introduces surprising side effects.  David put it into words
>very well:
>
>On Tue, 2 Jan 2018 19:07:36 -0700, David Ahern wrote:
>> The disagreement is in how they should be managed. I think my last
>> response concisely captures my concerns -- the principle of least surprise.
>> 
>> So with the initial commands above, all is fine. Then someone is
>> debugging a problem or wants to add another filter to ens8, so they run:
>> 
>> $ tc filter add dev ens8 ingress protocol ip pref 25 flower dst_ip
>> 192.168.1.0/16 action drop
>> 
>> Then traffic flows through ens7 break and some other user is struggling
>> to understand what just happened. That the new filter magically appears
>> on ens7 when the user operated on ens8 is a surprise. Nothing about that
>> last command acknowledges that it is changing a shared resource.

It is not that the new filter magically appears on ens7. No magic. ens8
and ens7 share the same block. User set the sharing up originally,
he can see it in "tc qdisc show". So if he does this, he cannot be
surprised. Also, he can list the filters if he does "tc filter show"
to see what is there per-qdisc.

If the user expects something else, he should go back and read the docs
and learn to understand how tc works.


>> 
>> Consider the commands being run by different people, and a time span
>> between. Allowing the shared block to be configured by any device using
>> the block is just setting up users for errors and confusion.
>> 
>> > forcing user to explicitly create some block entity and then to attach
>> > it to qdisc instances. I don't really see good reason for it. Could you
>> > please clear this up for me?  
>> 
>> It forces the user to acknowledge it is changing a resource that may be
>> shared by more than one device.
>> 
>> $ tc filter add dev ens8 ingress protocol ip pref 25 flower dst_ip
>> 192.168.1.0/16 action drop
>> Error: This qdisc is a shared block. Use the block API to configure.