Re: [PATCH 4/4] mm: variable length argument support

Peter Zijlstra
2007-06-06T06:02:21Z

On Tue, 2007-06-05 at 16:39 -0700, Andrew Morton wrote:

> > @@ -1620,6 +1600,34 @@ int expand_stack(struct vm_area_struct *
> > return error;
> > }
> >
> > +#ifdef CONFIG_STACK_GROWSUP
> > +int expand_stack(struct vm_area_struct *vma, unsigned long address)
> > +{
> > + return expand_upwards(vma, address);
> > +}
> > +
> > +struct vm_area_struct *
> > +find_extend_vma(struct mm_struct *mm, unsigned long addr)
> > +{
> > + struct vm_area_struct *vma, *prev;
> > +
> > + addr &= PAGE_MASK;
> > + vma = find_vma_prev(mm, addr, &prev);
> > + if (vma && (vma->vm_start <= addr))
> > + return vma;
> > + if (!prev || expand_stack(prev, addr))
> > + return NULL;
> > + if (prev->vm_flags & VM_LOCKED) {
> > + make_pages_present(addr, prev->vm_end);
> > + }
>
> unneeded braces
>
> We really should check and propagate the make_pages_present() return value
> when appropriate. It can fail under -ENOMEM, ulimit exceeded, overcommit,
> etc.

Right, this is not new ugliness, but indeed a good opportunity to clean
up.

> > + return prev;
> > +}
> > +#else
> > +int expand_stack(struct vm_area_struct *vma, unsigned long address)
> > +{
> > + return expand_downwards(vma, address);
> > +}
> > +
> > struct vm_area_struct *
> > find_extend_vma(struct mm_struct * mm, unsigned long addr)
> > {
> > Index: linux-2.6-2/mm/mprotect.c
> > ===================================================================
> > --- linux-2.6-2.orig/mm/mprotect.c 2007-06-05 16:23:16.000000000 +0200
> > +++ linux-2.6-2/mm/mprotect.c 2007-06-05 16:29:45.000000000 +0200
> > @@ -128,7 +128,7 @@ static void change_protection(struct vm_
> > flush_tlb_range(vma, start, end);
> > }
> >
> > -static int
> > +int
> > mprotect_fixup(struct vm_area_struct *vma, struct vm_area_struct **pprev,
> > unsigned long start, unsigned long end, unsigned long newflags)
> > {
> > Index: linux-2.6-2/arch/ia64/ia32/binfmt_elf32.c
> > ===================================================================
> > --- linux-2.6-2.orig/arch/ia64/ia32/binfmt_elf32.c 2007-06-05 16:23:16.000000000 +0200
> > +++ linux-2.6-2/arch/ia64/ia32/binfmt_elf32.c 2007-06-05 16:29:45.000000000 +0200
> > @@ -195,62 +195,23 @@ ia64_elf32_init (struct pt_regs *regs)
> > ia32_load_state(current);
> > }
> >
> > +#undef setup_arg_pages
>
> What's this for?

That file reads:

#define setup_arg_pages(bprm,tos,exec) ia32_setup_arg_pages(bprm,exec)

....

#include "../../../fs/binfmt_elf.c"

....

int
ia32_setup_arg_pages (struct linux_binprm *bprm, int executable_stack)
{
int ret;

ret = setup_arg_pages(bprm, IA32_STACK_TOP, executable_stack);
/---------------^
We really want to call the real setup_arg_pages() here, not ourselves please.

if (!ret) {
/*
* Can't do it in ia64_elf32_init(). Needs to be done before
* calls to elf32_map()
*/
current->thread.ppl = ia32_init_pp_list();
}

return ret;
}