Inconsistency and bug with *chflags functions

[Bruce Evans]

On Tue, 18 Jan 2011, Gleb Kurtsou wrote:

> On (17/01/2011 16:32), Garrett Cooper wrote:
>> Hi FS folks,
>>
>>     Originally I did this to determine why lchflags had an int
>> argument and chflags/fchflags had unsigned long arguments, and I ran

chflags/fchflags() have unsigned long args since these were wrong in
4.4BSD-Lite, and they still have unsigned long args since it is easier
not to disturb sleeping bugs.

lchflags() has an int arg since it was implemented in FreeBSD much later
than the others, and the type error in the others was apparently
intentionally avoided.  (I thought the non-error for lchflags() was
copied from NetBSD, but NetBSD seems to have always had u_long for all
three).

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.

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

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

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

>> Index: include/protocols/dumprestore.h
>> ===================================================================
>> --- include/protocols/dumprestore.h	(revision 217362)
>> +++ include/protocols/dumprestore.h	(working copy)
>> @@ -95,7 +95,7 @@
>>  		int64_t	c_mtime;	    /* last modified time, seconds */
>>  		int32_t	c_extsize;	    /* external attribute size */
>>  		int32_t	c_spare4[6];	    /* old block pointers */
>> -		u_int32_t c_file_flags;	    /* status flags (chflags) */
>> +		fflags_t c_file_flags;	    /* status flags (chflags) */
> 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.

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

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

>> @@ -2695,8 +2695,8 @@
>>   */
>>  #ifndef _SYS_SYSPROTO_H_
>>  struct chflags_args {
>> -	char	*path;
>> -	int	flags;
>> +	char		*path;
>> +	fflags_t	flags;
>>  };
>>  #endif
>>  int

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

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

Bruce