Inconsistency and bug with *chflags functions

[Garrett Cooper]

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.  Thus it is an error to pretend that the
> syscalls take args of other types.  Larger types would be handled
> correctly if syscalls.master actually pseudo-declared ths syscalls
> correctly, but it still says int as in 4.4BSD-Lite for chflags/fchflags().
> FreeBSD generates args structs containing the actual types from the
> data in syscalls.master.  If the latter is correct, then the correct
> conversions will be done without further type puns, by normal C
> conversions, e.g., setfflags(..., uap->flag), where `flags' has whatever
> type it has and the function call converts to int.  Plain ints work
> correctly except possibly for invalid args since the sign bit is not
> used by any supported flags value.

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

>>> into an interesting mini-project (by accident) after I found a few
>>> bugs lurking in the vfs_syscalls.c layer of the kernel.
>>>
>>>    So here's the deal...
>>>
>>> Problems:
>>> 1. chflags/fchflags claim that the flags argument is an unsigned long,
>>> whereas lchflags claims it's an int. This is inconsistent.
>>> 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.

>>> 3. There's a sizing/signedness discrepancy in a few other structures
>>> with fixed-width data types (uint32_t).
>
> There are zillions of these, and with missing consts.  Not a single
> vfs syscall supports its path arg being declared as const in
> syscalls.master.  Adding consts there would cause more problems than
> fixing type mismatches like the ones for fflags, since lower layers
> are almost perfectly devoid of "const", and "downcasting" for "const"
> involves removing it, and gcc would complain about that although it
> is even safer than downcasting to int for vfs syscalls.
>
>>> Solution:
>>> ...
>>>
>>> Compatibility:
>>> 1. NetBSD uses unsigned long in their chflags calls (both in kernel
>>> and userland) so they're more consistent than we are by not having
>>> mixed flags calling convention like us, but uses uint32_t in their
>>> data structures (like we do), so they have a 32-bit/64-bit biarch bug
>>> (again like we do).
>
> No, NetBSD has data in syscalls.master that actually matches the arg
> types for the user API for *chflags() (it even has "const" for open()),
> so, it does all the downcasting correctly and doesn't have any serious
> 64-bit problems.  At least in 2005, its set_fflags() is spelled
> change_flags() and takes a u_long flags, and this seems to reach
> VOP_SETATTR() correctly via "u_long va_flags;" (even 4.4BSD-Lite and
> FreeBSD have "u_long va_flags").  Even if the top flags bits were
> clipped early as in FreeBSD, there would only be minor problems from
> not detecting garbage in these bits (including the ability of applications
> to invoke undefined behaviour in the kernel, by passing garbage top
> bits that cause integer overflow on blind conversion to int).  Everone
> uses u_int32_t for st_flags, except u_int32_t is spelled fflags_t in
> FreeBSD, but this doesn't cause annoy problems since it is large enough
> and read-only.

Hmmm...

>>> 2. OpenBSD is using unsigned int, so I assume that their kernel layer
>>> is also using unsigned int (I am basing this purely off the manpage as
>>> I haven't looked at their sources, but I could be wrong).
>
> Was consistently u_long in 2005, except where it must be spelled
> "unsigned long" for namespace reasons.
>
>> You've changed syscalls definitions but didn't add backward
>> compatibility shims for kernel, libc and freebsd32.
>
> Probably not needed, especially of garbage in top bits is not checked
> for (and checking that is not really needed.  There are SF_SETTABLE
> and UF_SETTABLE masks which would cause garbage top bits to be silently
> ignored if they are masked before checking).  Bit the patch doesn't
> seem to touch syscalls.master.  That risks increasing the problem
> unless everything is changed to int to match there.  Even if you change
> like that, old 64-bit binaries still passing u_long will continue to
> work, since there are no big-endian ones and ints are padded to 64
> bits in the ABI (the big-endian case breaks by padding at the wrong
> end).
>
>> I'm working on changing ino_t to 64 bits and nlink_t to 32 bits, we
>> could join the effort, as projects look related. My old patch is
>> available here:
>> https://github.com/downloads/glk/freebsd-ino64/freebsd-ino64-patch.tgz
>
> Enlarging sizes is harder :-).

...

>> 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.

>>> 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).

>>> 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 :(?


...

> Now has excessive indentation.  This was consistently broken with
> syscalls.master.  Similarly for others.

Eh...? Wouldn't it be worse if fflags_t

>>> 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).
>>> */
>>
>> 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.

Thanks,
-Garrett