Re: [PATCH] - support inheritance of mlocks across fork/exec V2

Re: [PATCH] - support inheritance of mlocks across fork/exec V2

[Lee Schermerhorn]

Sorry for the resend.  I had the 'linux-api' address wrong on my reponse
to Andrew.  Want to include the api list in this exchange.

On Sat, 2008-12-06 at 22:07 -0800, Andrew Morton wrote:
> (cc linux-abi.  Again.)
> 
> On Wed, 03 Dec 2008 14:04:29 -0500 Lee Schermerhorn <Lee.Schermerhorn-VXdhtT5mjnY@public.gmane.org> wrote:
> 
> > 
> > 
> > Against;  2.6.28-rc7-mmotm-081203
> > 
> > V02:	rework vetting of flags argument as suggested by
> > 	Kosaki Motohiro.
> > 	enhance description as requested by Andrew Morton.
> > 
> > Add support for mlockall(MCL_INHERIT|MCL_RECURSIVE):
> > 
> > 	MCL_CURRENT[|MCL_FUTURE]|MCL_INHERIT - inherit memory locks
> > 		[vmas' VM_LOCKED flags] across fork(), and inherit
> > 		MCL_FUTURE behavior [mm's def_flags] across fork()
> > 		and exec().  Behaves as if child and/or new task
> > 		called mlockall(MCL_CURRENT|MCL_FUTURE) as first
> > 		instruction.
> > 
> > 	MCL_RECURSIVE - inherit MCL_CURRENT|MCL_FUTURE|MCL_INHERIT
> > 		[vmas' VM_LOCKED flags for fork() and mm's def_flags
> > 		and mcl_inherit across fork() and exec()] for all
> > 		future generations of calling process's descendants.
> > 		Behaves as if child and/or new task called
> > 		mlockall(MCL_CURRENT|MCL_FUTURE|MCL_INHERIT|MCL_RECURSIVE)
> > 		as the first instruction.
> > 
> > In support of a "lock prefix command"--e.g., mlock <cmd> <args> ...
> > Analogous to taskset(1) for cpu affinity or numactl(8) for numa memory
> > policy.
> > 
> > Together with patches to keep mlocked pages off the LRU, this will
> > allow users/admins to lock down applications without modifying them,
> > if their RLIMIT_MEMLOCK is sufficiently large, keeping their pages
> > off the LRU and out of consideration for reclaim.
> > 
> > Potentially useful, as well, in real-time environments to force
> > prefaulting and residency for applications that don't mlock themselves.
> > 
> > Jeff Sharkey at Montana State developed a similar patch for Linux
> > [link no longer accessible], but apparently he never submitted the patch.
> > 
> > I submitted an earlier version of this patch around a year ago.  I
> > resurrected it to test the unevictable lru/mlocked pages patches--
> > e.g., by "mlock -r make -j<N*nr_cpus> all".  This did shake out a few
> > races and vmstat accounting bugs, but NOT something I'd recommend as
> > general practice--for kernel builds, that is.
> > 
> > ----
> > 
> > Define MCL_INHERIT, MCL_RECURSIVE in <asm-*/mman.h>.
> > + x86 and  ia64 versions included.
> > + other arch can/will be created, if this patch deemed merge-worthy.
> > 
> > Similarly, I'll provide kernel man page update if/when needed.
> > 
> > Example "lock prefix command" in Documentation/vm/mlock.c
> > 
> >
> > ...
> >
> > --- linux-2.6.28-rc7-mmotm-081203.orig/mm/mlock.c	2008-12-03 10:33:11.000000000 -0500
> > +++ linux-2.6.28-rc7-mmotm-081203/mm/mlock.c	2008-12-03 10:33:29.000000000 -0500
> > @@ -573,15 +573,18 @@ asmlinkage long sys_munlock(unsigned lon
> >  static int do_mlockall(int flags)
> >  {
> >  	struct vm_area_struct * vma, * prev = NULL;
> > +	struct mm_struct *mm = current->mm;
> >  	unsigned int def_flags = 0;
> >  
> >  	if (flags & MCL_FUTURE)
> > -		def_flags = VM_LOCKED;
> > -	current->mm->def_flags = def_flags;
> > -	if (flags == MCL_FUTURE)
> > +		def_flags = VM_LOCKED;;
> 
> You get paid by the semicolon?

Nope.  Interestingly, ckeckpatch was fine with this.  [It did warn about
>80 char line in x86 mman.h, but there was already a much longer line,
so I left it.  Fixed now so that patch is squeaky clean, as far as
checkpatch is concerned.]

> 
> > +	mm->def_flags = def_flags;
> > +	if (flags & MCL_INHERIT)
> > +		mm->mcl_inherit = flags & (MCL_INHERIT | MCL_RECURSIVE);
> 
> aaaaaaarrrrrrggggggghhhhhhh!
> 
> So mm->mcl_inherit is _not_ a boolean meaning "this mm has MCL_INHERIT
> set", as I naively believed when I saw it.  Is it in fact a composite
> of both MCL_INHERIT and MCL_RECURSIVE, given a badly misleading name
> and a waffly comment.
> 
> Can we please make this clearer?

How about "mcl_inherit_mlocks_possibly_recursively" ?

Seriously, it made/makes sense to me.  If mcl_inherit is non-zero, it
WILL contain MCL_INHERIT, meaning that existing mlocks and any VM_LOCKED
in def_flags will be inherited across the next fork or exec [def_flags
only].  It may also contain MCL_RECURSIVE--tested separately--meaning
that the contents of mcl_inherit, itself, will also be inherited across
the next and future forks/execs, until/unless a descendant invokes
mlockall() w/o MCL_INHERIT or MCL_RECURSIVE to clear out mcl_inherit.

This seemed a bit much to try to explain on line where mcl_inherit is
declared in mm_types.  More appropriate to a man page, I would have
thought.   How about a Documentation/vm doc?

Meanwhile, perhaps:  "mcl_inherit" -> "mcl_inherit_flags" or "mcl_flags"
or "mlock_flags", or such?

> 
> >
> > ...
> >
> > --- linux-2.6.28-rc7-mmotm-081203.orig/kernel/fork.c	2008-12-03 10:18:15.000000000 -0500
> > +++ linux-2.6.28-rc7-mmotm-081203/kernel/fork.c	2008-12-03 10:33:29.000000000 -0500
> > @@ -278,7 +278,8 @@ static int dup_mmap(struct mm_struct *mm
> >  	 */
> >  	down_write_nested(&mm->mmap_sem, SINGLE_DEPTH_NESTING);
> >  
> > -	mm->locked_vm = 0;
> > +	if (!mm->mcl_inherit)
> > +		mm->locked_vm = 0;
> >  	mm->mmap = NULL;
> >  	mm->mmap_cache = NULL;
> >  	mm->free_area_cache = oldmm->mmap_base;
> > @@ -316,7 +317,8 @@ static int dup_mmap(struct mm_struct *mm
> >  		if (IS_ERR(pol))
> >  			goto fail_nomem_policy;
> >  		vma_set_policy(tmp, pol);
> > -		tmp->vm_flags &= ~VM_LOCKED;
> > +		if (!mm->mcl_inherit)
> > +			tmp->vm_flags &= ~VM_LOCKED;
> >  		tmp->vm_mm = mm;
> >  		tmp->vm_next = NULL;
> >  		anon_vma_link(tmp);
> > @@ -406,6 +408,8 @@ __cacheline_aligned_in_smp DEFINE_SPINLO
> >  
> >  static struct mm_struct * mm_init(struct mm_struct * mm, struct task_struct *p)
> >  {
> > +	unsigned long def_flags = 0;
> > +
> >  	atomic_set(&mm->mm_users, 1);
> >  	atomic_set(&mm->mm_count, 1);
> >  	init_rwsem(&mm->mmap_sem);
> > @@ -422,9 +426,14 @@ static struct mm_struct * mm_init(struct
> >  	mm->free_area_cache = TASK_UNMAPPED_BASE;
> >  	mm->cached_hole_size = ~0UL;
> >  	mm_init_owner(mm, p);
> > +	if (current->mm && current->mm->mcl_inherit) {
> > +		def_flags = current->mm->def_flags & VM_LOCKED;
> > +		if (mm->mcl_inherit & MCL_RECURSIVE)
> > +			mm->mcl_inherit  = current->mm->mcl_inherit;
> > +	}
> 
> hm.  That code looks familiar.  Not worth a helper function?

If we could remove the copy in load_elf_binary(), point would be moot.

More below.

> 
> >  	if (likely(!mm_alloc_pgd(mm))) {
> > -		mm->def_flags = 0;
> > +		mm->def_flags = def_flags;
> >  		mmu_notifier_mm_init(mm);
> >  		return mm;
> >  	}
> > Index: linux-2.6.28-rc7-mmotm-081203/fs/binfmt_elf.c
> > ===================================================================
> > --- linux-2.6.28-rc7-mmotm-081203.orig/fs/binfmt_elf.c	2008-12-03 10:19:21.000000000 -0500
> > +++ linux-2.6.28-rc7-mmotm-081203/fs/binfmt_elf.c	2008-12-03 10:33:29.000000000 -0500
> > @@ -585,6 +585,7 @@ static int load_elf_binary(struct linux_
> >  	unsigned long reloc_func_desc = 0;
> >  	int executable_stack = EXSTACK_DEFAULT;
> >  	unsigned long def_flags = 0;
> > +	int mcl_inherit = 0;
> >  	struct {
> >  		struct elfhdr elf_ex;
> >  		struct elfhdr interp_elf_ex;
> > @@ -749,6 +750,13 @@ static int load_elf_binary(struct linux_
> >  		SET_PERSONALITY(loc->elf_ex);
> >  	}
> >  
> > +	/* Optionally inherit MCL_FUTURE state before destroying old mm */
> > +	if (current->mm && current->mm->mcl_inherit) {
> 
> umm, OK, it looks like current->mm can be null here.

Probably not here.  IIRC, it can be NULL [for init] up in mm_init() and
I probably just cut and pasted, instead of a helper function...

> 
> > +		def_flags = current->mm->def_flags & VM_LOCKED;
> 
> ooh, we finally used local variable def_flags for something.  That's
> been wasting space since 2.2.26 (or earlier).

def_flags was being used to clear out the current->mm->def_flags.  Don't
know why, tho'.  See below...
> 
> > +		if (current->mm->mcl_inherit & MCL_RECURSIVE)
> > +			mcl_inherit  = current->mm->mcl_inherit;
> > +	}
> > +
> >  	/* Flush all traces of the currently running executable */
> >  	retval = flush_old_exec(bprm);
> >  	if (retval)
> > @@ -757,6 +765,7 @@ static int load_elf_binary(struct linux_
> >  	/* OK, This is the point of no return */
> >  	current->flags &= ~PF_FORKNOEXEC;
> >  	current->mm->def_flags = def_flags;
> > +	current->mm->mcl_inherit = mcl_inherit;
> >  
> >  	/* Do this immediately, since STACK_TOP as used in setup_arg_pages
> >  	   may depend on the personality.  */
> 
> Why is this done in binfmt_elf.c?  Doesn't this preclude the operation
> of these new flags for other binary formats?

Because binfmt_elf.c is the only binary format loader [that I can see]
that takes it upon itself to reinitialize mm->def_flags, which has
already been set up in mm_init().  I wondered, myself, why this is the
case, but I didn't dare just remove it, lest it reintroduce some obscure
bug.  So, I duplicated my version of def_flags [and mcl_inherit]
initialization here. 

Now that you've raised the question:  why, indeed, does the elf format
loader reinit def_flags?  Can we just remove this?

> 
> > Index: linux-2.6.28-rc7-mmotm-081203/arch/x86/include/asm/mman.h
> > ===================================================================
> > --- linux-2.6.28-rc7-mmotm-081203.orig/arch/x86/include/asm/mman.h	2008-12-03 10:16:26.000000000 -0500
> > +++ linux-2.6.28-rc7-mmotm-081203/arch/x86/include/asm/mman.h	2008-12-03 10:33:29.000000000 -0500
> > @@ -16,5 +16,8 @@
> >  
> >  #define MCL_CURRENT	1		/* lock all current mappings */
> >  #define MCL_FUTURE	2		/* lock all future mappings */
> > +#define MCL_INHERIT	4		/* inherit mlocks across fork */
> > +					/* inherit '_FUTURE flag across fork/exec */
> > +#define MCL_RECURSIVE	8		/* inherit mlocks recursively */
> >  
> >  #endif /* _ASM_X86_MMAN_H */
> > Index: linux-2.6.28-rc7-mmotm-081203/include/linux/mm_types.h
> > ===================================================================
> > --- linux-2.6.28-rc7-mmotm-081203.orig/include/linux/mm_types.h	2008-12-03 10:18:01.000000000 -0500
> > +++ linux-2.6.28-rc7-mmotm-081203/include/linux/mm_types.h	2008-12-03 10:33:29.000000000 -0500
> > @@ -235,6 +235,8 @@ struct mm_struct {
> >  	unsigned int token_priority;
> >  	unsigned int last_interval;
> >  
> > +	int mcl_inherit;		/* inherit current/future locks */
> > +
> >  	unsigned long flags; /* Must use atomic bitops to access the bits */
> 
> I was thinking that the boolean mcl_inherit could become a bit in
> mm.flags.  That was before I unmisled myself about it.
> 
> Perhaps we could use two bits.  That might get a bit nasty.

I did look for someplace to add these flags, before deciding on a new
member.  The "Must use atomic bitops", and the fact that mm->flags seems
used soley for controlling core dumping [and defined in sched.h, no
less!], put me off from going that route.  Similarly, because def_flags
is an initializer for the vma's vm_flags, I didn't want to pollute that.
I figured the 'int' would fill a padding slot on 64-bit archs, but does
add to mm size for 32-bit archs.

Suggestions?
I'll hold off on a re-spin, pending resolution of some of these issues.

Thanks for the thorough review.

Lee


--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] - support inheritance of mlocks across fork/exec V2

[Matt Mackall]

On Mon, 2008-12-08 at 16:05 -0500, Lee Schermerhorn wrote:
> > > In support of a "lock prefix command"--e.g., mlock <cmd>
> <args> ...
> > > Analogous to taskset(1) for cpu affinity or numactl(8) for numa memory
> > > policy.
> > > 
> > > Together with patches to keep mlocked pages off the LRU, this will
> > > allow users/admins to lock down applications without modifying them,
> > > if their RLIMIT_MEMLOCK is sufficiently large, keeping their pages
> > > off the LRU and out of consideration for reclaim.
> > > 
> > > Potentially useful, as well, in real-time environments to force
> > > prefaulting and residency for applications that don't mlock themselves.

This is a bit scary to me. Privilege and mode inheritance across
processes is the root of many nasty surprises, security and otherwise. 

Here's a crazy alternative: add a flag to containers instead? I think
this is a better match to what you're trying to do and will keep people
from being surprised when an mlockall call in one thread causes a
fork/exec in another thread to crash their box, but only sometimes.

-- 
Mathematics is the supreme nostalgia of our time.

--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] - support inheritance of mlocks across fork/exec V2

[Lee Schermerhorn]

On Mon, 2008-12-08 at 15:33 -0600, Matt Mackall wrote:
> On Mon, 2008-12-08 at 16:05 -0500, Lee Schermerhorn wrote:
> > > > In support of a "lock prefix command"--e.g., mlock <cmd>
> > <args> ...
> > > > Analogous to taskset(1) for cpu affinity or numactl(8) for numa memory
> > > > policy.
> > > > 
> > > > Together with patches to keep mlocked pages off the LRU, this will
> > > > allow users/admins to lock down applications without modifying them,
> > > > if their RLIMIT_MEMLOCK is sufficiently large, keeping their pages
> > > > off the LRU and out of consideration for reclaim.
> > > > 
> > > > Potentially useful, as well, in real-time environments to force
> > > > prefaulting and residency for applications that don't mlock themselves.
> 
> This is a bit scary to me. Privilege and mode inheritance across
> processes is the root of many nasty surprises, security and otherwise.

Hi, Matt:

Could you explain more about this issue?  I believe that the patch
doesn't provide any privileges or capabilities that a process doesn't
already have.  It just allows one to cause a process and, optionally,
its descendants, to behave as if one had modified the sources to invoke
mlockall() directly.  It is still subject to each individual task's
resource limits.  At least, that was my intent.  

> 
> Here's a crazy alternative: add a flag to containers instead? I think
> this is a better match to what you're trying to do and will keep people
> from being surprised when an mlockall call in one thread causes a
> fork/exec in another thread to crash their box, but only sometimes.

Not so crazy.  It isn't really a better match, IMO [more below], but I
can see providing a flag to enable/disable this ability in the
container.  Initially, the default could be disabled.  If/when it's been
around long enough that people are comfortable with it, perhaps the
default could be flipped to enabled.

As far as "what I'm trying to do":  I see this as exactly like using
taskset to set the cpu affinity of a task, or numactl to set the task
mempolicy without modifying the source.  If one had access to the
program source and wouldn't, e.g., void a support contract by modifying
it, one could just insert the calls into the source itself.  These would
still be subject to, but "finer grain" than, external administrative
controls, such as cpuset allowed mems and cpus.

But, sometimes, it's very convenient to use the prefix commands to
effect the same behavior.  I could certainly use it in the Oracle,
SAP, ... benchmarking that I occasionally help out with.  Actually, as I
have the patches, I can create custom kernels for the purpose of
demonstrating the effects.  However, I think it would be useful for
customers someday to have this feature in distro kernels.

I don't understand your comment about threads and crashing the box. If
an inherited mlockall() can crash Linux, then inserting the call into
the source could also crash it.  I think that's a bug we'd need to fix
in any case.  Indeed, using this facility during testing of the
unevictable lru/mlocked pages work DID uncover bugs that I might not
have seen otherwise.   If you meant "cause OOMs", I agree that this
could happen, as well but, again, I think this would be the same
behavior as if the calls were inserted into the source.

Regarding one thread being surprised by what another thread did:  if you
mean threads within the same executable, I'd expect them to part of the
same program.  One hopes that someone knows the big picture--what all
the threads are doing--obviating any such surprises.  But, I recall that
you work/ed in the embedded space, and perhaps threads mean something
different to you.

Anyway...

This is a pretty small patch that I can maintain out of tree for my own
use in testing if people don't think its usefulness outweighs any
concerns it raises.  Customers won't be able to use it, but thems the
breaks.

Regards,
Lee
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] - support inheritance of mlocks across fork/exec V2

[Matt Mackall]

On Tue, 2008-12-09 at 14:40 -0500, Lee Schermerhorn wrote:
> On Mon, 2008-12-08 at 15:33 -0600, Matt Mackall wrote:
> > On Mon, 2008-12-08 at 16:05 -0500, Lee Schermerhorn wrote:
> > > > > In support of a "lock prefix command"--e.g., mlock <cmd>
> > > <args> ...
> > > > > Analogous to taskset(1) for cpu affinity or numactl(8) for numa memory
> > > > > policy.
> > > > > 
> > > > > Together with patches to keep mlocked pages off the LRU, this will
> > > > > allow users/admins to lock down applications without modifying them,
> > > > > if their RLIMIT_MEMLOCK is sufficiently large, keeping their pages
> > > > > off the LRU and out of consideration for reclaim.
> > > > > 
> > > > > Potentially useful, as well, in real-time environments to force
> > > > > prefaulting and residency for applications that don't mlock themselves.
> > 
> > This is a bit scary to me. Privilege and mode inheritance across
> > processes is the root of many nasty surprises, security and otherwise.
> 
> Hi, Matt:
> 
> Could you explain more about this issue?  I believe that the patch
> doesn't provide any privileges or capabilities that a process doesn't
> already have.  It just allows one to cause a process and, optionally,
> its descendants, to behave as if one had modified the sources to invoke
> mlockall() directly.  It is still subject to each individual task's
> resource limits.  At least, that was my intent.  

Again, it's about inheriting *something* across processes. This
historically creates surprises. When the thing being inherited is a
privilege, the surprises are security holes. I can't tell you what the
surprises are, only that I expect there to be some.

mlockall is not a privilege in itself, but it is a non-standard mode of
operation that most processes are not designed for or expecting, and it
does relate to the handling of a finite resource with system stability
implications. If the thing that turns on this mode is buried inside some
poorly written app that decides it needs to mlockall somewhere, the
'surprise' can occur far away - in the child of a child of a thread an
hour later. And the surprise can be fatal - all memory eaten up, because
our process just happened to run without an rlimit.

I've seen this with RT. An app temporarily elevates itself to RT, and
unrelatedly forks another process. The second short-lived process kicks
off a daemon that sometime later consumes all CPU in a busy loop (in
this case waiting on I/O that never happens because everything else is
shut out).

Doing it as a container parameter means that you explicitly recognize
that 'everything in the container' gets this mode. And you've probably
also given a thought to 'how big is this container' and the like as
well.

> As far as "what I'm trying to do":  I see this as exactly like using
> taskset to set the cpu affinity of a task, or numactl to set the task
> mempolicy without modifying the source.

Oh sure, I completely get that and I think it's a useful notion. And I
think the above analogous interfaces have more or less the same issues,
except that mlockall is a much older and more widely used API.
Containers are a better match here too, but the above predate
containers.

>   If one had access to the
> program source and wouldn't, e.g., void a support contract by modifying
> it, one could just insert the calls into the source itself.

Huh? We would of course set up a container 'from the outside'. By
comparison, your mlockall() call traditionally operates from 'from the
inside', and you're proposing to add a flag and a helper program that
makes it work 'from the outside' too. Which is a bit hackish.

-- 
Mathematics is the supreme nostalgia of our time.

--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html