From owner-freebsd-bugs@FreeBSD.ORG Mon Jul 14 19:40:05 2008 Return-Path: Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 9AB151065677 for ; Mon, 14 Jul 2008 19:40:05 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id 7DC458FC1D for ; Mon, 14 Jul 2008 19:40:05 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.14.2/8.14.2) with ESMTP id m6EJe5Ar063726 for ; Mon, 14 Jul 2008 19:40:05 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.2/8.14.1/Submit) id m6EJe5Q5063725; Mon, 14 Jul 2008 19:40:05 GMT (envelope-from gnats) Resent-Date: Mon, 14 Jul 2008 19:40:05 GMT Resent-Message-Id: <200807141940.m6EJe5Q5063725@freefall.freebsd.org> Resent-From: FreeBSD-gnats-submit@FreeBSD.org (GNATS Filer) Resent-To: freebsd-bugs@FreeBSD.org Resent-Reply-To: FreeBSD-gnats-submit@FreeBSD.org, Jaakko Heinonen Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id D9A4E106564A for ; Mon, 14 Jul 2008 19:34:56 +0000 (UTC) (envelope-from jaakko@saunalahti.fi) Received: from gw02.mail.saunalahti.fi (gw02.mail.saunalahti.fi [195.197.172.116]) by mx1.freebsd.org (Postfix) with ESMTP id 2DCDE8FC12 for ; Mon, 14 Jul 2008 19:34:56 +0000 (UTC) (envelope-from jaakko@saunalahti.fi) Received: from ws64.jh.dy.fi (a91-153-120-204.elisa-laajakaista.fi [91.153.120.204]) by gw02.mail.saunalahti.fi (Postfix) with ESMTP id C78901397B1 for ; Mon, 14 Jul 2008 22:34:53 +0300 (EEST) Received: from ws64.jh.dy.fi (localhost [127.0.0.1]) by ws64.jh.dy.fi (8.14.2/8.14.2) with ESMTP id m6EJYrZo003639 for ; Mon, 14 Jul 2008 22:34:53 +0300 (EEST) (envelope-from jaakko@ws64.jh.dy.fi) Received: (from jaakko@localhost) by ws64.jh.dy.fi (8.14.2/8.14.2/Submit) id m6EJYr2C003638; Mon, 14 Jul 2008 22:34:53 +0300 (EEST) (envelope-from jaakko) Message-Id: <200807141934.m6EJYr2C003638@ws64.jh.dy.fi> Date: Mon, 14 Jul 2008 22:34:53 +0300 (EEST) From: Jaakko Heinonen To: FreeBSD-gnats-submit@FreeBSD.org X-Send-Pr-Version: 3.113 Cc: Subject: kern/125613: [patch] ACL problems with special files X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 14 Jul 2008 19:40:05 -0000 >Number: 125613 >Category: kern >Synopsis: [patch] ACL problems with special files >Confidential: no >Severity: non-critical >Priority: medium >Responsible: freebsd-bugs >State: open >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Mon Jul 14 19:40:05 UTC 2008 >Closed-Date: >Last-Modified: >Originator: Jaakko Heinonen >Release: FreeBSD 8.0-CURRENT >Organization: >Environment: FreeBSD x 8.0-CURRENT FreeBSD 8.0-CURRENT #9 @180378M: Wed Jul 9 13:07:52 EEST 2008 x@x:X i386 >Description: I have found several problems concerning device and fifo files on ACL enabled UFS file systems. 1) Setting ACLs for block device files panics the system. r115588 disabled ACL support for character devices: ------------------------------------------------------------------------ r115588 | rwatson | 2003-06-01 05:42:18 +0300 (Sun, 01 Jun 2003) | 13 lines Return EOPNOTSUPP for attempted EA operations on VCHR vnodes in UFS2; if we permit them to occur, the kernel panics due to our performing EA operations using VOP_STRATEGY on the vnode. This went unnoticed previously because there are very for users of device nodes on UFS2 due to the introduction of devfs. However, this can come up with the Linux compat directories and its hard-coded dev nodes (which will need to go away as we move away from hard-coded device numbers). This can come up if you use EA-intensive features such as ACLs and MAC. The proper fix is pretty complicated, but this band-aid would be an excellent MFC candidate for the release. ------------------------------------------------------------------------ Apparently the same problem exists for VBLK vnodes but the commit above disabled support for VCHR vnodes only. Panic information is found below. The panic is not reproducible without INVARIANTS because the panic is caused by KASSERT in bufstrategy(): KASSERT(vp->v_type != VCHR && vp->v_type != VBLK, ("Wrong vnode in bufstrategy(bp=%p, vp=%p)", bp, vp)); Surprisingly a quick test suggests that ACLs may actually work for block devices if INVARIANTS option is not set. --- panic begins here --- panic: Wrong vnode in bufstrategy(bp=0xcd1e70f0, vp=0xc3203118) cpuid = 0 KDB: enter: panic panic: from debugger cpuid = 0 Uptime: 3m16s Physical memory: 499 MB Dumping 36 MB: 21 5 #0 doadump () at pcpu.h:196 196 pcpu.h: No such file or directory. in pcpu.h (kgdb) bt #0 doadump () at pcpu.h:196 #1 0xc07735be in boot (howto=260) at /home/jaakko/src/sys/kern/kern_shutdown.c:418 #2 0xc0773883 in panic (fmt=Variable "fmt" is not available. ) at /home/jaakko/src/sys/kern/kern_shutdown.c:572 #3 0xc0493ff7 in db_panic (addr=Could not find the frame base for "db_panic". ) at /home/jaakko/src/sys/ddb/db_command.c:446 #4 0xc04949fc in db_command (last_cmdp=0xc0c02f94, cmd_table=0x0, dopager=1) at /home/jaakko/src/sys/ddb/db_command.c:413 #5 0xc0494b0a in db_command_loop () at /home/jaakko/src/sys/ddb/db_command.c:466 #6 0xc04962fd in db_trap (type=3, code=0) at /home/jaakko/src/sys/ddb/db_main.c:228 #7 0xc079e086 in kdb_trap (type=3, code=0, tf=0xd61f07b8) at /home/jaakko/src/sys/kern/subr_kdb.c:510 #8 0xc0a8152b in trap (frame=0xd61f07b8) at /home/jaakko/src/sys/i386/i386/trap.c:643 #9 0xc0a64ffb in calltrap () at /home/jaakko/src/sys/i386/i386/exception.s:146 #10 0xc079e20a in kdb_enter (why=0xc0b0d6c6 "panic", msg=0xc0b0d6c6 "panic") at cpufunc.h:60 #11 0xc077386c in panic ( fmt=0xc0b174b6 "Wrong vnode in bufstrategy(bp=%p, vp=%p)") at /home/jaakko/src/sys/kern/kern_shutdown.c:556 #12 0xc07dd222 in bufstrategy (bo=0xc32031c8, bp=0xcd1e70f0) at /home/jaakko/src/sys/kern/vfs_bio.c:3808 #13 0xc07e2579 in bufwrite (bp=0xcd1e70f0) at buf.h:397 #14 0xc0979e7c in ffs_close_ea (vp=0xc3203118, commit=Variable "commit" is not available. ) at buf.h:385 #15 0xc097a2eb in ffs_setextattr (ap=0xd61f098c) at /home/jaakko/src/sys/ufs/ffs/ffs_vnops.c:1714 #16 0xc0a8b2f5 in VOP_SETEXTATTR_APV (vop=0xc0be94a0, a=0xd61f098c) at vnode_if.c:2660 #17 0xc080225a in vn_extattr_set (vp=0xc3203118, ioflg=Variable "ioflg" is not available. ) at vnode_if.h:1446 #18 0xc097b13e in ufs_setacl (ap=0xd61f0a60) at /home/jaakko/src/sys/ufs/ufs/ufs_acl.c:349 #19 0xc0a8b915 in VOP_SETACL_APV (vop=0xc0be94a0, a=0xd61f0a60) at vnode_if.c:2292 #20 0xc07dc057 in vacl_set_acl (td=0xc31ebaa0, vp=0xc3203118, type=0, aclp=0x8103560) at vnode_if.h:1219 #21 0xc07dc2d1 in __acl_set_file (td=0xc31ebaa0, uap=0xd61f0cfc) at /home/jaakko/src/sys/kern/vfs_acl.c:240 #22 0xc0a80cb3 in syscall (frame=0xd61f0d38) at /home/jaakko/src/sys/i386/i386/trap.c:1026 #23 0xc0a65060 in Xint0x80_syscall () at /home/jaakko/src/sys/i386/i386/exception.s:203 #24 0x00000033 in ?? () --- panic ends here --- 2) pathconf(2) _PC_ACL_EXTENDED returns EINVAL for fifos although fifos support ACLs (on ACL enabled file systems). For block and character devices pathconf(2) claims that ACLs can be set by returning 1. This is not true as described above. The same applies to _PC_ACL_PATH_MAX too. 3) ls(1) uses pathconf(2) to determinate if ACLs are supported. In printlong() ls(1) disables ACL checking when it encounters first file failing pathconf() _PC_ACL_EXTENDED call. However there might be some files supporting ACLs and some that doesn't even in the same directory. This combined with the pathconf(2) bugs described in 2) cause ls(1) to behave erratically. >How-To-Repeat: (ACLs enabled) # mount|grep /img /dev/md0 on /img (ufs, local, acls) # cd /img Case 1: # mknod device2 b 1 1 # setfacl -m u:operator:r device2 (system panics in bufstrategy()) Cases 2 and 3: # mknod device c 1 1 # mkfifo fifo # touch file # setfacl -m u:operator:r file # setfacl -m u:operator:r fifo # ls -l total 12 drwxrwxr-x 2 root operator 512 Jul 8 15:20 .snap ls: ./device: Operation not supported crw-r--r-- 1 root wheel 1, 1 Jul 10 14:34 device prw-r--r-- 1 root wheel 0 Jul 10 14:34 fifo -rw-r--r-- 1 root wheel 0 Jul 10 14:34 file (ls should display "+" after mode string for files which have ACLs set) # ls -l file -rw-r--r--+ 1 root wheel 0 Jul 10 14:34 file # truss ls -l 2>&1|grep pathconf pathconf("./.snap",_PC_ACL_EXTENDED) = 1 (0x1) pathconf("./device",_PC_ACL_EXTENDED) = 1 (0x1) pathconf("./fifo",_PC_ACL_EXTENDED) ERR#22 'Invalid argument' >Fix: Here's a patch which makes following changes to UFS: - disable ACL support for VBLK vnodes (for VCHR nodes it's already disabled) - make ufs_pathconf() aware that ACLs are not supported for device files - implement pathconf VOP for fifos. ufsfifo_pathconf() returns proper values for _PC_ACL_EXTENDED and _PC_ACL_PATH_MAX --- ufs-acl-special-files-fixes.diff begins here --- Index: sys/ufs/ufs/ufs_vnops.c =================================================================== --- sys/ufs/ufs/ufs_vnops.c (revision 180363) +++ sys/ufs/ufs/ufs_vnops.c (working copy) @@ -113,6 +113,7 @@ static vop_symlink_t ufs_symlink; static vop_whiteout_t ufs_whiteout; static vop_close_t ufsfifo_close; static vop_kqfilter_t ufsfifo_kqfilter; +static vop_pathconf_t ufsfifo_pathconf; /* * A virgin directory (no blushing please). @@ -2094,7 +2095,9 @@ ufs_pathconf(ap) break; case _PC_ACL_EXTENDED: #ifdef UFS_ACL - if (ap->a_vp->v_mount->mnt_flag & MNT_ACLS) + /* ACLs are not supported for device files */ + if ((ap->a_vp->v_mount->mnt_flag & MNT_ACLS) && + ap->a_vp->v_type != VCHR && ap->a_vp->v_type != VBLK) *ap->a_retval = 1; else *ap->a_retval = 0; @@ -2104,7 +2107,9 @@ ufs_pathconf(ap) break; case _PC_ACL_PATH_MAX: #ifdef UFS_ACL - if (ap->a_vp->v_mount->mnt_flag & MNT_ACLS) + /* ACLs are not supported for device files */ + if ((ap->a_vp->v_mount->mnt_flag & MNT_ACLS) && + ap->a_vp->v_type != VCHR && ap->a_vp->v_type != VBLK) *ap->a_retval = ACL_MAX_ENTRIES; else *ap->a_retval = 3; @@ -2163,6 +2168,30 @@ ufs_pathconf(ap) } /* + * Return POSIX pathconf information applicable to fifos. + */ +static int +ufsfifo_pathconf(ap) + struct vop_pathconf_args /* { + struct vnode *a_vp; + int a_name; + int *a_retval; + } */ *ap; +{ + /* + * XXX: Check which variables fifos should support. + */ + switch (ap->a_name) { + case _PC_ACL_EXTENDED: + case _PC_ACL_PATH_MAX: + return (ufs_pathconf(ap)); + default: + return (fifo_specops.vop_pathconf(ap)); + } + /* NOTREACHED */ +} + +/* * Initialize the vnode associated with a new inode, handle aliased * vnodes. */ @@ -2476,6 +2505,7 @@ struct vop_vector ufs_fifoops = { .vop_getattr = ufs_getattr, .vop_inactive = ufs_inactive, .vop_kqfilter = ufsfifo_kqfilter, + .vop_pathconf = ufsfifo_pathconf, .vop_print = ufs_print, .vop_read = VOP_PANIC, .vop_reclaim = ufs_reclaim, Index: sys/ufs/ffs/ffs_vnops.c =================================================================== --- sys/ufs/ffs/ffs_vnops.c (revision 180363) +++ sys/ufs/ffs/ffs_vnops.c (working copy) @@ -1337,7 +1337,7 @@ struct vop_openextattr_args { ip = VTOI(ap->a_vp); fs = ip->i_fs; - if (ap->a_vp->v_type == VCHR) + if (ap->a_vp->v_type == VCHR || ap->a_vp->v_type == VBLK) return (EOPNOTSUPP); return (ffs_open_ea(ap->a_vp, ap->a_cred, ap->a_td)); @@ -1365,7 +1365,7 @@ struct vop_closeextattr_args { ip = VTOI(ap->a_vp); fs = ip->i_fs; - if (ap->a_vp->v_type == VCHR) + if (ap->a_vp->v_type == VCHR || ap->a_vp->v_type == VBLK) return (EOPNOTSUPP); if (ap->a_commit && (ap->a_vp->v_mount->mnt_flag & MNT_RDONLY)) @@ -1399,7 +1399,7 @@ vop_deleteextattr { ip = VTOI(ap->a_vp); fs = ip->i_fs; - if (ap->a_vp->v_type == VCHR) + if (ap->a_vp->v_type == VCHR || ap->a_vp->v_type == VBLK) return (EOPNOTSUPP); if (strlen(ap->a_name) == 0) @@ -1489,7 +1489,7 @@ vop_getextattr { ip = VTOI(ap->a_vp); fs = ip->i_fs; - if (ap->a_vp->v_type == VCHR) + if (ap->a_vp->v_type == VCHR || ap->a_vp->v_type == VBLK) return (EOPNOTSUPP); error = extattr_check_cred(ap->a_vp, ap->a_attrnamespace, @@ -1549,7 +1549,7 @@ vop_listextattr { ip = VTOI(ap->a_vp); fs = ip->i_fs; - if (ap->a_vp->v_type == VCHR) + if (ap->a_vp->v_type == VCHR || ap->a_vp->v_type == VBLK) return (EOPNOTSUPP); error = extattr_check_cred(ap->a_vp, ap->a_attrnamespace, @@ -1619,7 +1619,7 @@ vop_setextattr { ip = VTOI(ap->a_vp); fs = ip->i_fs; - if (ap->a_vp->v_type == VCHR) + if (ap->a_vp->v_type == VCHR || ap->a_vp->v_type == VBLK) return (EOPNOTSUPP); if (strlen(ap->a_name) == 0) --- ufs-acl-special-files-fixes.diff ends here --- I see two options to fix ls(1): 1) Remove pathconf(2) result caching completely. This increases number of pathconf(2) system calls on file systems with ACLs disabled. 2) Only make assumptions about file system supporting ACLs on regular files or directories. This patch implements 2): --- ls-haveacls-only-regular-files.diff begins here --- Index: bin/ls/print.c =================================================================== --- bin/ls/print.c (revision 180487) +++ bin/ls/print.c (working copy) @@ -629,23 +629,30 @@ aclmode(char *buf, const FTSENT *p, int else snprintf(name, sizeof(name), "%s/%s", p->fts_parent->fts_accpath, p->fts_name); + + *haveacls = 1; + /* * We have no way to tell whether a symbolic link has an ACL since * pathconf() and acl_get_file() both follow them. They also don't * support whiteouts. */ - if (S_ISLNK(p->fts_statp->st_mode) || S_ISWHT(p->fts_statp->st_mode)) { - *haveacls = 1; + if (S_ISLNK(p->fts_statp->st_mode) || S_ISWHT(p->fts_statp->st_mode)) return; - } + if ((ret = pathconf(name, _PC_ACL_EXTENDED)) <= 0) { + /* + * Only make assumptions about file system supporting ACLs on + * regular files or directories. UFS doesn't support ACLs for + * device files. + */ if (ret < 0 && errno != EINVAL) warn("%s", name); - else + else if (S_ISREG(p->fts_statp->st_mode) || + S_ISDIR(p->fts_statp->st_mode)) *haveacls = 0; return; } - *haveacls = 1; if ((facl = acl_get_file(name, ACL_TYPE_ACCESS)) != NULL) { if (acl_get_entry(facl, ACL_FIRST_ENTRY, &ae) == 1) { entries = 1; --- ls-haveacls-only-regular-files.diff ends here --- >Release-Note: >Audit-Trail: >Unformatted: