From owner-svn-src-all@freebsd.org Tue Nov 21 09:39:53 2017 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 33B0FDE8F22; Tue, 21 Nov 2017 09:39:53 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail110.syd.optusnet.com.au (mail110.syd.optusnet.com.au [211.29.132.97]) by mx1.freebsd.org (Postfix) with ESMTP id 9F4F966B83; Tue, 21 Nov 2017 09:39:52 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.103] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail110.syd.optusnet.com.au (Postfix) with ESMTPS id BBA9A107C8D; Tue, 21 Nov 2017 20:39:43 +1100 (AEDT) Date: Tue, 21 Nov 2017 20:39:42 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Conrad Meyer cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r326031 - head/sys/fs/msdosfs In-Reply-To: <201711202138.vAKLcOxL080933@repo.freebsd.org> Message-ID: <20171121172511.W1210@besplex.bde.org> References: <201711202138.vAKLcOxL080933@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=bc8baKHB c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=C6q32NwkAAAA:8 a=WYKF7o43fX-Tu_o1TVkA:9 a=CjuIK1q_8ugA:10 a=d0GpIgV8JInD5zQLk0HG:22 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 21 Nov 2017 09:39:53 -0000 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 > 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