[Patch net-next v3 0/5] net_sched: act: more cleanup and improvement

[Patch net-next v3 0/5] net_sched: act: more cleanup and improvement

[Cong Wang]

v2 -> v3:
* fix a mis-splitted patch
* keep hinfo as a pointer in ops

v1 -> v2:
* Fix a bug noticed by Jamal
* Drop patches already merged into net-next
* Add patch 5/5

Patches are cleanup's for the structures of tc actions, except patch 4
which is an improvement.

See each patch for details.

Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Cong Wang (5):
  net_sched: act: hide struct tcf_common from API
  net_sched: act: refactor cleanup ops
  net_sched: act: move tcf_hashinfo_init() into tcf_register_action()
  net_sched: act: refuse to remove bound action outside
  net_sched: act: clean up tca_action_flush()

 include/net/act_api.h           |  22 +++----
 include/net/tc_act/tc_csum.h    |   4 +-
 include/net/tc_act/tc_defact.h  |   4 +-
 include/net/tc_act/tc_gact.h    |   4 +-
 include/net/tc_act/tc_ipt.h     |   4 +-
 include/net/tc_act/tc_mirred.h  |   4 +-
 include/net/tc_act/tc_nat.h     |   4 +-
 include/net/tc_act/tc_pedit.h   |   4 +-
 include/net/tc_act/tc_skbedit.h |   4 +-
 net/sched/act_api.c             | 142 +++++++++++++++++++++++++---------------
 net/sched/act_csum.c            |  31 +++------
 net/sched/act_gact.c            |  34 +++-------
 net/sched/act_ipt.c             |  68 ++++++-------------
 net/sched/act_mirred.c          |  56 ++++------------
 net/sched/act_nat.c             |  33 +++-------
 net/sched/act_pedit.c           |  45 ++++---------
 net/sched/act_police.c          |  22 +------
 net/sched/act_simple.c          |  64 +++++-------------
 net/sched/act_skbedit.c         |  36 +++-------
 19 files changed, 213 insertions(+), 372 deletions(-)

-- 
1.8.3.1

[Patch net-next v3 1/5] net_sched: act: hide struct tcf_common from API

[Cong Wang]

Now we can totally hide it from modules. tcf_hash_*() API's
will operate on struct tc_action, modules don't need to care about
the details.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/act_api.h           | 16 +++++++--------
 include/net/tc_act/tc_csum.h    |  4 ++--
 include/net/tc_act/tc_defact.h  |  4 ++--
 include/net/tc_act/tc_gact.h    |  4 ++--
 include/net/tc_act/tc_ipt.h     |  4 ++--
 include/net/tc_act/tc_mirred.h  |  4 ++--
 include/net/tc_act/tc_nat.h     |  4 ++--
 include/net/tc_act/tc_pedit.h   |  4 ++--
 include/net/tc_act/tc_skbedit.h |  4 ++--
 net/sched/act_api.c             | 43 ++++++++++++++++++++++++++++-------------
 net/sched/act_csum.c            | 24 ++++++++---------------
 net/sched/act_gact.c            | 27 ++++++++------------------
 net/sched/act_ipt.c             | 39 ++++++++++++++-----------------------
 net/sched/act_mirred.c          | 32 +++++++++++-------------------
 net/sched/act_nat.c             | 25 ++++++++----------------
 net/sched/act_pedit.c           | 25 ++++++++++--------------
 net/sched/act_police.c          | 10 +---------
 net/sched/act_simple.c          | 39 +++++++++++++------------------------
 net/sched/act_skbedit.c         | 29 +++++++++------------------
 19 files changed, 135 insertions(+), 206 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 788d837..24ae910 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -98,16 +98,14 @@ struct tc_action_ops {
 };
 
 int tcf_hash_search(struct tc_action *a, u32 index);
-void tcf_hash_destroy(struct tcf_common *p, struct tcf_hashinfo *hinfo);
-int tcf_hash_release(struct tcf_common *p, int bind,
-		     struct tcf_hashinfo *hinfo);
+void tcf_hash_destroy(struct tc_action *a);
+int tcf_hash_release(struct tc_action *a, int bind);
 u32 tcf_hash_new_index(struct tcf_hashinfo *hinfo);
-struct tcf_common *tcf_hash_check(u32 index, struct tc_action *a,
-				  int bind);
-struct tcf_common *tcf_hash_create(u32 index, struct nlattr *est,
-				   struct tc_action *a, int size,
-				   int bind);
-void tcf_hash_insert(struct tcf_common *p, struct tcf_hashinfo *hinfo);
+int tcf_hash_check(u32 index, struct tc_action *a, int bind);
+int tcf_hash_create(u32 index, struct nlattr *est, struct tc_action *a,
+		    int size, int bind);
+void tcf_hash_cleanup(struct tc_action *a, struct nlattr *est);
+void tcf_hash_insert(struct tc_action *a);
 
 int tcf_register_action(struct tc_action_ops *a);
 int tcf_unregister_action(struct tc_action_ops *a);
diff --git a/include/net/tc_act/tc_csum.h b/include/net/tc_act/tc_csum.h
index 9e8710b..fa8f5fa 100644
--- a/include/net/tc_act/tc_csum.h
+++ b/include/net/tc_act/tc_csum.h
@@ -9,7 +9,7 @@ struct tcf_csum {
 
 	u32 update_flags;
 };
-#define to_tcf_csum(pc) \
-	container_of(pc,struct tcf_csum,common)
+#define to_tcf_csum(a) \
+	container_of(a->priv,struct tcf_csum,common)
 
 #endif /* __NET_TC_CSUM_H */
diff --git a/include/net/tc_act/tc_defact.h b/include/net/tc_act/tc_defact.h
index 65f024b..9763dcb 100644
--- a/include/net/tc_act/tc_defact.h
+++ b/include/net/tc_act/tc_defact.h
@@ -8,7 +8,7 @@ struct tcf_defact {
 	u32     		tcfd_datalen;
 	void    		*tcfd_defdata;
 };
-#define to_defact(pc) \
-	container_of(pc, struct tcf_defact, common)
+#define to_defact(a) \
+	container_of(a->priv, struct tcf_defact, common)
 
 #endif /* __NET_TC_DEF_H */
diff --git a/include/net/tc_act/tc_gact.h b/include/net/tc_act/tc_gact.h
index 9e3f676..9fc9b57 100644
--- a/include/net/tc_act/tc_gact.h
+++ b/include/net/tc_act/tc_gact.h
@@ -11,7 +11,7 @@ struct tcf_gact {
         int			tcfg_paction;
 #endif
 };
-#define to_gact(pc) \
-	container_of(pc, struct tcf_gact, common)
+#define to_gact(a) \
+	container_of(a->priv, struct tcf_gact, common)
 
 #endif /* __NET_TC_GACT_H */
diff --git a/include/net/tc_act/tc_ipt.h b/include/net/tc_act/tc_ipt.h
index f7d25df..c0f4193 100644
--- a/include/net/tc_act/tc_ipt.h
+++ b/include/net/tc_act/tc_ipt.h
@@ -11,7 +11,7 @@ struct tcf_ipt {
 	char			*tcfi_tname;
 	struct xt_entry_target	*tcfi_t;
 };
-#define to_ipt(pc) \
-	container_of(pc, struct tcf_ipt, common)
+#define to_ipt(a) \
+	container_of(a->priv, struct tcf_ipt, common)
 
 #endif /* __NET_TC_IPT_H */
diff --git a/include/net/tc_act/tc_mirred.h b/include/net/tc_act/tc_mirred.h
index cfe2943..4dd77a1 100644
--- a/include/net/tc_act/tc_mirred.h
+++ b/include/net/tc_act/tc_mirred.h
@@ -11,7 +11,7 @@ struct tcf_mirred {
 	struct net_device	*tcfm_dev;
 	struct list_head	tcfm_list;
 };
-#define to_mirred(pc) \
-	container_of(pc, struct tcf_mirred, common)
+#define to_mirred(a) \
+	container_of(a->priv, struct tcf_mirred, common)
 
 #endif /* __NET_TC_MIR_H */
diff --git a/include/net/tc_act/tc_nat.h b/include/net/tc_act/tc_nat.h
index 4a691f3..63d8e9c 100644
--- a/include/net/tc_act/tc_nat.h
+++ b/include/net/tc_act/tc_nat.h
@@ -13,9 +13,9 @@ struct tcf_nat {
 	u32 flags;
 };
 
-static inline struct tcf_nat *to_tcf_nat(struct tcf_common *pc)
+static inline struct tcf_nat *to_tcf_nat(struct tc_action *a)
 {
-	return container_of(pc, struct tcf_nat, common);
+	return container_of(a->priv, struct tcf_nat, common);
 }
 
 #endif /* __NET_TC_NAT_H */
diff --git a/include/net/tc_act/tc_pedit.h b/include/net/tc_act/tc_pedit.h
index e6f6e15..5b80998 100644
--- a/include/net/tc_act/tc_pedit.h
+++ b/include/net/tc_act/tc_pedit.h
@@ -9,7 +9,7 @@ struct tcf_pedit {
 	unsigned char		tcfp_flags;
 	struct tc_pedit_key	*tcfp_keys;
 };
-#define to_pedit(pc) \
-	container_of(pc, struct tcf_pedit, common)
+#define to_pedit(a) \
+	container_of(a->priv, struct tcf_pedit, common)
 
 #endif /* __NET_TC_PED_H */
diff --git a/include/net/tc_act/tc_skbedit.h b/include/net/tc_act/tc_skbedit.h
index dd5d86f..0df9a0d 100644
--- a/include/net/tc_act/tc_skbedit.h
+++ b/include/net/tc_act/tc_skbedit.h
@@ -29,7 +29,7 @@ struct tcf_skbedit {
 	u16			queue_mapping;
 	/* XXX: 16-bit pad here? */
 };
-#define to_skbedit(pc) \
-	container_of(pc, struct tcf_skbedit, common)
+#define to_skbedit(a) \
+	container_of(a->priv, struct tcf_skbedit, common)
 
 #endif /* __NET_TC_SKBEDIT_H */
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 72bdc71..4f2b807 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -27,8 +27,11 @@
 #include <net/act_api.h>
 #include <net/netlink.h>
 
-void tcf_hash_destroy(struct tcf_common *p, struct tcf_hashinfo *hinfo)
+void tcf_hash_destroy(struct tc_action *a)
 {
+	struct tcf_common *p = a->priv;
+	struct tcf_hashinfo *hinfo = a->ops->hinfo;
+
 	spin_lock_bh(&hinfo->lock);
 	hlist_del(&p->tcfc_head);
 	spin_unlock_bh(&hinfo->lock);
@@ -42,9 +45,9 @@ void tcf_hash_destroy(struct tcf_common *p, struct tcf_hashinfo *hinfo)
 }
 EXPORT_SYMBOL(tcf_hash_destroy);
 
-int tcf_hash_release(struct tcf_common *p, int bind,
-		     struct tcf_hashinfo *hinfo)
+int tcf_hash_release(struct tc_action *a, int bind)
 {
+	struct tcf_common *p = a->priv;
 	int ret = 0;
 
 	if (p) {
@@ -53,7 +56,7 @@ int tcf_hash_release(struct tcf_common *p, int bind,
 
 		p->tcfc_refcnt--;
 		if (p->tcfc_bindcnt <= 0 && p->tcfc_refcnt <= 0) {
-			tcf_hash_destroy(p, hinfo);
+			tcf_hash_destroy(a);
 			ret = 1;
 		}
 	}
@@ -127,7 +130,8 @@ static int tcf_del_walker(struct sk_buff *skb, struct tc_action *a)
 	for (i = 0; i < (hinfo->hmask + 1); i++) {
 		head = &hinfo->htab[tcf_hash(i, hinfo->hmask)];
 		hlist_for_each_entry_safe(p, n, head, tcfc_head) {
-			if (ACT_P_DELETED == tcf_hash_release(p, 0, hinfo)) {
+			a->priv = p;
+			if (ACT_P_DELETED == tcf_hash_release(a, 0)) {
 				module_put(a->ops->owner);
 				n_i++;
 			}
@@ -198,7 +202,7 @@ int tcf_hash_search(struct tc_action *a, u32 index)
 }
 EXPORT_SYMBOL(tcf_hash_search);
 
-struct tcf_common *tcf_hash_check(u32 index, struct tc_action *a, int bind)
+int tcf_hash_check(u32 index, struct tc_action *a, int bind)
 {
 	struct tcf_hashinfo *hinfo = a->ops->hinfo;
 	struct tcf_common *p = NULL;
@@ -207,19 +211,30 @@ struct tcf_common *tcf_hash_check(u32 index, struct tc_action *a, int bind)
 			p->tcfc_bindcnt++;
 		p->tcfc_refcnt++;
 		a->priv = p;
+		return 1;
 	}
-	return p;
+	return 0;
 }
 EXPORT_SYMBOL(tcf_hash_check);
 
-struct tcf_common *tcf_hash_create(u32 index, struct nlattr *est,
-				   struct tc_action *a, int size, int bind)
+void tcf_hash_cleanup(struct tc_action *a, struct nlattr *est)
+{
+	struct tcf_common *pc = a->priv;
+	if (est)
+		gen_kill_estimator(&pc->tcfc_bstats,
+				   &pc->tcfc_rate_est);
+	kfree_rcu(pc, tcfc_rcu);
+}
+EXPORT_SYMBOL(tcf_hash_cleanup);
+
+int tcf_hash_create(u32 index, struct nlattr *est, struct tc_action *a,
+		    int size, int bind)
 {
 	struct tcf_hashinfo *hinfo = a->ops->hinfo;
 	struct tcf_common *p = kzalloc(size, GFP_KERNEL);
 
 	if (unlikely(!p))
-		return ERR_PTR(-ENOMEM);
+		return -ENOMEM;
 	p->tcfc_refcnt = 1;
 	if (bind)
 		p->tcfc_bindcnt = 1;
@@ -234,17 +249,19 @@ struct tcf_common *tcf_hash_create(u32 index, struct nlattr *est,
 					    &p->tcfc_lock, est);
 		if (err) {
 			kfree(p);
-			return ERR_PTR(err);
+			return err;
 		}
 	}
 
 	a->priv = (void *) p;
-	return p;
+	return 0;
 }
 EXPORT_SYMBOL(tcf_hash_create);
 
-void tcf_hash_insert(struct tcf_common *p, struct tcf_hashinfo *hinfo)
+void tcf_hash_insert(struct tc_action *a)
 {
+	struct tcf_common *p = a->priv;
+	struct tcf_hashinfo *hinfo = a->ops->hinfo;
 	unsigned int h = tcf_hash(p->tcfc_index, hinfo->hmask);
 
 	spin_lock_bh(&hinfo->lock);
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 2210187..f0f6e7a 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -48,7 +48,6 @@ static int tcf_csum_init(struct net *n, struct nlattr *nla, struct nlattr *est,
 {
 	struct nlattr *tb[TCA_CSUM_MAX + 1];
 	struct tc_csum *parm;
-	struct tcf_common *pc;
 	struct tcf_csum *p;
 	int ret = 0, err;
 
@@ -63,38 +62,31 @@ static int tcf_csum_init(struct net *n, struct nlattr *nla, struct nlattr *est,
 		return -EINVAL;
 	parm = nla_data(tb[TCA_CSUM_PARMS]);
 
-	pc = tcf_hash_check(parm->index, a, bind);
-	if (!pc) {
-		pc = tcf_hash_create(parm->index, est, a, sizeof(*p), bind);
-		if (IS_ERR(pc))
-			return PTR_ERR(pc);
+	if (!tcf_hash_check(parm->index, a, bind)) {
+		ret = tcf_hash_create(parm->index, est, a, sizeof(*p), bind);
+		if (ret)
+			return ret;
 		ret = ACT_P_CREATED;
 	} else {
 		if (bind)/* dont override defaults */
 			return 0;
-		tcf_hash_release(pc, bind, a->ops->hinfo);
+		tcf_hash_release(a, bind);
 		if (!ovr)
 			return -EEXIST;
 	}
 
-	p = to_tcf_csum(pc);
+	p = to_tcf_csum(a);
 	spin_lock_bh(&p->tcf_lock);
 	p->tcf_action = parm->action;
 	p->update_flags = parm->update_flags;
 	spin_unlock_bh(&p->tcf_lock);
 
 	if (ret == ACT_P_CREATED)
-		tcf_hash_insert(pc, a->ops->hinfo);
+		tcf_hash_insert(a);
 
 	return ret;
 }
 
-static int tcf_csum_cleanup(struct tc_action *a, int bind)
-{
-	struct tcf_csum *p = a->priv;
-	return tcf_hash_release(&p->common, bind, &csum_hash_info);
-}
-
 /**
  * tcf_csum_skb_nextlayer - Get next layer pointer
  * @skb: sk_buff to use
@@ -574,7 +566,7 @@ static struct tc_action_ops act_csum_ops = {
 	.owner		= THIS_MODULE,
 	.act		= tcf_csum,
 	.dump		= tcf_csum_dump,
-	.cleanup	= tcf_csum_cleanup,
+	.cleanup	= tcf_hash_release,
 	.init		= tcf_csum_init,
 };
 
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index a0eed30..af6c0ac 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -57,7 +57,6 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
 	struct nlattr *tb[TCA_GACT_MAX + 1];
 	struct tc_gact *parm;
 	struct tcf_gact *gact;
-	struct tcf_common *pc;
 	int ret = 0;
 	int err;
 #ifdef CONFIG_GACT_PROB
@@ -86,21 +85,20 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
 	}
 #endif
 
-	pc = tcf_hash_check(parm->index, a, bind);
-	if (!pc) {
-		pc = tcf_hash_create(parm->index, est, a, sizeof(*gact), bind);
-		if (IS_ERR(pc))
-			return PTR_ERR(pc);
+	if (!tcf_hash_check(parm->index, a, bind)) {
+		ret = tcf_hash_create(parm->index, est, a, sizeof(*gact), bind);
+		if (ret)
+			return ret;
 		ret = ACT_P_CREATED;
 	} else {
 		if (bind)/* dont override defaults */
 			return 0;
-		tcf_hash_release(pc, bind, a->ops->hinfo);
+		tcf_hash_release(a, bind);
 		if (!ovr)
 			return -EEXIST;
 	}
 
-	gact = to_gact(pc);
+	gact = to_gact(a);
 
 	spin_lock_bh(&gact->tcf_lock);
 	gact->tcf_action = parm->action;
@@ -113,19 +111,10 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
 #endif
 	spin_unlock_bh(&gact->tcf_lock);
 	if (ret == ACT_P_CREATED)
-		tcf_hash_insert(pc, a->ops->hinfo);
+		tcf_hash_insert(a);
 	return ret;
 }
 
-static int tcf_gact_cleanup(struct tc_action *a, int bind)
-{
-	struct tcf_gact *gact = a->priv;
-
-	if (gact)
-		return tcf_hash_release(&gact->common, bind, a->ops->hinfo);
-	return 0;
-}
-
 static int tcf_gact(struct sk_buff *skb, const struct tc_action *a,
 		    struct tcf_result *res)
 {
@@ -196,7 +185,7 @@ static struct tc_action_ops act_gact_ops = {
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_gact,
 	.dump		=	tcf_gact_dump,
-	.cleanup	=	tcf_gact_cleanup,
+	.cleanup	=	tcf_hash_release,
 	.init		=	tcf_gact_init,
 };
 
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 0a6d621..f5e6978 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -69,8 +69,9 @@ static void ipt_destroy_target(struct xt_entry_target *t)
 	module_put(par.target->me);
 }
 
-static int tcf_ipt_release(struct tcf_ipt *ipt, int bind)
+static int tcf_ipt_release(struct tc_action *a, int bind)
 {
+	struct tcf_ipt *ipt = to_ipt(a);
 	int ret = 0;
 	if (ipt) {
 		if (bind)
@@ -80,7 +81,7 @@ static int tcf_ipt_release(struct tcf_ipt *ipt, int bind)
 			ipt_destroy_target(ipt->tcfi_t);
 			kfree(ipt->tcfi_tname);
 			kfree(ipt->tcfi_t);
-			tcf_hash_destroy(&ipt->common, &ipt_hash_info);
+			tcf_hash_destroy(a);
 			ret = ACT_P_DELETED;
 		}
 	}
@@ -99,7 +100,6 @@ static int tcf_ipt_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 {
 	struct nlattr *tb[TCA_IPT_MAX + 1];
 	struct tcf_ipt *ipt;
-	struct tcf_common *pc;
 	struct xt_entry_target *td, *t;
 	char *tname;
 	int ret = 0, err;
@@ -125,21 +125,20 @@ static int tcf_ipt_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 	if (tb[TCA_IPT_INDEX] != NULL)
 		index = nla_get_u32(tb[TCA_IPT_INDEX]);
 
-	pc = tcf_hash_check(index, a, bind);
-	if (!pc) {
-		pc = tcf_hash_create(index, est, a, sizeof(*ipt), bind);
-		if (IS_ERR(pc))
-			return PTR_ERR(pc);
+	if (!tcf_hash_check(index, a, bind) ) {
+		ret = tcf_hash_create(index, est, a, sizeof(*ipt), bind);
+		if (ret)
+			return ret;
 		ret = ACT_P_CREATED;
 	} else {
 		if (bind)/* dont override defaults */
 			return 0;
-		tcf_ipt_release(to_ipt(pc), bind);
+		tcf_ipt_release(a, bind);
 
 		if (!ovr)
 			return -EEXIST;
 	}
-	ipt = to_ipt(pc);
+	ipt = to_ipt(a);
 
 	hook = nla_get_u32(tb[TCA_IPT_HOOK]);
 
@@ -170,7 +169,7 @@ static int tcf_ipt_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 	ipt->tcfi_hook  = hook;
 	spin_unlock_bh(&ipt->tcf_lock);
 	if (ret == ACT_P_CREATED)
-		tcf_hash_insert(pc, a->ops->hinfo);
+		tcf_hash_insert(a);
 	return ret;
 
 err3:
@@ -178,21 +177,11 @@ err3:
 err2:
 	kfree(tname);
 err1:
-	if (ret == ACT_P_CREATED) {
-		if (est)
-			gen_kill_estimator(&pc->tcfc_bstats,
-					   &pc->tcfc_rate_est);
-		kfree_rcu(pc, tcfc_rcu);
-	}
+	if (ret == ACT_P_CREATED)
+		tcf_hash_cleanup(a, est);
 	return err;
 }
 
-static int tcf_ipt_cleanup(struct tc_action *a, int bind)
-{
-	struct tcf_ipt *ipt = a->priv;
-	return tcf_ipt_release(ipt, bind);
-}
-
 static int tcf_ipt(struct sk_buff *skb, const struct tc_action *a,
 		   struct tcf_result *res)
 {
@@ -289,7 +278,7 @@ static struct tc_action_ops act_ipt_ops = {
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_ipt,
 	.dump		=	tcf_ipt_dump,
-	.cleanup	=	tcf_ipt_cleanup,
+	.cleanup	=	tcf_ipt_release,
 	.init		=	tcf_ipt_init,
 };
 
@@ -300,7 +289,7 @@ static struct tc_action_ops act_xt_ops = {
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_ipt,
 	.dump		=	tcf_ipt_dump,
-	.cleanup	=	tcf_ipt_cleanup,
+	.cleanup	=	tcf_ipt_release,
 	.init		=	tcf_ipt_init,
 };
 
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 0b2c6d3..3edeeca 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -33,8 +33,9 @@
 static LIST_HEAD(mirred_list);
 static struct tcf_hashinfo mirred_hash_info;
 
-static int tcf_mirred_release(struct tcf_mirred *m, int bind)
+static int tcf_mirred_release(struct tc_action *a, int bind)
 {
+	struct tcf_mirred *m = to_mirred(a);
 	if (m) {
 		if (bind)
 			m->tcf_bindcnt--;
@@ -43,7 +44,7 @@ static int tcf_mirred_release(struct tcf_mirred *m, int bind)
 			list_del(&m->tcfm_list);
 			if (m->tcfm_dev)
 				dev_put(m->tcfm_dev);
-			tcf_hash_destroy(&m->common, &mirred_hash_info);
+			tcf_hash_destroy(a);
 			return 1;
 		}
 	}
@@ -61,7 +62,6 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
 	struct nlattr *tb[TCA_MIRRED_MAX + 1];
 	struct tc_mirred *parm;
 	struct tcf_mirred *m;
-	struct tcf_common *pc;
 	struct net_device *dev;
 	int ret, ok_push = 0;
 
@@ -101,21 +101,20 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
 		dev = NULL;
 	}
 
-	pc = tcf_hash_check(parm->index, a, bind);
-	if (!pc) {
+	if (!tcf_hash_check(parm->index, a, bind)) {
 		if (dev == NULL)
 			return -EINVAL;
-		pc = tcf_hash_create(parm->index, est, a, sizeof(*m), bind);
-		if (IS_ERR(pc))
-			return PTR_ERR(pc);
+		ret = tcf_hash_create(parm->index, est, a, sizeof(*m), bind);
+		if (ret)
+			return ret;
 		ret = ACT_P_CREATED;
 	} else {
 		if (!ovr) {
-			tcf_mirred_release(to_mirred(pc), bind);
+			tcf_mirred_release(a, bind);
 			return -EEXIST;
 		}
 	}
-	m = to_mirred(pc);
+	m = to_mirred(a);
 
 	spin_lock_bh(&m->tcf_lock);
 	m->tcf_action = parm->action;
@@ -131,21 +130,12 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
 	spin_unlock_bh(&m->tcf_lock);
 	if (ret == ACT_P_CREATED) {
 		list_add(&m->tcfm_list, &mirred_list);
-		tcf_hash_insert(pc, a->ops->hinfo);
+		tcf_hash_insert(a);
 	}
 
 	return ret;
 }
 
-static int tcf_mirred_cleanup(struct tc_action *a, int bind)
-{
-	struct tcf_mirred *m = a->priv;
-
-	if (m)
-		return tcf_mirred_release(m, bind);
-	return 0;
-}
-
 static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
 		      struct tcf_result *res)
 {
@@ -259,7 +249,7 @@ static struct tc_action_ops act_mirred_ops = {
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_mirred,
 	.dump		=	tcf_mirred_dump,
-	.cleanup	=	tcf_mirred_cleanup,
+	.cleanup	=	tcf_mirred_release,
 	.init		=	tcf_mirred_init,
 };
 
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 81f0404..ce9a391 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -44,7 +44,6 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 	struct tc_nat *parm;
 	int ret = 0, err;
 	struct tcf_nat *p;
-	struct tcf_common *pc;
 
 	if (nla == NULL)
 		return -EINVAL;
@@ -57,20 +56,19 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 		return -EINVAL;
 	parm = nla_data(tb[TCA_NAT_PARMS]);
 
-	pc = tcf_hash_check(parm->index, a, bind);
-	if (!pc) {
-		pc = tcf_hash_create(parm->index, est, a, sizeof(*p), bind);
-		if (IS_ERR(pc))
-			return PTR_ERR(pc);
+	if (!tcf_hash_check(parm->index, a, bind)) {
+		ret = tcf_hash_create(parm->index, est, a, sizeof(*p), bind);
+		if (ret)
+			return ret;
 		ret = ACT_P_CREATED;
 	} else {
 		if (bind)
 			return 0;
-		tcf_hash_release(pc, bind, a->ops->hinfo);
+		tcf_hash_release(a, bind);
 		if (!ovr)
 			return -EEXIST;
 	}
-	p = to_tcf_nat(pc);
+	p = to_tcf_nat(a);
 
 	spin_lock_bh(&p->tcf_lock);
 	p->old_addr = parm->old_addr;
@@ -82,18 +80,11 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 	spin_unlock_bh(&p->tcf_lock);
 
 	if (ret == ACT_P_CREATED)
-		tcf_hash_insert(pc, a->ops->hinfo);
+		tcf_hash_insert(a);
 
 	return ret;
 }
 
-static int tcf_nat_cleanup(struct tc_action *a, int bind)
-{
-	struct tcf_nat *p = a->priv;
-
-	return tcf_hash_release(&p->common, bind, &nat_hash_info);
-}
-
 static int tcf_nat(struct sk_buff *skb, const struct tc_action *a,
 		   struct tcf_result *res)
 {
@@ -298,7 +289,7 @@ static struct tc_action_ops act_nat_ops = {
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_nat,
 	.dump		=	tcf_nat_dump,
-	.cleanup	=	tcf_nat_cleanup,
+	.cleanup	=	tcf_hash_release,
 	.init		=	tcf_nat_init,
 };
 
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index be3f0f6..091ced3 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -39,7 +39,6 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 	struct tc_pedit *parm;
 	int ret = 0, err;
 	struct tcf_pedit *p;
-	struct tcf_common *pc;
 	struct tc_pedit_key *keys = NULL;
 	int ksize;
 
@@ -57,26 +56,22 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 	if (nla_len(tb[TCA_PEDIT_PARMS]) < sizeof(*parm) + ksize)
 		return -EINVAL;
 
-	pc = tcf_hash_check(parm->index, a, bind);
-	if (!pc) {
+	if (!tcf_hash_check(parm->index, a, bind)) {
 		if (!parm->nkeys)
 			return -EINVAL;
-		pc = tcf_hash_create(parm->index, est, a, sizeof(*p), bind);
-		if (IS_ERR(pc))
-			return PTR_ERR(pc);
-		p = to_pedit(pc);
+		ret = tcf_hash_create(parm->index, est, a, sizeof(*p), bind);
+		if (ret)
+			return ret;
+		p = to_pedit(a);
 		keys = kmalloc(ksize, GFP_KERNEL);
 		if (keys == NULL) {
-			if (est)
-				gen_kill_estimator(&pc->tcfc_bstats,
-						   &pc->tcfc_rate_est);
-			kfree_rcu(pc, tcfc_rcu);
+			tcf_hash_cleanup(a, est);
 			return -ENOMEM;
 		}
 		ret = ACT_P_CREATED;
 	} else {
-		p = to_pedit(pc);
-		tcf_hash_release(pc, bind, a->ops->hinfo);
+		p = to_pedit(a);
+		tcf_hash_release(a, bind);
 		if (bind)
 			return 0;
 		if (!ovr)
@@ -100,7 +95,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 	memcpy(p->tcfp_keys, parm->keys, ksize);
 	spin_unlock_bh(&p->tcf_lock);
 	if (ret == ACT_P_CREATED)
-		tcf_hash_insert(pc, a->ops->hinfo);
+		tcf_hash_insert(a);
 	return ret;
 }
 
@@ -110,7 +105,7 @@ static int tcf_pedit_cleanup(struct tc_action *a, int bind)
 
 	if (p) {
 		struct tc_pedit_key *keys = p->tcfp_keys;
-		if (tcf_hash_release(&p->common, bind, &pedit_hash_info)) {
+		if (tcf_hash_release(a, bind)) {
 			kfree(keys);
 			return 1;
 		}
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 1778209..4695d02 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -253,14 +253,6 @@ failure:
 	return err;
 }
 
-static int tcf_act_police_cleanup(struct tc_action *a, int bind)
-{
-	struct tcf_police *p = a->priv;
-	if (p)
-		return tcf_hash_release(&p->common, bind, &police_hash_info);
-	return 0;
-}
-
 static int tcf_act_police(struct sk_buff *skb, const struct tc_action *a,
 			  struct tcf_result *res)
 {
@@ -362,7 +354,7 @@ static struct tc_action_ops act_police_ops = {
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_act_police,
 	.dump		=	tcf_act_police_dump,
-	.cleanup	=	tcf_act_police_cleanup,
+	.cleanup	=	tcf_hash_release,
 	.init		=	tcf_act_police_locate,
 	.walk		=	tcf_act_police_walker
 };
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index 8ef2f1f..11c2922 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -47,8 +47,9 @@ static int tcf_simp(struct sk_buff *skb, const struct tc_action *a,
 	return d->tcf_action;
 }
 
-static int tcf_simp_release(struct tcf_defact *d, int bind)
+static int tcf_simp_release(struct tc_action *a, int bind)
 {
+	struct tcf_defact *d = to_defact(a);
 	int ret = 0;
 	if (d) {
 		if (bind)
@@ -56,7 +57,7 @@ static int tcf_simp_release(struct tcf_defact *d, int bind)
 		d->tcf_refcnt--;
 		if (d->tcf_bindcnt <= 0 && d->tcf_refcnt <= 0) {
 			kfree(d->tcfd_defdata);
-			tcf_hash_destroy(&d->common, &simp_hash_info);
+			tcf_hash_destroy(a);
 			ret = 1;
 		}
 	}
@@ -94,7 +95,6 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
 	struct nlattr *tb[TCA_DEF_MAX + 1];
 	struct tc_defact *parm;
 	struct tcf_defact *d;
-	struct tcf_common *pc;
 	char *defdata;
 	int ret = 0, err;
 
@@ -114,29 +114,25 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
 	parm = nla_data(tb[TCA_DEF_PARMS]);
 	defdata = nla_data(tb[TCA_DEF_DATA]);
 
-	pc = tcf_hash_check(parm->index, a, bind);
-	if (!pc) {
-		pc = tcf_hash_create(parm->index, est, a, sizeof(*d), bind);
-		if (IS_ERR(pc))
-			return PTR_ERR(pc);
+	if (!tcf_hash_check(parm->index, a, bind)) {
+		ret = tcf_hash_create(parm->index, est, a, sizeof(*d), bind);
+		if (ret)
+			return ret;
 
-		d = to_defact(pc);
+		d = to_defact(a);
 		ret = alloc_defdata(d, defdata);
 		if (ret < 0) {
-			if (est)
-				gen_kill_estimator(&pc->tcfc_bstats,
-						   &pc->tcfc_rate_est);
-			kfree_rcu(pc, tcfc_rcu);
+			tcf_hash_cleanup(a, est);
 			return ret;
 		}
 		d->tcf_action = parm->action;
 		ret = ACT_P_CREATED;
 	} else {
-		d = to_defact(pc);
+		d = to_defact(a);
 
 		if (bind)
 			return 0;
-		tcf_simp_release(d, bind);
+		tcf_simp_release(a, bind);
 		if (!ovr)
 			return -EEXIST;
 
@@ -144,19 +140,10 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
 	}
 
 	if (ret == ACT_P_CREATED)
-		tcf_hash_insert(pc, a->ops->hinfo);
+		tcf_hash_insert(a);
 	return ret;
 }
 
-static int tcf_simp_cleanup(struct tc_action *a, int bind)
-{
-	struct tcf_defact *d = a->priv;
-
-	if (d)
-		return tcf_simp_release(d, bind);
-	return 0;
-}
-
 static int tcf_simp_dump(struct sk_buff *skb, struct tc_action *a,
 			 int bind, int ref)
 {
@@ -192,7 +179,7 @@ static struct tc_action_ops act_simp_ops = {
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_simp,
 	.dump		=	tcf_simp_dump,
-	.cleanup	=	tcf_simp_cleanup,
+	.cleanup	=	tcf_simp_release,
 	.init		=	tcf_simp_init,
 };
 
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index 9872508..71fd2d4 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -65,7 +65,6 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 	struct nlattr *tb[TCA_SKBEDIT_MAX + 1];
 	struct tc_skbedit *parm;
 	struct tcf_skbedit *d;
-	struct tcf_common *pc;
 	u32 flags = 0, *priority = NULL, *mark = NULL;
 	u16 *queue_mapping = NULL;
 	int ret = 0, err;
@@ -100,19 +99,18 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 
 	parm = nla_data(tb[TCA_SKBEDIT_PARMS]);
 
-	pc = tcf_hash_check(parm->index, a, bind);
-	if (!pc) {
-		pc = tcf_hash_create(parm->index, est, a, sizeof(*d), bind);
-		if (IS_ERR(pc))
-			return PTR_ERR(pc);
+	if (!tcf_hash_check(parm->index, a, bind)) {
+		ret = tcf_hash_create(parm->index, est, a, sizeof(*d), bind);
+		if (ret)
+			return ret;
 
-		d = to_skbedit(pc);
+		d = to_skbedit(a);
 		ret = ACT_P_CREATED;
 	} else {
-		d = to_skbedit(pc);
+		d = to_skbedit(a);
 		if (bind)
 			return 0;
-		tcf_hash_release(pc, bind, a->ops->hinfo);
+		tcf_hash_release(a, bind);
 		if (!ovr)
 			return -EEXIST;
 	}
@@ -132,19 +130,10 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 	spin_unlock_bh(&d->tcf_lock);
 
 	if (ret == ACT_P_CREATED)
-		tcf_hash_insert(pc, a->ops->hinfo);
+		tcf_hash_insert(a);
 	return ret;
 }
 
-static int tcf_skbedit_cleanup(struct tc_action *a, int bind)
-{
-	struct tcf_skbedit *d = a->priv;
-
-	if (d)
-		return tcf_hash_release(&d->common, bind, &skbedit_hash_info);
-	return 0;
-}
-
 static int tcf_skbedit_dump(struct sk_buff *skb, struct tc_action *a,
 			    int bind, int ref)
 {
@@ -191,7 +180,7 @@ static struct tc_action_ops act_skbedit_ops = {
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_skbedit,
 	.dump		=	tcf_skbedit_dump,
-	.cleanup	=	tcf_skbedit_cleanup,
+	.cleanup	=	tcf_hash_release,
 	.init		=	tcf_skbedit_init,
 };
 
-- 
1.8.3.1

Re: [Patch net-next v3 1/5] net_sched: act: hide struct tcf_common from API

[Jamal Hadi Salim]

On 02/11/14 20:07, Cong Wang wrote:
> Now we can totally hide it from modules. tcf_hash_*() API's
> will operate on struct tc_action, modules don't need to care about
> the details.
>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>


> ---
>   include/net/act_api.h           | 16 +++++++--------
>   include/net/tc_act/tc_csum.h    |  4 ++--
>   include/net/tc_act/tc_defact.h  |  4 ++--
>   include/net/tc_act/tc_gact.h    |  4 ++--
>   include/net/tc_act/tc_ipt.h     |  4 ++--
>   include/net/tc_act/tc_mirred.h  |  4 ++--
>   include/net/tc_act/tc_nat.h     |  4 ++--
>   include/net/tc_act/tc_pedit.h   |  4 ++--
>   include/net/tc_act/tc_skbedit.h |  4 ++--
>   net/sched/act_api.c             | 43 ++++++++++++++++++++++++++++-------------
>   net/sched/act_csum.c            | 24 ++++++++---------------
>   net/sched/act_gact.c            | 27 ++++++++------------------
>   net/sched/act_ipt.c             | 39 ++++++++++++++-----------------------
>   net/sched/act_mirred.c          | 32 +++++++++++-------------------
>   net/sched/act_nat.c             | 25 ++++++++----------------
>   net/sched/act_pedit.c           | 25 ++++++++++--------------
>   net/sched/act_police.c          | 10 +---------
>   net/sched/act_simple.c          | 39 +++++++++++++------------------------
>   net/sched/act_skbedit.c         | 29 +++++++++------------------
>   19 files changed, 135 insertions(+), 206 deletions(-)
>
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index 788d837..24ae910 100644
> --- a/include/net/act_api.h
> +++ b/include/net/act_api.h
> @@ -98,16 +98,14 @@ struct tc_action_ops {
>   };
>
>   int tcf_hash_search(struct tc_action *a, u32 index);
> -void tcf_hash_destroy(struct tcf_common *p, struct tcf_hashinfo *hinfo);
> -int tcf_hash_release(struct tcf_common *p, int bind,
> -		     struct tcf_hashinfo *hinfo);
> +void tcf_hash_destroy(struct tc_action *a);
> +int tcf_hash_release(struct tc_action *a, int bind);
>   u32 tcf_hash_new_index(struct tcf_hashinfo *hinfo);
> -struct tcf_common *tcf_hash_check(u32 index, struct tc_action *a,
> -				  int bind);
> -struct tcf_common *tcf_hash_create(u32 index, struct nlattr *est,
> -				   struct tc_action *a, int size,
> -				   int bind);
> -void tcf_hash_insert(struct tcf_common *p, struct tcf_hashinfo *hinfo);
> +int tcf_hash_check(u32 index, struct tc_action *a, int bind);
> +int tcf_hash_create(u32 index, struct nlattr *est, struct tc_action *a,
> +		    int size, int bind);
> +void tcf_hash_cleanup(struct tc_action *a, struct nlattr *est);
> +void tcf_hash_insert(struct tc_action *a);
>
>   int tcf_register_action(struct tc_action_ops *a);
>   int tcf_unregister_action(struct tc_action_ops *a);
> diff --git a/include/net/tc_act/tc_csum.h b/include/net/tc_act/tc_csum.h
> index 9e8710b..fa8f5fa 100644
> --- a/include/net/tc_act/tc_csum.h
> +++ b/include/net/tc_act/tc_csum.h
> @@ -9,7 +9,7 @@ struct tcf_csum {
>
>   	u32 update_flags;
>   };
> -#define to_tcf_csum(pc) \
> -	container_of(pc,struct tcf_csum,common)
> +#define to_tcf_csum(a) \
> +	container_of(a->priv,struct tcf_csum,common)
>
>   #endif /* __NET_TC_CSUM_H */
> diff --git a/include/net/tc_act/tc_defact.h b/include/net/tc_act/tc_defact.h
> index 65f024b..9763dcb 100644
> --- a/include/net/tc_act/tc_defact.h
> +++ b/include/net/tc_act/tc_defact.h
> @@ -8,7 +8,7 @@ struct tcf_defact {
>   	u32     		tcfd_datalen;
>   	void    		*tcfd_defdata;
>   };
> -#define to_defact(pc) \
> -	container_of(pc, struct tcf_defact, common)
> +#define to_defact(a) \
> +	container_of(a->priv, struct tcf_defact, common)
>
>   #endif /* __NET_TC_DEF_H */
> diff --git a/include/net/tc_act/tc_gact.h b/include/net/tc_act/tc_gact.h
> index 9e3f676..9fc9b57 100644
> --- a/include/net/tc_act/tc_gact.h
> +++ b/include/net/tc_act/tc_gact.h
> @@ -11,7 +11,7 @@ struct tcf_gact {
>           int			tcfg_paction;
>   #endif
>   };
> -#define to_gact(pc) \
> -	container_of(pc, struct tcf_gact, common)
> +#define to_gact(a) \
> +	container_of(a->priv, struct tcf_gact, common)
>
>   #endif /* __NET_TC_GACT_H */
> diff --git a/include/net/tc_act/tc_ipt.h b/include/net/tc_act/tc_ipt.h
> index f7d25df..c0f4193 100644
> --- a/include/net/tc_act/tc_ipt.h
> +++ b/include/net/tc_act/tc_ipt.h
> @@ -11,7 +11,7 @@ struct tcf_ipt {
>   	char			*tcfi_tname;
>   	struct xt_entry_target	*tcfi_t;
>   };
> -#define to_ipt(pc) \
> -	container_of(pc, struct tcf_ipt, common)
> +#define to_ipt(a) \
> +	container_of(a->priv, struct tcf_ipt, common)
>
>   #endif /* __NET_TC_IPT_H */
> diff --git a/include/net/tc_act/tc_mirred.h b/include/net/tc_act/tc_mirred.h
> index cfe2943..4dd77a1 100644
> --- a/include/net/tc_act/tc_mirred.h
> +++ b/include/net/tc_act/tc_mirred.h
> @@ -11,7 +11,7 @@ struct tcf_mirred {
>   	struct net_device	*tcfm_dev;
>   	struct list_head	tcfm_list;
>   };
> -#define to_mirred(pc) \
> -	container_of(pc, struct tcf_mirred, common)
> +#define to_mirred(a) \
> +	container_of(a->priv, struct tcf_mirred, common)
>
>   #endif /* __NET_TC_MIR_H */
> diff --git a/include/net/tc_act/tc_nat.h b/include/net/tc_act/tc_nat.h
> index 4a691f3..63d8e9c 100644
> --- a/include/net/tc_act/tc_nat.h
> +++ b/include/net/tc_act/tc_nat.h
> @@ -13,9 +13,9 @@ struct tcf_nat {
>   	u32 flags;
>   };
>
> -static inline struct tcf_nat *to_tcf_nat(struct tcf_common *pc)
> +static inline struct tcf_nat *to_tcf_nat(struct tc_action *a)
>   {
> -	return container_of(pc, struct tcf_nat, common);
> +	return container_of(a->priv, struct tcf_nat, common);
>   }
>
>   #endif /* __NET_TC_NAT_H */
> diff --git a/include/net/tc_act/tc_pedit.h b/include/net/tc_act/tc_pedit.h
> index e6f6e15..5b80998 100644
> --- a/include/net/tc_act/tc_pedit.h
> +++ b/include/net/tc_act/tc_pedit.h
> @@ -9,7 +9,7 @@ struct tcf_pedit {
>   	unsigned char		tcfp_flags;
>   	struct tc_pedit_key	*tcfp_keys;
>   };
> -#define to_pedit(pc) \
> -	container_of(pc, struct tcf_pedit, common)
> +#define to_pedit(a) \
> +	container_of(a->priv, struct tcf_pedit, common)
>
>   #endif /* __NET_TC_PED_H */
> diff --git a/include/net/tc_act/tc_skbedit.h b/include/net/tc_act/tc_skbedit.h
> index dd5d86f..0df9a0d 100644
> --- a/include/net/tc_act/tc_skbedit.h
> +++ b/include/net/tc_act/tc_skbedit.h
> @@ -29,7 +29,7 @@ struct tcf_skbedit {
>   	u16			queue_mapping;
>   	/* XXX: 16-bit pad here? */
>   };
> -#define to_skbedit(pc) \
> -	container_of(pc, struct tcf_skbedit, common)
> +#define to_skbedit(a) \
> +	container_of(a->priv, struct tcf_skbedit, common)
>
>   #endif /* __NET_TC_SKBEDIT_H */
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 72bdc71..4f2b807 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -27,8 +27,11 @@
>   #include <net/act_api.h>
>   #include <net/netlink.h>
>
> -void tcf_hash_destroy(struct tcf_common *p, struct tcf_hashinfo *hinfo)
> +void tcf_hash_destroy(struct tc_action *a)
>   {
> +	struct tcf_common *p = a->priv;
> +	struct tcf_hashinfo *hinfo = a->ops->hinfo;
> +
>   	spin_lock_bh(&hinfo->lock);
>   	hlist_del(&p->tcfc_head);
>   	spin_unlock_bh(&hinfo->lock);
> @@ -42,9 +45,9 @@ void tcf_hash_destroy(struct tcf_common *p, struct tcf_hashinfo *hinfo)
>   }
>   EXPORT_SYMBOL(tcf_hash_destroy);
>
> -int tcf_hash_release(struct tcf_common *p, int bind,
> -		     struct tcf_hashinfo *hinfo)
> +int tcf_hash_release(struct tc_action *a, int bind)
>   {
> +	struct tcf_common *p = a->priv;
>   	int ret = 0;
>
>   	if (p) {
> @@ -53,7 +56,7 @@ int tcf_hash_release(struct tcf_common *p, int bind,
>
>   		p->tcfc_refcnt--;
>   		if (p->tcfc_bindcnt <= 0 && p->tcfc_refcnt <= 0) {
> -			tcf_hash_destroy(p, hinfo);
> +			tcf_hash_destroy(a);
>   			ret = 1;
>   		}
>   	}
> @@ -127,7 +130,8 @@ static int tcf_del_walker(struct sk_buff *skb, struct tc_action *a)
>   	for (i = 0; i < (hinfo->hmask + 1); i++) {
>   		head = &hinfo->htab[tcf_hash(i, hinfo->hmask)];
>   		hlist_for_each_entry_safe(p, n, head, tcfc_head) {
> -			if (ACT_P_DELETED == tcf_hash_release(p, 0, hinfo)) {
> +			a->priv = p;
> +			if (ACT_P_DELETED == tcf_hash_release(a, 0)) {
>   				module_put(a->ops->owner);
>   				n_i++;
>   			}
> @@ -198,7 +202,7 @@ int tcf_hash_search(struct tc_action *a, u32 index)
>   }
>   EXPORT_SYMBOL(tcf_hash_search);
>
> -struct tcf_common *tcf_hash_check(u32 index, struct tc_action *a, int bind)
> +int tcf_hash_check(u32 index, struct tc_action *a, int bind)
>   {
>   	struct tcf_hashinfo *hinfo = a->ops->hinfo;
>   	struct tcf_common *p = NULL;
> @@ -207,19 +211,30 @@ struct tcf_common *tcf_hash_check(u32 index, struct tc_action *a, int bind)
>   			p->tcfc_bindcnt++;
>   		p->tcfc_refcnt++;
>   		a->priv = p;
> +		return 1;
>   	}
> -	return p;
> +	return 0;
>   }
>   EXPORT_SYMBOL(tcf_hash_check);
>
> -struct tcf_common *tcf_hash_create(u32 index, struct nlattr *est,
> -				   struct tc_action *a, int size, int bind)
> +void tcf_hash_cleanup(struct tc_action *a, struct nlattr *est)
> +{
> +	struct tcf_common *pc = a->priv;
> +	if (est)
> +		gen_kill_estimator(&pc->tcfc_bstats,
> +				   &pc->tcfc_rate_est);
> +	kfree_rcu(pc, tcfc_rcu);
> +}
> +EXPORT_SYMBOL(tcf_hash_cleanup);
> +
> +int tcf_hash_create(u32 index, struct nlattr *est, struct tc_action *a,
> +		    int size, int bind)
>   {
>   	struct tcf_hashinfo *hinfo = a->ops->hinfo;
>   	struct tcf_common *p = kzalloc(size, GFP_KERNEL);
>
>   	if (unlikely(!p))
> -		return ERR_PTR(-ENOMEM);
> +		return -ENOMEM;
>   	p->tcfc_refcnt = 1;
>   	if (bind)
>   		p->tcfc_bindcnt = 1;
> @@ -234,17 +249,19 @@ struct tcf_common *tcf_hash_create(u32 index, struct nlattr *est,
>   					    &p->tcfc_lock, est);
>   		if (err) {
>   			kfree(p);
> -			return ERR_PTR(err);
> +			return err;
>   		}
>   	}
>
>   	a->priv = (void *) p;
> -	return p;
> +	return 0;
>   }
>   EXPORT_SYMBOL(tcf_hash_create);
>
> -void tcf_hash_insert(struct tcf_common *p, struct tcf_hashinfo *hinfo)
> +void tcf_hash_insert(struct tc_action *a)
>   {
> +	struct tcf_common *p = a->priv;
> +	struct tcf_hashinfo *hinfo = a->ops->hinfo;
>   	unsigned int h = tcf_hash(p->tcfc_index, hinfo->hmask);
>
>   	spin_lock_bh(&hinfo->lock);
> diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
> index 2210187..f0f6e7a 100644
> --- a/net/sched/act_csum.c
> +++ b/net/sched/act_csum.c
> @@ -48,7 +48,6 @@ static int tcf_csum_init(struct net *n, struct nlattr *nla, struct nlattr *est,
>   {
>   	struct nlattr *tb[TCA_CSUM_MAX + 1];
>   	struct tc_csum *parm;
> -	struct tcf_common *pc;
>   	struct tcf_csum *p;
>   	int ret = 0, err;
>
> @@ -63,38 +62,31 @@ static int tcf_csum_init(struct net *n, struct nlattr *nla, struct nlattr *est,
>   		return -EINVAL;
>   	parm = nla_data(tb[TCA_CSUM_PARMS]);
>
> -	pc = tcf_hash_check(parm->index, a, bind);
> -	if (!pc) {
> -		pc = tcf_hash_create(parm->index, est, a, sizeof(*p), bind);
> -		if (IS_ERR(pc))
> -			return PTR_ERR(pc);
> +	if (!tcf_hash_check(parm->index, a, bind)) {
> +		ret = tcf_hash_create(parm->index, est, a, sizeof(*p), bind);
> +		if (ret)
> +			return ret;
>   		ret = ACT_P_CREATED;
>   	} else {
>   		if (bind)/* dont override defaults */
>   			return 0;
> -		tcf_hash_release(pc, bind, a->ops->hinfo);
> +		tcf_hash_release(a, bind);
>   		if (!ovr)
>   			return -EEXIST;
>   	}
>
> -	p = to_tcf_csum(pc);
> +	p = to_tcf_csum(a);
>   	spin_lock_bh(&p->tcf_lock);
>   	p->tcf_action = parm->action;
>   	p->update_flags = parm->update_flags;
>   	spin_unlock_bh(&p->tcf_lock);
>
>   	if (ret == ACT_P_CREATED)
> -		tcf_hash_insert(pc, a->ops->hinfo);
> +		tcf_hash_insert(a);
>
>   	return ret;
>   }
>
> -static int tcf_csum_cleanup(struct tc_action *a, int bind)
> -{
> -	struct tcf_csum *p = a->priv;
> -	return tcf_hash_release(&p->common, bind, &csum_hash_info);
> -}
> -
>   /**
>    * tcf_csum_skb_nextlayer - Get next layer pointer
>    * @skb: sk_buff to use
> @@ -574,7 +566,7 @@ static struct tc_action_ops act_csum_ops = {
>   	.owner		= THIS_MODULE,
>   	.act		= tcf_csum,
>   	.dump		= tcf_csum_dump,
> -	.cleanup	= tcf_csum_cleanup,
> +	.cleanup	= tcf_hash_release,
>   	.init		= tcf_csum_init,
>   };
>
> diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
> index a0eed30..af6c0ac 100644
> --- a/net/sched/act_gact.c
> +++ b/net/sched/act_gact.c
> @@ -57,7 +57,6 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
>   	struct nlattr *tb[TCA_GACT_MAX + 1];
>   	struct tc_gact *parm;
>   	struct tcf_gact *gact;
> -	struct tcf_common *pc;
>   	int ret = 0;
>   	int err;
>   #ifdef CONFIG_GACT_PROB
> @@ -86,21 +85,20 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
>   	}
>   #endif
>
> -	pc = tcf_hash_check(parm->index, a, bind);
> -	if (!pc) {
> -		pc = tcf_hash_create(parm->index, est, a, sizeof(*gact), bind);
> -		if (IS_ERR(pc))
> -			return PTR_ERR(pc);
> +	if (!tcf_hash_check(parm->index, a, bind)) {
> +		ret = tcf_hash_create(parm->index, est, a, sizeof(*gact), bind);
> +		if (ret)
> +			return ret;
>   		ret = ACT_P_CREATED;
>   	} else {
>   		if (bind)/* dont override defaults */
>   			return 0;
> -		tcf_hash_release(pc, bind, a->ops->hinfo);
> +		tcf_hash_release(a, bind);
>   		if (!ovr)
>   			return -EEXIST;
>   	}
>
> -	gact = to_gact(pc);
> +	gact = to_gact(a);
>
>   	spin_lock_bh(&gact->tcf_lock);
>   	gact->tcf_action = parm->action;
> @@ -113,19 +111,10 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
>   #endif
>   	spin_unlock_bh(&gact->tcf_lock);
>   	if (ret == ACT_P_CREATED)
> -		tcf_hash_insert(pc, a->ops->hinfo);
> +		tcf_hash_insert(a);
>   	return ret;
>   }
>
> -static int tcf_gact_cleanup(struct tc_action *a, int bind)
> -{
> -	struct tcf_gact *gact = a->priv;
> -
> -	if (gact)
> -		return tcf_hash_release(&gact->common, bind, a->ops->hinfo);
> -	return 0;
> -}
> -
>   static int tcf_gact(struct sk_buff *skb, const struct tc_action *a,
>   		    struct tcf_result *res)
>   {
> @@ -196,7 +185,7 @@ static struct tc_action_ops act_gact_ops = {
>   	.owner		=	THIS_MODULE,
>   	.act		=	tcf_gact,
>   	.dump		=	tcf_gact_dump,
> -	.cleanup	=	tcf_gact_cleanup,
> +	.cleanup	=	tcf_hash_release,
>   	.init		=	tcf_gact_init,
>   };
>
> diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
> index 0a6d621..f5e6978 100644
> --- a/net/sched/act_ipt.c
> +++ b/net/sched/act_ipt.c
> @@ -69,8 +69,9 @@ static void ipt_destroy_target(struct xt_entry_target *t)
>   	module_put(par.target->me);
>   }
>
> -static int tcf_ipt_release(struct tcf_ipt *ipt, int bind)
> +static int tcf_ipt_release(struct tc_action *a, int bind)
>   {
> +	struct tcf_ipt *ipt = to_ipt(a);
>   	int ret = 0;
>   	if (ipt) {
>   		if (bind)
> @@ -80,7 +81,7 @@ static int tcf_ipt_release(struct tcf_ipt *ipt, int bind)
>   			ipt_destroy_target(ipt->tcfi_t);
>   			kfree(ipt->tcfi_tname);
>   			kfree(ipt->tcfi_t);
> -			tcf_hash_destroy(&ipt->common, &ipt_hash_info);
> +			tcf_hash_destroy(a);
>   			ret = ACT_P_DELETED;
>   		}
>   	}
> @@ -99,7 +100,6 @@ static int tcf_ipt_init(struct net *net, struct nlattr *nla, struct nlattr *est,
>   {
>   	struct nlattr *tb[TCA_IPT_MAX + 1];
>   	struct tcf_ipt *ipt;
> -	struct tcf_common *pc;
>   	struct xt_entry_target *td, *t;
>   	char *tname;
>   	int ret = 0, err;
> @@ -125,21 +125,20 @@ static int tcf_ipt_init(struct net *net, struct nlattr *nla, struct nlattr *est,
>   	if (tb[TCA_IPT_INDEX] != NULL)
>   		index = nla_get_u32(tb[TCA_IPT_INDEX]);
>
> -	pc = tcf_hash_check(index, a, bind);
> -	if (!pc) {
> -		pc = tcf_hash_create(index, est, a, sizeof(*ipt), bind);
> -		if (IS_ERR(pc))
> -			return PTR_ERR(pc);
> +	if (!tcf_hash_check(index, a, bind) ) {
> +		ret = tcf_hash_create(index, est, a, sizeof(*ipt), bind);
> +		if (ret)
> +			return ret;
>   		ret = ACT_P_CREATED;
>   	} else {
>   		if (bind)/* dont override defaults */
>   			return 0;
> -		tcf_ipt_release(to_ipt(pc), bind);
> +		tcf_ipt_release(a, bind);
>
>   		if (!ovr)
>   			return -EEXIST;
>   	}
> -	ipt = to_ipt(pc);
> +	ipt = to_ipt(a);
>
>   	hook = nla_get_u32(tb[TCA_IPT_HOOK]);
>
> @@ -170,7 +169,7 @@ static int tcf_ipt_init(struct net *net, struct nlattr *nla, struct nlattr *est,
>   	ipt->tcfi_hook  = hook;
>   	spin_unlock_bh(&ipt->tcf_lock);
>   	if (ret == ACT_P_CREATED)
> -		tcf_hash_insert(pc, a->ops->hinfo);
> +		tcf_hash_insert(a);
>   	return ret;
>
>   err3:
> @@ -178,21 +177,11 @@ err3:
>   err2:
>   	kfree(tname);
>   err1:
> -	if (ret == ACT_P_CREATED) {
> -		if (est)
> -			gen_kill_estimator(&pc->tcfc_bstats,
> -					   &pc->tcfc_rate_est);
> -		kfree_rcu(pc, tcfc_rcu);
> -	}
> +	if (ret == ACT_P_CREATED)
> +		tcf_hash_cleanup(a, est);
>   	return err;
>   }
>
> -static int tcf_ipt_cleanup(struct tc_action *a, int bind)
> -{
> -	struct tcf_ipt *ipt = a->priv;
> -	return tcf_ipt_release(ipt, bind);
> -}
> -
>   static int tcf_ipt(struct sk_buff *skb, const struct tc_action *a,
>   		   struct tcf_result *res)
>   {
> @@ -289,7 +278,7 @@ static struct tc_action_ops act_ipt_ops = {
>   	.owner		=	THIS_MODULE,
>   	.act		=	tcf_ipt,
>   	.dump		=	tcf_ipt_dump,
> -	.cleanup	=	tcf_ipt_cleanup,
> +	.cleanup	=	tcf_ipt_release,
>   	.init		=	tcf_ipt_init,
>   };
>
> @@ -300,7 +289,7 @@ static struct tc_action_ops act_xt_ops = {
>   	.owner		=	THIS_MODULE,
>   	.act		=	tcf_ipt,
>   	.dump		=	tcf_ipt_dump,
> -	.cleanup	=	tcf_ipt_cleanup,
> +	.cleanup	=	tcf_ipt_release,
>   	.init		=	tcf_ipt_init,
>   };
>
> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> index 0b2c6d3..3edeeca 100644
> --- a/net/sched/act_mirred.c
> +++ b/net/sched/act_mirred.c
> @@ -33,8 +33,9 @@
>   static LIST_HEAD(mirred_list);
>   static struct tcf_hashinfo mirred_hash_info;
>
> -static int tcf_mirred_release(struct tcf_mirred *m, int bind)
> +static int tcf_mirred_release(struct tc_action *a, int bind)
>   {
> +	struct tcf_mirred *m = to_mirred(a);
>   	if (m) {
>   		if (bind)
>   			m->tcf_bindcnt--;
> @@ -43,7 +44,7 @@ static int tcf_mirred_release(struct tcf_mirred *m, int bind)
>   			list_del(&m->tcfm_list);
>   			if (m->tcfm_dev)
>   				dev_put(m->tcfm_dev);
> -			tcf_hash_destroy(&m->common, &mirred_hash_info);
> +			tcf_hash_destroy(a);
>   			return 1;
>   		}
>   	}
> @@ -61,7 +62,6 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
>   	struct nlattr *tb[TCA_MIRRED_MAX + 1];
>   	struct tc_mirred *parm;
>   	struct tcf_mirred *m;
> -	struct tcf_common *pc;
>   	struct net_device *dev;
>   	int ret, ok_push = 0;
>
> @@ -101,21 +101,20 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
>   		dev = NULL;
>   	}
>
> -	pc = tcf_hash_check(parm->index, a, bind);
> -	if (!pc) {
> +	if (!tcf_hash_check(parm->index, a, bind)) {
>   		if (dev == NULL)
>   			return -EINVAL;
> -		pc = tcf_hash_create(parm->index, est, a, sizeof(*m), bind);
> -		if (IS_ERR(pc))
> -			return PTR_ERR(pc);
> +		ret = tcf_hash_create(parm->index, est, a, sizeof(*m), bind);
> +		if (ret)
> +			return ret;
>   		ret = ACT_P_CREATED;
>   	} else {
>   		if (!ovr) {
> -			tcf_mirred_release(to_mirred(pc), bind);
> +			tcf_mirred_release(a, bind);
>   			return -EEXIST;
>   		}
>   	}
> -	m = to_mirred(pc);
> +	m = to_mirred(a);
>
>   	spin_lock_bh(&m->tcf_lock);
>   	m->tcf_action = parm->action;
> @@ -131,21 +130,12 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
>   	spin_unlock_bh(&m->tcf_lock);
>   	if (ret == ACT_P_CREATED) {
>   		list_add(&m->tcfm_list, &mirred_list);
> -		tcf_hash_insert(pc, a->ops->hinfo);
> +		tcf_hash_insert(a);
>   	}
>
>   	return ret;
>   }
>
> -static int tcf_mirred_cleanup(struct tc_action *a, int bind)
> -{
> -	struct tcf_mirred *m = a->priv;
> -
> -	if (m)
> -		return tcf_mirred_release(m, bind);
> -	return 0;
> -}
> -
>   static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
>   		      struct tcf_result *res)
>   {
> @@ -259,7 +249,7 @@ static struct tc_action_ops act_mirred_ops = {
>   	.owner		=	THIS_MODULE,
>   	.act		=	tcf_mirred,
>   	.dump		=	tcf_mirred_dump,
> -	.cleanup	=	tcf_mirred_cleanup,
> +	.cleanup	=	tcf_mirred_release,
>   	.init		=	tcf_mirred_init,
>   };
>
> diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
> index 81f0404..ce9a391 100644
> --- a/net/sched/act_nat.c
> +++ b/net/sched/act_nat.c
> @@ -44,7 +44,6 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
>   	struct tc_nat *parm;
>   	int ret = 0, err;
>   	struct tcf_nat *p;
> -	struct tcf_common *pc;
>
>   	if (nla == NULL)
>   		return -EINVAL;
> @@ -57,20 +56,19 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
>   		return -EINVAL;
>   	parm = nla_data(tb[TCA_NAT_PARMS]);
>
> -	pc = tcf_hash_check(parm->index, a, bind);
> -	if (!pc) {
> -		pc = tcf_hash_create(parm->index, est, a, sizeof(*p), bind);
> -		if (IS_ERR(pc))
> -			return PTR_ERR(pc);
> +	if (!tcf_hash_check(parm->index, a, bind)) {
> +		ret = tcf_hash_create(parm->index, est, a, sizeof(*p), bind);
> +		if (ret)
> +			return ret;
>   		ret = ACT_P_CREATED;
>   	} else {
>   		if (bind)
>   			return 0;
> -		tcf_hash_release(pc, bind, a->ops->hinfo);
> +		tcf_hash_release(a, bind);
>   		if (!ovr)
>   			return -EEXIST;
>   	}
> -	p = to_tcf_nat(pc);
> +	p = to_tcf_nat(a);
>
>   	spin_lock_bh(&p->tcf_lock);
>   	p->old_addr = parm->old_addr;
> @@ -82,18 +80,11 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
>   	spin_unlock_bh(&p->tcf_lock);
>
>   	if (ret == ACT_P_CREATED)
> -		tcf_hash_insert(pc, a->ops->hinfo);
> +		tcf_hash_insert(a);
>
>   	return ret;
>   }
>
> -static int tcf_nat_cleanup(struct tc_action *a, int bind)
> -{
> -	struct tcf_nat *p = a->priv;
> -
> -	return tcf_hash_release(&p->common, bind, &nat_hash_info);
> -}
> -
>   static int tcf_nat(struct sk_buff *skb, const struct tc_action *a,
>   		   struct tcf_result *res)
>   {
> @@ -298,7 +289,7 @@ static struct tc_action_ops act_nat_ops = {
>   	.owner		=	THIS_MODULE,
>   	.act		=	tcf_nat,
>   	.dump		=	tcf_nat_dump,
> -	.cleanup	=	tcf_nat_cleanup,
> +	.cleanup	=	tcf_hash_release,
>   	.init		=	tcf_nat_init,
>   };
>
> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> index be3f0f6..091ced3 100644
> --- a/net/sched/act_pedit.c
> +++ b/net/sched/act_pedit.c
> @@ -39,7 +39,6 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
>   	struct tc_pedit *parm;
>   	int ret = 0, err;
>   	struct tcf_pedit *p;
> -	struct tcf_common *pc;
>   	struct tc_pedit_key *keys = NULL;
>   	int ksize;
>
> @@ -57,26 +56,22 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
>   	if (nla_len(tb[TCA_PEDIT_PARMS]) < sizeof(*parm) + ksize)
>   		return -EINVAL;
>
> -	pc = tcf_hash_check(parm->index, a, bind);
> -	if (!pc) {
> +	if (!tcf_hash_check(parm->index, a, bind)) {
>   		if (!parm->nkeys)
>   			return -EINVAL;
> -		pc = tcf_hash_create(parm->index, est, a, sizeof(*p), bind);
> -		if (IS_ERR(pc))
> -			return PTR_ERR(pc);
> -		p = to_pedit(pc);
> +		ret = tcf_hash_create(parm->index, est, a, sizeof(*p), bind);
> +		if (ret)
> +			return ret;
> +		p = to_pedit(a);
>   		keys = kmalloc(ksize, GFP_KERNEL);
>   		if (keys == NULL) {
> -			if (est)
> -				gen_kill_estimator(&pc->tcfc_bstats,
> -						   &pc->tcfc_rate_est);
> -			kfree_rcu(pc, tcfc_rcu);
> +			tcf_hash_cleanup(a, est);
>   			return -ENOMEM;
>   		}
>   		ret = ACT_P_CREATED;
>   	} else {
> -		p = to_pedit(pc);
> -		tcf_hash_release(pc, bind, a->ops->hinfo);
> +		p = to_pedit(a);
> +		tcf_hash_release(a, bind);
>   		if (bind)
>   			return 0;
>   		if (!ovr)
> @@ -100,7 +95,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
>   	memcpy(p->tcfp_keys, parm->keys, ksize);
>   	spin_unlock_bh(&p->tcf_lock);
>   	if (ret == ACT_P_CREATED)
> -		tcf_hash_insert(pc, a->ops->hinfo);
> +		tcf_hash_insert(a);
>   	return ret;
>   }
>
> @@ -110,7 +105,7 @@ static int tcf_pedit_cleanup(struct tc_action *a, int bind)
>
>   	if (p) {
>   		struct tc_pedit_key *keys = p->tcfp_keys;
> -		if (tcf_hash_release(&p->common, bind, &pedit_hash_info)) {
> +		if (tcf_hash_release(a, bind)) {
>   			kfree(keys);
>   			return 1;
>   		}
> diff --git a/net/sched/act_police.c b/net/sched/act_police.c
> index 1778209..4695d02 100644
> --- a/net/sched/act_police.c
> +++ b/net/sched/act_police.c
> @@ -253,14 +253,6 @@ failure:
>   	return err;
>   }
>
> -static int tcf_act_police_cleanup(struct tc_action *a, int bind)
> -{
> -	struct tcf_police *p = a->priv;
> -	if (p)
> -		return tcf_hash_release(&p->common, bind, &police_hash_info);
> -	return 0;
> -}
> -
>   static int tcf_act_police(struct sk_buff *skb, const struct tc_action *a,
>   			  struct tcf_result *res)
>   {
> @@ -362,7 +354,7 @@ static struct tc_action_ops act_police_ops = {
>   	.owner		=	THIS_MODULE,
>   	.act		=	tcf_act_police,
>   	.dump		=	tcf_act_police_dump,
> -	.cleanup	=	tcf_act_police_cleanup,
> +	.cleanup	=	tcf_hash_release,
>   	.init		=	tcf_act_police_locate,
>   	.walk		=	tcf_act_police_walker
>   };
> diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
> index 8ef2f1f..11c2922 100644
> --- a/net/sched/act_simple.c
> +++ b/net/sched/act_simple.c
> @@ -47,8 +47,9 @@ static int tcf_simp(struct sk_buff *skb, const struct tc_action *a,
>   	return d->tcf_action;
>   }
>
> -static int tcf_simp_release(struct tcf_defact *d, int bind)
> +static int tcf_simp_release(struct tc_action *a, int bind)
>   {
> +	struct tcf_defact *d = to_defact(a);
>   	int ret = 0;
>   	if (d) {
>   		if (bind)
> @@ -56,7 +57,7 @@ static int tcf_simp_release(struct tcf_defact *d, int bind)
>   		d->tcf_refcnt--;
>   		if (d->tcf_bindcnt <= 0 && d->tcf_refcnt <= 0) {
>   			kfree(d->tcfd_defdata);
> -			tcf_hash_destroy(&d->common, &simp_hash_info);
> +			tcf_hash_destroy(a);
>   			ret = 1;
>   		}
>   	}
> @@ -94,7 +95,6 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
>   	struct nlattr *tb[TCA_DEF_MAX + 1];
>   	struct tc_defact *parm;
>   	struct tcf_defact *d;
> -	struct tcf_common *pc;
>   	char *defdata;
>   	int ret = 0, err;
>
> @@ -114,29 +114,25 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
>   	parm = nla_data(tb[TCA_DEF_PARMS]);
>   	defdata = nla_data(tb[TCA_DEF_DATA]);
>
> -	pc = tcf_hash_check(parm->index, a, bind);
> -	if (!pc) {
> -		pc = tcf_hash_create(parm->index, est, a, sizeof(*d), bind);
> -		if (IS_ERR(pc))
> -			return PTR_ERR(pc);
> +	if (!tcf_hash_check(parm->index, a, bind)) {
> +		ret = tcf_hash_create(parm->index, est, a, sizeof(*d), bind);
> +		if (ret)
> +			return ret;
>
> -		d = to_defact(pc);
> +		d = to_defact(a);
>   		ret = alloc_defdata(d, defdata);
>   		if (ret < 0) {
> -			if (est)
> -				gen_kill_estimator(&pc->tcfc_bstats,
> -						   &pc->tcfc_rate_est);
> -			kfree_rcu(pc, tcfc_rcu);
> +			tcf_hash_cleanup(a, est);
>   			return ret;
>   		}
>   		d->tcf_action = parm->action;
>   		ret = ACT_P_CREATED;
>   	} else {
> -		d = to_defact(pc);
> +		d = to_defact(a);
>
>   		if (bind)
>   			return 0;
> -		tcf_simp_release(d, bind);
> +		tcf_simp_release(a, bind);
>   		if (!ovr)
>   			return -EEXIST;
>
> @@ -144,19 +140,10 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
>   	}
>
>   	if (ret == ACT_P_CREATED)
> -		tcf_hash_insert(pc, a->ops->hinfo);
> +		tcf_hash_insert(a);
>   	return ret;
>   }
>
> -static int tcf_simp_cleanup(struct tc_action *a, int bind)
> -{
> -	struct tcf_defact *d = a->priv;
> -
> -	if (d)
> -		return tcf_simp_release(d, bind);
> -	return 0;
> -}
> -
>   static int tcf_simp_dump(struct sk_buff *skb, struct tc_action *a,
>   			 int bind, int ref)
>   {
> @@ -192,7 +179,7 @@ static struct tc_action_ops act_simp_ops = {
>   	.owner		=	THIS_MODULE,
>   	.act		=	tcf_simp,
>   	.dump		=	tcf_simp_dump,
> -	.cleanup	=	tcf_simp_cleanup,
> +	.cleanup	=	tcf_simp_release,
>   	.init		=	tcf_simp_init,
>   };
>
> diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
> index 9872508..71fd2d4 100644
> --- a/net/sched/act_skbedit.c
> +++ b/net/sched/act_skbedit.c
> @@ -65,7 +65,6 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
>   	struct nlattr *tb[TCA_SKBEDIT_MAX + 1];
>   	struct tc_skbedit *parm;
>   	struct tcf_skbedit *d;
> -	struct tcf_common *pc;
>   	u32 flags = 0, *priority = NULL, *mark = NULL;
>   	u16 *queue_mapping = NULL;
>   	int ret = 0, err;
> @@ -100,19 +99,18 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
>
>   	parm = nla_data(tb[TCA_SKBEDIT_PARMS]);
>
> -	pc = tcf_hash_check(parm->index, a, bind);
> -	if (!pc) {
> -		pc = tcf_hash_create(parm->index, est, a, sizeof(*d), bind);
> -		if (IS_ERR(pc))
> -			return PTR_ERR(pc);
> +	if (!tcf_hash_check(parm->index, a, bind)) {
> +		ret = tcf_hash_create(parm->index, est, a, sizeof(*d), bind);
> +		if (ret)
> +			return ret;
>
> -		d = to_skbedit(pc);
> +		d = to_skbedit(a);
>   		ret = ACT_P_CREATED;
>   	} else {
> -		d = to_skbedit(pc);
> +		d = to_skbedit(a);
>   		if (bind)
>   			return 0;
> -		tcf_hash_release(pc, bind, a->ops->hinfo);
> +		tcf_hash_release(a, bind);
>   		if (!ovr)
>   			return -EEXIST;
>   	}
> @@ -132,19 +130,10 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
>   	spin_unlock_bh(&d->tcf_lock);
>
>   	if (ret == ACT_P_CREATED)
> -		tcf_hash_insert(pc, a->ops->hinfo);
> +		tcf_hash_insert(a);
>   	return ret;
>   }
>
> -static int tcf_skbedit_cleanup(struct tc_action *a, int bind)
> -{
> -	struct tcf_skbedit *d = a->priv;
> -
> -	if (d)
> -		return tcf_hash_release(&d->common, bind, &skbedit_hash_info);
> -	return 0;
> -}
> -
>   static int tcf_skbedit_dump(struct sk_buff *skb, struct tc_action *a,
>   			    int bind, int ref)
>   {
> @@ -191,7 +180,7 @@ static struct tc_action_ops act_skbedit_ops = {
>   	.owner		=	THIS_MODULE,
>   	.act		=	tcf_skbedit,
>   	.dump		=	tcf_skbedit_dump,
> -	.cleanup	=	tcf_skbedit_cleanup,
> +	.cleanup	=	tcf_hash_release,
>   	.init		=	tcf_skbedit_init,
>   };
>
>

[Patch net-next v3 2/5] net_sched: act: refactor cleanup ops

[Cong Wang]

For bindcnt and refcnt etc., they are common for all actions,
not need to repeat such operations for their own, they can be unified
now. Actions just need to do its specific cleanup if needed.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/act_api.h   |  2 +-
 net/sched/act_api.c     |  8 +++++---
 net/sched/act_csum.c    |  1 -
 net/sched/act_gact.c    |  1 -
 net/sched/act_ipt.c     | 21 +++++----------------
 net/sched/act_mirred.c  | 20 +++++---------------
 net/sched/act_nat.c     |  1 -
 net/sched/act_pedit.c   | 13 +++----------
 net/sched/act_police.c  |  1 -
 net/sched/act_simple.c  | 17 +++--------------
 net/sched/act_skbedit.c |  1 -
 11 files changed, 22 insertions(+), 64 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 24ae910..3d22f42 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -89,7 +89,7 @@ struct tc_action_ops {
 	struct module		*owner;
 	int     (*act)(struct sk_buff *, const struct tc_action *, struct tcf_result *);
 	int     (*dump)(struct sk_buff *, struct tc_action *, int, int);
-	int     (*cleanup)(struct tc_action *, int bind);
+	void	(*cleanup)(struct tc_action *, int bind);
 	int     (*lookup)(struct tc_action *, u32);
 	int     (*init)(struct net *net, struct nlattr *nla,
 			struct nlattr *est, struct tc_action *act, int ovr,
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 4f2b807..a5bf935 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -56,6 +56,8 @@ int tcf_hash_release(struct tc_action *a, int bind)
 
 		p->tcfc_refcnt--;
 		if (p->tcfc_bindcnt <= 0 && p->tcfc_refcnt <= 0) {
+			if (a->ops->cleanup)
+				a->ops->cleanup(a, bind);
 			tcf_hash_destroy(a);
 			ret = 1;
 		}
@@ -277,8 +279,8 @@ int tcf_register_action(struct tc_action_ops *act)
 {
 	struct tc_action_ops *a;
 
-	/* Must supply act, dump, cleanup and init */
-	if (!act->act || !act->dump || !act->cleanup || !act->init)
+	/* Must supply act, dump and init */
+	if (!act->act || !act->dump || !act->init)
 		return -EINVAL;
 
 	/* Supply defaults */
@@ -390,7 +392,7 @@ void tcf_action_destroy(struct list_head *actions, int bind)
 	struct tc_action *a, *tmp;
 
 	list_for_each_entry_safe(a, tmp, actions, list) {
-		if (a->ops->cleanup(a, bind) == ACT_P_DELETED)
+		if (tcf_hash_release(a, bind) == ACT_P_DELETED)
 			module_put(a->ops->owner);
 		list_del(&a->list);
 		kfree(a);
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index f0f6e7a..8df3060 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -566,7 +566,6 @@ static struct tc_action_ops act_csum_ops = {
 	.owner		= THIS_MODULE,
 	.act		= tcf_csum,
 	.dump		= tcf_csum_dump,
-	.cleanup	= tcf_hash_release,
 	.init		= tcf_csum_init,
 };
 
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index af6c0ac..094a1b5 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -185,7 +185,6 @@ static struct tc_action_ops act_gact_ops = {
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_gact,
 	.dump		=	tcf_gact_dump,
-	.cleanup	=	tcf_hash_release,
 	.init		=	tcf_gact_init,
 };
 
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index f5e6978..71f29f1 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -69,23 +69,12 @@ static void ipt_destroy_target(struct xt_entry_target *t)
 	module_put(par.target->me);
 }
 
-static int tcf_ipt_release(struct tc_action *a, int bind)
+static void tcf_ipt_release(struct tc_action *a, int bind)
 {
 	struct tcf_ipt *ipt = to_ipt(a);
-	int ret = 0;
-	if (ipt) {
-		if (bind)
-			ipt->tcf_bindcnt--;
-		ipt->tcf_refcnt--;
-		if (ipt->tcf_bindcnt <= 0 && ipt->tcf_refcnt <= 0) {
-			ipt_destroy_target(ipt->tcfi_t);
-			kfree(ipt->tcfi_tname);
-			kfree(ipt->tcfi_t);
-			tcf_hash_destroy(a);
-			ret = ACT_P_DELETED;
-		}
-	}
-	return ret;
+	ipt_destroy_target(ipt->tcfi_t);
+	kfree(ipt->tcfi_tname);
+	kfree(ipt->tcfi_t);
 }
 
 static const struct nla_policy ipt_policy[TCA_IPT_MAX + 1] = {
@@ -133,7 +122,7 @@ static int tcf_ipt_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 	} else {
 		if (bind)/* dont override defaults */
 			return 0;
-		tcf_ipt_release(a, bind);
+		tcf_hash_release(a, bind);
 
 		if (!ovr)
 			return -EEXIST;
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 3edeeca..0f00eb9 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -33,22 +33,12 @@
 static LIST_HEAD(mirred_list);
 static struct tcf_hashinfo mirred_hash_info;
 
-static int tcf_mirred_release(struct tc_action *a, int bind)
+static void tcf_mirred_release(struct tc_action *a, int bind)
 {
 	struct tcf_mirred *m = to_mirred(a);
-	if (m) {
-		if (bind)
-			m->tcf_bindcnt--;
-		m->tcf_refcnt--;
-		if (!m->tcf_bindcnt && m->tcf_refcnt <= 0) {
-			list_del(&m->tcfm_list);
-			if (m->tcfm_dev)
-				dev_put(m->tcfm_dev);
-			tcf_hash_destroy(a);
-			return 1;
-		}
-	}
-	return 0;
+	list_del(&m->tcfm_list);
+	if (m->tcfm_dev)
+		dev_put(m->tcfm_dev);
 }
 
 static const struct nla_policy mirred_policy[TCA_MIRRED_MAX + 1] = {
@@ -110,7 +100,7 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
 		ret = ACT_P_CREATED;
 	} else {
 		if (!ovr) {
-			tcf_mirred_release(a, bind);
+			tcf_hash_release(a, bind);
 			return -EEXIST;
 		}
 	}
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index ce9a391..9a3cb1d 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -289,7 +289,6 @@ static struct tc_action_ops act_nat_ops = {
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_nat,
 	.dump		=	tcf_nat_dump,
-	.cleanup	=	tcf_hash_release,
 	.init		=	tcf_nat_init,
 };
 
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 091ced3..8aa795b 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -99,18 +99,11 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 	return ret;
 }
 
-static int tcf_pedit_cleanup(struct tc_action *a, int bind)
+static void tcf_pedit_cleanup(struct tc_action *a, int bind)
 {
 	struct tcf_pedit *p = a->priv;
-
-	if (p) {
-		struct tc_pedit_key *keys = p->tcfp_keys;
-		if (tcf_hash_release(a, bind)) {
-			kfree(keys);
-			return 1;
-		}
-	}
-	return 0;
+	struct tc_pedit_key *keys = p->tcfp_keys;
+	kfree(keys);
 }
 
 static int tcf_pedit(struct sk_buff *skb, const struct tc_action *a,
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 4695d02..7ff7bef 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -354,7 +354,6 @@ static struct tc_action_ops act_police_ops = {
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_act_police,
 	.dump		=	tcf_act_police_dump,
-	.cleanup	=	tcf_hash_release,
 	.init		=	tcf_act_police_locate,
 	.walk		=	tcf_act_police_walker
 };
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index 11c2922..14b5e36 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -47,21 +47,10 @@ static int tcf_simp(struct sk_buff *skb, const struct tc_action *a,
 	return d->tcf_action;
 }
 
-static int tcf_simp_release(struct tc_action *a, int bind)
+static void tcf_simp_release(struct tc_action *a, int bind)
 {
 	struct tcf_defact *d = to_defact(a);
-	int ret = 0;
-	if (d) {
-		if (bind)
-			d->tcf_bindcnt--;
-		d->tcf_refcnt--;
-		if (d->tcf_bindcnt <= 0 && d->tcf_refcnt <= 0) {
-			kfree(d->tcfd_defdata);
-			tcf_hash_destroy(a);
-			ret = 1;
-		}
-	}
-	return ret;
+	kfree(d->tcfd_defdata);
 }
 
 static int alloc_defdata(struct tcf_defact *d, char *defdata)
@@ -132,7 +121,7 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
 
 		if (bind)
 			return 0;
-		tcf_simp_release(a, bind);
+		tcf_hash_release(a, bind);
 		if (!ovr)
 			return -EEXIST;
 
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index 71fd2d4..9f91928 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -180,7 +180,6 @@ static struct tc_action_ops act_skbedit_ops = {
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_skbedit,
 	.dump		=	tcf_skbedit_dump,
-	.cleanup	=	tcf_hash_release,
 	.init		=	tcf_skbedit_init,
 };
 
-- 
1.8.3.1

Re: [Patch net-next v3 2/5] net_sched: act: refactor cleanup ops

[Jamal Hadi Salim]

On 02/11/14 20:07, Cong Wang wrote:
> For bindcnt and refcnt etc., they are common for all actions,
> not need to repeat such operations for their own, they can be unified
> now. Actions just need to do its specific cleanup if needed.
>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>


> ---
>   include/net/act_api.h   |  2 +-
>   net/sched/act_api.c     |  8 +++++---
>   net/sched/act_csum.c    |  1 -
>   net/sched/act_gact.c    |  1 -
>   net/sched/act_ipt.c     | 21 +++++----------------
>   net/sched/act_mirred.c  | 20 +++++---------------
>   net/sched/act_nat.c     |  1 -
>   net/sched/act_pedit.c   | 13 +++----------
>   net/sched/act_police.c  |  1 -
>   net/sched/act_simple.c  | 17 +++--------------
>   net/sched/act_skbedit.c |  1 -
>   11 files changed, 22 insertions(+), 64 deletions(-)
>
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index 24ae910..3d22f42 100644
> --- a/include/net/act_api.h
> +++ b/include/net/act_api.h
> @@ -89,7 +89,7 @@ struct tc_action_ops {
>   	struct module		*owner;
>   	int     (*act)(struct sk_buff *, const struct tc_action *, struct tcf_result *);
>   	int     (*dump)(struct sk_buff *, struct tc_action *, int, int);
> -	int     (*cleanup)(struct tc_action *, int bind);
> +	void	(*cleanup)(struct tc_action *, int bind);
>   	int     (*lookup)(struct tc_action *, u32);
>   	int     (*init)(struct net *net, struct nlattr *nla,
>   			struct nlattr *est, struct tc_action *act, int ovr,
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 4f2b807..a5bf935 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -56,6 +56,8 @@ int tcf_hash_release(struct tc_action *a, int bind)
>
>   		p->tcfc_refcnt--;
>   		if (p->tcfc_bindcnt <= 0 && p->tcfc_refcnt <= 0) {
> +			if (a->ops->cleanup)
> +				a->ops->cleanup(a, bind);
>   			tcf_hash_destroy(a);
>   			ret = 1;
>   		}
> @@ -277,8 +279,8 @@ int tcf_register_action(struct tc_action_ops *act)
>   {
>   	struct tc_action_ops *a;
>
> -	/* Must supply act, dump, cleanup and init */
> -	if (!act->act || !act->dump || !act->cleanup || !act->init)
> +	/* Must supply act, dump and init */
> +	if (!act->act || !act->dump || !act->init)
>   		return -EINVAL;
>
>   	/* Supply defaults */
> @@ -390,7 +392,7 @@ void tcf_action_destroy(struct list_head *actions, int bind)
>   	struct tc_action *a, *tmp;
>
>   	list_for_each_entry_safe(a, tmp, actions, list) {
> -		if (a->ops->cleanup(a, bind) == ACT_P_DELETED)
> +		if (tcf_hash_release(a, bind) == ACT_P_DELETED)
>   			module_put(a->ops->owner);
>   		list_del(&a->list);
>   		kfree(a);
> diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
> index f0f6e7a..8df3060 100644
> --- a/net/sched/act_csum.c
> +++ b/net/sched/act_csum.c
> @@ -566,7 +566,6 @@ static struct tc_action_ops act_csum_ops = {
>   	.owner		= THIS_MODULE,
>   	.act		= tcf_csum,
>   	.dump		= tcf_csum_dump,
> -	.cleanup	= tcf_hash_release,
>   	.init		= tcf_csum_init,
>   };
>
> diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
> index af6c0ac..094a1b5 100644
> --- a/net/sched/act_gact.c
> +++ b/net/sched/act_gact.c
> @@ -185,7 +185,6 @@ static struct tc_action_ops act_gact_ops = {
>   	.owner		=	THIS_MODULE,
>   	.act		=	tcf_gact,
>   	.dump		=	tcf_gact_dump,
> -	.cleanup	=	tcf_hash_release,
>   	.init		=	tcf_gact_init,
>   };
>
> diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
> index f5e6978..71f29f1 100644
> --- a/net/sched/act_ipt.c
> +++ b/net/sched/act_ipt.c
> @@ -69,23 +69,12 @@ static void ipt_destroy_target(struct xt_entry_target *t)
>   	module_put(par.target->me);
>   }
>
> -static int tcf_ipt_release(struct tc_action *a, int bind)
> +static void tcf_ipt_release(struct tc_action *a, int bind)
>   {
>   	struct tcf_ipt *ipt = to_ipt(a);
> -	int ret = 0;
> -	if (ipt) {
> -		if (bind)
> -			ipt->tcf_bindcnt--;
> -		ipt->tcf_refcnt--;
> -		if (ipt->tcf_bindcnt <= 0 && ipt->tcf_refcnt <= 0) {
> -			ipt_destroy_target(ipt->tcfi_t);
> -			kfree(ipt->tcfi_tname);
> -			kfree(ipt->tcfi_t);
> -			tcf_hash_destroy(a);
> -			ret = ACT_P_DELETED;
> -		}
> -	}
> -	return ret;
> +	ipt_destroy_target(ipt->tcfi_t);
> +	kfree(ipt->tcfi_tname);
> +	kfree(ipt->tcfi_t);
>   }
>
>   static const struct nla_policy ipt_policy[TCA_IPT_MAX + 1] = {
> @@ -133,7 +122,7 @@ static int tcf_ipt_init(struct net *net, struct nlattr *nla, struct nlattr *est,
>   	} else {
>   		if (bind)/* dont override defaults */
>   			return 0;
> -		tcf_ipt_release(a, bind);
> +		tcf_hash_release(a, bind);
>
>   		if (!ovr)
>   			return -EEXIST;
> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> index 3edeeca..0f00eb9 100644
> --- a/net/sched/act_mirred.c
> +++ b/net/sched/act_mirred.c
> @@ -33,22 +33,12 @@
>   static LIST_HEAD(mirred_list);
>   static struct tcf_hashinfo mirred_hash_info;
>
> -static int tcf_mirred_release(struct tc_action *a, int bind)
> +static void tcf_mirred_release(struct tc_action *a, int bind)
>   {
>   	struct tcf_mirred *m = to_mirred(a);
> -	if (m) {
> -		if (bind)
> -			m->tcf_bindcnt--;
> -		m->tcf_refcnt--;
> -		if (!m->tcf_bindcnt && m->tcf_refcnt <= 0) {
> -			list_del(&m->tcfm_list);
> -			if (m->tcfm_dev)
> -				dev_put(m->tcfm_dev);
> -			tcf_hash_destroy(a);
> -			return 1;
> -		}
> -	}
> -	return 0;
> +	list_del(&m->tcfm_list);
> +	if (m->tcfm_dev)
> +		dev_put(m->tcfm_dev);
>   }
>
>   static const struct nla_policy mirred_policy[TCA_MIRRED_MAX + 1] = {
> @@ -110,7 +100,7 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
>   		ret = ACT_P_CREATED;
>   	} else {
>   		if (!ovr) {
> -			tcf_mirred_release(a, bind);
> +			tcf_hash_release(a, bind);
>   			return -EEXIST;
>   		}
>   	}
> diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
> index ce9a391..9a3cb1d 100644
> --- a/net/sched/act_nat.c
> +++ b/net/sched/act_nat.c
> @@ -289,7 +289,6 @@ static struct tc_action_ops act_nat_ops = {
>   	.owner		=	THIS_MODULE,
>   	.act		=	tcf_nat,
>   	.dump		=	tcf_nat_dump,
> -	.cleanup	=	tcf_hash_release,
>   	.init		=	tcf_nat_init,
>   };
>
> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> index 091ced3..8aa795b 100644
> --- a/net/sched/act_pedit.c
> +++ b/net/sched/act_pedit.c
> @@ -99,18 +99,11 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
>   	return ret;
>   }
>
> -static int tcf_pedit_cleanup(struct tc_action *a, int bind)
> +static void tcf_pedit_cleanup(struct tc_action *a, int bind)
>   {
>   	struct tcf_pedit *p = a->priv;
> -
> -	if (p) {
> -		struct tc_pedit_key *keys = p->tcfp_keys;
> -		if (tcf_hash_release(a, bind)) {
> -			kfree(keys);
> -			return 1;
> -		}
> -	}
> -	return 0;
> +	struct tc_pedit_key *keys = p->tcfp_keys;
> +	kfree(keys);
>   }
>
>   static int tcf_pedit(struct sk_buff *skb, const struct tc_action *a,
> diff --git a/net/sched/act_police.c b/net/sched/act_police.c
> index 4695d02..7ff7bef 100644
> --- a/net/sched/act_police.c
> +++ b/net/sched/act_police.c
> @@ -354,7 +354,6 @@ static struct tc_action_ops act_police_ops = {
>   	.owner		=	THIS_MODULE,
>   	.act		=	tcf_act_police,
>   	.dump		=	tcf_act_police_dump,
> -	.cleanup	=	tcf_hash_release,
>   	.init		=	tcf_act_police_locate,
>   	.walk		=	tcf_act_police_walker
>   };
> diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
> index 11c2922..14b5e36 100644
> --- a/net/sched/act_simple.c
> +++ b/net/sched/act_simple.c
> @@ -47,21 +47,10 @@ static int tcf_simp(struct sk_buff *skb, const struct tc_action *a,
>   	return d->tcf_action;
>   }
>
> -static int tcf_simp_release(struct tc_action *a, int bind)
> +static void tcf_simp_release(struct tc_action *a, int bind)
>   {
>   	struct tcf_defact *d = to_defact(a);
> -	int ret = 0;
> -	if (d) {
> -		if (bind)
> -			d->tcf_bindcnt--;
> -		d->tcf_refcnt--;
> -		if (d->tcf_bindcnt <= 0 && d->tcf_refcnt <= 0) {
> -			kfree(d->tcfd_defdata);
> -			tcf_hash_destroy(a);
> -			ret = 1;
> -		}
> -	}
> -	return ret;
> +	kfree(d->tcfd_defdata);
>   }
>
>   static int alloc_defdata(struct tcf_defact *d, char *defdata)
> @@ -132,7 +121,7 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
>
>   		if (bind)
>   			return 0;
> -		tcf_simp_release(a, bind);
> +		tcf_hash_release(a, bind);
>   		if (!ovr)
>   			return -EEXIST;
>
> diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
> index 71fd2d4..9f91928 100644
> --- a/net/sched/act_skbedit.c
> +++ b/net/sched/act_skbedit.c
> @@ -180,7 +180,6 @@ static struct tc_action_ops act_skbedit_ops = {
>   	.owner		=	THIS_MODULE,
>   	.act		=	tcf_skbedit,
>   	.dump		=	tcf_skbedit_dump,
> -	.cleanup	=	tcf_hash_release,
>   	.init		=	tcf_skbedit_init,
>   };
>
>

[Patch net-next v3 3/5] net_sched: act: move tcf_hashinfo_init() into tcf_register_action()

[Cong Wang]

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/act_api.h   |  2 +-
 net/sched/act_api.c     | 16 +++++++++++++++-
 net/sched/act_csum.c    |  8 +-------
 net/sched/act_gact.c    |  8 +-------
 net/sched/act_ipt.c     | 14 +++-----------
 net/sched/act_mirred.c  | 10 +---------
 net/sched/act_nat.c     |  9 +--------
 net/sched/act_pedit.c   |  9 +--------
 net/sched/act_police.c  | 13 ++-----------
 net/sched/act_simple.c  | 14 ++------------
 net/sched/act_skbedit.c |  8 +-------
 11 files changed, 29 insertions(+), 82 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 3d22f42..969cac6 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -107,7 +107,7 @@ int tcf_hash_create(u32 index, struct nlattr *est, struct tc_action *a,
 void tcf_hash_cleanup(struct tc_action *a, struct nlattr *est);
 void tcf_hash_insert(struct tc_action *a);
 
-int tcf_register_action(struct tc_action_ops *a);
+int tcf_register_action(struct tc_action_ops *a, unsigned int mask);
 int tcf_unregister_action(struct tc_action_ops *a);
 void tcf_action_destroy(struct list_head *actions, int bind);
 int tcf_action_exec(struct sk_buff *skb, const struct list_head *actions,
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index a5bf935..c88d382 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -275,9 +275,10 @@ EXPORT_SYMBOL(tcf_hash_insert);
 static LIST_HEAD(act_base);
 static DEFINE_RWLOCK(act_mod_lock);
 
-int tcf_register_action(struct tc_action_ops *act)
+int tcf_register_action(struct tc_action_ops *act, unsigned int mask)
 {
 	struct tc_action_ops *a;
+	int err;
 
 	/* Must supply act, dump and init */
 	if (!act->act || !act->dump || !act->init)
@@ -289,10 +290,21 @@ int tcf_register_action(struct tc_action_ops *act)
 	if (!act->walk)
 		act->walk = tcf_generic_walker;
 
+	act->hinfo = kmalloc(sizeof(struct tcf_hashinfo), GFP_KERNEL);
+	if (!act->hinfo)
+		return -ENOMEM;
+	err = tcf_hashinfo_init(act->hinfo, mask);
+	if (err) {
+		kfree(act->hinfo);
+		return err;
+	}
+
 	write_lock(&act_mod_lock);
 	list_for_each_entry(a, &act_base, head) {
 		if (act->type == a->type || (strcmp(act->kind, a->kind) == 0)) {
 			write_unlock(&act_mod_lock);
+			tcf_hashinfo_destroy(act->hinfo);
+			kfree(act->hinfo);
 			return -EEXIST;
 		}
 	}
@@ -311,6 +323,8 @@ int tcf_unregister_action(struct tc_action_ops *act)
 	list_for_each_entry(a, &act_base, head) {
 		if (a == act) {
 			list_del(&act->head);
+			tcf_hashinfo_destroy(act->hinfo);
+			kfree(act->hinfo);
 			err = 0;
 			break;
 		}
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 8df3060..edbf40d 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -37,7 +37,6 @@
 #include <net/tc_act/tc_csum.h>
 
 #define CSUM_TAB_MASK 15
-static struct tcf_hashinfo csum_hash_info;
 
 static const struct nla_policy csum_policy[TCA_CSUM_MAX + 1] = {
 	[TCA_CSUM_PARMS] = { .len = sizeof(struct tc_csum), },
@@ -561,7 +560,6 @@ nla_put_failure:
 
 static struct tc_action_ops act_csum_ops = {
 	.kind		= "csum",
-	.hinfo		= &csum_hash_info,
 	.type		= TCA_ACT_CSUM,
 	.owner		= THIS_MODULE,
 	.act		= tcf_csum,
@@ -574,11 +572,7 @@ MODULE_LICENSE("GPL");
 
 static int __init csum_init_module(void)
 {
-	int err = tcf_hashinfo_init(&csum_hash_info, CSUM_TAB_MASK);
-	if (err)
-		return err;
-
-	return tcf_register_action(&act_csum_ops);
+	return tcf_register_action(&act_csum_ops, CSUM_TAB_MASK);
 }
 
 static void __exit csum_cleanup_module(void)
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index 094a1b5..d6bcbd9 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -24,7 +24,6 @@
 #include <net/tc_act/tc_gact.h>
 
 #define GACT_TAB_MASK	15
-static struct tcf_hashinfo gact_hash_info;
 
 #ifdef CONFIG_GACT_PROB
 static int gact_net_rand(struct tcf_gact *gact)
@@ -180,7 +179,6 @@ nla_put_failure:
 
 static struct tc_action_ops act_gact_ops = {
 	.kind		=	"gact",
-	.hinfo		=	&gact_hash_info,
 	.type		=	TCA_ACT_GACT,
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_gact,
@@ -194,21 +192,17 @@ MODULE_LICENSE("GPL");
 
 static int __init gact_init_module(void)
 {
-	int err = tcf_hashinfo_init(&gact_hash_info, GACT_TAB_MASK);
-	if (err)
-		return err;
 #ifdef CONFIG_GACT_PROB
 	pr_info("GACT probability on\n");
 #else
 	pr_info("GACT probability NOT on\n");
 #endif
-	return tcf_register_action(&act_gact_ops);
+	return tcf_register_action(&act_gact_ops, GACT_TAB_MASK);
 }
 
 static void __exit gact_cleanup_module(void)
 {
 	tcf_unregister_action(&act_gact_ops);
-	tcf_hashinfo_destroy(&gact_hash_info);
 }
 
 module_init(gact_init_module);
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 71f29f1..8a64a07 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -29,7 +29,6 @@
 
 
 #define IPT_TAB_MASK     15
-static struct tcf_hashinfo ipt_hash_info;
 
 static int ipt_init_target(struct xt_entry_target *t, char *table, unsigned int hook)
 {
@@ -262,7 +261,6 @@ nla_put_failure:
 
 static struct tc_action_ops act_ipt_ops = {
 	.kind		=	"ipt",
-	.hinfo		=	&ipt_hash_info,
 	.type		=	TCA_ACT_IPT,
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_ipt,
@@ -273,7 +271,6 @@ static struct tc_action_ops act_ipt_ops = {
 
 static struct tc_action_ops act_xt_ops = {
 	.kind		=	"xt",
-	.hinfo		=	&ipt_hash_info,
 	.type		=	TCA_ACT_XT,
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_ipt,
@@ -289,20 +286,16 @@ MODULE_ALIAS("act_xt");
 
 static int __init ipt_init_module(void)
 {
-	int ret1, ret2, err;
-	err = tcf_hashinfo_init(&ipt_hash_info, IPT_TAB_MASK);
-	if (err)
-		return err;
+	int ret1, ret2;
 
-	ret1 = tcf_register_action(&act_xt_ops);
+	ret1 = tcf_register_action(&act_xt_ops, IPT_TAB_MASK);
 	if (ret1 < 0)
 		printk("Failed to load xt action\n");
-	ret2 = tcf_register_action(&act_ipt_ops);
+	ret2 = tcf_register_action(&act_ipt_ops, IPT_TAB_MASK);
 	if (ret2 < 0)
 		printk("Failed to load ipt action\n");
 
 	if (ret1 < 0 && ret2 < 0) {
-		tcf_hashinfo_destroy(&ipt_hash_info);
 		return ret1;
 	} else
 		return 0;
@@ -312,7 +305,6 @@ static void __exit ipt_cleanup_module(void)
 {
 	tcf_unregister_action(&act_xt_ops);
 	tcf_unregister_action(&act_ipt_ops);
-	tcf_hashinfo_destroy(&ipt_hash_info);
 }
 
 module_init(ipt_init_module);
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 0f00eb9..4f912c0 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -31,7 +31,6 @@
 
 #define MIRRED_TAB_MASK     7
 static LIST_HEAD(mirred_list);
-static struct tcf_hashinfo mirred_hash_info;
 
 static void tcf_mirred_release(struct tc_action *a, int bind)
 {
@@ -234,7 +233,6 @@ static struct notifier_block mirred_device_notifier = {
 
 static struct tc_action_ops act_mirred_ops = {
 	.kind		=	"mirred",
-	.hinfo		=	&mirred_hash_info,
 	.type		=	TCA_ACT_MIRRED,
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_mirred,
@@ -253,19 +251,13 @@ static int __init mirred_init_module(void)
 	if (err)
 		return err;
 
-	err = tcf_hashinfo_init(&mirred_hash_info, MIRRED_TAB_MASK);
-	if (err) {
-		unregister_netdevice_notifier(&mirred_device_notifier);
-		return err;
-	}
 	pr_info("Mirror/redirect action on\n");
-	return tcf_register_action(&act_mirred_ops);
+	return tcf_register_action(&act_mirred_ops, MIRRED_TAB_MASK);
 }
 
 static void __exit mirred_cleanup_module(void)
 {
 	tcf_unregister_action(&act_mirred_ops);
-	tcf_hashinfo_destroy(&mirred_hash_info);
 	unregister_netdevice_notifier(&mirred_device_notifier);
 }
 
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 9a3cb1d..270a030 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -31,8 +31,6 @@
 
 #define NAT_TAB_MASK	15
 
-static struct tcf_hashinfo nat_hash_info;
-
 static const struct nla_policy nat_policy[TCA_NAT_MAX + 1] = {
 	[TCA_NAT_PARMS]	= { .len = sizeof(struct tc_nat) },
 };
@@ -284,7 +282,6 @@ nla_put_failure:
 
 static struct tc_action_ops act_nat_ops = {
 	.kind		=	"nat",
-	.hinfo		=	&nat_hash_info,
 	.type		=	TCA_ACT_NAT,
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_nat,
@@ -297,16 +294,12 @@ MODULE_LICENSE("GPL");
 
 static int __init nat_init_module(void)
 {
-	int err = tcf_hashinfo_init(&nat_hash_info, NAT_TAB_MASK);
-	if (err)
-		return err;
-	return tcf_register_action(&act_nat_ops);
+	return tcf_register_action(&act_nat_ops, NAT_TAB_MASK);
 }
 
 static void __exit nat_cleanup_module(void)
 {
 	tcf_unregister_action(&act_nat_ops);
-	tcf_hashinfo_destroy(&nat_hash_info);
 }
 
 module_init(nat_init_module);
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 8aa795b..5f9bcb2 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -25,8 +25,6 @@
 
 #define PEDIT_TAB_MASK	15
 
-static struct tcf_hashinfo pedit_hash_info;
-
 static const struct nla_policy pedit_policy[TCA_PEDIT_MAX + 1] = {
 	[TCA_PEDIT_PARMS]	= { .len = sizeof(struct tc_pedit) },
 };
@@ -218,7 +216,6 @@ nla_put_failure:
 
 static struct tc_action_ops act_pedit_ops = {
 	.kind		=	"pedit",
-	.hinfo		=	&pedit_hash_info,
 	.type		=	TCA_ACT_PEDIT,
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_pedit,
@@ -233,15 +230,11 @@ MODULE_LICENSE("GPL");
 
 static int __init pedit_init_module(void)
 {
-	int err = tcf_hashinfo_init(&pedit_hash_info, PEDIT_TAB_MASK);
-	if (err)
-		return err;
-	return tcf_register_action(&act_pedit_ops);
+	return tcf_register_action(&act_pedit_ops, PEDIT_TAB_MASK);
 }
 
 static void __exit pedit_cleanup_module(void)
 {
-	tcf_hashinfo_destroy(&pedit_hash_info);
 	tcf_unregister_action(&act_pedit_ops);
 }
 
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 7ff7bef..0566e46 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -41,7 +41,6 @@ struct tcf_police {
 	container_of(pc, struct tcf_police, common)
 
 #define POL_TAB_MASK     15
-static struct tcf_hashinfo police_hash_info;
 
 /* old policer structure from before tc actions */
 struct tc_police_compat {
@@ -234,7 +233,7 @@ override:
 
 	police->tcfp_t_c = ktime_to_ns(ktime_get());
 	police->tcf_index = parm->index ? parm->index :
-		tcf_hash_new_index(a->ops->hinfo);
+		tcf_hash_new_index(hinfo);
 	h = tcf_hash(police->tcf_index, POL_TAB_MASK);
 	spin_lock_bh(&hinfo->lock);
 	hlist_add_head(&police->tcf_head, &hinfo->htab[h]);
@@ -349,7 +348,6 @@ MODULE_LICENSE("GPL");
 
 static struct tc_action_ops act_police_ops = {
 	.kind		=	"police",
-	.hinfo		=	&police_hash_info,
 	.type		=	TCA_ID_POLICE,
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_act_police,
@@ -361,19 +359,12 @@ static struct tc_action_ops act_police_ops = {
 static int __init
 police_init_module(void)
 {
-	int err = tcf_hashinfo_init(&police_hash_info, POL_TAB_MASK);
-	if (err)
-		return err;
-	err = tcf_register_action(&act_police_ops);
-	if (err)
-		tcf_hashinfo_destroy(&police_hash_info);
-	return err;
+	return tcf_register_action(&act_police_ops, POL_TAB_MASK);
 }
 
 static void __exit
 police_cleanup_module(void)
 {
-	tcf_hashinfo_destroy(&police_hash_info);
 	tcf_unregister_action(&act_police_ops);
 }
 
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index 14b5e36..992c231 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -25,7 +25,6 @@
 #include <net/tc_act/tc_defact.h>
 
 #define SIMP_TAB_MASK     7
-static struct tcf_hashinfo simp_hash_info;
 
 #define SIMP_MAX_DATA	32
 static int tcf_simp(struct sk_buff *skb, const struct tc_action *a,
@@ -163,7 +162,6 @@ nla_put_failure:
 
 static struct tc_action_ops act_simp_ops = {
 	.kind		=	"simple",
-	.hinfo		=	&simp_hash_info,
 	.type		=	TCA_ACT_SIMP,
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_simp,
@@ -178,23 +176,15 @@ MODULE_LICENSE("GPL");
 
 static int __init simp_init_module(void)
 {
-	int err, ret;
-	err = tcf_hashinfo_init(&simp_hash_info, SIMP_TAB_MASK);
-	if (err)
-		return err;
-
-	ret = tcf_register_action(&act_simp_ops);
+	int ret;
+	ret = tcf_register_action(&act_simp_ops, SIMP_TAB_MASK);
 	if (!ret)
 		pr_info("Simple TC action Loaded\n");
-	else
-		tcf_hashinfo_destroy(&simp_hash_info);
-
 	return ret;
 }
 
 static void __exit simp_cleanup_module(void)
 {
-	tcf_hashinfo_destroy(&simp_hash_info);
 	tcf_unregister_action(&act_simp_ops);
 }
 
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index 9f91928..fcfeeaf 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -28,7 +28,6 @@
 #include <net/tc_act/tc_skbedit.h>
 
 #define SKBEDIT_TAB_MASK     15
-static struct tcf_hashinfo skbedit_hash_info;
 
 static int tcf_skbedit(struct sk_buff *skb, const struct tc_action *a,
 		       struct tcf_result *res)
@@ -175,7 +174,6 @@ nla_put_failure:
 
 static struct tc_action_ops act_skbedit_ops = {
 	.kind		=	"skbedit",
-	.hinfo		=	&skbedit_hash_info,
 	.type		=	TCA_ACT_SKBEDIT,
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_skbedit,
@@ -189,15 +187,11 @@ MODULE_LICENSE("GPL");
 
 static int __init skbedit_init_module(void)
 {
-	int err = tcf_hashinfo_init(&skbedit_hash_info, SKBEDIT_TAB_MASK);
-	if (err)
-		return err;
-	return tcf_register_action(&act_skbedit_ops);
+	return tcf_register_action(&act_skbedit_ops, SKBEDIT_TAB_MASK);
 }
 
 static void __exit skbedit_cleanup_module(void)
 {
-	tcf_hashinfo_destroy(&skbedit_hash_info);
 	tcf_unregister_action(&act_skbedit_ops);
 }
 
-- 
1.8.3.1

Re: [Patch net-next v3 3/5] net_sched: act: move tcf_hashinfo_init() into tcf_register_action()

[Jamal Hadi Salim]

On 02/11/14 20:07, Cong Wang wrote:
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>


Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

> ---
>   include/net/act_api.h   |  2 +-
>   net/sched/act_api.c     | 16 +++++++++++++++-
>   net/sched/act_csum.c    |  8 +-------
>   net/sched/act_gact.c    |  8 +-------
>   net/sched/act_ipt.c     | 14 +++-----------
>   net/sched/act_mirred.c  | 10 +---------
>   net/sched/act_nat.c     |  9 +--------
>   net/sched/act_pedit.c   |  9 +--------
>   net/sched/act_police.c  | 13 ++-----------
>   net/sched/act_simple.c  | 14 ++------------
>   net/sched/act_skbedit.c |  8 +-------
>   11 files changed, 29 insertions(+), 82 deletions(-)
>
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index 3d22f42..969cac6 100644
> --- a/include/net/act_api.h
> +++ b/include/net/act_api.h
> @@ -107,7 +107,7 @@ int tcf_hash_create(u32 index, struct nlattr *est, struct tc_action *a,
>   void tcf_hash_cleanup(struct tc_action *a, struct nlattr *est);
>   void tcf_hash_insert(struct tc_action *a);
>
> -int tcf_register_action(struct tc_action_ops *a);
> +int tcf_register_action(struct tc_action_ops *a, unsigned int mask);
>   int tcf_unregister_action(struct tc_action_ops *a);
>   void tcf_action_destroy(struct list_head *actions, int bind);
>   int tcf_action_exec(struct sk_buff *skb, const struct list_head *actions,
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index a5bf935..c88d382 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -275,9 +275,10 @@ EXPORT_SYMBOL(tcf_hash_insert);
>   static LIST_HEAD(act_base);
>   static DEFINE_RWLOCK(act_mod_lock);
>
> -int tcf_register_action(struct tc_action_ops *act)
> +int tcf_register_action(struct tc_action_ops *act, unsigned int mask)
>   {
>   	struct tc_action_ops *a;
> +	int err;
>
>   	/* Must supply act, dump and init */
>   	if (!act->act || !act->dump || !act->init)
> @@ -289,10 +290,21 @@ int tcf_register_action(struct tc_action_ops *act)
>   	if (!act->walk)
>   		act->walk = tcf_generic_walker;
>
> +	act->hinfo = kmalloc(sizeof(struct tcf_hashinfo), GFP_KERNEL);
> +	if (!act->hinfo)
> +		return -ENOMEM;
> +	err = tcf_hashinfo_init(act->hinfo, mask);
> +	if (err) {
> +		kfree(act->hinfo);
> +		return err;
> +	}
> +
>   	write_lock(&act_mod_lock);
>   	list_for_each_entry(a, &act_base, head) {
>   		if (act->type == a->type || (strcmp(act->kind, a->kind) == 0)) {
>   			write_unlock(&act_mod_lock);
> +			tcf_hashinfo_destroy(act->hinfo);
> +			kfree(act->hinfo);
>   			return -EEXIST;
>   		}
>   	}
> @@ -311,6 +323,8 @@ int tcf_unregister_action(struct tc_action_ops *act)
>   	list_for_each_entry(a, &act_base, head) {
>   		if (a == act) {
>   			list_del(&act->head);
> +			tcf_hashinfo_destroy(act->hinfo);
> +			kfree(act->hinfo);
>   			err = 0;
>   			break;
>   		}
> diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
> index 8df3060..edbf40d 100644
> --- a/net/sched/act_csum.c
> +++ b/net/sched/act_csum.c
> @@ -37,7 +37,6 @@
>   #include <net/tc_act/tc_csum.h>
>
>   #define CSUM_TAB_MASK 15
> -static struct tcf_hashinfo csum_hash_info;
>
>   static const struct nla_policy csum_policy[TCA_CSUM_MAX + 1] = {
>   	[TCA_CSUM_PARMS] = { .len = sizeof(struct tc_csum), },
> @@ -561,7 +560,6 @@ nla_put_failure:
>
>   static struct tc_action_ops act_csum_ops = {
>   	.kind		= "csum",
> -	.hinfo		= &csum_hash_info,
>   	.type		= TCA_ACT_CSUM,
>   	.owner		= THIS_MODULE,
>   	.act		= tcf_csum,
> @@ -574,11 +572,7 @@ MODULE_LICENSE("GPL");
>
>   static int __init csum_init_module(void)
>   {
> -	int err = tcf_hashinfo_init(&csum_hash_info, CSUM_TAB_MASK);
> -	if (err)
> -		return err;
> -
> -	return tcf_register_action(&act_csum_ops);
> +	return tcf_register_action(&act_csum_ops, CSUM_TAB_MASK);
>   }
>
>   static void __exit csum_cleanup_module(void)
> diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
> index 094a1b5..d6bcbd9 100644
> --- a/net/sched/act_gact.c
> +++ b/net/sched/act_gact.c
> @@ -24,7 +24,6 @@
>   #include <net/tc_act/tc_gact.h>
>
>   #define GACT_TAB_MASK	15
> -static struct tcf_hashinfo gact_hash_info;
>
>   #ifdef CONFIG_GACT_PROB
>   static int gact_net_rand(struct tcf_gact *gact)
> @@ -180,7 +179,6 @@ nla_put_failure:
>
>   static struct tc_action_ops act_gact_ops = {
>   	.kind		=	"gact",
> -	.hinfo		=	&gact_hash_info,
>   	.type		=	TCA_ACT_GACT,
>   	.owner		=	THIS_MODULE,
>   	.act		=	tcf_gact,
> @@ -194,21 +192,17 @@ MODULE_LICENSE("GPL");
>
>   static int __init gact_init_module(void)
>   {
> -	int err = tcf_hashinfo_init(&gact_hash_info, GACT_TAB_MASK);
> -	if (err)
> -		return err;
>   #ifdef CONFIG_GACT_PROB
>   	pr_info("GACT probability on\n");
>   #else
>   	pr_info("GACT probability NOT on\n");
>   #endif
> -	return tcf_register_action(&act_gact_ops);
> +	return tcf_register_action(&act_gact_ops, GACT_TAB_MASK);
>   }
>
>   static void __exit gact_cleanup_module(void)
>   {
>   	tcf_unregister_action(&act_gact_ops);
> -	tcf_hashinfo_destroy(&gact_hash_info);
>   }
>
>   module_init(gact_init_module);
> diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
> index 71f29f1..8a64a07 100644
> --- a/net/sched/act_ipt.c
> +++ b/net/sched/act_ipt.c
> @@ -29,7 +29,6 @@
>
>
>   #define IPT_TAB_MASK     15
> -static struct tcf_hashinfo ipt_hash_info;
>
>   static int ipt_init_target(struct xt_entry_target *t, char *table, unsigned int hook)
>   {
> @@ -262,7 +261,6 @@ nla_put_failure:
>
>   static struct tc_action_ops act_ipt_ops = {
>   	.kind		=	"ipt",
> -	.hinfo		=	&ipt_hash_info,
>   	.type		=	TCA_ACT_IPT,
>   	.owner		=	THIS_MODULE,
>   	.act		=	tcf_ipt,
> @@ -273,7 +271,6 @@ static struct tc_action_ops act_ipt_ops = {
>
>   static struct tc_action_ops act_xt_ops = {
>   	.kind		=	"xt",
> -	.hinfo		=	&ipt_hash_info,
>   	.type		=	TCA_ACT_XT,
>   	.owner		=	THIS_MODULE,
>   	.act		=	tcf_ipt,
> @@ -289,20 +286,16 @@ MODULE_ALIAS("act_xt");
>
>   static int __init ipt_init_module(void)
>   {
> -	int ret1, ret2, err;
> -	err = tcf_hashinfo_init(&ipt_hash_info, IPT_TAB_MASK);
> -	if (err)
> -		return err;
> +	int ret1, ret2;
>
> -	ret1 = tcf_register_action(&act_xt_ops);
> +	ret1 = tcf_register_action(&act_xt_ops, IPT_TAB_MASK);
>   	if (ret1 < 0)
>   		printk("Failed to load xt action\n");
> -	ret2 = tcf_register_action(&act_ipt_ops);
> +	ret2 = tcf_register_action(&act_ipt_ops, IPT_TAB_MASK);
>   	if (ret2 < 0)
>   		printk("Failed to load ipt action\n");
>
>   	if (ret1 < 0 && ret2 < 0) {
> -		tcf_hashinfo_destroy(&ipt_hash_info);
>   		return ret1;
>   	} else
>   		return 0;
> @@ -312,7 +305,6 @@ static void __exit ipt_cleanup_module(void)
>   {
>   	tcf_unregister_action(&act_xt_ops);
>   	tcf_unregister_action(&act_ipt_ops);
> -	tcf_hashinfo_destroy(&ipt_hash_info);
>   }
>
>   module_init(ipt_init_module);
> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> index 0f00eb9..4f912c0 100644
> --- a/net/sched/act_mirred.c
> +++ b/net/sched/act_mirred.c
> @@ -31,7 +31,6 @@
>
>   #define MIRRED_TAB_MASK     7
>   static LIST_HEAD(mirred_list);
> -static struct tcf_hashinfo mirred_hash_info;
>
>   static void tcf_mirred_release(struct tc_action *a, int bind)
>   {
> @@ -234,7 +233,6 @@ static struct notifier_block mirred_device_notifier = {
>
>   static struct tc_action_ops act_mirred_ops = {
>   	.kind		=	"mirred",
> -	.hinfo		=	&mirred_hash_info,
>   	.type		=	TCA_ACT_MIRRED,
>   	.owner		=	THIS_MODULE,
>   	.act		=	tcf_mirred,
> @@ -253,19 +251,13 @@ static int __init mirred_init_module(void)
>   	if (err)
>   		return err;
>
> -	err = tcf_hashinfo_init(&mirred_hash_info, MIRRED_TAB_MASK);
> -	if (err) {
> -		unregister_netdevice_notifier(&mirred_device_notifier);
> -		return err;
> -	}
>   	pr_info("Mirror/redirect action on\n");
> -	return tcf_register_action(&act_mirred_ops);
> +	return tcf_register_action(&act_mirred_ops, MIRRED_TAB_MASK);
>   }
>
>   static void __exit mirred_cleanup_module(void)
>   {
>   	tcf_unregister_action(&act_mirred_ops);
> -	tcf_hashinfo_destroy(&mirred_hash_info);
>   	unregister_netdevice_notifier(&mirred_device_notifier);
>   }
>
> diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
> index 9a3cb1d..270a030 100644
> --- a/net/sched/act_nat.c
> +++ b/net/sched/act_nat.c
> @@ -31,8 +31,6 @@
>
>   #define NAT_TAB_MASK	15
>
> -static struct tcf_hashinfo nat_hash_info;
> -
>   static const struct nla_policy nat_policy[TCA_NAT_MAX + 1] = {
>   	[TCA_NAT_PARMS]	= { .len = sizeof(struct tc_nat) },
>   };
> @@ -284,7 +282,6 @@ nla_put_failure:
>
>   static struct tc_action_ops act_nat_ops = {
>   	.kind		=	"nat",
> -	.hinfo		=	&nat_hash_info,
>   	.type		=	TCA_ACT_NAT,
>   	.owner		=	THIS_MODULE,
>   	.act		=	tcf_nat,
> @@ -297,16 +294,12 @@ MODULE_LICENSE("GPL");
>
>   static int __init nat_init_module(void)
>   {
> -	int err = tcf_hashinfo_init(&nat_hash_info, NAT_TAB_MASK);
> -	if (err)
> -		return err;
> -	return tcf_register_action(&act_nat_ops);
> +	return tcf_register_action(&act_nat_ops, NAT_TAB_MASK);
>   }
>
>   static void __exit nat_cleanup_module(void)
>   {
>   	tcf_unregister_action(&act_nat_ops);
> -	tcf_hashinfo_destroy(&nat_hash_info);
>   }
>
>   module_init(nat_init_module);
> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> index 8aa795b..5f9bcb2 100644
> --- a/net/sched/act_pedit.c
> +++ b/net/sched/act_pedit.c
> @@ -25,8 +25,6 @@
>
>   #define PEDIT_TAB_MASK	15
>
> -static struct tcf_hashinfo pedit_hash_info;
> -
>   static const struct nla_policy pedit_policy[TCA_PEDIT_MAX + 1] = {
>   	[TCA_PEDIT_PARMS]	= { .len = sizeof(struct tc_pedit) },
>   };
> @@ -218,7 +216,6 @@ nla_put_failure:
>
>   static struct tc_action_ops act_pedit_ops = {
>   	.kind		=	"pedit",
> -	.hinfo		=	&pedit_hash_info,
>   	.type		=	TCA_ACT_PEDIT,
>   	.owner		=	THIS_MODULE,
>   	.act		=	tcf_pedit,
> @@ -233,15 +230,11 @@ MODULE_LICENSE("GPL");
>
>   static int __init pedit_init_module(void)
>   {
> -	int err = tcf_hashinfo_init(&pedit_hash_info, PEDIT_TAB_MASK);
> -	if (err)
> -		return err;
> -	return tcf_register_action(&act_pedit_ops);
> +	return tcf_register_action(&act_pedit_ops, PEDIT_TAB_MASK);
>   }
>
>   static void __exit pedit_cleanup_module(void)
>   {
> -	tcf_hashinfo_destroy(&pedit_hash_info);
>   	tcf_unregister_action(&act_pedit_ops);
>   }
>
> diff --git a/net/sched/act_police.c b/net/sched/act_police.c
> index 7ff7bef..0566e46 100644
> --- a/net/sched/act_police.c
> +++ b/net/sched/act_police.c
> @@ -41,7 +41,6 @@ struct tcf_police {
>   	container_of(pc, struct tcf_police, common)
>
>   #define POL_TAB_MASK     15
> -static struct tcf_hashinfo police_hash_info;
>
>   /* old policer structure from before tc actions */
>   struct tc_police_compat {
> @@ -234,7 +233,7 @@ override:
>
>   	police->tcfp_t_c = ktime_to_ns(ktime_get());
>   	police->tcf_index = parm->index ? parm->index :
> -		tcf_hash_new_index(a->ops->hinfo);
> +		tcf_hash_new_index(hinfo);
>   	h = tcf_hash(police->tcf_index, POL_TAB_MASK);
>   	spin_lock_bh(&hinfo->lock);
>   	hlist_add_head(&police->tcf_head, &hinfo->htab[h]);
> @@ -349,7 +348,6 @@ MODULE_LICENSE("GPL");
>
>   static struct tc_action_ops act_police_ops = {
>   	.kind		=	"police",
> -	.hinfo		=	&police_hash_info,
>   	.type		=	TCA_ID_POLICE,
>   	.owner		=	THIS_MODULE,
>   	.act		=	tcf_act_police,
> @@ -361,19 +359,12 @@ static struct tc_action_ops act_police_ops = {
>   static int __init
>   police_init_module(void)
>   {
> -	int err = tcf_hashinfo_init(&police_hash_info, POL_TAB_MASK);
> -	if (err)
> -		return err;
> -	err = tcf_register_action(&act_police_ops);
> -	if (err)
> -		tcf_hashinfo_destroy(&police_hash_info);
> -	return err;
> +	return tcf_register_action(&act_police_ops, POL_TAB_MASK);
>   }
>
>   static void __exit
>   police_cleanup_module(void)
>   {
> -	tcf_hashinfo_destroy(&police_hash_info);
>   	tcf_unregister_action(&act_police_ops);
>   }
>
> diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
> index 14b5e36..992c231 100644
> --- a/net/sched/act_simple.c
> +++ b/net/sched/act_simple.c
> @@ -25,7 +25,6 @@
>   #include <net/tc_act/tc_defact.h>
>
>   #define SIMP_TAB_MASK     7
> -static struct tcf_hashinfo simp_hash_info;
>
>   #define SIMP_MAX_DATA	32
>   static int tcf_simp(struct sk_buff *skb, const struct tc_action *a,
> @@ -163,7 +162,6 @@ nla_put_failure:
>
>   static struct tc_action_ops act_simp_ops = {
>   	.kind		=	"simple",
> -	.hinfo		=	&simp_hash_info,
>   	.type		=	TCA_ACT_SIMP,
>   	.owner		=	THIS_MODULE,
>   	.act		=	tcf_simp,
> @@ -178,23 +176,15 @@ MODULE_LICENSE("GPL");
>
>   static int __init simp_init_module(void)
>   {
> -	int err, ret;
> -	err = tcf_hashinfo_init(&simp_hash_info, SIMP_TAB_MASK);
> -	if (err)
> -		return err;
> -
> -	ret = tcf_register_action(&act_simp_ops);
> +	int ret;
> +	ret = tcf_register_action(&act_simp_ops, SIMP_TAB_MASK);
>   	if (!ret)
>   		pr_info("Simple TC action Loaded\n");
> -	else
> -		tcf_hashinfo_destroy(&simp_hash_info);
> -
>   	return ret;
>   }
>
>   static void __exit simp_cleanup_module(void)
>   {
> -	tcf_hashinfo_destroy(&simp_hash_info);
>   	tcf_unregister_action(&act_simp_ops);
>   }
>
> diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
> index 9f91928..fcfeeaf 100644
> --- a/net/sched/act_skbedit.c
> +++ b/net/sched/act_skbedit.c
> @@ -28,7 +28,6 @@
>   #include <net/tc_act/tc_skbedit.h>
>
>   #define SKBEDIT_TAB_MASK     15
> -static struct tcf_hashinfo skbedit_hash_info;
>
>   static int tcf_skbedit(struct sk_buff *skb, const struct tc_action *a,
>   		       struct tcf_result *res)
> @@ -175,7 +174,6 @@ nla_put_failure:
>
>   static struct tc_action_ops act_skbedit_ops = {
>   	.kind		=	"skbedit",
> -	.hinfo		=	&skbedit_hash_info,
>   	.type		=	TCA_ACT_SKBEDIT,
>   	.owner		=	THIS_MODULE,
>   	.act		=	tcf_skbedit,
> @@ -189,15 +187,11 @@ MODULE_LICENSE("GPL");
>
>   static int __init skbedit_init_module(void)
>   {
> -	int err = tcf_hashinfo_init(&skbedit_hash_info, SKBEDIT_TAB_MASK);
> -	if (err)
> -		return err;
> -	return tcf_register_action(&act_skbedit_ops);
> +	return tcf_register_action(&act_skbedit_ops, SKBEDIT_TAB_MASK);
>   }
>
>   static void __exit skbedit_cleanup_module(void)
>   {
> -	tcf_hashinfo_destroy(&skbedit_hash_info);
>   	tcf_unregister_action(&act_skbedit_ops);
>   }
>
>

[Patch net-next v3 4/5] net_sched: act: refuse to remove bound action outside

[Cong Wang]

When an action is bonnd to a filter, there is no point to
remove it outside. Currently we just silently decrease the refcnt,
we should reject this explicitly with EPERM.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/act_api.h |  2 +-
 net/sched/act_api.c   | 26 ++++++++++++++++++++------
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 969cac6..3ee4c92 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -109,7 +109,7 @@ void tcf_hash_insert(struct tc_action *a);
 
 int tcf_register_action(struct tc_action_ops *a, unsigned int mask);
 int tcf_unregister_action(struct tc_action_ops *a);
-void tcf_action_destroy(struct list_head *actions, int bind);
+int tcf_action_destroy(struct list_head *actions, int bind);
 int tcf_action_exec(struct sk_buff *skb, const struct list_head *actions,
 		    struct tcf_result *res);
 int tcf_action_init(struct net *net, struct nlattr *nla,
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index c88d382..27e4c53 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -53,6 +53,8 @@ int tcf_hash_release(struct tc_action *a, int bind)
 	if (p) {
 		if (bind)
 			p->tcfc_bindcnt--;
+		else if (p->tcfc_bindcnt > 0)
+			return -EPERM;
 
 		p->tcfc_refcnt--;
 		if (p->tcfc_bindcnt <= 0 && p->tcfc_refcnt <= 0) {
@@ -123,6 +125,7 @@ static int tcf_del_walker(struct sk_buff *skb, struct tc_action *a)
 	struct tcf_common *p;
 	struct nlattr *nest;
 	int i = 0, n_i = 0;
+	int ret = -EINVAL;
 
 	nest = nla_nest_start(skb, a->order);
 	if (nest == NULL)
@@ -133,10 +136,12 @@ static int tcf_del_walker(struct sk_buff *skb, struct tc_action *a)
 		head = &hinfo->htab[tcf_hash(i, hinfo->hmask)];
 		hlist_for_each_entry_safe(p, n, head, tcfc_head) {
 			a->priv = p;
-			if (ACT_P_DELETED == tcf_hash_release(a, 0)) {
+			ret = tcf_hash_release(a, 0);
+			if (ret == ACT_P_DELETED) {
 				module_put(a->ops->owner);
 				n_i++;
-			}
+			} else if (ret < 0)
+				goto nla_put_failure;
 		}
 	}
 	if (nla_put_u32(skb, TCA_FCNT, n_i))
@@ -146,7 +151,7 @@ static int tcf_del_walker(struct sk_buff *skb, struct tc_action *a)
 	return n_i;
 nla_put_failure:
 	nla_nest_cancel(skb, nest);
-	return -EINVAL;
+	return ret;
 }
 
 static int tcf_generic_walker(struct sk_buff *skb, struct netlink_callback *cb,
@@ -401,16 +406,21 @@ exec_done:
 }
 EXPORT_SYMBOL(tcf_action_exec);
 
-void tcf_action_destroy(struct list_head *actions, int bind)
+int tcf_action_destroy(struct list_head *actions, int bind)
 {
 	struct tc_action *a, *tmp;
+	int ret = 0;
 
 	list_for_each_entry_safe(a, tmp, actions, list) {
-		if (tcf_hash_release(a, bind) == ACT_P_DELETED)
+		ret = tcf_hash_release(a, bind);
+		if (ret == ACT_P_DELETED)
 			module_put(a->ops->owner);
+		else if (ret < 0)
+			return ret;
 		list_del(&a->list);
 		kfree(a);
 	}
+	return ret;
 }
 
 int
@@ -838,7 +848,11 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions,
 	}
 
 	/* now do the delete */
-	tcf_action_destroy(actions, 0);
+	ret = tcf_action_destroy(actions, 0);
+	if (ret < 0) {
+		kfree_skb(skb);
+		return ret;
+	}
 
 	ret = rtnetlink_send(skb, net, portid, RTNLGRP_TC,
 			     n->nlmsg_flags & NLM_F_ECHO);
-- 
1.8.3.1

Re: [Patch net-next v3 4/5] net_sched: act: refuse to remove bound action outside

[Jamal Hadi Salim]

On 02/11/14 20:07, Cong Wang wrote:
> When an action is bonnd to a filter, there is no point to
> remove it outside. Currently we just silently decrease the refcnt,
> we should reject this explicitly with EPERM.
>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>


Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

> ---
>   include/net/act_api.h |  2 +-
>   net/sched/act_api.c   | 26 ++++++++++++++++++++------
>   2 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index 969cac6..3ee4c92 100644
> --- a/include/net/act_api.h
> +++ b/include/net/act_api.h
> @@ -109,7 +109,7 @@ void tcf_hash_insert(struct tc_action *a);
>
>   int tcf_register_action(struct tc_action_ops *a, unsigned int mask);
>   int tcf_unregister_action(struct tc_action_ops *a);
> -void tcf_action_destroy(struct list_head *actions, int bind);
> +int tcf_action_destroy(struct list_head *actions, int bind);
>   int tcf_action_exec(struct sk_buff *skb, const struct list_head *actions,
>   		    struct tcf_result *res);
>   int tcf_action_init(struct net *net, struct nlattr *nla,
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index c88d382..27e4c53 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -53,6 +53,8 @@ int tcf_hash_release(struct tc_action *a, int bind)
>   	if (p) {
>   		if (bind)
>   			p->tcfc_bindcnt--;
> +		else if (p->tcfc_bindcnt > 0)
> +			return -EPERM;
>
>   		p->tcfc_refcnt--;
>   		if (p->tcfc_bindcnt <= 0 && p->tcfc_refcnt <= 0) {
> @@ -123,6 +125,7 @@ static int tcf_del_walker(struct sk_buff *skb, struct tc_action *a)
>   	struct tcf_common *p;
>   	struct nlattr *nest;
>   	int i = 0, n_i = 0;
> +	int ret = -EINVAL;
>
>   	nest = nla_nest_start(skb, a->order);
>   	if (nest == NULL)
> @@ -133,10 +136,12 @@ static int tcf_del_walker(struct sk_buff *skb, struct tc_action *a)
>   		head = &hinfo->htab[tcf_hash(i, hinfo->hmask)];
>   		hlist_for_each_entry_safe(p, n, head, tcfc_head) {
>   			a->priv = p;
> -			if (ACT_P_DELETED == tcf_hash_release(a, 0)) {
> +			ret = tcf_hash_release(a, 0);
> +			if (ret == ACT_P_DELETED) {
>   				module_put(a->ops->owner);
>   				n_i++;
> -			}
> +			} else if (ret < 0)
> +				goto nla_put_failure;
>   		}
>   	}
>   	if (nla_put_u32(skb, TCA_FCNT, n_i))
> @@ -146,7 +151,7 @@ static int tcf_del_walker(struct sk_buff *skb, struct tc_action *a)
>   	return n_i;
>   nla_put_failure:
>   	nla_nest_cancel(skb, nest);
> -	return -EINVAL;
> +	return ret;
>   }
>
>   static int tcf_generic_walker(struct sk_buff *skb, struct netlink_callback *cb,
> @@ -401,16 +406,21 @@ exec_done:
>   }
>   EXPORT_SYMBOL(tcf_action_exec);
>
> -void tcf_action_destroy(struct list_head *actions, int bind)
> +int tcf_action_destroy(struct list_head *actions, int bind)
>   {
>   	struct tc_action *a, *tmp;
> +	int ret = 0;
>
>   	list_for_each_entry_safe(a, tmp, actions, list) {
> -		if (tcf_hash_release(a, bind) == ACT_P_DELETED)
> +		ret = tcf_hash_release(a, bind);
> +		if (ret == ACT_P_DELETED)
>   			module_put(a->ops->owner);
> +		else if (ret < 0)
> +			return ret;
>   		list_del(&a->list);
>   		kfree(a);
>   	}
> +	return ret;
>   }
>
>   int
> @@ -838,7 +848,11 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions,
>   	}
>
>   	/* now do the delete */
> -	tcf_action_destroy(actions, 0);
> +	ret = tcf_action_destroy(actions, 0);
> +	if (ret < 0) {
> +		kfree_skb(skb);
> +		return ret;
> +	}
>
>   	ret = rtnetlink_send(skb, net, portid, RTNLGRP_TC,
>   			     n->nlmsg_flags & NLM_F_ECHO);
>

[Patch net-next v3 5/5] net_sched: act: clean up tca_action_flush()

[Cong Wang]

We could allocate tc_action on stack in tca_action_flush(),
since it is not large.

Also, we could use create_a() in tcf_action_get_1().

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/act_api.c | 53 +++++++++++++++++++++++------------------------------
 1 file changed, 23 insertions(+), 30 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 27e4c53..8a5ba5a 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -685,6 +685,20 @@ act_get_notify(struct net *net, u32 portid, struct nlmsghdr *n,
 	return rtnl_unicast(skb, net, portid);
 }
 
+static struct tc_action *create_a(int i)
+{
+	struct tc_action *act;
+
+	act = kzalloc(sizeof(*act), GFP_KERNEL);
+	if (act == NULL) {
+		pr_debug("create_a: failed to alloc!\n");
+		return NULL;
+	}
+	act->order = i;
+	INIT_LIST_HEAD(&act->list);
+	return act;
+}
+
 static struct tc_action *
 tcf_action_get_1(struct nlattr *nla, struct nlmsghdr *n, u32 portid)
 {
@@ -704,11 +718,10 @@ tcf_action_get_1(struct nlattr *nla, struct nlmsghdr *n, u32 portid)
 	index = nla_get_u32(tb[TCA_ACT_INDEX]);
 
 	err = -ENOMEM;
-	a = kzalloc(sizeof(struct tc_action), GFP_KERNEL);
+	a = create_a(0);
 	if (a == NULL)
 		goto err_out;
 
-	INIT_LIST_HEAD(&a->list);
 	err = -EINVAL;
 	a->ops = tc_lookup_action(tb[TCA_ACT_KIND]);
 	if (a->ops == NULL) /* could happen in batch of actions */
@@ -738,20 +751,6 @@ static void cleanup_a(struct list_head *actions)
 	}
 }
 
-static struct tc_action *create_a(int i)
-{
-	struct tc_action *act;
-
-	act = kzalloc(sizeof(*act), GFP_KERNEL);
-	if (act == NULL) {
-		pr_debug("create_a: failed to alloc!\n");
-		return NULL;
-	}
-	act->order = i;
-	INIT_LIST_HEAD(&act->list);
-	return act;
-}
-
 static int tca_action_flush(struct net *net, struct nlattr *nla,
 			    struct nlmsghdr *n, u32 portid)
 {
@@ -763,18 +762,12 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
 	struct nlattr *nest;
 	struct nlattr *tb[TCA_ACT_MAX + 1];
 	struct nlattr *kind;
-	struct tc_action *a = create_a(0);
+	struct tc_action a;
 	int err = -ENOMEM;
 
-	if (a == NULL) {
-		pr_debug("tca_action_flush: couldnt create tc_action\n");
-		return err;
-	}
-
 	skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
 	if (!skb) {
 		pr_debug("tca_action_flush: failed skb alloc\n");
-		kfree(a);
 		return err;
 	}
 
@@ -786,8 +779,10 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
 
 	err = -EINVAL;
 	kind = tb[TCA_ACT_KIND];
-	a->ops = tc_lookup_action(kind);
-	if (a->ops == NULL) /*some idjot trying to flush unknown action */
+	memset(&a, 0, sizeof(struct tc_action));
+	INIT_LIST_HEAD(&a.list);
+	a.ops = tc_lookup_action(kind);
+	if (a.ops == NULL) /*some idjot trying to flush unknown action */
 		goto err_out;
 
 	nlh = nlmsg_put(skb, portid, n->nlmsg_seq, RTM_DELACTION, sizeof(*t), 0);
@@ -802,7 +797,7 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
 	if (nest == NULL)
 		goto out_module_put;
 
-	err = a->ops->walk(skb, &dcb, RTM_DELACTION, a);
+	err = a.ops->walk(skb, &dcb, RTM_DELACTION, &a);
 	if (err < 0)
 		goto out_module_put;
 	if (err == 0)
@@ -812,8 +807,7 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
 
 	nlh->nlmsg_len = skb_tail_pointer(skb) - b;
 	nlh->nlmsg_flags |= NLM_F_ROOT;
-	module_put(a->ops->owner);
-	kfree(a);
+	module_put(a.ops->owner);
 	err = rtnetlink_send(skb, net, portid, RTNLGRP_TC,
 			     n->nlmsg_flags & NLM_F_ECHO);
 	if (err > 0)
@@ -822,11 +816,10 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
 	return err;
 
 out_module_put:
-	module_put(a->ops->owner);
+	module_put(a.ops->owner);
 err_out:
 noflush_out:
 	kfree_skb(skb);
-	kfree(a);
 	return err;
 }
 
-- 
1.8.3.1

Re: [Patch net-next v3 5/5] net_sched: act: clean up tca_action_flush()

[Jamal Hadi Salim]

On 02/11/14 20:07, Cong Wang wrote:
> We could allocate tc_action on stack in tca_action_flush(),
> since it is not large.
>
> Also, we could use create_a() in tcf_action_get_1().
>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>


Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

> ---
>   net/sched/act_api.c | 53 +++++++++++++++++++++++------------------------------
>   1 file changed, 23 insertions(+), 30 deletions(-)
>
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 27e4c53..8a5ba5a 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -685,6 +685,20 @@ act_get_notify(struct net *net, u32 portid, struct nlmsghdr *n,
>   	return rtnl_unicast(skb, net, portid);
>   }
>
> +static struct tc_action *create_a(int i)
> +{
> +	struct tc_action *act;
> +
> +	act = kzalloc(sizeof(*act), GFP_KERNEL);
> +	if (act == NULL) {
> +		pr_debug("create_a: failed to alloc!\n");
> +		return NULL;
> +	}
> +	act->order = i;
> +	INIT_LIST_HEAD(&act->list);
> +	return act;
> +}
> +
>   static struct tc_action *
>   tcf_action_get_1(struct nlattr *nla, struct nlmsghdr *n, u32 portid)
>   {
> @@ -704,11 +718,10 @@ tcf_action_get_1(struct nlattr *nla, struct nlmsghdr *n, u32 portid)
>   	index = nla_get_u32(tb[TCA_ACT_INDEX]);
>
>   	err = -ENOMEM;
> -	a = kzalloc(sizeof(struct tc_action), GFP_KERNEL);
> +	a = create_a(0);
>   	if (a == NULL)
>   		goto err_out;
>
> -	INIT_LIST_HEAD(&a->list);
>   	err = -EINVAL;
>   	a->ops = tc_lookup_action(tb[TCA_ACT_KIND]);
>   	if (a->ops == NULL) /* could happen in batch of actions */
> @@ -738,20 +751,6 @@ static void cleanup_a(struct list_head *actions)
>   	}
>   }
>
> -static struct tc_action *create_a(int i)
> -{
> -	struct tc_action *act;
> -
> -	act = kzalloc(sizeof(*act), GFP_KERNEL);
> -	if (act == NULL) {
> -		pr_debug("create_a: failed to alloc!\n");
> -		return NULL;
> -	}
> -	act->order = i;
> -	INIT_LIST_HEAD(&act->list);
> -	return act;
> -}
> -
>   static int tca_action_flush(struct net *net, struct nlattr *nla,
>   			    struct nlmsghdr *n, u32 portid)
>   {
> @@ -763,18 +762,12 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
>   	struct nlattr *nest;
>   	struct nlattr *tb[TCA_ACT_MAX + 1];
>   	struct nlattr *kind;
> -	struct tc_action *a = create_a(0);
> +	struct tc_action a;
>   	int err = -ENOMEM;
>
> -	if (a == NULL) {
> -		pr_debug("tca_action_flush: couldnt create tc_action\n");
> -		return err;
> -	}
> -
>   	skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
>   	if (!skb) {
>   		pr_debug("tca_action_flush: failed skb alloc\n");
> -		kfree(a);
>   		return err;
>   	}
>
> @@ -786,8 +779,10 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
>
>   	err = -EINVAL;
>   	kind = tb[TCA_ACT_KIND];
> -	a->ops = tc_lookup_action(kind);
> -	if (a->ops == NULL) /*some idjot trying to flush unknown action */
> +	memset(&a, 0, sizeof(struct tc_action));
> +	INIT_LIST_HEAD(&a.list);
> +	a.ops = tc_lookup_action(kind);
> +	if (a.ops == NULL) /*some idjot trying to flush unknown action */
>   		goto err_out;
>
>   	nlh = nlmsg_put(skb, portid, n->nlmsg_seq, RTM_DELACTION, sizeof(*t), 0);
> @@ -802,7 +797,7 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
>   	if (nest == NULL)
>   		goto out_module_put;
>
> -	err = a->ops->walk(skb, &dcb, RTM_DELACTION, a);
> +	err = a.ops->walk(skb, &dcb, RTM_DELACTION, &a);
>   	if (err < 0)
>   		goto out_module_put;
>   	if (err == 0)
> @@ -812,8 +807,7 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
>
>   	nlh->nlmsg_len = skb_tail_pointer(skb) - b;
>   	nlh->nlmsg_flags |= NLM_F_ROOT;
> -	module_put(a->ops->owner);
> -	kfree(a);
> +	module_put(a.ops->owner);
>   	err = rtnetlink_send(skb, net, portid, RTNLGRP_TC,
>   			     n->nlmsg_flags & NLM_F_ECHO);
>   	if (err > 0)
> @@ -822,11 +816,10 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
>   	return err;
>
>   out_module_put:
> -	module_put(a->ops->owner);
> +	module_put(a.ops->owner);
>   err_out:
>   noflush_out:
>   	kfree_skb(skb);
> -	kfree(a);
>   	return err;
>   }
>
>

Re: [Patch net-next v3 0/5] net_sched: act: more cleanup and improvement

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Tue, 11 Feb 2014 17:07:30 -0800

> v2 -> v3:
> * fix a mis-splitted patch
> * keep hinfo as a pointer in ops
> 
> v1 -> v2:
> * Fix a bug noticed by Jamal
> * Drop patches already merged into net-next
> * Add patch 5/5
> 
> Patches are cleanup's for the structures of tc actions, except patch 4
> which is an improvement.
> 
> See each patch for details.
> 
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Series applied, thanks.