From owner-freebsd-fs@FreeBSD.ORG Thu Apr 18 18:49:53 2013 Return-Path: Delivered-To: fs@FreeBSD.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 360AB935; Thu, 18 Apr 2013 18:49:53 +0000 (UTC) (envelope-from ken@kdm.org) Received: from nargothrond.kdm.org (nargothrond.kdm.org [70.56.43.81]) by mx1.freebsd.org (Postfix) with ESMTP id BE290138E; Thu, 18 Apr 2013 18:49:52 +0000 (UTC) Received: from nargothrond.kdm.org (localhost [127.0.0.1]) by nargothrond.kdm.org (8.14.2/8.14.2) with ESMTP id r3IInpvi019293; Thu, 18 Apr 2013 12:49:51 -0600 (MDT) (envelope-from ken@nargothrond.kdm.org) Received: (from ken@localhost) by nargothrond.kdm.org (8.14.2/8.14.2/Submit) id r3IInp7l019292; Thu, 18 Apr 2013 12:49:51 -0600 (MDT) (envelope-from ken) Date: Thu, 18 Apr 2013 12:49:51 -0600 From: "Kenneth D. Merry" To: Bruce Evans Subject: Re: patches to add new stat(2) file flags Message-ID: <20130418184951.GA18777@nargothrond.kdm.org> References: <20130307000533.GA38950@nargothrond.kdm.org> <20130307222553.P981@besplex.bde.org> <20130308232155.GA47062@nargothrond.kdm.org> <20130310181127.D2309@besplex.bde.org> <20130409190838.GA60733@nargothrond.kdm.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130409190838.GA60733@nargothrond.kdm.org> User-Agent: Mutt/1.4.2i Cc: arch@FreeBSD.org, fs@FreeBSD.org X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 18 Apr 2013 18:49:53 -0000 On Tue, Apr 09, 2013 at 13:08:38 -0600, Kenneth D. Merry wrote: > On Sun, Mar 10, 2013 at 19:21:57 +1100, Bruce Evans wrote: > > On Fri, 8 Mar 2013, Kenneth D. Merry wrote: > > > > >On Fri, Mar 08, 2013 at 00:37:15 +1100, Bruce Evans wrote: > > >>On Wed, 6 Mar 2013, Kenneth D. Merry wrote: > > >> > > >>>I have attached diffs against head for some additional stat(2) file > > >>>flags. > > >>> > > >>>The primary purpose of these flags is to improve compatibility with CIFS, > > >>>both from the client and the server side. > > >>>... > > >> > > >>I missed looking at the diffs in my previous reply. > > >> > > >>% --- //depot/users/kenm/FreeBSD-test3/bin/chflags/chflags.1 2013-03-04 > > >>17:51:12.000000000 -0700 > > >>% +++ /usr/home/kenm/perforce4/kenm/FreeBSD-test3/bin/chflags/chflags.1 > > >>2013-03-04 17:51:12.000000000 -0700 > > >>% --- /tmp/tmp.49594.86 2013-03-06 16:42:43.000000000 -0700 > > >>% +++ /usr/home/kenm/perforce4/kenm/FreeBSD-test3/bin/chflags/chflags.1 > > >>2013-03-06 14:47:25.987128763 -0700 > > >>% @@ -117,6 +117,16 @@ > > >>% set the user immutable flag (owner or super-user only) > > >>% .It Cm uunlnk , uunlink > > >>% set the user undeletable flag (owner or super-user only) > > >>% +.It Cm system , usystem > > >>% +set the Windows system flag (owner or super-user only) > > >> > > >>This begins unsorting of the list. > > > > > >Fixed. > > > > > >>It's not just a Windows flag, since it also works in DOS. > > > > > >Fixed. > > > > Thanks. Hopefully all the simple bugs are fixed now. > > > > >>"Owner or" is too strict for msdosfs, since files can only have a > > >>single owner so it is controlling access using groups is needed. I > > >>use owner root and group msdosfs for msdosfs mounts. This works for > > >>normal operations like open/read/write, but fails for most attributes > > >>including file flags. msdosfs doesn't support many attributes but > > >>this change is supposed to add support for 3 new file flags so it would > > >>be good if it didn't restrict the support to root. > > > > > >I wasn't trying to change the existing security model for msdosfs, but if > > >you've got a suggested patch to fix it I can add that in. > > > > I can't think of anything better than making group write permission enough > > for attributes. > > > > msdosfs also has some style bugs in this area. It uses VOP_ACCESS() > > with VADMIN for the non-VA_UTIMES_NULL case of utimes(), but for all > > other attributes it hard-codes a direct uid check followed a > > priv_check_cred() with PRIV_VFS_ADMIN. VADMIN requires even more than > > user write permission for POSIX file systems and using it unchanged > > for all the attributes would be even more restrictive unless we changed > > it, but it would be easier to make it uniformly less restrictive for > > msdosfs by using it consistently. > > > > Oops, that was in the old version of ffs. ffs now has related > > complications and unnecessary style bugs (verboseness and misformatting) > > to support ACLs. It now uses VOP_ACCESSX() with VWRITE_ATTRIBUTES for > > utimes(), and VOP_ACCESSX() with other VFOO for all attributes except > > flags. It still uses VOP_ACCESS() with VADMIN() for flags. > > > > >>... > > >>% .It Dv SF_ARCHIVED > > >>... > > >>% +Filesystems in FreeBSD may or may not have special handling for this > > >>flag. > > >>% +For instance, ZFS tracks changes to files and will clear this bit when > > >>a > > >>% +file is updated. > > >>% +UFS only stores the flag, and relies on the application to change it > > >>when > > >>% +needed. > > >> > > >>I think that is useless, since changing it is needed whenever the file > > >>changes, and applications can do that (short of running as daemons and > > >>watching for changes). > > > > > >Do you mean applications can't do that or can? > > > > Oops, can't. > > > > It is still hard for users to know how their file system supports. > > Even programmers don't know that it is backwards :-). > > > > >>% --- //depot/users/kenm/FreeBSD-test3/sys/fs/msdosfs/msdosfs_vnops.c > > >>2013-03-04 17:51:12.000000000 -0700 > > >>% +++ > > >>/usr/home/kenm/perforce4/kenm/FreeBSD-test3/sys/fs/msdosfs/msdosfs_vnops.c > > >>2013-03-04 17:51:12.000000000 -0700 > > >>% --- /tmp/tmp.49594.370 2013-03-06 16:42:43.000000000 -0700 > > >>% +++ > > >>/usr/home/kenm/perforce4/kenm/FreeBSD-test3/sys/fs/msdosfs/msdosfs_vnops.c > > >>2013-03-06 14:49:47.179130318 -0700 > > >>% @@ -345,8 +345,17 @@ > > >>% vap->va_birthtime.tv_nsec = 0; > > >>% } > > >>% vap->va_flags = 0; > > >>% + /* > > >>% + * The DOS Archive attribute means that a file needs to be > > >>% + * archived. The BSD SF_ARCHIVED attribute means that a file has > > >>% + * been archived. Thus the inversion here. > > >>% + */ > > >> > > >>No need to document it again. It goes without saying that ARCHIVE > > >>!= ARCHIVED. > > > > > >I disagree. It wasn't immediately obvious to me that SF_ARCHIVED was > > >generally used as the inverse of the DOS Archived bit until I started > > >digging into this. If this helps anyone figure that out more quickly, it's > > >useful. > > > > The surprising thing is that it is backwards in FreeBSD and not really > > supported except in msdosfs. Now several file systems have the comment > > about it being inverted, but man pages still don't. > > I made the change to UF_ARCHIVE, and updated the man pages. > > > >>% @@ -420,12 +429,21 @@ > > >>% if (error) > > >>% return (error); > > >>% } > > >> > > >>The permissions check before this is delicate and was broken and is > > >>more broken now. It is still short-circuit to handle setting the > > >>single flag that used to be supported, and is slightly broken for that: > > >>- unprivileged user asks to set ARCHIVE by passing !SF_ARCHIVED. We > > >> allow that, although this may toggle the flag and normal semantics > > >> for SF flags is to not allow toggling. > > >>- unprivileged user asks to clear ARCHIVE by passing SF_ARCHIVED. We > > >> don't allow that. But we should allow preserving ARCHIVE if it is > > >> already clear. > > >>The bug wasn't very important when only 1 flag was supported. Now it > > >>prevents unprivileged users managing the new UF flags if ARCHIVE is > > >>clear. Fortunately, this is the unusual case. Anyway, unprivileged > > >>users can set ARCHIVE by doing some other operation. Even the chflags() > > >>operation should set ARCHIVE and thus allow further chflags()'s that now > > >>keep ARCHIVE set. Except it is very confusing if a chflags() asks for > > >>ARCHIVE to be clear. This request might be just to try to preserve > > >>the current setting and not want it if other things are changed, or > > >>it might be to purposely clear it. Changing it from set to clear should > > >>still be privileged. > > > > > >I changed it to allow setting or clearing SF_ARCHIVED. Now I can set or > > >clear the flag as non-root: > > > > Actually, it seems OK, since there are no old or new SF_ immututable flags. > > Some of the actions are broken in the old and new code for directories -- > > see below. > > > > >>See the more complicated permissions check in ffs. It would be safest > > >>to duplicate most of it, to get different permissions checking for the > > >>SF and UF flags. Then decide if we want to keep allowing setting > > >>ARCHIVE without privilege. > > > > > >I think we should allow getting and setting SF_ARCHIVED without special > > >privileges. Given how it is generally used, I don't think it should be > > >restricted to the super-user. > > > > I don't really like that since changing the flags is mainly needed for > > the failry privileged operation of managing other OS's file systems. > > However, since we're mapping the SYSTEM flag to a UF_ flag, the SYSTEM > > flag will require less privilege than the ARCHIVE flag. This is backwards, > > so we might as well require less privilege for ARCHIVE too. I think we, > > that is, you should use a new UF_ARCHIVE flag with the correct sense. > > Okay, done. The patches are attached with UF_ARCHIVE used instead of > SF_ARCHIVED, with the sense reversed. > > > >Can you provide some code demonstrating how the permissions code should > > >be changed in msdosfs? I don't know that much about that sort of thing, > > >so I'll probably spend an inordinate amount of time stumbling > > >through it. > > > > Now I think only cleanups are needed. > > Okay. > > > >>% return EOPNOTSUPP; > > >>% if (vap->va_flags & SF_ARCHIVED) > > >>% dep->de_Attributes &= ~ATTR_ARCHIVE; > > >>% else if (!(dep->de_Attributes & ATTR_DIRECTORY)) > > >>% dep->de_Attributes |= ATTR_ARCHIVE; > > >> > > >>The comment before this says that we ignore attmps to set ATTR_ARCHIVED > > >>for directories. However, it is out of date. WinXP allows setting it > > >>and all the new flags for directories, and so do we. > > > > > >Do you mean we allow setting it in UFS, or where? Obviously the code above > > >won't set it on a directory. > > > > I meant it here. Actually, the comment matches the code -- I somehow missed > > the test in the code. However, the code is wrong. All directories except > > the root directory have this and other attributes, but FreeBSD refuses to > > set them. More below. > > > > >>The WinXP attrib command (at least on a FAT32 fs) doesn't allow setting > > >>or clearing ARCHIVE (even if it is already set or clear) if any of > > >>HIDDEN, READONLY or SYSTEM is already set and remains set after the > > >>command. Thus the HRS attributes act a bit like immutable flags, but > > >>subtly differently. (ffs has the quite different and worse behaviour > > >>of allowing chflags() irrespective of immutable flags being set before > > >>or after, provided there is enough privilege to change the immutable > > >>flags.) Anyway, they should all give some aspects of immutability. > > > > > >We could do that for msdosfs, but why add more things for the user to trip > > >over given how the filesystem is typically used? Most people probably > > >use it for USB thumb drives these days. Or perahps on a dual boot system > > >to access their Windows partition. > > > > The small data drives won't have many files with attributes (except > > ARCHIVE). For multiple-boot, I think the permssions shouldn't be too > > much different than the foreign OS's. I used not to worry about this > > and liked deleting WinXP files without asking it, but recently I spent > > a lot of time recovering a WinXP ntfs partition and changed a bit too > > much using FreeBSD and Cygwin because I didn't understand the > > permissions (especially ACLs). ntfs in FreeBSD was less than r/o so it > > couldn't even back up the permissions (for file flags, it returned the > > garbage in its internal inode flags without translation...). > > > > >*** src/bin/chflags/chflags.1.orig > > >--- src/bin/chflags/chflags.1 > > >*************** > > >*** 101,120 **** > > > .Bl -tag -offset indent -width ".Cm opaque" > > > .It Cm arch , archived > > > set the archived flag (super-user only) > > > .It Cm opaque > > > set the opaque flag (owner or super-user only) > > >- .It Cm nodump > > >- set the nodump flag (owner or super-user only) > > > .It Cm sappnd , sappend > > > > The opaque flag is UF_ too. > > Yes, but all of the flag descriptions are sorted in alphabetical order. > How would you suggest sorting them instead? (SF first and then UF, both in > some version of alphabetical order?) > > > >+ .It Cm snapshot > > >+ set the snapshot flag (most filesystems do not allow changing this flag) > > > > I think none do. It can only be displayed. > > Fixed. > > > chflags(1) doesn't display flags, so this shouldn't be here. The problem > > is that this man page is the only place where the flag names are documented. > > ls(1) and strtofflags(3) just point to here. strtofflags(3) says that the > > flag names are documented here, but ls(1) just has an Xref to here. > > I fixed ls(1) at least. > > > >*** src/lib/libc/sys/chflags.2.orig > > >--- src/lib/libc/sys/chflags.2 > > >--- 71,127 ---- > > > the following values > > > .Pp > > > .Bl -tag -width ".Dv SF_IMMUTABLE" -compact -offset indent > > >! .It Dv SF_APPEND > > > The file may only be appended to. > > > .It Dv SF_ARCHIVED > > >! The file has been archived. > > >! This flag means the opposite of the Windows and CIFS > > >FILE_ATTRIBUTE_ARCHIVE > > > > DOS, Windows and CIFS... > > Fixed. > > > >! attribute. > > >! That attribute means that the file should be archived, whereas > > >! .Dv SF_ARCHIVED > > >! means that the file has been archived. > > >! Filesystems in FreeBSD may or may not have special handling for this > > >flag. > > >! For instance, ZFS tracks changes to files and will clear this bit when a > > >! file is updated. > > > > Does zfs clear it in other circumstances? WinXP doesn't for msdosfs (or > > ntfs?), but FreeBSD clears it when changing some attributes, even for > > null changes (these are: times except for atimes, and the HIDDEN attribute > > when it is changed by chmod() -- even for null changes --, but not for > > the HIDDEN attribute when it is changed (or preserved) by chflags() in > > your new code). I want to to be cleared for metadata so that backup > > utilities can trust the ARCHIVE flag for metadata changes. > > Well, it does look like changing a file or touching it causes the archive > flag to get set with ZFS: > > # touch foo > # ls -lao foo > -rw-r--r-- 1 root wheel uarch 0 Apr 8 21:45 foo > # chflags 0 foo > # ls -lao foo > -rw-r--r-- 1 root wheel - 0 Apr 8 21:45 foo > # echo "hello" >> foo > # ls -lao foo > -rw-r--r-- 1 root wheel uarch 6 Apr 8 21:46 foo > # chflags 0 foo > # ls -lao foo > -rw-r--r-- 1 root wheel - 6 Apr 8 21:46 foo > # touch foo > # ls -lao foo > -rw-r--r-- 1 root wheel uarch 6 Apr 8 21:46 foo > > > >+ .It Dv UF_IMMUTABLE > > >+ The file may not be changed. > > >+ Filesystems may use this flag to maintain compatibility with the Windows > > >and > > >+ CIFS FILE_ATTRIBUTE_READONLY attribute. > > > > So READONLY is only mapped to UFS_IMMUTABLE if it gives immutability? > > No, it's mapped to whatever the CIFS server decides. In my changes to > Likewise, I mapped it to UF_IMMUTABLE. I mapped UF_IMMUTABLE to the ZFS > READONLY flag. As Pawel pointed out, there has been some talk on the > Illumos developers list about just storing the READONLY bit and not > enforcing it in ZFS: > > http://www.listbox.com/member/archive/182179/2013/03/sort/time_rev/page/2/?search_for=readonly > > That complicates things somewhat in the Illumos CIFS server, and so I think > it's a reasonable thing to just record the bit and let the CIFS server > enforce things where it needs to. > > UFS does honor the UF_IMMUTABLE flag, so it may be that we need to create > a UF_READONLY flag that corresponds to the DOS readonly flag and is only > stored, and the enforcement would happen in the CIFS server. > > > >*** src/sys/fs/msdosfs/msdosfs_vnops.c.orig > > >--- src/sys/fs/msdosfs/msdosfs_vnops.c > > >*************** > > >*** 415,431 **** > > > * set ATTR_ARCHIVE for directories `cp -pr' from a more > > > * sensible filesystem attempts it a lot. > > > */ > > >! if (vap->va_flags & SF_SETTABLE) { > > > error = priv_check_cred(cred, PRIV_VFS_SYSFLAGS, 0); > > > if (error) > > > return (error); > > > } > > >! if (vap->va_flags & ~SF_ARCHIVED) > > > return EOPNOTSUPP; > > > if (vap->va_flags & SF_ARCHIVED) > > > dep->de_Attributes &= ~ATTR_ARCHIVE; > > > else if (!(dep->de_Attributes & ATTR_DIRECTORY)) > > > dep->de_Attributes |= ATTR_ARCHIVE; > > > dep->de_flag |= DE_MODIFIED; > > > } > > > > > >--- 424,448 ---- > > > * set ATTR_ARCHIVE for directories `cp -pr' from a more > > > * sensible filesystem attempts it a lot. > > > */ > > >! if (vap->va_flags & (SF_SETTABLE & ~(SF_ARCHIVED))) { > > > > Excessive parentheses. > > Fixed, by moving to UF_ARCHIVE. > > > > error = priv_check_cred(cred, PRIV_VFS_SYSFLAGS, 0); > > > if (error) > > > return (error); > > > } > > > > VADMIN is still needed, and that is too strict. This is a general problem > > and should be fixed separately. > > I took out the check, since I changed the code to use UF_ARCHIVE instead of > SF_ARCHIVED. > > > >! if (vap->va_flags & ~(SF_ARCHIVED | UF_HIDDEN | UF_SYSTEM)) > > > return EOPNOTSUPP; > > > if (vap->va_flags & SF_ARCHIVED) > > > dep->de_Attributes &= ~ATTR_ARCHIVE; > > > else if (!(dep->de_Attributes & ATTR_DIRECTORY)) > > > dep->de_Attributes |= ATTR_ARCHIVE; > > >+ if (vap->va_flags & UF_HIDDEN) > > >+ dep->de_Attributes |= ATTR_HIDDEN; > > >+ else > > >+ dep->de_Attributes &= ~ATTR_HIDDEN; > > >+ if (vap->va_flags & UF_SYSTEM) > > >+ dep->de_Attributes |= ATTR_SYSTEM; > > >+ else > > >+ dep->de_Attributes &= ~ATTR_SYSTEM; > > > dep->de_flag |= DE_MODIFIED; > > > } > > > > Technical old and new problems with msdosfs: > > - all directories except the root directory support the 3 attributes > > handled above, and READONLY > > - the special case for the root directory is because before FAT32, the > > root directory didn't have an entry for itself (and was otherwise > > special). With FAT32, the root directory is not so special, but > > still doesn't have an entry for itself. > > - thus the old code in the above is wrong for all directories except > > the root directory > > - thus the new code in the above is wrong for the root directory. It > > will make changes to the in-core denode. These can be seen by stat() > > for a while, but go away when the vnode is recycled. > > - other code is wrong for directories too. deupdat() refuses to > > convert from the in-core denode to the disk directory entry for > > directories. So even when the above changes values for directories, > > the changes only get synced to the disk accidentally when there is > > a large change (such as for extending the directory), to the directory > > entry. > > - being the root directory is best tested for using VV_ROOT. I use the > > following to fix the corresponding bugs in utimes(): > > > > /* Was: silently ignore the non-error or error for all dirs. > > */ > > if (DETOV(dep)->v_vflag & VV_ROOT) > > return (EINVAL); > > /* Otherwise valid. */ > > > > deupdat() needs a similar change to not ignore all directories. > > Okay, I think these issues should now be fixed. We now refuse to change > attributes only on the root directory. And I updatd deupdat() to do the > same. > > When a directory is created or a file is added, the archive bit is not > changed on the directory. Not sure if we need to do that or not. (Simply > changing msdosfs_mkdir() to set ATTR_ARCHIVE was not enough to get the > archive bit set on directory creation.) Bruce, any comment on this? Thanks, Ken -- Kenneth Merry ken@FreeBSD.ORG