kern/125613: [patch] ACL problems with special files
Jaakko Heinonen
jh at saunalahti.fi
Mon Jul 14 19:40:05 UTC 2008
>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 at 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:
More information about the freebsd-bugs
mailing list