Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 18 Apr 2013 12:49:51 -0600
From:      "Kenneth D. Merry" <ken@FreeBSD.org>
To:        Bruce Evans <brde@optusnet.com.au>
Cc:        arch@FreeBSD.org, fs@FreeBSD.org
Subject:   Re: patches to add new stat(2) file flags
Message-ID:  <20130418184951.GA18777@nargothrond.kdm.org>
In-Reply-To: <20130409190838.GA60733@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>

next in thread | previous in thread | raw e-mail | index | archive | help
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



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130418184951.GA18777>