From: Vlad Buslov <vladbu@mellanox•com>
To: Cong Wang <xiyou.wangcong@gmail•com>
Cc: Linux Kernel Network Developers <netdev@vger•kernel.org>,
Jamal Hadi Salim <jhs@mojatatu•com>,
Jiri Pirko <jiri@resnulli•us>, David Miller <davem@davemloft•net>
Subject: Re: [PATCH net-next] net: sched: set dedicated tcf_walker flag when tp is empty
Date: Mon, 4 Mar 2019 13:57:12 +0000 [thread overview]
Message-ID: <vbfsgw2oij8.fsf@mellanox.com> (raw)
In-Reply-To: <CAM_iQpXfPoc_5rg1Yj3ACMXNqqq7ERmjgJc8oFhjLL8s5nH4VQ@mail.gmail.com>
On Sat 02 Mar 2019 at 01:17, Cong Wang <xiyou.wangcong@gmail•com> wrote:
> On Thu, Feb 28, 2019 at 7:49 AM Vlad Buslov <vladbu@mellanox•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".
prev parent reply other threads:[~2019-03-04 13:57 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-25 15:38 [PATCH net-next] net: sched: set dedicated tcf_walker flag when tp is empty Vlad Buslov
2019-02-25 18:18 ` David Miller
2019-02-25 22:52 ` Cong Wang
2019-02-26 15:08 ` Vlad Buslov
2019-02-26 22:38 ` Cong Wang
2019-02-27 14:28 ` Vlad Buslov
2019-02-27 23:09 ` Cong Wang
2019-02-28 15:49 ` Vlad Buslov
2019-03-02 1:17 ` Cong Wang
2019-03-04 13:57 ` Vlad Buslov [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=vbfsgw2oij8.fsf@mellanox.com \
--to=vladbu@mellanox$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=jhs@mojatatu$(echo .)com \
--cc=jiri@resnulli$(echo .)us \
--cc=netdev@vger$(echo .)kernel.org \
--cc=xiyou.wangcong@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox