Inconsistency and bug with *chflags functions

[Garrett Cooper]

Hi Gleb!

On Mon, Jan 17, 2011 at 8:14 PM, Gleb Kurtsou <gleb.kurtsou at gmail.com> 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
>> 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.
>> 3. There's a sizing/signedness discrepancy in a few other structures
>> with fixed-width data types (uint32_t).
>>
>> Solution:
>> 1. I opted to convert fflags_t to uint32_t and convert all references
>> which did direct conversions to and from st_flags to fflags_t to avoid
>> 32-bit / 64-bit biarch issues. I downgraded it to uint32_t as we're no
>> where near the limit for the number of usable flags to *chflags(2).
>> 2. *chflags now uses fflags_t instead of int/unsigned long.
>> 3. c_file_flags in dumprestore.h was changed to be in sync with st_flags.
>> 4. di_flags in ufs/ufs/dinode.h was changed to be in sync with st_flags.
>>
>> 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).
>> 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).
>
> Hi Garrett,
>
> You've changed syscalls definitions but didn't add backward
> compatibility shims for kernel, libc and freebsd32.

Please enlighten me on how to do that (docs are fine) and I'll go
ahead and add the compatibility shims. I'm not a committer so I don't
yet know these things :).

The only thing is that the old behavior on biarch platforms like amd64
with COMPAT_FREEBSD32, etc was broken so I don't know if there's any
value in emulating broken behavior. Straight 32-bit and straight
64-bit archs didn't have this problem.

> 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

Hmmm... I'll take a look at that. FWIW, I was avoiding bumping
fflags_t to uint64_t because it would change alignment of kernel
structures, but I'll take your advice in kind for resolving this
problem. There are other ways to avoid this by downcasting (or just
casting between types in general), which is fine today but I thought
this solution was simple because it was easier to root out affected
areas. Either way, I know that this method is a bit of a damned if you
do, damned if you don't because it restricts us from future additions
if fflags_t goes exceeds 2^32, but that's still the case today if the
field changes in the on-disk structures anyhow.

> Please find comments to the patch below.

Ok :)!

>>     I'm running make universe just to see if it will barf in the
>> build, but would someone please review this change (I made the change
>> as minimal as possible for ease of review) and provide feedback?

...

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

fflags_t in this case is uint32_t, so this is a drop-in-place
replacement -- or are you arguing the type more than the width of the
type?

>>               int32_t c_spare5[2];        /* old blocks, generation number */

...

>> -     u_int32_t       di_flags;       /*  88: Status flags (chflags). */
>> +     fflags_t        di_flags;       /*  88: Status flags (chflags). */
> Please revert for the same reason.

My response here is the same as above.

Thanks!
-Garrett