Re: [regression v2.6.38] Re: [PATCH v2] brk: fix min_brk lower boundcomputation for COMPAT_BRK

[Jiri Kosina]

On Sat, 26 Mar 2011, Geert Uytterhoeven wrote:

> >> > From: Jiri Kosina <jkosina@xxxxxxx>
> >> > Subject: [PATCH] brk: fix min_brk lower bound computation for COMPAT_BRK
> >> >
> >> > Even if CONFIG_COMPAT_BRK is set in the kernel configuration, it can still
> >> > be overriden by randomize_va_space sysctl.
> >> >
> >> > If this is the case, the min_brk computation in sys_brk() implementation
> >> > is wrong, as it solely takes into account COMPAT_BRK setting, assuming
> >> > that brk start is not randomized. But that might not be the case if
> >> > randomize_va_space sysctl has been set to '2' at the time the binary has
> >> > been loaded from disk.
> >> >
> >> > In such case, the check has to be done in a same way as in
> >> > !CONFIG_COMPAT_BRK case.
> >> >
> >> > In addition to that, the check for the COMPAT_BRK case introduced back in
> >> > a5b4592c ("brk: make sys_brk() honor COMPAT_BRK when computing lower
> >> > bound") is slightly wrong -- the lower bound shouldn't be mm->end_code,
> >> > but mm->end_data instead, as that's where the legacy applications expect
> >> > brk section to start (i.e. immediately after last global variable).
> >> >
> >> > Signed-off-by: Jiri Kosina <jkosina@xxxxxxx>
> >> > ---
> >> > Âmm/mmap.c | Â 10 +++++++++-
> >> > Â1 files changed, 9 insertions(+), 1 deletions(-)
> >> >
> >> > diff --git a/mm/mmap.c b/mm/mmap.c
> >> > index 50a4aa0..ca2f164 100644
> >> > --- a/mm/mmap.c
> >> > +++ b/mm/mmap.c
> >> > @@ -253,7 +253,15 @@ SYSCALL_DEFINE1(brk, unsigned long, brk)
> >> > Â Â Â Âdown_write(&mm->mmap_sem);
> >> >
> >> > Â#ifdef CONFIG_COMPAT_BRK
> >> > - Â Â Â min_brk = mm->end_code;
> >> > + Â Â Â /*
> >> > + Â Â Â Â* CONFIG_COMPAT_BRK can still be overridden by setting
> >> > + Â Â Â Â* randomize_va_space to 2, which will still make mm->start_brk
> >> > + Â Â Â Â* to be arbitrarily shifted
> >> > + Â Â Â Â*/
> >> > + Â Â Â if (mm->start_brk > PAGE_ALIGN(mm->end_data))
> >> > + Â Â Â Â Â Â Â min_brk = mm->start_brk;
> >> > + Â Â Â else
> >> > + Â Â Â Â Â Â Â min_brk = mm->end_data;
> >> > Â#else
> >> > Â Â Â Âmin_brk = mm->start_brk;
> >> > Â#endif
> >> > --
> >> > 1.7.3.1
> >>
> >> Sorry for chiming in this late, but I've just bisected a problem in
> >> 2.6.38 to commit
> >> 5520e89485252c759ee60d313e9422447659947b ("brk: fix min_brk lower bound
> >> computation for COMPAT_BRK").
> >>
> >> When booting my very old test ramdisk on Amiga/m68k, it fails like this:
> >>
> >> | RAMDISK: gzip image found at block 0
> >> | VFS: Mounted root (ext2 filesystem) readonly on device 1:0.
> >> | warning: process `update' used the obsolete bdflush system call
> >> | Fix your initscripts?
> >> | init: cannot open inittab
> >> | Kernel panic - not syncing: Attempted to kill init!
> >>
> >> Sorry for not noticing earlier, I usually boot full Debians under ARAnyM,
> >> instead of booting old ramdisks with libc5-based binaries that once were
> >> considered new.
> >
> > Oh well, one has to love the libc5-based binaries indeed.
> 
> Yeah, binaries from 1996 ;-)
> 
> > Is the patch below fixing the issue you are seeing on your Amiga/m68k?
> > Thanks.
> >
> >
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 2ec8eb5..0a02531 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -262,7 +262,7 @@ SYSCALL_DEFINE1(brk, unsigned long, brk)
> > Â Â Â Âif (mm->start_brk > PAGE_ALIGN(mm->end_data))
> > Â Â Â Â Â Â Â Âmin_brk = mm->start_brk;
> > Â Â Â Âelse
> > - Â Â Â Â Â Â Â min_brk = mm->end_data;
> > + Â Â Â Â Â Â Â min_brk = mm->end_code;
> > Â#else
> > Â Â Â Âmin_brk = mm->start_brk;
> > Â#endif
> 
> Unfortunately not...
> 
> I added some printk()s:
> 
> mm->start_brk = 0x8000a000, PAGE_ALIGN(mm->end_data = 0x8000a000)
> mm->start_brk = 0x8000a000, PAGE_ALIGN(mm->end_data = 0x8000a000)
> mm->start_brk = 0x8000a000, PAGE_ALIGN(mm->end_data = 0x8000a000)
> mm->start_brk = 0x8000a000, PAGE_ALIGN(mm->end_data = 0x8000a000)
> mm->start_brk = 0x80006000, PAGE_ALIGN(mm->end_data = 0x80004000)
> 
> I.e. just before the failure, "mm->start_brk > PAGE_ALIGN(mm->end_data)"
> became true.

Actually I believe that this is a different binary being executed -- the 
first 4 lines correspond to some other binary being loaded (which works 
fine).

The whole point of 

	if (mm->start_brk > PAGE_ALIGN(mm->end_data))

is to decide whether start_brk has been randomized even though COMPAT_BRK 
has been set (forcing it by randomize_va_space == 2).
But apparently even this doesn't work in your ancienc-libc5-static(?) 
library case, as start_brk is one page off anyway.

So I am a little bit clueless how to do the detection properly (we 
definitely can't account this one extra page as that will break other 
cases).
And I really wouldn't like to add MMF_BRK_RANDOMIZED flag to 
mm_struct->flags just for this very legacy case.

So any ideas are welcome.

The strace of the failing binary would still be helpful.

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/