Subject: Re: [PATCH net-next] net: sched: set dedicated tcf_walker flag when tp is empty

From: Vlad Buslov <[email-masked]>
Date: Mon, 4 Mar 2019 13:57:12 +0000

On Sat 02 Mar 2019 at 01:17, Cong Wang <[email-masked]> wrote:
> On Thu, Feb 28, 2019 at 7:49 AM Vlad Buslov <[email-masked]> 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".