Inconsistency and bug with *chflags functions

Tue Jan 18 16:49:10 UTC 2011

On Tue, 18 Jan 2011, Garrett Cooper wrote:

> On Tue, Jan 18, 2011 at 6:18 AM, Bruce Evans <brde at optusnet.com.au> wrote:
>> On Tue, 18 Jan 2011, Gleb Kurtsou wrote:
>
> ...
>
>> As you found, the kernel wants everything to be an int, and setfflags()
>> still only accepts an int. ...

[Quote trimmed at the first hard \xa0]

> Signedness isn't important for this flag. Only width and data
> alignment as you suggested elsewhere.

Not important, but it allows undefined behaviour.

>>>> 2. The kernel interfaces are all implicitly downcasting the flags
>>>> argument to int.
>>
>> The downcast in the call to setfflags() is safe, but the type puns given
>> by the wrong types in syscalls.master are not.  I think the big-endian
>> 64-bit long case is the only case seriously broken by these puns, but
>> FreeBSD doesn't notice since it doesn't support any big-endian 64-bit
>> arches.
>
> This is incorrect unfortunately. The newest port of the PowerPC family
> (Sony's PS3) is actually Big Endian, and many of the other archs like
> ARM and MIPS of course are mixed endian :(... so I think that bugs
> like these will become more apparent as time goes on.

I think these are still unsupported, since chflags() cannot work on them :-).

>>> The struct represents on-disk protocol description, and thus shouldn't be
>>> changed.
>>
>> Indeed.  fflags_t is essentially only the type used in struct stat's API and
>> ABI.  It started as u_int32_t too.  Changing the API and ABI for all
>> *chflags() to use it is probably safe too.
>
> Which was my reasoning, but I was a bit uneasy about this.

You can't really start from 1 read-only API.  This API only tells us that
there are at most 32 flags.

>
>>>> Index: sbin/restore/tape.c
>>>> ===================================================================
>>>> --- sbin/restore/tape.c (revision 217362)
>>>> +++ sbin/restore/tape.c (working copy)
>>>> @@ -558,7 +558,7 @@
>>>>  int
>>>>  extractfile(char *name)
>>>>  {
>>>> -       int flags;
>>>> +       fflags_t flags;
>>>>        uid_t uid;
>>>>        gid_t gid;
>>>>        mode_t mode;
>>
>> Unclear from the patch alone if it needs to match dump's ABI.  Probably not.
>
> It does (there were some bits missing that I need to submit in a new patch).

I checked it and don't see any problems except possibly compiler
warnings about type matches that turn out to be harmless.  The value
just gets converted to int when read from the dump (was u_int32_t
there) and from int when passed to chflags() (was u_long there).
"int" is large enough unless the dump has garbage flags.

>>>> Index: sys/kern/vfs_syscalls.c
>>>> ===================================================================
>>>> --- sys/kern/vfs_syscalls.c     (revision 217362)
>>>> +++ sys/kern/vfs_syscalls.c     (working copy)
>>>> @@ -96,7 +96,7 @@
>>>>  static int getutimes(const struct timeval *, enum uio_seg, struct
>>>> timespec *);
>>>>  static int setfown(struct thread *td, struct vnode *, uid_t, gid_t);
>>>>  static int setfmode(struct thread *td, struct vnode *, int);
>>>> -static int setfflags(struct thread *td, struct vnode *, int);
>>>> +static int setfflags(struct thread *td, struct vnode *, fflags_t);
>>>>  static int setutimes(struct thread *td, struct vnode *,
>>>>     const struct timespec *, int, int);
>>>>  static int vn_access(struct vnode *vp, int user_flags, struct ucred
>>>> *cred,
>>
>> Better to change this to whatever va_flags is.  The latter can remain as
>> u_long.
>
> But silent value truncation is bad, right :(?

Yes, but we always did it.  You don't want to make ABI problems larger by
adding strictness now.

I checked what ufs does.  It is sloppy for root -- allows anything in
the user-supplied to be put into on-disk flags (after truncation to 32
bits).  For non-root, it only allows user flags to be changed, but the
mask for this includes 11 flags that have no assigned meaning.  Users
can use this to store 11 bits of data in an empty file :-).

% 
% Script started on Wed Jan 19 03:36:19 2011
% ttyv6:bde at besplex:/tmp/q> >z
% ttyv6:bde at besplex:/tmp/q> chflags 177777 z
% ttyv6:bde at besplex:/tmp/q> ls -lo z
% -rw-r--r--  1 bde  wheel  uappnd,uchg,nodump,opaque,uunlnk 0 Jan 19 03:36 z
% ttyv6:bde at besplex:/tmp/q> /usr/bin/stat z
% 256768 1186 -rw-r--r-- 1 bde wheel 0 0 "Jan 19 03:36:21 2011" "Jan 19 03:36:21 2011" "Jan 19 03:36:29 2011" "Jan  1 10:00:00 1970" 8192 0 0xffff z

0xffff is my 11 bites of data (5 flags and 11 bits not reported by ls
or flagstostr() generally.

% ttyv6:bde at besplex:/tmp/q> chflags 377777 z
% chflags: z: Operation not permitted

Setting 1 system bit is correctly not allowed.

% ttyv6:bde at besplex:/tmp/q> chflags 0 z
% ttyv6:bde at besplex:/tmp/q> rm z
% ttyv6:bde at besplex:/tmp/q> exit
% 
% Script done on Wed Jan 19 03:37:00 2011

> ...
>
>> Now has excessive indentation.  This was consistently broken with
>> syscalls.master.  Similarly for others.
>
> Eh...? Wouldn't it be worse if fflags_t

The quote isn't there and the reply is truncated :-).

Indentation is 8 columns after a type.  I don't like indenting all
the variable names because 1 type name is too verbose.

>>>> Index: sys/ufs/ufs/dinode.h
>>>> ===================================================================
>>>> --- sys/ufs/ufs/dinode.h        (revision 217362)
>>>> +++ sys/ufs/ufs/dinode.h        (working copy)
>>>> @@ -140,7 +140,7 @@
>>>>        int32_t         di_birthnsec;   /*  76: Inode creation time. */
>>>>        int32_t         di_gen;         /*  80: Generation number. */
>>>>        u_int32_t       di_kernflags;   /*  84: Kernel flags. */
>>>> -       u_int32_t       di_flags;       /*  88: Status flags (chflags).
>>>> */
>>>> +       fflags_t        di_flags;       /*  88: Status flags (chflags).
>>>> */

Unreadble due to MUA mangling of spaces.

>>> Please revert for the same reason.
>>
>> Indeed.
>
> I'm still confused as to why silent type [width] parity issues with
> UFS is a good thing, especially when this change serves to unite
> everything under the same bit-width type (well, minus the syscalls,
> but that was done because it's easier to deal with function call
> breakage as opposed to data interpretation breakage when reading off
> the disk.

ffs can only hold 32-bit flags no matter what fflags_t is.  If you
want to change this in ffs, then you have to write ffs3, and there are
more important things to change in it then.  OTOH, changing fflags_t
wont affect ffs, even if some new bits in it are actually used.  ffs
just won't understand these bits and will return an error for attempts
to set them, at least when it is fixed to not silently ignore them.
(Actually using old bits is more of a problem.  Users may already be
using them for storing data :-)).

Bruce