[PATCH 0/5] net_sched: actions - Add default lookup and walker

[PATCH 0/5] net_sched: actions - Add default lookup and walker

[Jamal Hadi Salim]

This set of patches provide defaults for lookup and walkers for actions.
Users can override when needed.


Jamal Hadi Salim (5):
  Default action lookup method for actions that dont provide one
  Use default action lookup functions Users are free to override
  Provide default walker function for actions
  Use default action walker methods Users can still override
  Fail if missing mandatory action operation methods

 net/sched/act_api.c     |   27 +++++++++++++++------------
 net/sched/act_csum.c    |    2 --
 net/sched/act_gact.c    |    2 --
 net/sched/act_ipt.c     |    4 ----
 net/sched/act_mirred.c  |    2 --
 net/sched/act_nat.c     |    2 --
 net/sched/act_pedit.c   |    2 --
 net/sched/act_police.c  |    1 -
 net/sched/act_simple.c  |    1 -
 net/sched/act_skbedit.c |    1 -
 10 files changed, 15 insertions(+), 29 deletions(-)

-- 
1.7.10.4

[PATCH 0/5] net_sched: actions - Add default lookup and walker

[Jamal Hadi Salim]

This set of patches provide defaults for lookup and walkers for actions.
Users can override when needed.
It also enforces mandatory action methods to be passed.

Thanks to David Miller who paid attention and made this patch set 
much better.

Jamal Hadi Salim (5):
  net_sched: Fail if missing mandatory action operation methods
  net_sched: Default action lookup method for actions
  net_sched: Use default action lookup functions
  net_sched: Provide default walker function for actions
  net_sched: Use default action walker methods

 net/sched/act_api.c     |   26 ++++++++++++++------------
 net/sched/act_csum.c    |    2 --
 net/sched/act_gact.c    |    2 --
 net/sched/act_ipt.c     |    4 ----
 net/sched/act_mirred.c  |    2 --
 net/sched/act_nat.c     |    2 --
 net/sched/act_pedit.c   |    2 --
 net/sched/act_police.c  |    1 -
 net/sched/act_simple.c  |    1 -
 net/sched/act_skbedit.c |    1 -
 10 files changed, 14 insertions(+), 29 deletions(-)

-- 
1.7.10.4

[PATCH 1/5] net_sched: Fail if missing mandatory action operation methods

[Jamal Hadi Salim]

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/act_api.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index fd70728..618695e 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -270,6 +270,10 @@ int tcf_register_action(struct tc_action_ops *act)
 {
 	struct tc_action_ops *a, **ap;
 
+	/* Must supply act, dump, cleanup and init */
+	if (!act->act || !act->dump || !act->cleanup || !act->init)
+		return -EINVAL;
+
 	write_lock(&act_mod_lock);
 	for (ap = &act_base; (a = *ap) != NULL; ap = &a->next) {
 		if (act->type == a->type || (strcmp(act->kind, a->kind) == 0)) {
@@ -381,7 +385,7 @@ int tcf_action_exec(struct sk_buff *skb, const struct tc_action *act,
 	}
 	while ((a = act) != NULL) {
 repeat:
-		if (a->ops && a->ops->act) {
+		if (a->ops) {
 			ret = a->ops->act(skb, a, res);
 			if (TC_MUNGED & skb->tc_verd) {
 				/* copied already, allow trampling */
@@ -405,7 +409,7 @@ void tcf_action_destroy(struct tc_action *act, int bind)
 	struct tc_action *a;
 
 	for (a = act; a; a = act) {
-		if (a->ops && a->ops->cleanup) {
+		if (a->ops) {
 			if (a->ops->cleanup(a, bind) == ACT_P_DELETED)
 				module_put(a->ops->owner);
 			act = act->next;
@@ -424,7 +428,7 @@ tcf_action_dump_old(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
 {
 	int err = -EINVAL;
 
-	if (a->ops == NULL || a->ops->dump == NULL)
+	if (a->ops == NULL)
 		return err;
 	return a->ops->dump(skb, a, bind, ref);
 }
@@ -436,7 +440,7 @@ tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
 	unsigned char *b = skb_tail_pointer(skb);
 	struct nlattr *nest;
 
-	if (a->ops == NULL || a->ops->dump == NULL)
+	if (a->ops == NULL)
 		return err;
 
 	if (nla_put_string(skb, TCA_KIND, a->ops->kind))
-- 
1.7.10.4

[PATCH 2/5] net_sched: Default action lookup method for actions

[Jamal Hadi Salim]

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/act_api.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 618695e..d1a022e 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -274,6 +274,9 @@ int tcf_register_action(struct tc_action_ops *act)
 	if (!act->act || !act->dump || !act->cleanup || !act->init)
 		return -EINVAL;
 
+	if (!act->lookup)
+		act->lookup = tcf_hash_search;
+
 	write_lock(&act_mod_lock);
 	for (ap = &act_base; (a = *ap) != NULL; ap = &a->next) {
 		if (act->type == a->type || (strcmp(act->kind, a->kind) == 0)) {
@@ -727,8 +730,6 @@ tcf_action_get_1(struct nlattr *nla, struct nlmsghdr *n, u32 portid)
 	a->ops = tc_lookup_action(tb[TCA_ACT_KIND]);
 	if (a->ops == NULL)
 		goto err_free;
-	if (a->ops->lookup == NULL)
-		goto err_mod;
 	err = -ENOENT;
 	if (a->ops->lookup(a, index) == 0)
 		goto err_mod;
-- 
1.7.10.4

[PATCH 3/5] net_sched: Use default action lookup functions

[Jamal Hadi Salim]

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/act_csum.c   |    1 -
 net/sched/act_gact.c   |    1 -
 net/sched/act_ipt.c    |    2 --
 net/sched/act_mirred.c |    1 -
 net/sched/act_nat.c    |    1 -
 net/sched/act_pedit.c  |    1 -
 net/sched/act_police.c |    1 -
 7 files changed, 8 deletions(-)

diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 3a4c0ca..4225a93 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -585,7 +585,6 @@ static struct tc_action_ops act_csum_ops = {
 	.act		= tcf_csum,
 	.dump		= tcf_csum_dump,
 	.cleanup	= tcf_csum_cleanup,
-	.lookup		= tcf_hash_search,
 	.init		= tcf_csum_init,
 	.walk		= tcf_generic_walker
 };
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index fd2b3cf..15851da 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -206,7 +206,6 @@ static struct tc_action_ops act_gact_ops = {
 	.act		=	tcf_gact,
 	.dump		=	tcf_gact_dump,
 	.cleanup	=	tcf_gact_cleanup,
-	.lookup		=	tcf_hash_search,
 	.init		=	tcf_gact_init,
 	.walk		=	tcf_generic_walker
 };
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 60d88b6..1d3e191 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -298,7 +298,6 @@ static struct tc_action_ops act_ipt_ops = {
 	.act		=	tcf_ipt,
 	.dump		=	tcf_ipt_dump,
 	.cleanup	=	tcf_ipt_cleanup,
-	.lookup		=	tcf_hash_search,
 	.init		=	tcf_ipt_init,
 	.walk		=	tcf_generic_walker
 };
@@ -312,7 +311,6 @@ static struct tc_action_ops act_xt_ops = {
 	.act		=	tcf_ipt,
 	.dump		=	tcf_ipt_dump,
 	.cleanup	=	tcf_ipt_cleanup,
-	.lookup		=	tcf_hash_search,
 	.init		=	tcf_ipt_init,
 	.walk		=	tcf_generic_walker
 };
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 977c10e..6cb16ec 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -271,7 +271,6 @@ static struct tc_action_ops act_mirred_ops = {
 	.act		=	tcf_mirred,
 	.dump		=	tcf_mirred_dump,
 	.cleanup	=	tcf_mirred_cleanup,
-	.lookup		=	tcf_hash_search,
 	.init		=	tcf_mirred_init,
 	.walk		=	tcf_generic_walker
 };
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 876f0ef..30c13de 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -308,7 +308,6 @@ static struct tc_action_ops act_nat_ops = {
 	.act		=	tcf_nat,
 	.dump		=	tcf_nat_dump,
 	.cleanup	=	tcf_nat_cleanup,
-	.lookup		=	tcf_hash_search,
 	.init		=	tcf_nat_init,
 	.walk		=	tcf_generic_walker
 };
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 7ed78c9..ab4fc56 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -243,7 +243,6 @@ static struct tc_action_ops act_pedit_ops = {
 	.act		=	tcf_pedit,
 	.dump		=	tcf_pedit_dump,
 	.cleanup	=	tcf_pedit_cleanup,
-	.lookup		=	tcf_hash_search,
 	.init		=	tcf_pedit_init,
 	.walk		=	tcf_generic_walker
 };
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 272d8e9..16a62c3 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -407,7 +407,6 @@ static struct tc_action_ops act_police_ops = {
 	.act		=	tcf_act_police,
 	.dump		=	tcf_act_police_dump,
 	.cleanup	=	tcf_act_police_cleanup,
-	.lookup		=	tcf_hash_search,
 	.init		=	tcf_act_police_locate,
 	.walk		=	tcf_act_police_walker
 };
-- 
1.7.10.4

[PATCH 4/5] net_sched: Provide default walker function for actions

[Jamal Hadi Salim]

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/act_api.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index d1a022e..69cb848 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -274,8 +274,11 @@ int tcf_register_action(struct tc_action_ops *act)
 	if (!act->act || !act->dump || !act->cleanup || !act->init)
 		return -EINVAL;
 
+	/* Supply defaults */
 	if (!act->lookup)
 		act->lookup = tcf_hash_search;
+	if (!act->walk)
+		act->walk = tcf_generic_walker;
 
 	write_lock(&act_mod_lock);
 	for (ap = &act_base; (a = *ap) != NULL; ap = &a->next) {
@@ -1089,12 +1092,6 @@ tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
 	memset(&a, 0, sizeof(struct tc_action));
 	a.ops = a_o;
 
-	if (a_o->walk == NULL) {
-		WARN(1, "tc_dump_action: %s !capable of dumping table\n",
-		     a_o->kind);
-		goto out_module_put;
-	}
-
 	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
 			cb->nlh->nlmsg_type, sizeof(*t), 0);
 	if (!nlh)
-- 
1.7.10.4

[PATCH 5/5] net_sched: Use default action walker methods

[Jamal Hadi Salim]

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/act_csum.c    |    1 -
 net/sched/act_gact.c    |    1 -
 net/sched/act_ipt.c     |    2 --
 net/sched/act_mirred.c  |    1 -
 net/sched/act_nat.c     |    1 -
 net/sched/act_pedit.c   |    1 -
 net/sched/act_simple.c  |    1 -
 net/sched/act_skbedit.c |    1 -
 8 files changed, 9 deletions(-)

diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 4225a93..5c5edf5 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -586,7 +586,6 @@ static struct tc_action_ops act_csum_ops = {
 	.dump		= tcf_csum_dump,
 	.cleanup	= tcf_csum_cleanup,
 	.init		= tcf_csum_init,
-	.walk		= tcf_generic_walker
 };
 
 MODULE_DESCRIPTION("Checksum updating actions");
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index 15851da..5645a4d 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -207,7 +207,6 @@ static struct tc_action_ops act_gact_ops = {
 	.dump		=	tcf_gact_dump,
 	.cleanup	=	tcf_gact_cleanup,
 	.init		=	tcf_gact_init,
-	.walk		=	tcf_generic_walker
 };
 
 MODULE_AUTHOR("Jamal Hadi Salim(2002-4)");
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 1d3e191..882a897 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -299,7 +299,6 @@ static struct tc_action_ops act_ipt_ops = {
 	.dump		=	tcf_ipt_dump,
 	.cleanup	=	tcf_ipt_cleanup,
 	.init		=	tcf_ipt_init,
-	.walk		=	tcf_generic_walker
 };
 
 static struct tc_action_ops act_xt_ops = {
@@ -312,7 +311,6 @@ static struct tc_action_ops act_xt_ops = {
 	.dump		=	tcf_ipt_dump,
 	.cleanup	=	tcf_ipt_cleanup,
 	.init		=	tcf_ipt_init,
-	.walk		=	tcf_generic_walker
 };
 
 MODULE_AUTHOR("Jamal Hadi Salim(2002-13)");
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 6cb16ec..2523781 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -272,7 +272,6 @@ static struct tc_action_ops act_mirred_ops = {
 	.dump		=	tcf_mirred_dump,
 	.cleanup	=	tcf_mirred_cleanup,
 	.init		=	tcf_mirred_init,
-	.walk		=	tcf_generic_walker
 };
 
 MODULE_AUTHOR("Jamal Hadi Salim(2002)");
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 30c13de..6a15ace 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -309,7 +309,6 @@ static struct tc_action_ops act_nat_ops = {
 	.dump		=	tcf_nat_dump,
 	.cleanup	=	tcf_nat_cleanup,
 	.init		=	tcf_nat_init,
-	.walk		=	tcf_generic_walker
 };
 
 MODULE_DESCRIPTION("Stateless NAT actions");
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index ab4fc56..03b6767 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -244,7 +244,6 @@ static struct tc_action_ops act_pedit_ops = {
 	.dump		=	tcf_pedit_dump,
 	.cleanup	=	tcf_pedit_cleanup,
 	.init		=	tcf_pedit_init,
-	.walk		=	tcf_generic_walker
 };
 
 MODULE_AUTHOR("Jamal Hadi Salim(2002-4)");
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index 7725eb4..31157d3 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -201,7 +201,6 @@ static struct tc_action_ops act_simp_ops = {
 	.dump		=	tcf_simp_dump,
 	.cleanup	=	tcf_simp_cleanup,
 	.init		=	tcf_simp_init,
-	.walk		=	tcf_generic_walker,
 };
 
 MODULE_AUTHOR("Jamal Hadi Salim(2005)");
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index cb42211..35ea643 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -203,7 +203,6 @@ static struct tc_action_ops act_skbedit_ops = {
 	.dump		=	tcf_skbedit_dump,
 	.cleanup	=	tcf_skbedit_cleanup,
 	.init		=	tcf_skbedit_init,
-	.walk		=	tcf_generic_walker,
 };
 
 MODULE_AUTHOR("Alexander Duyck, <alexander.h.duyck@intel.com>");
-- 
1.7.10.4

Re: [PATCH 1/5] net_sched: Fail if missing mandatory action operation methods

[Eric Dumazet]

On Wed, 2013-12-04 at 09:26 -0500, Jamal Hadi Salim wrote:
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> ---
>  net/sched/act_api.c |   12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index fd70728..618695e 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -270,6 +270,10 @@ int tcf_register_action(struct tc_action_ops *act)
>  {
>  	struct tc_action_ops *a, **ap;
>  
> +	/* Must supply act, dump, cleanup and init */
> +	if (!act->act || !act->dump || !act->cleanup || !act->init)
> +		return -EINVAL;
> +
>  	write_lock(&act_mod_lock);
>  	for (ap = &act_base; (a = *ap) != NULL; ap = &a->next) {
>  		if (act->type == a->type || (strcmp(act->kind, a->kind) == 0)) {
> @@ -381,7 +385,7 @@ int tcf_action_exec(struct sk_buff *skb, const struct tc_action *act,
>  	}
>  	while ((a = act) != NULL) {
>  repeat:
> -		if (a->ops && a->ops->act) {
> +		if (a->ops) {

Could we remove all these tests on a->ops being NULL or not ?

IMHO a->ops cannot be NULL. If NULL, just crash by NULL deref.

Re: [PATCH 1/5] net_sched: Fail if missing mandatory action operation methods

[Jamal Hadi Salim]

On 12/04/13 12:03, Eric Dumazet wrote:

> Could we remove all these tests on a->ops being NULL or not ?
>

Yes, i think those should go away as well.
I will wait for Dave to swallow these other patches then send one that 
gets rid of those.

cheers,
jamal

> IMHO a->ops cannot be NULL. If NULL, just crash by NULL deref.
>
>
>
>

Re: [PATCH 0/5] net_sched: actions - Add default lookup and walker

[David Miller]

From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Wed,  4 Dec 2013 09:26:51 -0500

> This set of patches provide defaults for lookup and walkers for actions.
> Users can override when needed.
> It also enforces mandatory action methods to be passed.
> 
> Thanks to David Miller who paid attention and made this patch set 
> much better.

This looks great, applied, thanks Jamal.