From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Wagner Subject: Re: [PATCH v1 3/5] cgroup: Protect access to task_cls_classid() when built as module Date: Mon, 20 Aug 2012 13:29:38 +0200 Message-ID: <20120820112938.GA22415@candlejack.bmw-carit.intra> References: <1345215494-9181-1-git-send-email-wagi@monom.org> <1345215494-9181-4-git-send-email-wagi@monom.org> <20120817182855.GA11607@hmsreliant.think-freely.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Daniel Wagner , "David S. Miller" , Gao feng , Jamal Hadi Salim , John Fastabend , Li Zefan , Tejun Heo To: Neil Horman Return-path: Content-Disposition: inline In-Reply-To: <20120817182855.GA11607-B26myB8xz7F8NnZeBjwnZQMhkBWG/bsMQH7oEaQurus@public.gmane.org> Sender: cgroups-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: netdev.vger.kernel.org On Fri, Aug 17, 2012 at 02:28:55PM -0400, Neil Horman wrote: > On Fri, Aug 17, 2012 at 04:58:12PM +0200, Daniel Wagner wrote: > > From: Daniel Wagner > > > > The module version of task_cls_classid() checks if net_cls_sbusys_id > > is valid to indentify when it is okay to access the controller. > > > > Instead relying on the subusys_id to be set, make it explicit > > with a jump label. > > > > Signed-off-by: Daniel Wagner > > Cc: "David S. Miller" > > Cc: Gao feng > > Cc: Jamal Hadi Salim > > Cc: John Fastabend > > Cc: Li Zefan > > Cc: Neil Horman > > Cc: Tejun Heo > > Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > > Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > > --- > > include/net/cls_cgroup.h | 5 ++++- > > net/core/sock.c | 5 +++++ > > net/sched/cls_cgroup.c | 9 +++++++++ > > 3 files changed, 18 insertions(+), 1 deletion(-) > > > > diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h > > index 401672c..bbbd957 100644 > > --- a/include/net/cls_cgroup.h > > +++ b/include/net/cls_cgroup.h > > @@ -16,6 +16,7 @@ > > #include > > #include > > #include > > +#include > > > > #ifdef CONFIG_CGROUPS > > struct cgroup_cls_state > > @@ -44,6 +45,8 @@ static inline u32 task_cls_classid(struct task_struct *p) > > } > > > > #elif IS_MODULE(CONFIG_NET_CLS_CGROUP) > > +extern struct static_key cgroup_cls_enabled; > > +#define clscg_enabled static_key_false(&cgroup_cls_enabled) > > > > extern int net_cls_subsys_id; > > > > @@ -52,7 +55,7 @@ static inline u32 task_cls_classid(struct task_struct *p) > > int id; > > u32 classid = 0; > > > > - if (in_interrupt()) > > + if (!clscg_enabled || in_interrupt()) > > return 0; > > > > rcu_read_lock(); > > diff --git a/net/core/sock.c b/net/core/sock.c > > index 8f67ced..8106e77 100644 > > --- a/net/core/sock.c > > +++ b/net/core/sock.c > > @@ -327,6 +327,11 @@ int __sk_backlog_rcv(struct sock *sk, struct sk_buff *skb) > > EXPORT_SYMBOL(__sk_backlog_rcv); > > > > #if defined(CONFIG_CGROUPS) > > +#if IS_MODULE(CONFIG_NET_CLS_CGROUP) > > +struct static_key cgroup_cls_enabled = STATIC_KEY_INIT_FALSE; > > +EXPORT_SYMBOL_GPL(cgroup_cls_enabled); > > +#endif > > + > > #if !defined(CONFIG_NET_CLS_CGROUP) > > int net_cls_subsys_id = -1; > > EXPORT_SYMBOL_GPL(net_cls_subsys_id); > > diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c > > index 7743ea8..0635894 100644 > > --- a/net/sched/cls_cgroup.c > > +++ b/net/sched/cls_cgroup.c > > @@ -44,12 +44,21 @@ static struct cgroup_subsys_state *cgrp_create(struct cgroup *cgrp) > > > > if (cgrp->parent) > > cs->classid = cgrp_cls_state(cgrp->parent)->classid; > > +#if IS_MODULE(CONFIG_NET_CLS_CGROUP) > > + else if (!clscg_enabled) > > + static_key_slow_inc(&cgroup_cls_enabled); > This is racy I think. The read of the static key is atomic with other reads, > but the entire conditional is not atomic. If two cpus were creating cgroups in > parallel, it would be possible for both to read the static key as being zero > (the second cpu would read the key before the first cpu could increment it). D'oh, That is racy. > > +#endif > > > > return &cs->css; > > } > > > > static void cgrp_destroy(struct cgroup *cgrp) > > { > > +#if IS_MODULE(CONFIG_NET_CLS_CGROUP) > > + if (!cgrp->parent && clscg_enabled) > > + static_key_slow_dec(&cgroup_cls_enabled); > Ditto here with the race above. I think what you want is one of: > > 1) Use static_key_slow_[inc|dec] unconditionally While the static_key_slow_inc() case will work, I am not so sure about the static_key_slow_dec(), e.g. we could still access inside task_cls_classid() a destroyed container. > 2) Keep a separate internal counter to track the number of cgroup instances > so that you only inc the static key on the first create and dec it on the last > delete. If I got you right, than this would not be different then direclty using static_key_slow_[inc|dec]. > I would think (1) would be sufficent. It looks like static_key_slow_inc uses > atomic_inc_not_zero to just do an inc anyway in the event that multiple inc > events are made. Would something like this work? static inline u32 task_cls_classid(struct task_struct *p) { u32 classid; struct cgroup_cls_state *css; if (!clscg_enabled || in_interrupt()) return 0; rcu_read_lock(); css = container_of(task_subsys_state(p, net_cls_subsys_id), struct cgroup_cls_state, css); if (!css) classid = css->classid; else classid = 0; rcu_read_unlock(); return classid; } Daniel