bug in recent linux mmap changes ?

[Roman Divacky]

On Mon, Feb 11, 2008 at 06:16:21PM +0200, Andriy Gapon wrote:
> After upgrading two machines, one to 6.3 and the other to 7.0-RC1, I can
> not run linux heroes3 anymore (statically linked, threaded application
> originally written for kernel 2.2.X). On 6.3 the process "hangs", on
> 7.0-RC1 one of the threads/processes crashes with SIGSEGV.
> Everything was OK as recently as 6.2.
> 
> I can provide more diagnostics later, if needed, but in both cases I see
> that the last system call in a troublesome thread/process is linux_mmap().
> I did a brief search through recent linux_mmap changes and I think that
> there is a bug in the following commit (I am writing this hastiliy, so I
> haven't yet tested a possible fix):
> http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/i386/linux/linux_machdep.c.diff?r1=1.48.2.4;r2=1.48.2.5;f=h
> 
> Namely, old code:
> ====================================================
> /* This gives us TOS */
> bsd_args.addr = linux_args->addr + linux_args->len;
> 
> if (bsd_args.addr > p->p_vmspace->vm_maxsaddr) {
> [block folded]
> }
> 
> /* This gives us our maximum stack size */
> if (linux_args->len > STACK_SIZE - GUARD_SIZE)
>         bsd_args.len = linux_args->len;
> else
>         bsd_args.len  = STACK_SIZE - GUARD_SIZE;
> 
> /*
> [comment folded]
>  */
> bsd_args.addr -= bsd_args.len;
> ====================================================
> 
> New code:
> ====================================================
> if ((caddr_t)PTRIN(linux_args->addr) + linux_args->len >
>     p->p_vmspace->vm_maxsaddr) {
> [block folded]
> }
> 
> /* This gives us our maximum stack size */
> if (linux_args->len > STACK_SIZE - GUARD_SIZE)
>         bsd_args.len = linux_args->len;
> else
>         bsd_args.len  = STACK_SIZE - GUARD_SIZE;
> 
> /*
> [comment foled]
>  */
> bsd_args.addr = (caddr_t)PTRIN(linux_args->addr) -
>     bsd_args.len;
> ====================================================
> 
> Please now note that the new code doesn't have initial bsd_args.addr
> assignment line. So, in summary, old code does the following:
> bsd_args.addr = linux_args->addr + linux_args->len;
> ...
> bsd_args.addr -= bsd_args.len;
> 
> While new code does:
> bsd_args.addr = (caddr_t)PTRIN(linux_args->addr) - bsd_args.len;

I think you are right....  does changing

            bsd_args.addr = (caddr_t)PTRIN(linux_args->addr) -
                    bsd_args.len;

to

            bsd_args.addr = (caddr_t)PTRIN(linux_args->addr) +
                    linux_args->len - bsd_args.len;

fix that? or possibly just

            bsd_args.addr = (caddr_t)PTRIN(linux_args->addr);

can someone test this please? asap... it would be nice to squeeze this into 7.0R

roman