patches to add new stat(2) file flags
Kenneth D. Merry
ken at FreeBSD.org
Thu Apr 18 18:49:53 UTC 2013
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 at FreeBSD.ORG
More information about the freebsd-fs
mailing list