bug in recent linux mmap changes ?

[Jung-uk Kim]

On Monday 11 February 2008 11:16 am, 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_mach
>dep.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;

Good catch!  Can you test the attached patch?

Thanks!

Jung-uk Kim
-------------- next part --------------
Index: sys/amd64/linux32/linux32_machdep.c
===================================================================
RCS file: /home/ncvs/src/sys/amd64/linux32/linux32_machdep.c,v
retrieving revision 1.45
diff -u -r1.45 linux32_machdep.c
--- sys/amd64/linux32/linux32_machdep.c	4 Jul 2007 23:06:43 -0000	1.45
+++ sys/amd64/linux32/linux32_machdep.c	11 Feb 2008 17:38:08 -0000
@@ -907,21 +907,22 @@
 			PROC_UNLOCK(p);
 		}
 
-		/* 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;
-
 		/*
-		 * This gives us a new BOS.  If we're using VM_STACK, then
-		 * mmap will just map the top SGROWSIZ bytes, and let
-		 * the stack grow down to the limit at BOS.  If we're
-		 * not using VM_STACK we map the full stack, since we
-		 * don't have a way to autogrow it.
+		 * This gives us our maximum stack size and a new BOS.
+		 * If we're using VM_STACK, then mmap will just map
+		 * the top SGROWSIZ bytes, and let the stack grow down
+		 * to the limit at BOS.  If we're not using VM_STACK
+		 * we map the full stack, since we don't have a way
+		 * to autogrow it.
 		 */
-		bsd_args.addr = (caddr_t)PTRIN(linux_args->addr) -
-		    bsd_args.len;
+		if (linux_args->len > STACK_SIZE - GUARD_SIZE) {
+			bsd_args.len = linux_args->len;
+			bsd_args.addr = (caddr_t)PTRIN(linux_args->addr);
+		} else {
+			bsd_args.len = STACK_SIZE - GUARD_SIZE;
+			bsd_args.addr = (caddr_t)PTRIN(linux_args->addr) +
+			    linux_args->len - bsd_args.len;
+		}
 	} else {
 		bsd_args.addr = (caddr_t)PTRIN(linux_args->addr);
 		bsd_args.len  = linux_args->len;
Index: sys/i386/linux/linux_machdep.c
===================================================================
RCS file: /home/ncvs/src/sys/i386/linux/linux_machdep.c,v
retrieving revision 1.79
diff -u -r1.79 linux_machdep.c
--- sys/i386/linux/linux_machdep.c	26 Nov 2007 11:06:19 -0000	1.79
+++ sys/i386/linux/linux_machdep.c	11 Feb 2008 17:38:08 -0000
@@ -758,21 +758,22 @@
 			PROC_UNLOCK(p);
 		}
 
-		/* 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;
-
-		/* 
-		 * This gives us a new BOS.  If we're using VM_STACK, then
-		 * mmap will just map the top SGROWSIZ bytes, and let
-		 * the stack grow down to the limit at BOS.  If we're
-		 * not using VM_STACK we map the full stack, since we
-		 * don't have a way to autogrow it.
+		/*
+		 * This gives us our maximum stack size and a new BOS.
+		 * If we're using VM_STACK, then mmap will just map
+		 * the top SGROWSIZ bytes, and let the stack grow down
+		 * to the limit at BOS.  If we're not using VM_STACK
+		 * we map the full stack, since we don't have a way
+		 * to autogrow it.
 		 */
-		bsd_args.addr = (caddr_t)PTRIN(linux_args->addr) -
-		    bsd_args.len;
+		if (linux_args->len > STACK_SIZE - GUARD_SIZE) {
+			bsd_args.len = linux_args->len;
+			bsd_args.addr = (caddr_t)PTRIN(linux_args->addr);
+		} else {
+			bsd_args.len = STACK_SIZE - GUARD_SIZE;
+			bsd_args.addr = (caddr_t)PTRIN(linux_args->addr) +
+			    linux_args->len - bsd_args.len;
+		}
 	} else {
 		bsd_args.addr = (caddr_t)PTRIN(linux_args->addr);
 		bsd_args.len  = linux_args->len;