netdev - Re: [PATCH net-next 3/4] net/tc: introduce TC_ACT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 20 Jul 2018 11:54:26 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Eyal Birger <eyal.birger@...il.com>
Subject: Re: [PATCH net-next 3/4] net/tc: introduce TC_ACT_MIRRED.

Hi,

Jiri, Cong, thank you for the feedback. Please allow me to give a
single reply to both of you, as you rised similar concers.

On Thu, 2018-07-19 at 11:07 -0700, Cong Wang wrote:
> On Thu, Jul 19, 2018 at 6:03 AM Paolo Abeni <pabeni@...hat.com> wrote:
> > 
> > This is similar TC_ACT_REDIRECT, but with a slightly different
> > semantic:
> > - on ingress the mirred skbs are passed to the target device
> > network stack without any additional check not scrubbing.
> > - the rcu-protected stats provided via the tcf_result struct
> >   are updated on error conditions.
> 
> At least its name sucks, it means to skip the skb_clone(),
> that is avoid a copy, but you still call it MIRRED...
> 
> MIRRED means MIRror and REDirect.

I was not satified with the name, too, but I also wanted to collect
some feedback, as the different time zones are not helping here.

Would TC_ACT_REINJECT be a better choice? (renaming skb_tc_redirect as
skb_tc_reinject, too). Do you have some better name?

Thanks!

> Also, I don't understand why this new TC_ACT code needs
> to be visible to user-space, whether to clone or not is purely
> internal.

Note this is what already happens with TC_ACT_REDIRECT: currently the
user space uses it freely, even if only {cls,act}_bpf can return such
value in a meaningful way, and only from the ingress and the egress
hooks.

I think we can add a clear separation between the values accessible
from user-space, and the ones used interanally by the kernel, with
something like the code below (basically unknown actions are explicitly
mapped to TC_ACT_UNSPEC), WDYT?

Note: as TC_ACT_REDIRECT is already part of the uAPI, it will remain
accessible from user-space, so patch 1/4 would be still needed.

Cheers,

Paolo

---
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index e4252a176eec..9079e4ee2bbe 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -7,6 +7,9 @@
 #include <net/sch_generic.h>
 #include <net/act_api.h>
 
+/* TC action not accessible from user space */
+#define TC_ACT_REINJECT		(TC_ACT_MAX + 1)
+
 /* Basic packet classifier frontend definitions. */
 
 struct tcf_walker {
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index c4262d911596..c8a24861d4c8 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -45,6 +45,7 @@ enum {
 				   * the skb and act like everything
 				   * is alright.
 				   */
+#define TC_ACT_VALUE_MAX	TC_ACT_TRAP
 
 /* There is a special kind of actions called "extended actions",
  * which need a value parameter. These have a local opcode located in
@@ -55,11 +56,12 @@ enum {
 #define __TC_ACT_EXT_SHIFT 28
 #define __TC_ACT_EXT(local) ((local) << __TC_ACT_EXT_SHIFT)
 #define TC_ACT_EXT_VAL_MASK ((1 << __TC_ACT_EXT_SHIFT) - 1)
-#define TC_ACT_EXT_CMP(combined, opcode) \
-	(((combined) & (~TC_ACT_EXT_VAL_MASK)) == opcode)
+#define TC_ACT_EXT_OPCODE(combined) ((combined) & (~TC_ACT_EXT_VAL_MASK))
+#define TC_ACT_EXT_CMP(combined, opcode) (TC_ACT_EXT_OPCODE(combined) == opcode)
 
 #define TC_ACT_JUMP __TC_ACT_EXT(1)
 #define TC_ACT_GOTO_CHAIN __TC_ACT_EXT(2)
+#define TC_ACT_EXT_OPCODE_MAX	TC_ACT_GOTO_CHAIN
 
 /* Action type identifiers*/
 enum {
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 148a89ab789b..657c3d99698d 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -798,6 +798,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 	char act_name[IFNAMSIZ];
 	struct nlattr *tb[TCA_ACT_MAX + 1];
 	struct nlattr *kind;
+	int opcode;
 	int err;
 
 	if (name == NULL) {
@@ -895,6 +896,14 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 		}
 	}
 
+	opcode = TC_ACT_EXT_OPCODE(a->tcfa_action);
+	if ((!opcode && a->tcfa_action > TC_ACT_VALUE_MAX) ||
+	    (opcode && opcode > TC_ACT_EXT_OPCODE_MAX)) {
+		net_warn_ratelimited("invalid %d action value",
+				     a->tcfa_action);
+		a->tcfa_action = TC_ACT_UNSPEC;
+	}
+
 	return a;
 
 err_mod: