>Number:         28134
>Category:       kern
>Synopsis:       invalid signed left-shifts in ffs_vfsops.c
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Nov 09 11:05:54 +0000 2004
>Originator:     David A. Holland <dholland@eecs.harvard.edu>
>Release:        NetBSD 2.0G (actually -20041029)
>Organization:
   Harvard EECS
>Environment:
	
	
System: NetBSD alicante 2.0G NetBSD 2.0G (ALICANTE) #9: Fri Aug 27 17:49:09 EDT 2004 dholland@alicante:/usr/src/sys/arch/i386/compile/ALICANTE i386
Architecture: i386
Machine: i386
>Description:
	In ffs_mount() the "please fsck(8)" message includes a
	printout of the value of ->fs_clean. It's printed with %x;
	however, fs_clean is int8_t. Thus, if the high bit gets set
	the printed value is sign-extended. This is merely untidy.

	However, the way the high bit gets set is via the left shifts
	in ffs_mount() and ffs_mountfs(). Shifting into the sign bit
	is formally undefined and a Bad Thing.

	As best I can tell the best solution is to make fs_clean
	unsigned (uint8_t); it's only used in a few places. 

	However, those places include fsck and clri and other userland
	tools. And the on-disk superblock. So I'm not sure if such a
	change is really a good idea, even though it won't break
	binary compatibility. So I'm just filing the PR and making
	someone else decide. :-)

	This discussion (and the patches below) apply to version
	1.44 of sys/ufs/ffs/fs.h and version 1.155 of
	sys/ufs/ffs/ffs_vfsops.c, both of which are still current as
	of this writing.

	I also looked at

		sbin/clri/clri.c		1.18
		sbin/fsck_ffs/setup.c		1.73
		sbin/fsck_ffs/utilities.c	1.47
		sbin/fsdb/fsdb.c		1.29
		sbin/newfs/mkfs.c		1.88
		usr.sbin/dumpfs/dumpfs.c	1.47
		usr.sbin/makefs/ffs/mkfs.c	1.20

	but only with grep so it's conceivable I missed something.
	These are from -20041029, even though my running kernel is
	older.

>How-To-Repeat:

	If I understand correctly, produce an unclean filesystem that
	"was clean" and mount it a few more times without fscking; you
	get

	   /dev/wd0k: file system not clean (fs_clean=4); please fsck(8)
	   /dev/wd0k: lost blocks 0 files 0
	   /dev/wd0k: file system not clean (fs_clean=8); please fsck(8)
	   /dev/wd0k: lost blocks 0 files 0
	   /dev/wd0k: file system not clean (fs_clean=10); please fsck(8)
	   /dev/wd0k: lost blocks 0 files 0
	   /dev/wd0k: file system not clean (fs_clean=20); please fsck(8)
	   /dev/wd0k: lost blocks 0 files 0
	   /dev/wd0k: file system not clean (fs_clean=40); please fsck(8)
	   /dev/wd0k: lost blocks 0 files 0
	   /dev/wd0k: file system not clean (fs_clean=ffffff80); please fsck(8)
	   /dev/wd0k: lost blocks 0 files 0
	   /dev/wd0k: file system not clean (fs_clean=0); please fsck(8)
	   /dev/wd0k: lost blocks 0 files 0

	(This was observed by someone other than me, so yes it can
	really happen but I'm not absolutely certain of the mechanism.)

>Fix:

This patch makes fs_clean unsigned:

Index: fs.h
===================================================================
RCS file: /cvsroot/src/sys/ufs/ffs/fs.h,v
retrieving revision 1.44
diff -u -r1.44 fs.h
--- fs.h	25 May 2004 14:54:59 -0000	1.44
+++ fs.h	9 Nov 2004 09:23:05 -0000
@@ -310,7 +310,7 @@
 	struct	csum fs_old_cstotal;	/* cylinder summary information */
 /* these fields are cleared at mount time */
 	int8_t	 fs_fmod;		/* super block modified flag */
-	int8_t	 fs_clean;		/* file system is clean flag */
+	uint8_t	 fs_clean;		/* file system is clean flag */
 	int8_t	 fs_ronly;		/* mounted read-only flag */
 	uint8_t	 fs_old_flags;		/* see FS_ flags below */
 	u_char	 fs_fsmnt[MAXMNTLEN];	/* name mounted on */


I haven't actually tested this patch, but I've applied it to my tree,
so since I've marked this PR low priority I probably will have by the
time anyone looks at it. :-)

I don't see anything (in the kernel or in userland either) that
requires any adjustment to match. It's printed with %d in
sbin/fsck_ffs/setup.c, but that's harmless.


However, if that change is no good, this patch suppresses the
sign-extended printout:

Index: ffs_vfsops.c
===================================================================
RCS file: /cvsroot/src/sys/ufs/ffs/ffs_vfsops.c,v
retrieving revision 1.155
diff -u -r1.155 ffs_vfsops.c
--- ffs_vfsops.c	21 Sep 2004 03:10:36 -0000	1.155
+++ ffs_vfsops.c	9 Nov 2004 08:28:47 -0000
@@ -428,7 +428,8 @@
 			fs->fs_time = time.tv_sec;
 		else {
 			printf("%s: file system not clean (fs_clean=%x); please fsck(8)\n",
-			    mp->mnt_stat.f_mntfromname, fs->fs_clean);
+			    mp->mnt_stat.f_mntfromname, 
+			    (unsigned)(u_int8_t)fs->fs_clean);
 			printf("%s: lost blocks %" PRId64 " files %d\n",
 			    mp->mnt_stat.f_mntfromname, fs->fs_pendingblocks,
 			    fs->fs_pendinginodes);


I haven't, however, written a patch to avoid the undefined shifts
without making fs_clean unsigned.

>Unformatted: