Subject: ffs fs_flags superblock update sign extension bug
To: None <tech-kern@netbsd.org>
From: Darrin B. Jewell <dbj@netbsd.org>
List: tech-kern
Date: 09/28/2003 18:20:30
Revision 1.120 of ffs_vfsops dated 2003/09/13 introduced a bug in the
superblock update code that results in fs_flags having a value of
0x7fffff00 after the update.

Specifically, line 995 of ffs_vfsops.c is this:
   fs->fs_flags |= (fs->fs_old_flags & ~FS_FLAGS_UPDATED);

bug since fs_old_flags was int8_t and fs_flags is int32_t, and
FS_FLAGS_UPDATED is 0x80, the fs_old_flags value gets sign extended
before it gets copied into fs_flags.  The high bit gets later stripped
by FS_INTERNAL being forced to zero.  (This behavior also makes me
nervous, but is tangental to the issue addressed here.)

Although that line could be updated to remove this problem,
I think it is a better fix to declare these fields uint8_t and
uint32_t as per the patch included below.

Filesystems that were upgraded by -current kernels in the intermediate
time will have bad values of fs_flags which will cause a problem which
will not be exhibited until some future date when those flag fields
are used.  If desired, I can post a simple program which can go in and
unset these flags.  Since the lifespan of the incorrect code was
small, I do not recommend implementing compatibility in the kernel.

Please review the following patch.  I would like to commit
it shortly.

Thanks,
Darrin

Index: sys/ufs/ffs/fs.h
===================================================================
RCS file: /cvsroot/src/sys/ufs/ffs/fs.h,v
retrieving revision 1.34
diff -u -r1.34 fs.h
--- sys/ufs/ffs/fs.h    21 Aug 2003 14:41:00 -0000      1.34
+++ sys/ufs/ffs/fs.h    28 Sep 2003 21:57:02 -0000
@@ -272,7 +272,7 @@
        int8_t   fs_fmod;               /* super block modified flag */
        int8_t   fs_clean;              /* file system is clean flag */
        int8_t   fs_ronly;              /* mounted read-only flag */
-       int8_t   fs_old_flags;          /* see FS_ flags below */
+       uint8_t  fs_old_flags;          /* see FS_ flags below */
        u_char   fs_fsmnt[MAXMNTLEN];   /* name mounted on */
        u_char   fs_volname[MAXVOLLEN]; /* volume name */
        uint64_t fs_swuid;              /* system-wide uid */
@@ -300,7 +300,7 @@
        int32_t  fs_avgfpdir;           /* expected # of files per directory */
        int32_t  fs_save_cgsize;        /* save real cg size to use fs_bsize */
        int32_t  fs_sparecon32[26];     /* reserved for future constants */
-       int32_t  fs_flags;              /* see FS_ flags below */
+       uint32_t fs_flags;              /* see FS_ flags below */
        int32_t  fs_contigsumsize;      /* size of cluster summary array */ 
        int32_t  fs_maxsymlinklen;      /* max length of an internal symlink */
        int32_t  fs_old_inodefmt;       /* format of on-disk inodes */