Date: Thu, 13 Jul 2006 20:50:12 GMT From: John Baldwin <jhb@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 101488 for review Message-ID: <200607132050.k6DKoCaP076081@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=101488 Change 101488 by jhb@jhb_mutex on 2006/07/13 20:50:08 - Add conditional VFS Giant locking to linux_uselib() and mark it MPSAFE. - Remove a Doesn't-Ever-Happen(tm) error case. - Hold the vnode lock longer to protect setting VV_TEXT and note that we don't clear it later on if we get an error. - Document where the error return values differ from exec_check_permissions() and add a competing XXX to rwatson's explaining that vn_open() would be a poor choice here. This code is much closer to exec_check_permissions(). - Audit the pathname lookup for the library. Affected files ... .. //depot/projects/smpng/sys/compat/linux/linux_misc.c#66 edit .. //depot/projects/smpng/sys/i386/linux/syscalls.master#35 edit .. //depot/projects/smpng/sys/notes#79 edit Differences ... ==== //depot/projects/smpng/sys/compat/linux/linux_misc.c#66 (text+ko) ==== @@ -229,7 +229,7 @@ unsigned long bss_size; char *library; int error; - int locked; + int locked, vfslocked; LCONVPATHEXIST(td, args->library, &library); @@ -239,32 +239,24 @@ #endif a_out = NULL; + vfslocked = 0; locked = 0; vp = NULL; - /* - * XXX: This code should make use of vn_open(), rather than doing - * all this stuff itself. - */ - NDINIT(&ni, LOOKUP, ISOPEN|FOLLOW|LOCKLEAF, UIO_SYSSPACE, library, td); + NDINIT(&ni, LOOKUP, ISOPEN | FOLLOW | LOCKLEAF | MPSAFE | AUDITVNODE1, + UIO_SYSSPACE, library, td); error = namei(&ni); LFREEPATH(library); if (error) goto cleanup; vp = ni.ni_vp; - /* - * XXX - This looks like a bogus check. A LOCKLEAF namei should not - * succeed without returning a vnode. - */ - if (vp == NULL) { - error = ENOEXEC; /* ?? */ - goto cleanup; - } + vfslocked = NDHASGIANT(&ni); NDFREE(&ni, NDF_ONLY_PNBUF); /* * From here on down, we have a locked vnode that must be unlocked. + * XXX: The code below largely duplicates exec_check_permissions(). */ locked++; @@ -281,6 +273,7 @@ if ((vp->v_mount->mnt_flag & MNT_NOEXEC) || ((attr.va_mode & 0111) == 0) || (attr.va_type != VREG)) { + /* EACCESS is what exec(2) returns. */ error = ENOEXEC; goto cleanup; } @@ -299,6 +292,8 @@ /* * XXX: This should use vn_open() so that it is properly authorized, * and to reduce code redundancy all over the place here. + * XXX: Not really, it duplicates far more of exec_check_permissions() + * than vn_open(). */ #ifdef MAC error = mac_check_vnode_open(td->td_ucred, vp, FREAD); @@ -312,12 +307,6 @@ /* Pull in executable header into kernel_map */ error = vm_mmap(kernel_map, (vm_offset_t *)&a_out, PAGE_SIZE, VM_PROT_READ, VM_PROT_READ, 0, OBJT_VNODE, vp, 0); - /* - * Lock no longer needed - */ - locked = 0; - VOP_UNLOCK(vp, 0, td); - if (error) goto cleanup; @@ -372,11 +361,21 @@ } PROC_UNLOCK(td->td_proc); - mp_fixme("Unlocked vflags access."); - /* prevent more writers */ + /* + * Prevent more writers. + * XXX: Note that if any of the VM operations fail below we don't + * clear this flag. + */ vp->v_vflag |= VV_TEXT; /* + * Lock no longer needed + */ + locked = 0; + VOP_UNLOCK(vp, 0, td); + VFS_UNLOCK_GIANT(vfslocked); + + /* * Check if file_offset page aligned. Currently we cannot handle * misalinged file offsets, and so we read in the entire image * (what a waste). @@ -453,8 +452,10 @@ cleanup: /* Unlock vnode if needed */ - if (locked) + if (locked) { VOP_UNLOCK(vp, 0, td); + VFS_UNLOCK_GIANT(vfslocked); + } /* Release the kernel mapping. */ if (a_out) ==== //depot/projects/smpng/sys/i386/linux/syscalls.master#35 (text+ko) ==== @@ -165,7 +165,7 @@ 84 AUE_LSTAT MSTD { int linux_lstat(char *path, struct ostat *up); } 85 AUE_READLINK MSTD { int linux_readlink(char *name, char *buf, \ l_int count); } -86 AUE_USELIB STD { int linux_uselib(char *library); } +86 AUE_USELIB MSTD { int linux_uselib(char *library); } 87 AUE_SWAPON MNOPROTO { int swapon(char *name); } 88 AUE_REBOOT MSTD { int linux_reboot(l_int magic1, \ l_int magic2, l_uint cmd, void *arg); } ==== //depot/projects/smpng/sys/notes#79 (text+ko) ==== @@ -90,8 +90,8 @@ - svr4_sys_waitsys() - svr4_sys_fchroot() - svr4_sys_resolvepath() - - linux - - linux_uselib() + + linux + + linux_uselib() - ibcs2 - ibcs2_mount() - ibcs2_umount()
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200607132050.k6DKoCaP076081>