[PATCH net] net/sched: fix ->get helper of the matchall cls

[PATCH net] net/sched: fix ->get helper of the matchall cls

[Nicolas Dichtel]

It returned always NULL, thus it was never possible to get the filter.

Example:
$ ip link add foo type dummy
$ ip link add bar type dummy
$ tc qdisc add dev foo clsact
$ tc filter add dev foo protocol all pref 1 ingress handle 1234 \
	matchall action mirred ingress mirror dev bar

Before the patch:
$ tc filter get dev foo protocol all pref 1 ingress handle 1234 matchall
Error: Specified filter handle not found.
We have an error talking to the kernel

After:
$ tc filter get dev foo protocol all pref 1 ingress handle 1234 matchall
filter ingress protocol all pref 1 matchall chain 0 handle 0x4d2
  not_in_hw
        action order 1: mirred (Ingress Mirror to device bar) pipe
        index 1 ref 1 bind 1

CC: Yotam Gigi <yotamg@mellanox.com>
CC: Jiri Pirko <jiri@mellanox.com>
Fixes: fd62d9f5c575 ("net/sched: matchall: Fix configuration race")
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/sched/cls_matchall.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index 459921bd3d87..a13bc351a414 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -130,6 +130,11 @@ static void mall_destroy(struct tcf_proto *tp, bool rtnl_held,
 
 static void *mall_get(struct tcf_proto *tp, u32 handle)
 {
+	struct cls_mall_head *head = rtnl_dereference(tp->root);
+
+	if (head && head->handle == handle)
+		return head;
+
 	return NULL;
 }
 
-- 
2.21.0

Re: [PATCH net] net/sched: fix ->get helper of the matchall cls

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Thu, 28 Mar 2019 10:35:06 +0100

> It returned always NULL, thus it was never possible to get the filter.
> 
> Example:
> $ ip link add foo type dummy
> $ ip link add bar type dummy
> $ tc qdisc add dev foo clsact
> $ tc filter add dev foo protocol all pref 1 ingress handle 1234 \
> 	matchall action mirred ingress mirror dev bar
> 
> Before the patch:
> $ tc filter get dev foo protocol all pref 1 ingress handle 1234 matchall
> Error: Specified filter handle not found.
> We have an error talking to the kernel
> 
> After:
> $ tc filter get dev foo protocol all pref 1 ingress handle 1234 matchall
> filter ingress protocol all pref 1 matchall chain 0 handle 0x4d2
>   not_in_hw
>         action order 1: mirred (Ingress Mirror to device bar) pipe
>         index 1 ref 1 bind 1
> 
> CC: Yotam Gigi <yotamg@mellanox.com>
> CC: Jiri Pirko <jiri@mellanox.com>
> Fixes: fd62d9f5c575 ("net/sched: matchall: Fix configuration race")
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

I'll apply this and queue it up for -stable, but we really should add
a testcase to selftests for this if we don't have one already.

[PATCH net] selftests: add a tc matchall test case

[Nicolas Dichtel]

This is a follow up of the commit 0db6f8befc32 ("net/sched: fix ->get
helper of the matchall cls").

To test it:
$ cd tools/testing/selftests/tc-testing
$ ln -s ../plugin-lib/nsPlugin.py plugins/20-nsPlugin.py
$ ./tdc.py -n -e 2638

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---

I'm not sure to understand the goal of this series:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?h=45b3a3762721

But after it, tc selftests are not intuitive anymore. When naively running them
(./tdy.py), all fw filter tests fail. It's hard to understand that the problem
is that they require the 'nsPlugin' plugin to be enabled (by adding a symlink
*and* providing the '-n' option to tdc.py).

Brenda, why putting the netns code into a plugin that is disabled by default,
knowing that this code is mandatory to run the test?

Regards,
Nicolas

 .../tc-testing/tc-tests/filters/tests.json    | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json b/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
index 99a5ffca1088..2d096b2abf2c 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
@@ -18,6 +18,26 @@
             "$TC qdisc del dev $DEV1 ingress"
         ]
     },
+    {
+        "id": "2638",
+        "name": "Add matchall and try to get it",
+        "category": [
+            "filter",
+            "matchall"
+        ],
+        "setup": [
+            "$TC qdisc add dev $DEV1 clsact",
+            "$TC filter add dev $DEV1 protocol all pref 1 ingress handle 0x1234 matchall action ok"
+        ],
+        "cmdUnderTest": "$TC filter get dev $DEV1 protocol all pref 1 ingress handle 0x1234 matchall",
+        "expExitCode": "0",
+        "verifyCmd": "$TC filter show dev $DEV1 ingress",
+        "matchPattern": "filter protocol all pref 1 matchall chain 0 handle 0x1234",
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $DEV1 clsact"
+        ]
+    },
     {
         "id": "d052",
         "name": "Add 1M filters with the same action",
-- 
2.21.0

Re: [PATCH net] selftests: add a tc matchall test case

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Fri,  5 Apr 2019 14:20:24 +0200

> This is a follow up of the commit 0db6f8befc32 ("net/sched: fix ->get
> helper of the matchall cls").
> 
> To test it:
> $ cd tools/testing/selftests/tc-testing
> $ ln -s ../plugin-lib/nsPlugin.py plugins/20-nsPlugin.py
> $ ./tdc.py -n -e 2638
> 
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Applied, thanks Nicolas.

> I'm not sure to understand the goal of this series:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?h=45b3a3762721
> 
> But after it, tc selftests are not intuitive anymore. When naively running them
> (./tdy.py), all fw filter tests fail. It's hard to understand that the problem
> is that they require the 'nsPlugin' plugin to be enabled (by adding a symlink
> *and* providing the '-n' option to tdc.py).
> 
> Brenda, why putting the netns code into a plugin that is disabled by default,
> knowing that this code is mandatory to run the test?

Brenda, please respond.

Re: [PATCH net] selftests: add a tc matchall test case

[Lucas Bates]

On Sun, Apr 7, 2019 at 10:32 PM David Miller <davem@davemloft.net> wrote:
> > I'm not sure to understand the goal of this series:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?h=45b3a3762721
> >
> > But after it, tc selftests are not intuitive anymore. When naively running them
> > (./tdy.py), all fw filter tests fail. It's hard to understand that the problem
> > is that they require the 'nsPlugin' plugin to be enabled (by adding a symlink
> > *and* providing the '-n' option to tdc.py).
> >
> > Brenda, why putting the netns code into a plugin that is disabled by default,
> > knowing that this code is mandatory to run the test?
>
> Brenda, please respond.

This would be a documentation issue - the usage of nsPlugin is not
covered in the README.  I'll submit a patch by end of the day tomorrow
to address it.

The namespace code isn't mandatory, strictly speaking.  It's not
required at all when running the action tests, only with the filter
tests as those need a specific device to be attached to.

The actual problem is that the nsPlugin creates a veth pair using the
names found in the config file (DEV0/DEV1) - but if you aren't using
the plugin, then a port won't be created.  The intention is that you'd
create a port manually before starting the tests.

Re: [PATCH net] selftests: add a tc matchall test case

[David Miller]

From: Lucas Bates <lucasb@mojatatu.com>
Date: Mon, 8 Apr 2019 18:24:08 -0400

> On Sun, Apr 7, 2019 at 10:32 PM David Miller <davem@davemloft.net> wrote:
>> > I'm not sure to understand the goal of this series:
>> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?h=45b3a3762721
>> >
>> > But after it, tc selftests are not intuitive anymore. When naively running them
>> > (./tdy.py), all fw filter tests fail. It's hard to understand that the problem
>> > is that they require the 'nsPlugin' plugin to be enabled (by adding a symlink
>> > *and* providing the '-n' option to tdc.py).
>> >
>> > Brenda, why putting the netns code into a plugin that is disabled by default,
>> > knowing that this code is mandatory to run the test?
>>
>> Brenda, please respond.
> 
> This would be a documentation issue - the usage of nsPlugin is not
> covered in the README.  I'll submit a patch by end of the day tomorrow
> to address it.
> 
> The namespace code isn't mandatory, strictly speaking.  It's not
> required at all when running the action tests, only with the filter
> tests as those need a specific device to be attached to.
> 
> The actual problem is that the nsPlugin creates a veth pair using the
> names found in the config file (DEV0/DEV1) - but if you aren't using
> the plugin, then a port won't be created.  The intention is that you'd
> create a port manually before starting the tests.

That's poor behavior.

If I just want to run all the tests, I should be able to just execute
the script with no special arguments.

This setup sucks.

Re: [PATCH net] selftests: add a tc matchall test case

[Nicolas Dichtel]

Le 09/04/2019 à 00:45, David Miller a écrit :
> From: Lucas Bates <lucasb@mojatatu.com>
[snip]
>> This would be a documentation issue - the usage of nsPlugin is not
>> covered in the README.  I'll submit a patch by end of the day tomorrow
>> to address it.
>>
>> The namespace code isn't mandatory, strictly speaking.  It's not
>> required at all when running the action tests, only with the filter
>> tests as those need a specific device to be attached to.
>>
>> The actual problem is that the nsPlugin creates a veth pair using the
>> names found in the config file (DEV0/DEV1) - but if you aren't using
>> the plugin, then a port won't be created.  The intention is that you'd
>> create a port manually before starting the tests.
> 
> That's poor behavior.
> 
> If I just want to run all the tests, I should be able to just execute
> the script with no special arguments.
+1

If we want that people use those kind of tests, it must work without any operations.


Regards,
Nicolas