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

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

[Vlad Buslov]

Using tcf_walker->stop flag to determine when tcf_walker->fn() was called
at least once is unreliable. Some classifiers set 'stop' flag on error
before calling walker callback, other classifiers used to call it with NULL
filter pointer when empty. In order to prevent further regressions, extend
tcf_walker structure with dedicated 'nonempty' flag. Set this flag in
tcf_walker->fn() implementation that is used to check if classifier has
filters configured.

Fixes: 8b64678e0af8 ("net: sched: refactor tp insert/delete for concurrent execution")
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/pkt_cls.h |  1 +
 net/sched/cls_api.c   | 13 +++++++++----
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 232f801f2a21..422dd8800478 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -17,6 +17,7 @@ struct tcf_walker {
 	int	stop;
 	int	skip;
 	int	count;
+	bool	nonempty;
 	unsigned long cookie;
 	int	(*fn)(struct tcf_proto *, void *node, struct tcf_walker *);
 };
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index e2c888961379..3543be31d400 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -238,18 +238,23 @@ static void tcf_proto_put(struct tcf_proto *tp, bool rtnl_held,
 		tcf_proto_destroy(tp, rtnl_held, extack);
 }
 
-static int walker_noop(struct tcf_proto *tp, void *d, struct tcf_walker *arg)
+static int walker_check_empty(struct tcf_proto *tp, void *d,
+			      struct tcf_walker *arg)
 {
-	return -1;
+	if (tp) {
+		arg->nonempty = true;
+		return -1;
+	}
+	return 0;
 }
 
 static bool tcf_proto_is_empty(struct tcf_proto *tp, bool rtnl_held)
 {
-	struct tcf_walker walker = { .fn = walker_noop, };
+	struct tcf_walker walker = { .fn = walker_check_empty, };
 
 	if (tp->ops->walk) {
 		tp->ops->walk(tp, &walker, rtnl_held);
-		return !walker.stop;
+		return !walker.nonempty;
 	}
 	return true;
 }
-- 
2.13.6

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

[David Miller]

From: Vlad Buslov <vladbu@mellanox.com>
Date: Mon, 25 Feb 2019 17:38:31 +0200

> Using tcf_walker->stop flag to determine when tcf_walker->fn() was called
> at least once is unreliable. Some classifiers set 'stop' flag on error
> before calling walker callback, other classifiers used to call it with NULL
> filter pointer when empty. In order to prevent further regressions, extend
> tcf_walker structure with dedicated 'nonempty' flag. Set this flag in
> tcf_walker->fn() implementation that is used to check if classifier has
> filters configured.
> 
> Fixes: 8b64678e0af8 ("net: sched: refactor tp insert/delete for concurrent execution")
> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
> Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied, thanks.

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

[Cong Wang]

On Mon, Feb 25, 2019 at 7:38 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>
> Using tcf_walker->stop flag to determine when tcf_walker->fn() was called
> at least once is unreliable. Some classifiers set 'stop' flag on error
> before calling walker callback, other classifiers used to call it with NULL
> filter pointer when empty. In order to prevent further regressions, extend
> tcf_walker structure with dedicated 'nonempty' flag. Set this flag in
> tcf_walker->fn() implementation that is used to check if classifier has
> filters configured.


So, after this patch commits like 31a998487641 ("net: sched: fw: don't
set arg->stop in fw_walk() when empty") can be reverted??


>
> Fixes: 8b64678e0af8 ("net: sched: refactor tp insert/delete for concurrent execution")
> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
> Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  include/net/pkt_cls.h |  1 +
>  net/sched/cls_api.c   | 13 +++++++++----
>  2 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index 232f801f2a21..422dd8800478 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -17,6 +17,7 @@ struct tcf_walker {
>         int     stop;
>         int     skip;
>         int     count;
> +       bool    nonempty;
>         unsigned long cookie;
>         int     (*fn)(struct tcf_proto *, void *node, struct tcf_walker *);
>  };
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index e2c888961379..3543be31d400 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -238,18 +238,23 @@ static void tcf_proto_put(struct tcf_proto *tp, bool rtnl_held,
>                 tcf_proto_destroy(tp, rtnl_held, extack);
>  }
>
> -static int walker_noop(struct tcf_proto *tp, void *d, struct tcf_walker *arg)
> +static int walker_check_empty(struct tcf_proto *tp, void *d,
> +                             struct tcf_walker *arg)
>  {
> -       return -1;
> +       if (tp) {
> +               arg->nonempty = true;
> +               return -1;
> +       }
> +       return 0;

How does this even work? If we can simply check tp!=NULL as
non-empty, why do we even need a walker??

For me, it must be pushed down to each implementation to
determine how it is empty.

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

[Vlad Buslov]

On Mon 25 Feb 2019 at 22:52, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Mon, Feb 25, 2019 at 7:38 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>>
>> Using tcf_walker->stop flag to determine when tcf_walker->fn() was called
>> at least once is unreliable. Some classifiers set 'stop' flag on error
>> before calling walker callback, other classifiers used to call it with NULL
>> filter pointer when empty. In order to prevent further regressions, extend
>> tcf_walker structure with dedicated 'nonempty' flag. Set this flag in
>> tcf_walker->fn() implementation that is used to check if classifier has
>> filters configured.
>
>
> So, after this patch commits like 31a998487641 ("net: sched: fw: don't
> set arg->stop in fw_walk() when empty") can be reverted??

Yes, it is safe now to revert following commits:

3027ff41f67c ("net: sched: route: don't set arg->stop in route4_walk() when empty")
31a998487641 ("net: sched: fw: don't set arg->stop in fw_walk() when empty")

>
>
>>
>> Fixes: 8b64678e0af8 ("net: sched: refactor tp insert/delete for concurrent execution")
>> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
>> Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
>> ---
>>  include/net/pkt_cls.h |  1 +
>>  net/sched/cls_api.c   | 13 +++++++++----
>>  2 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>> index 232f801f2a21..422dd8800478 100644
>> --- a/include/net/pkt_cls.h
>> +++ b/include/net/pkt_cls.h
>> @@ -17,6 +17,7 @@ struct tcf_walker {
>>         int     stop;
>>         int     skip;
>>         int     count;
>> +       bool    nonempty;
>>         unsigned long cookie;
>>         int     (*fn)(struct tcf_proto *, void *node, struct tcf_walker *);
>>  };
>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>> index e2c888961379..3543be31d400 100644
>> --- a/net/sched/cls_api.c
>> +++ b/net/sched/cls_api.c
>> @@ -238,18 +238,23 @@ static void tcf_proto_put(struct tcf_proto *tp, bool rtnl_held,
>>                 tcf_proto_destroy(tp, rtnl_held, extack);
>>  }
>>
>> -static int walker_noop(struct tcf_proto *tp, void *d, struct tcf_walker *arg)
>> +static int walker_check_empty(struct tcf_proto *tp, void *d,
>> +                             struct tcf_walker *arg)
>>  {
>> -       return -1;
>> +       if (tp) {
>> +               arg->nonempty = true;
>> +               return -1;
>> +       }
>> +       return 0;
>
> How does this even work? If we can simply check tp!=NULL as
> non-empty, why do we even need a walker??
>
> For me, it must be pushed down to each implementation to
> determine how it is empty.

Sorry, this is a typo. Intention is to check the filter pointer (void
*d). Sending the fix.

Thanks for spotting this!

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

[Cong Wang]

On Tue, Feb 26, 2019 at 7:08 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>
>
> On Mon 25 Feb 2019 at 22:52, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > On Mon, Feb 25, 2019 at 7:38 AM Vlad Buslov <vladbu@mellanox.com> wrote:
> >>
> >> Using tcf_walker->stop flag to determine when tcf_walker->fn() was called
> >> at least once is unreliable. Some classifiers set 'stop' flag on error
> >> before calling walker callback, other classifiers used to call it with NULL
> >> filter pointer when empty. In order to prevent further regressions, extend
> >> tcf_walker structure with dedicated 'nonempty' flag. Set this flag in
> >> tcf_walker->fn() implementation that is used to check if classifier has
> >> filters configured.
> >
> >
> > So, after this patch commits like 31a998487641 ("net: sched: fw: don't
> > set arg->stop in fw_walk() when empty") can be reverted??
>
> Yes, it is safe now to revert following commits:
>
> 3027ff41f67c ("net: sched: route: don't set arg->stop in route4_walk() when empty")
> 31a998487641 ("net: sched: fw: don't set arg->stop in fw_walk() when empty")

Yeah, and probably commit d66022cd1623
("net: sched: matchall: verify that filter is not NULL in mall_walk()").

Please send a patch to revert them all.

Thanks.

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

[Vlad Buslov]

On Tue 26 Feb 2019 at 22:38, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Tue, Feb 26, 2019 at 7:08 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>>
>>
>> On Mon 25 Feb 2019 at 22:52, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> > On Mon, Feb 25, 2019 at 7:38 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>> >>
>> >> Using tcf_walker->stop flag to determine when tcf_walker->fn() was called
>> >> at least once is unreliable. Some classifiers set 'stop' flag on error
>> >> before calling walker callback, other classifiers used to call it with NULL
>> >> filter pointer when empty. In order to prevent further regressions, extend
>> >> tcf_walker structure with dedicated 'nonempty' flag. Set this flag in
>> >> tcf_walker->fn() implementation that is used to check if classifier has
>> >> filters configured.
>> >
>> >
>> > So, after this patch commits like 31a998487641 ("net: sched: fw: don't
>> > set arg->stop in fw_walk() when empty") can be reverted??
>>
>> Yes, it is safe now to revert following commits:
>>
>> 3027ff41f67c ("net: sched: route: don't set arg->stop in route4_walk() when empty")
>> 31a998487641 ("net: sched: fw: don't set arg->stop in fw_walk() when empty")
>
> Yeah, and probably commit d66022cd1623
> ("net: sched: matchall: verify that filter is not NULL in mall_walk()").
>
> Please send a patch to revert them all.
>
> Thanks.

I think commit d66022cd1623 ("net: sched: matchall: verify that filter
is not NULL in mall_walk()") and commit 8b58d12f4ae1 ("net: sched:
cgroup: verify that filter is not NULL during walk") shouldn't be
reverted. They are still necessary to prevent tcf_chain_dump() from
dumping NULL filter pointer. It can happen when dump is initiated in
parallel with inserting first filter to unlocked classifier.
tcf_fill_node() verifies that filter pointer is not NULL, so it will not
crash, but will output tcf_proto info for second time. This might
"confuse" user-space.

What do you think?

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

[Cong Wang]

On Wed, Feb 27, 2019 at 6:28 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>
>
> On Tue 26 Feb 2019 at 22:38, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > On Tue, Feb 26, 2019 at 7:08 AM Vlad Buslov <vladbu@mellanox.com> wrote:
> >>
> >>
> >> On Mon 25 Feb 2019 at 22:52, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >> > On Mon, Feb 25, 2019 at 7:38 AM Vlad Buslov <vladbu@mellanox.com> wrote:
> >> >>
> >> >> Using tcf_walker->stop flag to determine when tcf_walker->fn() was called
> >> >> at least once is unreliable. Some classifiers set 'stop' flag on error
> >> >> before calling walker callback, other classifiers used to call it with NULL
> >> >> filter pointer when empty. In order to prevent further regressions, extend
> >> >> tcf_walker structure with dedicated 'nonempty' flag. Set this flag in
> >> >> tcf_walker->fn() implementation that is used to check if classifier has
> >> >> filters configured.
> >> >
> >> >
> >> > So, after this patch commits like 31a998487641 ("net: sched: fw: don't
> >> > set arg->stop in fw_walk() when empty") can be reverted??
> >>
> >> Yes, it is safe now to revert following commits:
> >>
> >> 3027ff41f67c ("net: sched: route: don't set arg->stop in route4_walk() when empty")
> >> 31a998487641 ("net: sched: fw: don't set arg->stop in fw_walk() when empty")
> >
> > Yeah, and probably commit d66022cd1623
> > ("net: sched: matchall: verify that filter is not NULL in mall_walk()").
> >
> > Please send a patch to revert them all.
> >
> > Thanks.
>
> I think commit d66022cd1623 ("net: sched: matchall: verify that filter
> is not NULL in mall_walk()") and commit 8b58d12f4ae1 ("net: sched:
> cgroup: verify that filter is not NULL during walk") shouldn't be
> reverted. They are still necessary to prevent tcf_chain_dump() from
> dumping NULL filter pointer. It can happen when dump is initiated in
> parallel with inserting first filter to unlocked classifier.
> tcf_fill_node() verifies that filter pointer is not NULL, so it will not
> crash, but will output tcf_proto info for second time. This might
> "confuse" user-space.

I don't get this.

First of all, what's confused here?

Secondly, if there is something confusing, isn't it all because of
your parallel algorithm? That is, the retry logic. I don't see how
commit d66022cd1623 could be useful in this context, it helps
to prevent a NULL crash which isn't a concern as long as it is
checked in tcf_fill_node() as you described.


Thanks.

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

[Vlad Buslov]

On Wed 27 Feb 2019 at 23:09, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Wed, Feb 27, 2019 at 6:28 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>>
>>
>> On Tue 26 Feb 2019 at 22:38, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> > On Tue, Feb 26, 2019 at 7:08 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>> >>
>> >>
>> >> On Mon 25 Feb 2019 at 22:52, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> >> > On Mon, Feb 25, 2019 at 7:38 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>> >> >>
>> >> >> Using tcf_walker->stop flag to determine when tcf_walker->fn() was called
>> >> >> at least once is unreliable. Some classifiers set 'stop' flag on error
>> >> >> before calling walker callback, other classifiers used to call it with NULL
>> >> >> filter pointer when empty. In order to prevent further regressions, extend
>> >> >> tcf_walker structure with dedicated 'nonempty' flag. Set this flag in
>> >> >> tcf_walker->fn() implementation that is used to check if classifier has
>> >> >> filters configured.
>> >> >
>> >> >
>> >> > So, after this patch commits like 31a998487641 ("net: sched: fw: don't
>> >> > set arg->stop in fw_walk() when empty") can be reverted??
>> >>
>> >> Yes, it is safe now to revert following commits:
>> >>
>> >> 3027ff41f67c ("net: sched: route: don't set arg->stop in route4_walk() when empty")
>> >> 31a998487641 ("net: sched: fw: don't set arg->stop in fw_walk() when empty")
>> >
>> > Yeah, and probably commit d66022cd1623
>> > ("net: sched: matchall: verify that filter is not NULL in mall_walk()").
>> >
>> > Please send a patch to revert them all.
>> >
>> > Thanks.
>>
>> I think commit d66022cd1623 ("net: sched: matchall: verify that filter
>> is not NULL in mall_walk()") and commit 8b58d12f4ae1 ("net: sched:
>> cgroup: verify that filter is not NULL during walk") shouldn't be
>> reverted. They are still necessary to prevent tcf_chain_dump() from
>> dumping NULL filter pointer. It can happen when dump is initiated in
>> parallel with inserting first filter to unlocked classifier.
>> tcf_fill_node() verifies that filter pointer is not NULL, so it will not
>> crash, but will output tcf_proto info for second time. This might
>> "confuse" user-space.
>
> I don't get this.
>
> First of all, what's confused here?

Let me describe it in more details. tcf_chain_dump() calls
tcf_fill_node() with NULL filter for every tp. When called like that
tcf_fill_node() outputs general tp information (iproute2 tc text
output):

filter protocol ip pref 1 flower chain 0

After that tcf_fill_node() is called for each filter on tp by
ops->walk(). When invoked in with non-NULL filter pointer it outputs tp
info and filter details:

filter protocol ip pref 1 flower chain 0 handle 0x1
  dst_mac e4:12:00:00:00:00
  src_mac e4:11:00:00:00:00
  eth_type ipv4
  ip_proto udp
  dst_ip 192.168.111.2
  src_ip 192.168.111.1
  dst_port 1
  src_port 1
  skip_hw
  not_in_hw
        action order 1: gact action drop
         random type none pass val 0
         index 1 ref 1 bind 1 installed 33 sec used 33 sec
        Action statistics:
        Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
        backlog 0b 0p requeues 0

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

This is what I meant in previous email by "confuse". Sorry for not being
more clear.

>
> Secondly, if there is something confusing, isn't it all because of
> your parallel algorithm? That is, the retry logic.

Yes, this is a side effect of my implementation. Dump code can see
transient tp instances before first filter is inserted. Doesn't have
anything to do with retry mechanism specifically, just a result of
unlocked execution. I didn't expect some classifiers to call
tcf_walker->fn() with NULL filters when I implemented it.

> I don't see how
> commit d66022cd1623 could be useful in this context, it helps
> to prevent a NULL crash which isn't a concern as long as it is
> checked in tcf_fill_node() as you described.

It prevents ops->walk() from calling tcf_walker->fn() with NULL filter
pointer. Besides my new tcf_proto_is_empty() function ops->walk() is
also used by dump code, which will not crash, but will generate output
described above.

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

[Cong Wang]

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.

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

[Vlad Buslov]

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".