Inconsistency and bug with *chflags functions

[Gleb Kurtsou]

On (17/01/2011 20:47), Garrett Cooper wrote:
> 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 :).
It's unlikely to be documented. You can't just change syscall
definition, you should add a new one and mark old COMPAT8 (in this
particular case). Implement kernel level wrappers, etc. Symbol
versioning within libc is rather different story, I'd suggest you take a
look at my patch it should be straightforward.

> 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.
Yes, they are broken, but due to amd64 being little ending and flag
argument being bit mask, I think both chflags and fchaflags actually
work, although it should also be fixed, imho.  Wrapper won't be needed
for the new version (with fflags_t).

> > 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.
32 bits should be good enough. Extended attributes are getting more
widely used nowadays.

> > 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?
It's common practice, this way fflags_t could be changed to different
type in the future, but binary compatibility (dump/restore protocol
here) will be preserved. As it happened with uid_t, pid_t, off_t, or
taking place with ino_t and nlink_t.

No typedefs are used in struct ufs2_dinode or protocols/dumprestore.h
(ino_t is being fixed).

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