Re: namei() returns EISDIR for / (Re: svn commit: r203990 - head/lib/libc/sys)
On 2010-03-05, Jaakko Heinonen wrote: I have updated the patch taking some of bde's comments into account. The new version also includes updates for namei(9) manual page. Yet another update: http://people.freebsd.org/~jh/patches/lookup-root.4.diff I have committed the relookup() part as r205682. Unfortunately errno translation is needed also for open(/, O_CREAT) and undelete(/). -- Jaakko ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: namei() returns EISDIR for / (Re: svn commit: r203990 - head/lib/libc/sys)
Jaakko Heinonen schrieb am 2010-03-26: On 2010-03-05, Jaakko Heinonen wrote: I have updated the patch taking some of bde's comments into account. The new version also includes updates for namei(9) manual page. Yet another update: http://people.freebsd.org/~jh/patches/lookup-root.4.diff I have committed the relookup() part as r205682. Unfortunately errno translation is needed also for open(/, O_CREAT) and undelete(/). you might want to consider posting your patch on http://reviews.freebsdish.org. that way it'll be easier to review it. -- Alexander Best ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: namei() returns EISDIR for / (Re: svn commit: r203990 - head/lib/libc/sys)
On 2010-03-10, Alexander Best wrote: could this panic have been triggered by the patch? It doesn't look like it's caused by the patch. panic() at panic+0x15f _mtx_lock_flags() at _mtx_lock_flags+0xc5 fdesc_allocvp() at fdesc_allocvp+0xbf fdesc_lookup() at fdesc_lookup+0x15c this was 100% reducible when doing `portsnap fetch` though i changed a lot of stuff in my kernel config and reverted a lot of src patches to resolve the issue so i'm not sure what exactly was causing it. The panic happened in fdescfs code. Did you have local patches related to fdescfs? -- Jaakko ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: namei() returns EISDIR for / (Re: svn commit: r203990 - head/lib/libc/sys)
Jaakko Heinonen schrieb am 2010-03-11: On 2010-03-10, Alexander Best wrote: could this panic have been triggered by the patch? It doesn't look like it's caused by the patch. panic() at panic+0x15f _mtx_lock_flags() at _mtx_lock_flags+0xc5 fdesc_allocvp() at fdesc_allocvp+0xbf fdesc_lookup() at fdesc_lookup+0x15c this was 100% reducible when doing `portsnap fetch` though i changed a lot of stuff in my kernel config and reverted a lot of src patches to resolve the issue so i'm not sure what exactly was causing it. The panic happened in fdescfs code. Did you have local patches related to fdescfs? after reverting a few patches (including yours) i got rid of the problem. i then re-applied your patch and noticed that (as you said) it wasn't causing the panic. i don't have any fdescfs specific patches in my src. i suspect however [1] being responsible for the panic. after backing it out i got no more panics in connection with `portsnap fetch`. [1] http://www.mail-archive.com/freebsd-hackers@freebsd.org/msg70400.html thanks for the help oh...and btw.: i'm not sure but i think i've asked this question once before in this thread: in sys/kern/vfs_syscalls.c:kern_rmdirat() there's still local code to check for . and / after applying your patch. isn't this all being done by calling namei() now? cheers. alex ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: namei() returns EISDIR for / (Re: svn commit: r203990 - head/lib/libc/sys)
On 2010-03-11, Alexander Best wrote: in sys/kern/vfs_syscalls.c:kern_rmdirat() there's still local code to check for . and / after applying your patch. isn't this all being done by calling namei() now? Looking at it quickly I think that the . case is handled by lookup() since r199137. However the VV_ROOT test is for all mount points not just for /. -- Jaakko ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: namei() returns EISDIR for / (Re: svn commit: r203990 - head/lib/libc/sys)
Jaakko Heinonen schrieb am 2010-03-05: On 2010-02-28, Jaakko Heinonen wrote: http://people.freebsd.org/~jh/patches/lookup-root.diff I have updated the patch taking some of bde's comments into account. The new version also includes updates for namei(9) manual page. could this panic have been triggered by the patch? Fatal trap 12: page fault while in kernel mode cpuid = 0; apic id = 00 fault virtual address = 0x12 fault code = supervisor read data, page not present instruction pointer = 0x20:0x8048af0f stack pointer = 0x28:0xff8040495000 frame pointer = 0x28:0xff8040495020 code segment= base 0x0, limit 0xf, type 0x1b = DPL 0, pres 1, long 1, def32 0, gran 1 processor eflags= interrupt enabled, resume, IOPL = 0 current process = 1402 (sh) Tracing pid 1402 tid 100092 td 0xff000729d000 strlen() at strlen+0x1f kvprintf() at kvprintf+0x1027 vsnprintf() at vsnprintf+0x48 panic() at panic+0x15f _mtx_lock_flags() at _mtx_lock_flags+0xc5 fdesc_allocvp() at fdesc_allocvp+0xbf fdesc_lookup() at fdesc_lookup+0x15c VOP_LOOKUP_APV() at VOP_LOOKUP_APV+0x11c VOP_LOOKUP() at VOP_LOOKUP+0x45 lookup() at lookup+0x8cf namei() at namei+0x730 vn_open_cred() at vn_open_cred+0x114 vn_open() at vn_open+0x55 kern_openat() at kern_openat+0x178 kern_open() at kern_open+0x3e open() at open+0x26 syscall() at syscall+0x2a7 Xfast_syscall() at Xfast_syscall+0xe1 --- syscall (5, FreeBSD ELF64, open), rip = 0x8009a3bec, rsp = 0x7fffd3c8, rbp = 0x800e143b0 --- this was 100% reducible when doing `portsnap fetch` though i changed a lot of stuff in my kernel config and reverted a lot of src patches to resolve the issue so i'm not sure what exactly was causing it. cheers. alex The patch is attached to this mail and also found at: http://people.freebsd.org/~jh/patches/lookup-root.2.diff ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: namei() returns EISDIR for / (Re: svn commit: r203990 - head/lib/libc/sys)
On 2010-03-06, Garrett Cooper wrote: http://people.freebsd.org/~jh/patches/lookup-root.2.diff 1. EBUSY's new definition doesn't look correct for rename(2) given POSIX's definition ( http://www.opengroup.org/onlinepubs/009695399/functions/rename.html ): [EBUSY] [CX] [Option Start] The directory named by old or new is currently in use by the system or another process, and the implementation considers this an error. [Option End] Well, I don't think that it conflicts with POSIX. The root directory is in use by the system and it's not allowed to be renamed. 2. I also did some quick snooping around and all filesystems that support symlinks that FreeBSD has, sans ZFS [*], seem to support EMLINK. Should we add this to the documentation? Probably, but it's not related to the changes made by the patch. -- Jaakko ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: namei() returns EISDIR for / (Re: svn commit: r203990 - head/lib/libc/sys)
On Thu, Mar 4, 2010 at 9:57 PM, Jaakko Heinonen j...@freebsd.org wrote: On 2010-02-28, Jaakko Heinonen wrote: http://people.freebsd.org/~jh/patches/lookup-root.diff I have updated the patch taking some of bde's comments into account. The new version also includes updates for namei(9) manual page. The patch is attached to this mail and also found at: http://people.freebsd.org/~jh/patches/lookup-root.2.diff 1. EBUSY's new definition doesn't look correct for rename(2) given POSIX's definition ( http://www.opengroup.org/onlinepubs/009695399/functions/rename.html ): [EBUSY] [CX] [Option Start] The directory named by old or new is currently in use by the system or another process, and the implementation considers this an error. [Option End] 2. I also did some quick snooping around and all filesystems that support symlinks that FreeBSD has, sans ZFS [*], seem to support EMLINK. Should we add this to the documentation? [EMLINK] [CX] [Option Start] The file named by old is a directory, and the link count of the parent directory of new would exceed {LINK_MAX}. [Option End] Thanks! -Garrett [*] I took a really superficial look at ZFS and this is what I found (from zfs_pathconf): switch (cmd) { case _PC_LINK_MAX: *valp = INT_MAX; return (0); ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: namei() returns EISDIR for / (Re: svn commit: r203990 - head/lib/libc/sys)
On 2010-02-28, Jaakko Heinonen wrote: http://people.freebsd.org/~jh/patches/lookup-root.diff I have updated the patch taking some of bde's comments into account. The new version also includes updates for namei(9) manual page. The patch is attached to this mail and also found at: http://people.freebsd.org/~jh/patches/lookup-root.2.diff -- Jaakko Index: sys/kern/vfs_lookup.c === --- sys/kern/vfs_lookup.c (revision 204620) +++ sys/kern/vfs_lookup.c (working copy) @@ -565,18 +565,26 @@ dirloop: } /* - * Check for degenerate name (e.g. / or ) - * which is a way of talking about a directory, - * e.g. like /. or .. + * Check for which represents the root directory after slash + * removal. */ if (cnp-cn_nameptr[0] == '\0') { - if (dp-v_type != VDIR) { - error = ENOTDIR; - goto bad; - } - if (cnp-cn_nameiop != LOOKUP) { - error = EISDIR; - goto bad; + KASSERT(dp-v_type == VDIR, (dp is not a directory)); + /* + * We can't provide a parent node for CREATE, + * DELETE and RENAME operations. + */ + switch (cnp-cn_nameiop) { + case CREATE: + error = EEXIST; + goto bad; + case DELETE: + case RENAME: + error = EBUSY; + goto bad; + default: + KASSERT(cnp-cn_nameiop == LOOKUP, + (nameiop must be LOOKUP)); } if (wantparent) { ndp-ni_dvp = dp; @@ -948,19 +956,17 @@ relookup(struct vnode *dvp, struct vnode #endif /* - * Check for degenerate name (e.g. / or ) - * which is a way of talking about a directory, - * e.g. like /. or .. + * Check for which represents the root directory after slash + * removal. */ if (cnp-cn_nameptr[0] == '\0') { - if (cnp-cn_nameiop != LOOKUP || wantparent) { - error = EISDIR; - goto bad; - } - if (dp-v_type != VDIR) { - error = ENOTDIR; - goto bad; - } + KASSERT(dp-v_type == VDIR, (dp is not a directory)); + /* + * Support only LOOKUP for / because lookup() + * can't succeed for CREATE, DELETE and RENAME. + */ + KASSERT(cnp-cn_nameiop == LOOKUP, (nameiop must be LOOKUP)); + if (!(cnp-cn_flags LOCKLEAF)) VOP_UNLOCK(dp, 0); *vpp = dp; Index: sys/kern/vfs_syscalls.c === --- sys/kern/vfs_syscalls.c (revision 204620) +++ sys/kern/vfs_syscalls.c (working copy) @@ -1841,7 +1841,7 @@ restart: NDINIT_AT(nd, DELETE, LOCKPARENT | LOCKLEAF | MPSAFE | AUDITVNODE1, pathseg, path, fd, td); if ((error = namei(nd)) != 0) - return (error == EINVAL ? EPERM : error); + return ((error == EINVAL || error == EBUSY) ? EPERM : error); vfslocked = NDHASGIANT(nd); vp = nd.ni_vp; if (vp-v_type == VDIR oldinum == 0) { @@ -3617,9 +3617,6 @@ kern_renameat(struct thread *td, int old if (fromnd.ni_vp-v_type == VDIR) tond.ni_cnd.cn_flags |= WILLBEDIR; if ((error = namei(tond)) != 0) { - /* Translate error code for rename(dir1, dir2/.). */ - if (error == EISDIR fvp-v_type == VDIR) - error = EINVAL; NDFREE(fromnd, NDF_ONLY_PNBUF); vrele(fromnd.ni_dvp); vrele(fvp); Index: share/man/man9/namei.9 === --- share/man/man9/namei.9 (revision 204620) +++ share/man/man9/namei.9 (working copy) @@ -338,8 +338,17 @@ An attempt is made to access a file in a permissions. .It Bq Er ELOOP Too many symbolic links were encountered in translating the pathname. -.It Bq Er EISDIR -An attempt is made to open a directory with write mode specified. +.It Bq Er EINVAL +The last component of the pathname specified for the RENAME operation is +.Ql \. . +.It Bq Er EEXIST +The root directory +.Pq Ql / +was specified for the CREATE operation. +.It Bq Er EBUSY +The root directory +.Pq Ql / +was specified for the DELETE operation. .It Bq Er EROFS An attempt is made to modify a file or directory on a read-only file system. .El Index: lib/libc/sys/rename.2 === --- lib/libc/sys/rename.2 (revision 204620) +++ lib/libc/sys/rename.2 (working copy) @@ -252,6 +252,12 @@ The .Fa to argument is a directory and is not empty. +.It Bq Er EBUSY +Either +.Fa from +or +.Fa to +is the root directory. .El .Pp In addition to the errors returned by the Index: lib/libc/sys/unlink.2 === --- lib/libc/sys/unlink.2 (revision 204620) +++ lib/libc/sys/unlink.2 (working copy) @@ -114,8 +114,6 @@ succeeds unless: .Bl -tag -width Er .It Bq Er ENOTDIR A component of the path prefix is not a directory. -.It Bq Er EISDIR -The named file is a directory. .It Bq Er ENAMETOOLONG A component of a pathname exceeded 255 characters, or an entire path name exceeded 1023 characters. ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: namei() returns EISDIR for / (Re: svn commit: r203990 - head/lib/libc/sys)
On Sun, 28 Feb 2010 18:26:05 -0800 Garrett Cooper yanef...@gmail.com wrote: On Sun, Feb 28, 2010 at 5:11 PM, Alexander Best alexbes...@wwu.de wrote: i have a small test app to check {rm|mk}dir()'s errnos with certain args like /, ., /proc and non-empty dirs. i'll submit it to this thread as soon as i also add testcases for syscalls like rename(), unlink(), etc. most of the errno codes returned after applying your patch look correct. i wonder however why rmdir(/proc) returns EACCESS as unprivileged user. wouldn't it make more sense to also return EBUSY? why complain about permission related matters when even root won't be able to perform the operation. Hmm.. good question. POSIX doesn't fully expound on this case (http://www.opengroup.org/onlinepubs/009695399/functions/rmdir.html), and either seem possible... This maybe from rmdir(2)? [EACCES] Write permission is denied on the directory containing the link to be removed. ls -ldo / drwxr-xr-x 44 root wheel - 1536 Feb 28 18:36 / ls -ldo /proc dr-xr-xr-x 2 root wheel - 512 Sep 7 2008 /proc --- Gary Jennejohn ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: namei() returns EISDIR for / (Re: svn commit: r203990 - head/lib/libc/sys)
On Sun, 28 Feb 2010, Garrett Cooper wrote: On Sun, Feb 28, 2010 at 5:11 PM, Alexander Best alexbes...@wwu.de wrote: i have a small test app to check {rm|mk}dir()'s errnos with certain args like /, ., /proc and non-empty dirs. i'll submit it to this thread as soon as i also add testcases for syscalls like rename(), unlink(), etc. most of the errno codes returned after applying your patch look correct. i wonder however why rmdir(/proc) returns EACCESS as unprivileged user. wouldn't it make more sense to also return EBUSY? why complain about permission related matters when even root won't be able to perform the operation. Hmm.. good question. POSIX doesn't fully expound on this case (http://www.opengroup.org/onlinepubs/009695399/functions/rmdir.html), and either seem possible... At: http://www.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_03.html#tag_02_03 we have If more than one error occurs in processing a function call, any one of the possible errors may be returned, as the order of detection is undefined. So we're okay standard-wise. In general, though, I'd think it makes sense to do permissions checks before anything else, because in some cases the error code can leak information. For instance, if you try to open() a nonexistent file in a directory for which you don't have search permission ('x' bit), it's very important that open() fail with EACCES instead of ENOENT, since you aren't suppposed to be able to find out whether or not the file exists. Obviously that doesn't apply in this case, because anyone is entitled to know that /proc is the root of a mounted filesystem, but it seems to me that it's a good habit to check permission first. -- Nate Eldredge n...@thatsmathematics.com ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
namei() returns EISDIR for / (Re: svn commit: r203990 - head/lib/libc/sys)
[Moving discussion to -hackers. This is not directly related to the original commit anymore.] On 2010-02-26, Bruce Evans wrote: http://people.freebsd.org/~jh/patches/lookup-root.diff This is in relookup(). I think relookup() is only called from rename(), so the failing case is unreachable I don't think it's true for unionfs. Looking at the code it seems that it can call relookup() also with CREATE and DELETE nameiops. % @@ -3618,9 +3618,6 @@ kern_renameat(struct thread *td, int old % if (fromnd.ni_vp-v_type == VDIR) % tond.ni_cnd.cn_flags |= WILLBEDIR; % if ((error = namei(tond)) != 0) { % - /* Translate error code for rename(dir1, dir2/.). */ % - if (error == EISDIR fvp-v_type == VDIR) % - error = EINVAL; I think this has nothing to do with the root directory (as its comment says), but it is to translate the EISDIR returned by ufs_lookup(), etc., when `tond' is for a (directory) pathname ending in .. So it should not be removed, except possibly after changing ufs_lookup(), etc., to return EINVAL. The EISDIR in ufs_lookup() is only for RENAME, so it is strange that any translation is needed. I apparently put the translation here to avoid looking at all leaf file systems. After r199137 the case of dir2/. is handled in lookup() before the VOP_LOOKUP() call. I am not sure if it should be removed but it seems that there's no need for the translation after r199137. -- Jaakko ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: namei() returns EISDIR for / (Re: svn commit: r203990 - head/lib/libc/sys)
Bruce Evans said that the doesn't like this comment you added in your patchset /* * Check for which is a way of talking about the root directory. * We can't provide a parent node for CREATE, DELETE and RENAME * operations. */ and would like to keep / the way it is instead of stripping the slash. however this comment /* * Replace multiple slashes by a single slash and trailing slashes * by a null. This must be done before VOP_LOOKUP() because some * fs's don't know about trailing slashes. Remember if there were * trailing slashes to handle symlinks, existing non-directories * and non-existing files that won't be directories specially later. */ says that some fs's can't handle trailing slashes. how exactly can the root dir be expressed on these fs's? is it in fact ? and which fs's exactly are lacking support for trailing slashes? cheers. alex ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: namei() returns EISDIR for / (Re: svn commit: r203990 - head/lib/libc/sys)
i have a small test app to check {rm|mk}dir()'s errnos with certain args like /, ., /proc and non-empty dirs. i'll submit it to this thread as soon as i also add testcases for syscalls like rename(), unlink(), etc. most of the errno codes returned after applying your patch look correct. i wonder however why rmdir(/proc) returns EACCESS as unprivileged user. wouldn't it make more sense to also return EBUSY? why complain about permission related matters when even root won't be able to perform the operation. also: since namei() takes care of handling the / and . cases couldn't those checks in kern_rmdirat() and kern_unlinkat() be removed? it might also be interesting to look at the changes netbsd, openbsd and dragonflybsd have made to vfs_lookup.c. just had a quick look at revision 1.121 from netbsd and they have split lookup() into loads of smaller functions (see revision 1.117 and 1.118). seems they have been doing a lot of work here (using heavy XXX-commenting however). openbsd hasn't made to many changes to vfs_lookup.c. matthew dillon seems to have done an incredible job on dragonfly in connection with vfs_lookup.c. basically they completely got rid of namei() and are now using nlookup() in commit ad57d0edbfceb0cebfb1dce61490df78fcc4a97. the commit message is quite long and claims due to this change all syscalls which used to call namei() have become a lot less complex after switching to nlookup(). right now vfs_lookup.c in dragonfly contains only some legacy code used for compatibility. again: an incredible job! +1 for adapting those changes. ;) cheers. alex btw.: there're a few NAMEI_DIAGNOSTIC ifdefs in vfs_lookup.c to increase verbosity. ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: namei() returns EISDIR for / (Re: svn commit: r203990 - head/lib/libc/sys)
On Sun, Feb 28, 2010 at 5:11 PM, Alexander Best alexbes...@wwu.de wrote: i have a small test app to check {rm|mk}dir()'s errnos with certain args like /, ., /proc and non-empty dirs. i'll submit it to this thread as soon as i also add testcases for syscalls like rename(), unlink(), etc. most of the errno codes returned after applying your patch look correct. i wonder however why rmdir(/proc) returns EACCESS as unprivileged user. wouldn't it make more sense to also return EBUSY? why complain about permission related matters when even root won't be able to perform the operation. Hmm.. good question. POSIX doesn't fully expound on this case (http://www.opengroup.org/onlinepubs/009695399/functions/rmdir.html), and either seem possible... also: since namei() takes care of handling the / and . cases couldn't those checks in kern_rmdirat() and kern_unlinkat() be removed? it might also be interesting to look at the changes netbsd, openbsd and dragonflybsd have made to vfs_lookup.c. just had a quick look at revision 1.121 from netbsd and they have split lookup() into loads of smaller functions (see revision 1.117 and 1.118). seems they have been doing a lot of work here (using heavy XXX-commenting however). openbsd hasn't made to many changes to vfs_lookup.c. matthew dillon seems to have done an incredible job on dragonfly in connection with vfs_lookup.c. basically they completely got rid of namei() and are now using nlookup() in commit ad57d0edbfceb0cebfb1dce61490df78fcc4a97. the commit message is quite long and claims due to this change all syscalls which used to call namei() have become a lot less complex after switching to nlookup(). right now vfs_lookup.c in dragonfly contains only some legacy code used for compatibility. again: an incredible job! +1 for adapting those changes. ;) cheers. alex btw.: there're a few NAMEI_DIAGNOSTIC ifdefs in vfs_lookup.c to increase verbosity. Thanks, -Garrett ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org