From owner-cvs-src@FreeBSD.ORG Mon Oct 22 10:45:56 2007 Return-Path: Delivered-To: cvs-src@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 3A69516A417; Mon, 22 Oct 2007 10:45:56 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from fallbackmx02.syd.optusnet.com.au (fallbackmx02.syd.optusnet.com.au [211.29.133.72]) by mx1.freebsd.org (Postfix) with ESMTP id 908E613C4A8; Mon, 22 Oct 2007 10:45:55 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail12.syd.optusnet.com.au (mail12.syd.optusnet.com.au [211.29.132.193]) by fallbackmx02.syd.optusnet.com.au (8.12.11.20060308/8.12.11) with ESMTP id l9L77XJ7002764; Sun, 21 Oct 2007 17:13:49 +1000 Received: from besplex.bde.org (c220-239-235-248.carlnfd3.nsw.optusnet.com.au [220.239.235.248]) by mail12.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id l9L3jvgv000585 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 21 Oct 2007 13:46:06 +1000 Date: Sun, 21 Oct 2007 13:45:57 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Bruce Evans In-Reply-To: <20071001212813.X2796@besplex.bde.org> Message-ID: <20071021131632.J1869@besplex.bde.org> References: <200709241339.l8ODd6k6059694@repoman.freebsd.org> <47002674.8050707@tomjudge.com> <47002E2F.3030306@tomjudge.com> <20071001090547.H85753@maildrop.int.zabbadoz.net> <20071001201359.M2557@besplex.bde.org> <20071001110057.V85753@maildrop.int.zabbadoz.net> <20071001212813.X2796@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: cvs-src@FreeBSD.org, "Bjoern A. Zeeb" , cvs-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: cvs commit: src/sys/ufs/ffs ffs_alloc.c X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 22 Oct 2007 10:45:56 -0000 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