[PATCH net 0/2] net_sched: reject unknown tcfa_action values

[PATCH net 0/2] net_sched: reject unknown tcfa_action values

[Paolo Abeni]

As agreed some time ago, this changeset reject unknown tcfa_action values,
instead of changing such values under the hood.

A tdc test is included to verify the new behavior.

Paolo Abeni (2):
  net_sched: reject unknown tcfa_action values
  tc-testing: add test-cases for numeric and invalid control action

 net/sched/act_api.c                           | 14 ++++--
 .../tc-testing/tc-tests/actions/police.json   | 48 +++++++++++++++++++
 2 files changed, 58 insertions(+), 4 deletions(-)

-- 
2.17.1

[PATCH net 1/2] net_sched: reject unknown tcfa_action values

[Paolo Abeni]

After the commit 802bfb19152c ("net/sched: user-space can't set
unknown tcfa_action values"), unknown tcfa_action values are
converted to TC_ACT_UNSPEC, but the common agreement is instead
rejecting such configurations.

This change also introduce an helper to simplify the destruction
of a single action, avoding code duplication.

Fixes: 802bfb19152c ("net/sched: user-space can't set unknown tcfa_action values")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/sched/act_api.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index db83dac1e7f4..8614f2c282e8 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -662,6 +662,13 @@ int tcf_action_destroy(struct tc_action *actions[], int bind)
 	return ret;
 }
 
+int tcf_action_destroy_one(struct tc_action *a, int bind)
+{
+	struct tc_action *actions[] = { a, NULL };
+
+	return tcf_action_destroy(actions, bind);
+}
+
 static int tcf_action_put(struct tc_action *p)
 {
 	return __tcf_action_put(p, false);
@@ -881,17 +888,16 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 	if (TC_ACT_EXT_CMP(a->tcfa_action, TC_ACT_GOTO_CHAIN)) {
 		err = tcf_action_goto_chain_init(a, tp);
 		if (err) {
-			struct tc_action *actions[] = { a, NULL };
-
-			tcf_action_destroy(actions, bind);
 			NL_SET_ERR_MSG(extack, "Failed to init TC action chain");
+			tcf_action_destroy_one(a, bind);
 			return ERR_PTR(err);
 		}
 	}
 
 	if (!tcf_action_valid(a->tcfa_action)) {
 		NL_SET_ERR_MSG(extack, "invalid action value, using TC_ACT_UNSPEC instead");
-		a->tcfa_action = TC_ACT_UNSPEC;
+		tcf_action_destroy_one(a, bind);
+		return ERR_PTR(-EINVAL);
 	}
 
 	return a;
-- 
2.17.1

[PATCH net 2/2] tc-testing: add test-cases for numeric and invalid control action

[Paolo Abeni]

Only the police action allows us to specify an arbitrary numeric value
for the control action. This change introduces an explicit test case
for the above feature and then leverage it for testing the kernel behavior
for invalid control actions (reject).

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 .../tc-testing/tc-tests/actions/police.json   | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/police.json b/tools/testing/selftests/tc-testing/tc-tests/actions/police.json
index f03763d81617..30f9b54bd666 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/police.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/police.json
@@ -312,6 +312,54 @@
             "$TC actions flush action police"
         ]
     },
+    {
+        "id": "6aaf",
+        "name": "Add police actions with conform-exceed control pass/pipe [with numeric values]",
+        "category": [
+            "actions",
+            "police"
+        ],
+        "setup": [
+            [
+                "$TC actions flush action police",
+                0,
+                1,
+                255
+            ]
+        ],
+        "cmdUnderTest": "$TC actions add action police rate 3mbit burst 250k conform-exceed 0/3 index 1",
+        "expExitCode": "0",
+        "verifyCmd": "$TC actions get action police index 1",
+        "matchPattern": "action order [0-9]*:  police 0x1 rate 3Mbit burst 250Kb mtu 2Kb action pass/pipe",
+        "matchCount": "1",
+        "teardown": [
+            "$TC actions flush action police"
+        ]
+    },
+    {
+        "id": "29b1",
+        "name": "Add police actions with conform-exceed control <invalid>/drop",
+        "category": [
+            "actions",
+            "police"
+        ],
+        "setup": [
+            [
+                "$TC actions flush action police",
+                0,
+                1,
+                255
+            ]
+        ],
+        "cmdUnderTest": "$TC actions add action police rate 3mbit burst 250k conform-exceed 10/drop index 1",
+        "expExitCode": "255",
+        "verifyCmd": "$TC actions ls action police",
+        "matchPattern": "action order [0-9]*:  police 0x1 rate 3Mbit burst 250Kb mtu 2Kb action ",
+        "matchCount": "0",
+        "teardown": [
+            "$TC actions flush action police"
+        ]
+    },
     {
         "id": "c26f",
         "name": "Add police action with invalid peakrate value",
-- 
2.17.1

Re: [PATCH net 1/2] net_sched: reject unknown tcfa_action values

[Cong Wang]

On Tue, Aug 28, 2018 at 7:25 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> +int tcf_action_destroy_one(struct tc_action *a, int bind)
> +{
> +       struct tc_action *actions[] = { a, NULL };
> +
> +       return tcf_action_destroy(actions, bind);
> +}

Make it static.


> +
>  static int tcf_action_put(struct tc_action *p)
>  {
>         return __tcf_action_put(p, false);
> @@ -881,17 +888,16 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
>         if (TC_ACT_EXT_CMP(a->tcfa_action, TC_ACT_GOTO_CHAIN)) {
>                 err = tcf_action_goto_chain_init(a, tp);
>                 if (err) {
> -                       struct tc_action *actions[] = { a, NULL };
> -
> -                       tcf_action_destroy(actions, bind);
>                         NL_SET_ERR_MSG(extack, "Failed to init TC action chain");
> +                       tcf_action_destroy_one(a, bind);
>                         return ERR_PTR(err);
>                 }
>         }
>
>         if (!tcf_action_valid(a->tcfa_action)) {
>                 NL_SET_ERR_MSG(extack, "invalid action value, using TC_ACT_UNSPEC instead");


You need to adjust this extack too.



> -               a->tcfa_action = TC_ACT_UNSPEC;
> +               tcf_action_destroy_one(a, bind);
> +               return ERR_PTR(-EINVAL);
>         }

Thanks.