Date: Sun, 10 Mar 2013 19:21:57 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: "Kenneth D. Merry" <ken@FreeBSD.org> Cc: arch@FreeBSD.org, fs@FreeBSD.org Subject: Re: patches to add new stat(2) file flags Message-ID: <20130310181127.D2309@besplex.bde.org> In-Reply-To: <20130308232155.GA47062@nargothrond.kdm.org> References: <20130307000533.GA38950@nargothrond.kdm.org> <20130307222553.P981@besplex.bde.org> <20130308232155.GA47062@nargothrond.kdm.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
>> % @@ -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.
> 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.
>> % 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.
> + .It Cm snapshot
> + set the snapshot flag (most filesystems do not allow changing this flag)
I think none do. It can only be displayed.
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.
> *** 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...
> ! 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.
> + .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?
> *** 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.
> 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.
> ! 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.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130310181127.D2309>