svn commit: r326031 - head/sys/fs/msdosfs
Bruce Evans
brde at optusnet.com.au
Tue Nov 21 09:39:53 UTC 2017
On Mon, 20 Nov 2017, Conrad Meyer wrote:
> Log:
> msdosfs(5): Reflect READONLY attribute in file mode
>
> Msdosfs allows setting READONLY by clearing the owner write bit of the file
> mode. (While here, correct the misspelling of S_IWUSR as VWRITE. No
> functional change.)
>
> In msdosfs_getattr, intuitively reflect that READONLY attribute to userspace
> in the file mode.
>
> Reported by: Karl Denninger <karl AT denninger.net>
> Sponsored by: Dell EMC Isilon
This undoes only a small part of READONLY changes in r254627 (which are
small part of of r253627). I think r254627 got the READONLY semantics
wrong, but they were almost consistent and well documented in the commit
log. Now they are more seriously inconsistent.
r254627 did the following for READONLY:
- support it as the file flag UF_READONLY, including normal read/write
support for it as a file flag
- remove checking it in msdosfs_access(), so that it it is ignored under
FreeBSD except as a file flag that is not ignored by all other systems
- neglect to remove setting of it as a permission in msdosfs_setattr().
This is inconsistent.
- ignore it in msdosfs_getattr(), so that its null effect on permissions
can be seen by stat(). This is consistent.
The current commit does:
- undo ignoring it in msdosfs_getattr(). This gives consistent inconsistency
for chmod() and stat(). The flag is still write-only except for reading
it as a flag and now for reading effects that it doesn't really have on
permissions.
The new bugs are only in 1 direction. E.g., for a file with permissions
-rwxr-xr-x, chmod 0 used to change only the READONLY attribute (set it)
and keep the permssions unchanged (because the READONLY attribute is
write-only and the other permissions are read-only and fake and changes
to them are ignored). The file is still writable by the owner, as shown
by the w bit. Now stat() reports the permissions as -r-xr-xr-x, but the
file is still writable by the owner, inconsistently with the w bit.
In the other direction, the w bit is usually set for the owner in the fake
permissions, so remains set in the usual case where the READONLY attribute
is not set. However, if the w bit is intentionally not set in the fake
permissions, this make files readonly by default and you wouldn't want
the READONLY bit not being set to make files writable since it is the usual
case for the READONLY bit to not be set. Also the problems with having
only 1 READONLY bit but 3 r bits are larger in this direction -- if we
make !READONLY to set w bits, then we get either insecurity by setting
S_IWGTP and/or S_IWOTH, or complicated rules to prevent this.
I checked what an old version of Cygwin does. It does the same as FreeBSD
before r254627 (emulate READONLY using permisson(s) bit). Old Cygwin
doesn't support file flags at all, but that made no difference for READONLY
attribute relative to old FreeBSD which didn't support READONLY as a file
flag.
> Modified: head/sys/fs/msdosfs/msdosfs_vnops.c
> ==============================================================================
> --- head/sys/fs/msdosfs/msdosfs_vnops.c Mon Nov 20 20:55:41 2017 (r326030)
> +++ head/sys/fs/msdosfs/msdosfs_vnops.c Mon Nov 20 21:38:24 2017 (r326031)
> @@ -287,6 +287,8 @@ msdosfs_getattr(struct vop_getattr_args *ap)
> vap->va_fileid = fileid;
>
> mode = S_IRWXU|S_IRWXG|S_IRWXO;
> + if (dep->de_Attributes & ATTR_READONLY)
> + mode &= ~(S_IWUSR|S_IWGRP|S_IWOTH);
> vap->va_mode = mode &
> (ap->a_vp->v_type == VDIR ? pmp->pm_dirmask : pmp->pm_mask);
> vap->va_uid = pmp->pm_uid;
> ...
Now the mode reported is not the one that is used.
Full diff for 254627 on msdosfs_vnops.c with comments on related parts:
X Index: msdosfs_vnops.c
X ===================================================================
X --- msdosfs_vnops.c (revision 254626)
X +++ msdosfs_vnops.c (revision 254627)
X @@ -172,8 +172,7 @@
X if (error)
X goto bad;
X
X - ndirent.de_Attributes = (ap->a_vap->va_mode & VWRITE) ?
X - ATTR_ARCHIVE : ATTR_ARCHIVE | ATTR_READONLY;
X + ndirent.de_Attributes = ATTR_ARCHIVE;
X ndirent.de_LowerCase = 0;
X ndirent.de_StartCluster = 0;
X ndirent.de_FileSize = 0;
X @@ -256,8 +255,7 @@
X mode_t file_mode;
X accmode_t accmode = ap->a_accmode;
X
X - file_mode = (S_IXUSR|S_IXGRP|S_IXOTH) | (S_IRUSR|S_IRGRP|S_IROTH) |
X - ((dep->de_Attributes & ATTR_READONLY) ? 0 : (S_IWUSR|S_IWGRP|S_IWOTH));
X + file_mode = S_IRWXU|S_IRWXG|S_IRWXO;
X file_mode &= (vp->v_type == VDIR ? pmp->pm_dirmask : pmp->pm_mask);
X
X /*
This is the main change to make the ATTR_READONLY write-only as a permission
(by ignoring it in access checks).
This also fixes some style bugs (extra parentheses, missing spaces around
binary operators, and verboseness from not using standard macros).
X @@ -266,8 +264,8 @@
X */
X if (accmode & VWRITE) {
X switch (vp->v_type) {
X + case VREG:
X case VDIR:
X - case VREG:
X if (vp->v_mount->mnt_flag & MNT_RDONLY)
X return (EROFS);
X break;
X @@ -322,10 +320,7 @@
X else
X vap->va_fileid = (long)fileid;
X
X - if ((dep->de_Attributes & ATTR_READONLY) == 0)
X - mode = S_IRWXU|S_IRWXG|S_IRWXO;
X - else
X - mode = S_IRUSR|S_IXUSR|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH;
X + mode = S_IRWXU|S_IRWXG|S_IRWXO;
X vap->va_mode = mode &
X (ap->a_vp->v_type == VDIR ? pmp->pm_dirmask : pmp->pm_mask);
X vap->va_uid = pmp->pm_uid;
This completes makeing ATTR_READONLY write-only as a permission (by ignoring
it when reporting permissions via stat() and friends (mostly for userland
while ignoring it for access is mostly for the kernel).
This also fixes smaller style bugs (missing spaces and verboseness).
This is what the current comment undoes, except it has different style
bugs (the if-else expression is arguably clearer, but too verbose. I
prefer using the conditional operator, but that would be differently
verbose. A conditional operator is used in the next statement and it
is clearly a style bug to mix if/else logic with that.
X @@ -345,8 +340,14 @@
X vap->va_birthtime.tv_nsec = 0;
X }
X vap->va_flags = 0;
X - if ((dep->de_Attributes & ATTR_ARCHIVE) == 0)
X - vap->va_flags |= SF_ARCHIVED;
X + if (dep->de_Attributes & ATTR_ARCHIVE)
X + vap->va_flags |= UF_ARCHIVE;
X + if (dep->de_Attributes & ATTR_HIDDEN)
X + vap->va_flags |= UF_HIDDEN;
X + if (dep->de_Attributes & ATTR_READONLY)
X + vap->va_flags |= UF_READONLY;
X + if (dep->de_Attributes & ATTR_SYSTEM)
X + vap->va_flags |= UF_SYSTEM;
X vap->va_gen = 0;
X vap->va_blocksize = pmp->pm_bpcluster;
X vap->va_bytes =
The other attributes are too different to emulate using permissions,
so we don't try to, and for some reason we stopped trying to for
ATTR_READONLY too.
X @@ -395,6 +396,18 @@
X #endif
X return (EINVAL);
X }
X +
X + /*
X + * We don't allow setting attributes on the root directory.
X + * The special case for the root directory is because before
X + * FAT32, the root directory didn't have an entry for itself
X + * (and was otherwise special). With FAT32, the root
X + * directory is not so special, but still doesn't have an
X + * entry for itself.
X + */
X + if (vp->v_vflag & VV_ROOT)
X + return (EINVAL);
X +
X if (vap->va_flags != VNOVAL) {
X if (vp->v_mount->mnt_flag & MNT_RDONLY)
X return (EROFS);
X @@ -408,24 +421,29 @@
X * attributes. We ignored the access time and the
X * read and execute bits. We were strict for the other
X * attributes.
X - *
X - * Here we are strict, stricter than ufs in not allowing
X - * users to attempt to set SF_SETTABLE bits or anyone to
X - * set unsupported bits. However, we ignore attempts to
X - * set ATTR_ARCHIVE for directories `cp -pr' from a more
X - * sensible filesystem attempts it a lot.
X */
X - if (vap->va_flags & SF_SETTABLE) {
X - error = priv_check_cred(cred, PRIV_VFS_SYSFLAGS, 0);
X - if (error)
X - return (error);
X - }
X - if (vap->va_flags & ~SF_ARCHIVED)
X + if (vap->va_flags & ~(UF_ARCHIVE | UF_HIDDEN | UF_READONLY |
X + UF_SYSTEM))
X return EOPNOTSUPP;
X - if (vap->va_flags & SF_ARCHIVED)
X + if (vap->va_flags & UF_ARCHIVE)
X + dep->de_Attributes |= ATTR_ARCHIVE;
X + else
X dep->de_Attributes &= ~ATTR_ARCHIVE;
X - else if (!(dep->de_Attributes & ATTR_DIRECTORY))
X - dep->de_Attributes |= ATTR_ARCHIVE;
X + if (vap->va_flags & UF_HIDDEN)
X + dep->de_Attributes |= ATTR_HIDDEN;
X + else
X + dep->de_Attributes &= ~ATTR_HIDDEN;
X + /* We don't allow changing the readonly bit on directories. */
X + if (vp->v_type != VDIR) {
X + if (vap->va_flags & UF_READONLY)
X + dep->de_Attributes |= ATTR_READONLY;
X + else
X + dep->de_Attributes &= ~ATTR_READONLY;
X + }
X + if (vap->va_flags & UF_SYSTEM)
X + dep->de_Attributes |= ATTR_SYSTEM;
X + else
X + dep->de_Attributes &= ~ATTR_SYSTEM;
X dep->de_flag |= DE_MODIFIED;
X }
X
X @@ -489,21 +507,24 @@
X error = VOP_ACCESS(vp, VWRITE, cred, td);
X } else
X error = VOP_ACCESS(vp, VADMIN, cred, td);
X - if (vp->v_type != VDIR) {
X - if ((pmp->pm_flags & MSDOSFSMNT_NOWIN95) == 0 &&
X - vap->va_atime.tv_sec != VNOVAL) {
X - dep->de_flag &= ~DE_ACCESS;
X - timespec2fattime(&vap->va_atime, 0,
X - &dep->de_ADate, NULL, NULL);
X - }
X - if (vap->va_mtime.tv_sec != VNOVAL) {
X - dep->de_flag &= ~DE_UPDATE;
X - timespec2fattime(&vap->va_mtime, 0,
X - &dep->de_MDate, &dep->de_MTime, NULL);
X - }
X + if ((pmp->pm_flags & MSDOSFSMNT_NOWIN95) == 0 &&
X + vap->va_atime.tv_sec != VNOVAL) {
X + dep->de_flag &= ~DE_ACCESS;
X + timespec2fattime(&vap->va_atime, 0,
X + &dep->de_ADate, NULL, NULL);
X + }
X + if (vap->va_mtime.tv_sec != VNOVAL) {
X + dep->de_flag &= ~DE_UPDATE;
X + timespec2fattime(&vap->va_mtime, 0,
X + &dep->de_MDate, &dep->de_MTime, NULL);
X + }
X + /*
X + * We don't set the archive bit when modifying the time of
X + * a directory to emulate the Windows/DOS behavior.
X + */
X + if (vp->v_type != VDIR)
X dep->de_Attributes |= ATTR_ARCHIVE;
X - dep->de_flag |= DE_MODIFIED;
X - }
X + dep->de_flag |= DE_MODIFIED;
X }
X /*
X * DOS files only have the ability to have their writability
Just after here is where r254627 negects to remove toggling of ATTR_READONLY
for permissions changes. The current commit touches this, but only fixes a
style bug:
> @@ -502,7 +504,7 @@ msdosfs_setattr(struct vop_setattr_args *ap)
> }
> if (vp->v_type != VDIR) {
> /* We ignore the read and execute bits. */
> - if (vap->va_mode & VWRITE)
> + if (vap->va_mode & S_IWUSR)
Style fix.
> dep->de_Attributes &= ~ATTR_READONLY;
> else
> dep->de_Attributes |= ATTR_READONLY;
ATTR_READONLY was only accessible via file flags APIs except for this.
It is write-only here too.
Most relevant parts of the commit log for r254627:
X msdosfs_denode.c,
X msdosfs_vnops.c: Add support for getting and setting
X UF_HIDDEN, UF_SYSTEM and UF_READONLY
X in MSDOSFS.
X
X It supported SF_ARCHIVED, but this has been
X changed to be UF_ARCHIVE, which has the same
X semantics as the DOS archive attribute instead
X of inverse semantics like SF_ARCHIVED.
X
X After discussion with Bruce Evans, change
X several things in the msdosfs behavior:
X
X Use UF_READONLY to indicate whether a file
X is writeable instead of file permissions, but
X don't actually enforce it.
^^^^^^^^^^^^^^^^^^^^^^^^^
I forget if I asked for not enforcing it or for removing the old enforcement.
X
X Refuse to change attributes on the root
X directory, because it is special in FAT
X filesystems, but allow most other attribute
X changes on directories.
X
X Don't set the archive attribute on a directory
X when its modification time is updated.
X Windows and DOS don't set the archive attribute
X in that scenario, so we are now bug-for-bug
X compatible.
r254627 adds support for UF_READONLY to many other file systems. msdosfs
should be compatible with the others if possible. zfs and ffs are most
interesting. I don't know what zfs does, but ffs doesn't need UF_READONLY
for permissions so it is write only except for reading it as a flag there,
and msdosfs is compatible with that. The main point of this is that you
can do cp -p back and forth between file systems that support flags without
destroying _all_ of the flags or always getting warnings or errors for
copying failures. At least compatible flags can be copied safely. Flags
that are actually used are unlikely to be compatible and might be unsafe
to copy since they give difference secutity holes on different file systems.
Bruce
More information about the svn-src-head
mailing list