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>