netdev - Re: [PATCH net-next] net: sched: set dedicated tcf

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: <vbfsgw2oij8.fsf@mellanox.com>
Date:   Mon, 4 Mar 2019 13:57:12 +0000
From:   Vlad Buslov <vladbu@...lanox.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
CC:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>,
        David Miller <davem@...emloft.net>
Subject: Re: [PATCH net-next] net: sched: set dedicated tcf_walker flag when
 tp is empty

On Sat 02 Mar 2019 at 01:17, Cong Wang <xiyou.wangcong@...il.com> wrote:
> On Thu, Feb 28, 2019 at 7:49 AM Vlad Buslov <vladbu@...lanox.com> wrote:
>>
>> However, if we revert NULL fixes dump will print general tp information
>> for same tp twice (once correctly before dumping all filters on the tp, and
>> second time when called for bogus NULL filter), like this:
>>
>> filter protocol ip pref 1 flower chain 0
>> filter protocol ip pref 1 flower chain 0
>
> What I don't understand is how could we dump twice here,
> particularly the second dump. You insert a bogus NULL filter
> in case for retry? I thought you do the same like for tc actions,
> where you insert an error pointer which can't be dumped.

This is not related in any way to retry. Before my changes creating a
new tp and inserting of first filter on it was atomic - filter insertion
either succeeded or tp was deleted before releasing rtnl lock, in case
of failure. Now, in case of unlocked, classifiers dump code can see them
before they have first filter inserted. Dump prints all filters on tp
with ops->walk(), which calls arg->fn() (assigned to tcf_node_dump() in
this case) on every filter. Some buggy classifiers call arg->fn() with
NULL filter pointer if their ops->walk() is called before inserting any
filters.

This behavior caused problems in my tcf_proto_is_empty() function, but
all ops->walk users can be similarly affected when not protected by rtnl
lock. This is not really an issue at the moment because affected
classifiers (matchall and cgroup) are not marker as "unlocked", so cls
API takes rtnl lock before accessing them, but I would prefer to keep
ops->walk() behavior unified to prevent any further "surprises".