cvs commit: src/sys/ufs/ffs ffs_alloc.c

[Bruce Evans]

On Mon, 1 Oct 2007, Bruce Evans wrote:

> On Mon, 1 Oct 2007, Bjoern A. Zeeb wrote:
>
>> On Mon, 1 Oct 2007, Bruce Evans wrote:
>> 
>>> On Mon, 1 Oct 2007, Bjoern A. Zeeb wrote:
>>>>> s/fis/fix/
>>>> 
>>>> yes it should. I closed the PR, See the comment there.
>>> 
>>> s/fix/work around/
>>> 
>>> The bug is in newfs and tunefs permitting garbage parameters, so it cannot
>>> be fixed in ffs_alloc.c.
>> 
>> No matter what iput the kernel gets and from where, it MUST NOT (or at
>> least SHOULD not;) panic unless explicitly request by KASSERT/panic/..
>
> Not quite true.  There are hundreds or thousands of sysctls that can
> be used to set critical parameters to garbage which will cause a panic.
> Bounds checking for sysctl parameters is almost completely absent, and
> this is sometimes useful for investigating the limits of useful parameters
> without rebuilding the kernel.  Also, a division by zero trap is preferable
> to a panic since it is restartable.
>
>> So this commit fixes a DIV0 bug in the kernel.
>> 
>> Of course you are right, that the values should be checked by the tools
>> that we have in the tree so that this problem would not occur.
>> We could even check if the values given make sense at all, but that still
>> is a different story to a kernel panic.
>
> You deleted the part where I said where the fix belongs in the kernel
> (next to related fixups).
>
> ffs does almost no runtime checking by design.  It depends on fsck or
> mount-time fixups doing all the necessary checking and fixups, so that
> the main code can be simpler and faster.  New code shouldn't do things
> differently.

Here is a correct fix for the kernel part.

%%%
Index: ffs_vfsops.c
===================================================================
RCS file: /home/ncvs/src/sys/ufs/ffs/ffs_vfsops.c,v
retrieving revision 1.233
diff -u -1 -r1.233 ffs_vfsops.c
--- ffs_vfsops.c	16 Jun 2004 09:47:25 -0000	1.233
+++ ffs_vfsops.c	1 Oct 2007 12:41:25 -0000
@@ -885,3 +937,3 @@
  	}
-	/* Compatibility for old filesystems */
+	/* Compatibility and/or fixups for old filesystems/utilities. */
  	if (fs->fs_avgfilesize <= 0)
@@ -890,2 +942,4 @@
  		fs->fs_avgfpdir = AFPDIR;
+	if ((int64_t)fs->fs_avgfilesize * fs->fs_avgfpdir > INT32_MAX)
+		fs->fs_avgfpdir = INT32_MAX / fs->fs_avgfilesize;
  	if (bigcgs) {
%%%

This lets fs_avgfpdir be set to any value between 1 and INT32_MAX
inclusive.  Then, if the product would overflow, fs_avgfpdir is clamped
to the maximum value that doesn't cause overflow of the product.

In light but lengthy testing of this (with a "normal" and small tree
from /usr/src or /usr only, but with many parameters tried), I couldn't
find any useful use for having these parameters.  All parameters tried
except silly ones like 1 for one and INT32_MAX for the other gave only
minor differences, while the silly ones gave a significant pessimization.
In particular, setting the parameters to the current averages for the
tree made no significant difference (IIRC, setting the parameters to
several times the averages is better, and the default parameters give
this for /usr).

The parameters might be useful for very large trees with all files and
directories having nearly the same size, but testing would be lengthier.

Bruce