linux-kernel - Re: [RFC] [PATCH v3 1/2] cgroups: read-write lock CLONE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Wed, 28 Jul 2010 11:41:07 -0400
From:	Ben Blum <bblum@...rew.cmu.edu>
To:	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
Cc:	Ben Blum <bblum@...rew.cmu.edu>, linux-kernel@...r.kernel.org,
	containers@...ts.linux-foundation.org, akpm@...ux-foundation.org,
	ebiederm@...ssion.com, lizf@...fujitsu.com, matthltc@...ibm.com,
	menage@...gle.com, oleg@...hat.com
Subject: Re: [RFC] [PATCH v3 1/2] cgroups: read-write lock CLONE_THREAD
 forking per threadgroup

On Wed, Jul 28, 2010 at 06:28:13PM +0900, KAMEZAWA Hiroyuki wrote:
> On Wed, 28 Jul 2010 04:29:53 -0400
> Ben Blum <bblum@...rew.cmu.edu> wrote:
> 
> > On Mon, Jun 28, 2010 at 11:43:59AM -0400, Ben Blum wrote:
> > > On Mon, Jun 28, 2010 at 04:10:31PM +0900, KAMEZAWA Hiroyuki wrote:
> > > > By the way, IMHO, hiding lock in cgroup_fork() and cgroup_post_fork() doesn't
> > > > seem good idea. How about a code like this ?
> > > > 
> > > >   read_lock_thread_clone(current);
> > > >   cgroup_fork();
> > > >  .....
> > > >   cgroup_post_fork();
> > > >   read_unlock_thrad_clone(current);
> > > > 
> > > > We may have chances to move these lock to better position if cgroup is
> > > > an only user.
> > > 
> > > I didn't do that out of a desire to change fork.c as little as possible,
> > > but that does look better than what I've got. Those two functions should
> > > be in fork.c under #ifdef CONFIG_CGROUPS.
> > 
> > I'm looking at this now and am not sure where the best place to put
> > these is:
> > 
> > 1) Don't make new functions, just put:
> > 
> >     #ifdef CONFIG_CGROUPS
> >         if (clone_flags & CLONE_THREAD)
> >             down/up_read(...);
> >     #endif
> > 
> > directly in copy_process() in fork.c. Simplest, but uglifies the code.
> > 
> > 2) Make static helper functions in fork.c. Good, but not consistent with
> > directly using the lock in write-side (attach_proc).
> > 
> > 3) Define inline functions under #ifdef CONFIG_CGROUPS in sched.h, just
> > under the declaration of the lock. Most robust, but I'm hesitant to add
> > unneeded stuff to such a popular header file.
> > 
> > Any opinions?
> > 
> 
> My point was simple. Because copy_process() is very important path,
> the new lock should be visible in copy_process() or kernek/fork.c.
> "If the lock is visible in copy_process(), the reader can notice it".
> 
> Then, I offer you 2 options.
> 
> rename cgroup_fork() and cgroup_post_fork() as
>        cgroup_fork_lock() and cgroup_post_fork_unlock() 
> 
> Now, the lock is visible and the change is minimum.
> 
> Or
>        add the definition of lock/unlock to cgroup.h and include it 
>        from kernel/fork.c
> 
> Thanks,
> -Kame

I don't like either of these. Renaming to cgroup_fork_lock not only
conveys the sense that a cgroup-specific lock is taken, but also hides
the real purpose of these functions, which is to manipulate cgroup
pointers. And it's not a cgroup-specific lock - only write-side is
*currently* used by cgroups - so it shouldn't go in cgroup.h.

-- Ben
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/