Re: mlock/guard page/xen-tools & lenny
On Tue, Aug 31, 2010 at 11:58:32AM +0100, Ian Campbell wrote:
> On Tue, 2010-08-31 at 11:28 +0100, Ian Campbell wrote:
> > On Tue, 2010-08-31 at 11:00 +0100, Ian Campbell wrote:
> > > On Mon, 2010-08-30 at 22:57 -0600, dann frazier wrote:
> > > > On Mon, Aug 30, 2010 at 08:22:23AM -0600, dann frazier wrote:
> > > > > hey Ian,
> > > > > I haven't seen any reports of xen problems w/ the latest lenny DSA
> > > > > that added the guard page code. I looked at backporting the 3 patches
> > > > > from Linus - but the mlock patch touches code that didn't exist in
> > > > > .26, so I'm wondering if that patch is just not needed.
> > > >
> > > > fyi, I setup a Xen/lenny system and didn't have any problems w/
> > > > save/restore, so we're good afaict :)
> > >
> > > Thanks, I can't see code in the Lenny kernel equivalent to that which
> > > was broken either.
> > >
> > > I wrote a pretty skanky test program to test for the issue (mlock.c,
> > > attached, run with "./mlock lock" to trigger the issue). I don't have a
> > > Lenny system to hand right not where I can run it but if you still have
> > > a Lenny system setup then it might be interesting to try. (the test
> > > program doesn't actually require Xen to demonstrate the issue so any
> > > Lenny system should do).
> >
> > I installed up a Lenny VM and ran the test case there and it did
> > fail :-(.
Thanks for the test case, that's very helpful.
I can reproduce. 2.6.26-24 does not fail, 2.6.26-24lenny1 does.
> > However, unless we come across a report of an actual failure with the
> > real toolstack I don't think it is worth worrying about.
>
> FWIW I think the below is the moral equivalent of:
> commit 0e8e50e20c837eeec8323bba7dcd25fe5479194c
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Fri Aug 20 16:49:40 2010 -0700
>
> mm: make stack guard page logic use vm_prev pointer
I've actually already included a backport for this in 2.6.26-25 (in
p-u), and I've verified that your test case does not fail:
dannf@dl380g5:~$ ./mlock lock
pad1 at 0xffc15380-0xffc1737f
LCK at 0xffc17380-0xffc17384
pad2 at 0xffc17384-0xffc19383
esp: 0xffc15360
locking 0xffc17380-0xffc17384 -> 0xffc17000-0xffc18000 -> 0
minor faults: 157 -> 157
major faults: 0 -> 0
So looks like we're ok?
-dann
> Like the mlock() change previously, this makes the stack guard check
> code use vma->vm_prev to see what the mapping below the current stack
> is, rather than have to look it up with find_vma().
>
> Also, accept an abutting stack segment, since that happens naturally if
> you split the stack with mlock or mprotect.
>
> Tested-by: Ian Campbell <ijc@hellion.org.uk>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
>
> without the reliance on vm_prev (IOW it implements only the "Also, ...
> bit". I'm just building a test kernel to check it now.
>
> $ cat debian/patches/bugfix/all/mm-stack-guard-accept-abutting-stack-segment.patch
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -2287,11 +2287,18 @@
> {
> address &= PAGE_MASK;
> if ((vma->vm_flags & VM_GROWSDOWN) && address == vma->vm_start) {
> - address -= PAGE_SIZE;
> - if (find_vma(vma->vm_mm, address) != vma)
> - return -ENOMEM;
> + struct vm_area_struct *prev = find_vma(vma->vm_mm, address - PAGE_SIZE);
>
> - expand_stack(vma, address);
> + /*
> + * Is there a mapping abutting this one below?
> + *
> + * That's only ok if it's the same stack mapping
> + * that has gotten split..
> + */
> + if (prev && prev->vm_end == address)
> + return prev->vm_flags & VM_GROWSDOWN ? 0 : -ENOMEM;
> +
> + expand_stack(vma, address - PAGE_SIZE);
> }
> return 0;
> }
>
--
dann frazier
Reply to: