Date: Tue, 27 Jun 1995 03:16:58 +0800 (WST) From: Peter Wemm <peter@haywire.DIALix.COM> To: Terry Lambert <terry@cs.weber.edu> Cc: current@freebsd.org, sos@freebsd.org Subject: Re: bug in Linux^H^H^H^H^HDoom Emulator Message-ID: <Pine.SV4.3.91.950627024851.12512F-100000@haywire.DIALix.COM> In-Reply-To: <9506261812.AA28518@cs.weber.edu>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 26 Jun 1995, Terry Lambert wrote: > > vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv > DO NOT COMMIT THIS WITHOUT FURTHER CHANGES! > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Seconded! :-) > > *** linux_misc.c.dist Mon Jun 26 01:32:37 1995 > > --- linux_misc.c Mon Jun 26 22:38:41 1995 > > *************** > > *** 188,194 **** > > printf("Linux-emul(%d): uselib(%s)\n", p->p_pid, path); > > #endif > > > > ! NDINIT(&ni, LOOKUP, FOLLOW, UIO_SYSSPACE, path, p); > > if (error = namei(&ni)) > > return error; > > > > --- 188,194 ---- > > printf("Linux-emul(%d): uselib(%s)\n", p->p_pid, path); > > #endif > > > > ! NDINIT(&ni, LOOKUP, FOLLOW | LOCKLEAF, UIO_SYSSPACE, path, p); > > if (error = namei(&ni)) > > return error; > > > > *************** > > *** 215,220 **** > > --- 215,222 ---- > > > > if (error = VOP_OPEN(vnodep, FREAD, p->p_ucred, p)) > > return error; > > + > > + VOP_UNLOCK(vnodep); /* lock no longer needed */ > > > > error = vm_mmap(kernel_map, (vm_offset_t *)&a_out, 1024, > > VM_PROT_READ, VM_PROT_READ, 0, (caddr_t)vnodep, 0); > > > At the *absolute* least: > > if (error = VOP_OPEN(vnodep, FREAD, p->p_ucred, p)) { > VOP_UNLOCK(vnodep); /* lock no longer needed */ > return error; > } > > VOP_UNLOCK(vnodep); /* lock no longer needed */ > > Unfortunately, from context, I can't tell if there are other intervening > error returns or not. If so, they need the same change! You are, of course, correct.. This might be more appropriate: (sorry about the unidiff - but it shows the change much more clearly) As before, please don't commit this either until it's checked! --- linux_misc.c.dist Mon Jun 26 01:32:37 1995 +++ linux_misc.c Tue Jun 27 02:54:00 1995 @@ -188,7 +188,7 @@ printf("Linux-emul(%d): uselib(%s)\n", p->p_pid, path); #endif - NDINIT(&ni, LOOKUP, FOLLOW, UIO_SYSSPACE, path, p); + NDINIT(&ni, LOOKUP, FOLLOW | LOCKLEAF, UIO_SYSSPACE, path, p); if (error = namei(&ni)) return error; @@ -196,25 +196,39 @@ if (vnodep == NULL) return ENOEXEC; - if (vnodep->v_writecount) + if (vnodep->v_writecount) { + VOP_UNLOCK(vnodep); return ETXTBSY; + } - if (error = VOP_GETATTR(vnodep, &attr, p->p_ucred, p)) - return error; + if (error = VOP_GETATTR(vnodep, &attr, p->p_ucred, p)) { + VOP_UNLOCK(vnodep); + return error; + } if ((vnodep->v_mount->mnt_flag & MNT_NOEXEC) || ((attr.va_mode & 0111) == 0) - || (attr.va_type != VREG)) + || (attr.va_type != VREG)) { + VOP_UNLOCK(vnodep); return ENOEXEC; + } - if (attr.va_size == 0) + if (attr.va_size == 0) { + VOP_UNLOCK(vnodep); return ENOEXEC; + } - if (error = VOP_ACCESS(vnodep, VEXEC, p->p_ucred, p)) + if (error = VOP_ACCESS(vnodep, VEXEC, p->p_ucred, p)) { + VOP_UNLOCK(vnodep); return error; + } - if (error = VOP_OPEN(vnodep, FREAD, p->p_ucred, p)) + if (error = VOP_OPEN(vnodep, FREAD, p->p_ucred, p)) { + VOP_UNLOCK(vnodep); return error; + } + + VOP_UNLOCK(vnodep); /* lock no longer needed */ error = vm_mmap(kernel_map, (vm_offset_t *)&a_out, 1024, VM_PROT_READ, VM_PROT_READ, 0, (caddr_t)vnodep, 0); @@ -225,7 +239,7 @@ * Is it a Linux binary ? */ if (((a_out->a_magic >> 16) & 0xff) != 0x64) - return -1; + return ENOEXEC; /* * Set file/virtual offset based on a.out variant. @@ -240,7 +254,7 @@ file_offset = 0; break; default: - return (-1); + return ENOEXEC; } vnodep->v_flag |= VTEXT; Note that I've changed two "return -1"'s to "return ENOEXEC" - is that correct? The code appears to be designed to return an errno, and -1 doesn't appear right at first glance... Next question: should those be VOP_UNLOCK's (like in kern_exec.c) or vput() or vrele()? I think it probably needs a vput() instead of the VOP_UNLOCK() for all the cases before the error return, otherwise the reference counts are going to constantly increase. I suspect that after the VOP_UNLOCK (before the vm_mmap), all error returns probably need a vrele() first to clear the reference left by namei(). Is that about right? (the patch above does not do this - that's why it should not be committed). Comments? [Now, is this version going to eat my filesystem? Or just use up all my vnodes? :-)] Thanks Terry! (And a special thanks to Soren for letting the code out for public scrutiny like this!) -Peter
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.SV4.3.91.950627024851.12512F-100000>