Re: [Patch net 2/2] net_sched: hold netns refcnt for each action

[Cong Wang]

On Wed, Nov 1, 2017 at 10:23 AM, Cong Wang  wrote:
> TC actions have been destroyed asynchronously for a long time,
> previously in a RCU callback and now in a workqueue. If we
> don't hold a refcnt for its netns, we could use the per netns
> data structure, struct tcf_idrinfo, after it has been freed by
> netns workqueue.
>
> Hold refcnt to ensure netns destroy happens after all actions
> are gone.

This in fact is wrong. If we hold that refcnt, the netns can never
be destroyed until all actions are destroyed by user, this breaks
our netns design.

I am going to send a revert and a right way to fix it. It is more
complicated that I thought due to all of these flying RCU callbacks
and workqueue again, sigh...

Sorry about it.

[Patch net 2/2] net_sched: hold netns refcnt for each action

[Cong Wang]

TC actions have been destroyed asynchronously for a long time,
previously in a RCU callback and now in a workqueue. If we
don't hold a refcnt for its netns, we could use the per netns
data structure, struct tcf_idrinfo, after it has been freed by
netns workqueue.

Hold refcnt to ensure netns destroy happens after all actions
are gone.

Fixes: ddf97ccdd7cb ("net_sched: add network namespace support for tc actions")
Reported-by: Lucas Bates 
Tested-by: Lucas Bates 
Cc: Jamal Hadi Salim 
Cc: Jiri Pirko 
Signed-off-by: Cong Wang 
---
 include/net/act_api.h  | 4 +++-
 net/sched/act_api.c| 2 ++
 net/sched/act_bpf.c| 2 +-
 net/sched/act_connmark.c   | 2 +-
 net/sched/act_csum.c   | 2 +-
 net/sched/act_gact.c   | 2 +-
 net/sched/act_ife.c| 2 +-
 net/sched/act_ipt.c| 4 ++--
 net/sched/act_mirred.c | 2 +-
 net/sched/act_nat.c| 2 +-
 net/sched/act_pedit.c  | 2 +-
 net/sched/act_police.c | 2 +-
 net/sched/act_sample.c | 2 +-
 net/sched/act_simple.c | 2 +-
 net/sched/act_skbedit.c| 2 +-
 net/sched/act_skbmod.c | 2 +-
 net/sched/act_tunnel_key.c | 2 +-
 net/sched/act_vlan.c   | 2 +-
 18 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 5072446d5f06..c68551255032 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -13,6 +13,7 @@
 struct tcf_idrinfo {
spinlock_t  lock;
struct idr  action_idr;
+   struct net  *net;
 };
 
 struct tc_action_ops;
@@ -104,7 +105,7 @@ struct tc_action_net {
 
 static inline
 int tc_action_net_init(struct tc_action_net *tn,
-  const struct tc_action_ops *ops)
+  const struct tc_action_ops *ops, struct net *net)
 {
int err = 0;
 
@@ -112,6 +113,7 @@ int tc_action_net_init(struct tc_action_net *tn,
if (!tn->idrinfo)
return -ENOMEM;
tn->ops = ops;
+   tn->idrinfo->net = net;
spin_lock_init(>idrinfo->lock);
idr_init(>idrinfo->action_idr);
return err;
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 8f2c63514956..ca2ff0b3123f 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -78,6 +78,7 @@ static void tcf_idr_remove(struct tcf_idrinfo *idrinfo, 
struct tc_action *p)
spin_lock_bh(>lock);
idr_remove_ext(>action_idr, p->tcfa_index);
spin_unlock_bh(>lock);
+   put_net(idrinfo->net);
gen_kill_estimator(>tcfa_rate_est);
free_tcf(p);
 }
@@ -336,6 +337,7 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, 
struct nlattr *est,
p->idrinfo = idrinfo;
p->ops = ops;
INIT_LIST_HEAD(>list);
+   get_net(idrinfo->net);
*a = p;
return 0;
 }
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index c0c707eb2c96..9bce8cc84cbb 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -398,7 +398,7 @@ static __net_init int bpf_init_net(struct net *net)
 {
struct tc_action_net *tn = net_generic(net, bpf_net_id);
 
-   return tc_action_net_init(tn, _bpf_ops);
+   return tc_action_net_init(tn, _bpf_ops, net);
 }
 
 static void __net_exit bpf_exit_net(struct net *net)
diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
index 10b7a8855a6c..34e52d01a5dd 100644
--- a/net/sched/act_connmark.c
+++ b/net/sched/act_connmark.c
@@ -206,7 +206,7 @@ static __net_init int connmark_init_net(struct net *net)
 {
struct tc_action_net *tn = net_generic(net, connmark_net_id);
 
-   return tc_action_net_init(tn, _connmark_ops);
+   return tc_action_net_init(tn, _connmark_ops, net);
 }
 
 static void __net_exit connmark_exit_net(struct net *net)
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 1c40caadcff9..35171df2ebef 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -626,7 +626,7 @@ static __net_init int csum_init_net(struct net *net)
 {
struct tc_action_net *tn = net_generic(net, csum_net_id);
 
-   return tc_action_net_init(tn, _csum_ops);
+   return tc_action_net_init(tn, _csum_ops, net);
 }
 
 static void __net_exit csum_exit_net(struct net *net)
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index e29a48ef7fc3..ef7f7f39d26d 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -232,7 +232,7 @@ static __net_init int gact_init_net(struct net *net)
 {
struct tc_action_net *tn = net_generic(net, gact_net_id);
 
-   return tc_action_net_init(tn, _gact_ops);
+   return tc_action_net_init(tn, _gact_ops, net);
 }
 
 static void __net_exit gact_exit_net(struct net *net)
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 8ccd35825b6b..f65e4b5058e0 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -818,7 +818,7 @@ static __net_init int ife_init_net(struct net *net)
 {