From owner-p4-projects@FreeBSD.ORG Thu Jul 13 20:50:16 2006 Return-Path: X-Original-To: p4-projects@freebsd.org Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id 0DCCF16A4E8; Thu, 13 Jul 2006 20:50:16 +0000 (UTC) X-Original-To: perforce@freebsd.org Delivered-To: perforce@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id C294E16A4E6 for ; Thu, 13 Jul 2006 20:50:15 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from repoman.freebsd.org (repoman.freebsd.org [216.136.204.115]) by mx1.FreeBSD.org (Postfix) with ESMTP id 37A8B43D46 for ; Thu, 13 Jul 2006 20:50:13 +0000 (GMT) (envelope-from jhb@freebsd.org) Received: from repoman.freebsd.org (localhost [127.0.0.1]) by repoman.freebsd.org (8.13.6/8.13.6) with ESMTP id k6DKoDTl076084 for ; Thu, 13 Jul 2006 20:50:13 GMT (envelope-from jhb@freebsd.org) Received: (from perforce@localhost) by repoman.freebsd.org (8.13.6/8.13.4/Submit) id k6DKoCaP076081 for perforce@freebsd.org; Thu, 13 Jul 2006 20:50:12 GMT (envelope-from jhb@freebsd.org) Date: Thu, 13 Jul 2006 20:50:12 GMT Message-Id: <200607132050.k6DKoCaP076081@repoman.freebsd.org> X-Authentication-Warning: repoman.freebsd.org: perforce set sender to jhb@freebsd.org using -f From: John Baldwin To: Perforce Change Reviews Cc: Subject: PERFORCE change 101488 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 13 Jul 2006 20:50:16 -0000 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()