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 <pab...@redhat.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:
