From owner-cvs-all@FreeBSD.ORG Fri Feb 2 01:06:34 2007 Return-Path: X-Original-To: cvs-all@freebsd.org Delivered-To: cvs-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 3DA6016A400; Fri, 2 Feb 2007 01:06:34 +0000 (UTC) (envelope-from mpp@mppsystems.com) Received: from mail.mppsystems.com (mppsystems.com [208.210.148.205]) by mx1.freebsd.org (Postfix) with ESMTP id DE5EF13C461; Fri, 2 Feb 2007 01:06:33 +0000 (UTC) (envelope-from mpp@mppsystems.com) Received: by mail.mppsystems.com (Postfix, from userid 1000) id 9CA2F115E1; Thu, 1 Feb 2007 19:06:32 -0600 (CST) Date: Thu, 1 Feb 2007 19:06:32 -0600 From: Mike Pritchard To: Bruce Evans Message-ID: <20070202010632.GA8848@mail.mppsystems.com> References: <200702010101.l1111v4H029618@repoman.freebsd.org> <20070202015415.G924@delplex.bde.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20070202015415.G924@delplex.bde.org> User-Agent: mutt-ng/devel-r804 (FreeBSD) Cc: cvs-src@freebsd.org, Mike Pritchard , src-committers@freebsd.org, cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/ufs/ufs ufs_quota.c X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 Feb 2007 01:06:34 -0000 On Fri, Feb 02, 2007 at 03:34:15AM +1100, Bruce Evans wrote: > On Thu, 1 Feb 2007, Mike Pritchard wrote: > > >mpp 2007-02-01 01:01:57 UTC > > > > FreeBSD src repository > > > > Modified files: > > sys/ufs/ufs ufs_quota.c > > Log: > > Disallow negative UIDs when processing quotactl options. > > Er, uids are unsigned, so they cannot be negative. There are 1 or 2 utilities out there that will display a uid/gid of UINT_MAX-2 as -2 :) > The function actually takes a u_long id and now uses a bogus cast > ((int)id) to check for "negative" values. The correct check is > something like (id <= QUOTA_ID_MAX). ((int)id) would work to restrict > the id to <= INT_MAX due to previous bogusness (*), but I don't see > the point of that. If ints are 32-bits then id = INT_MAX gives an > offset that is about half as huge as id = UINT_MAX (64G?), and if > ints are 64 bits then id = INT_MAX and id = UINT_MAX both give > physically impossible offsets. Is the problem with negative ids > mainly that they are standard for nfs without maproot? The basic problem is that the quota file will grow in size to sizeof(struct dqblk) * highest_id. sizeof(struct dqblk) = 32. At system startup quotacheck has to read the entire file, which if the highest_id is extremely large (as a file copied from an nfs file system without maproot might have, or from some type of archive file that may have the id = -2) The data file was also being incorrectly truncated to a maximum size of 2^32 bytes due to some incorrect casting when writing out the dqblock data. A high id value of 2^24 (16.7 million) allows quotacheck to run in a "reasonable" (1 min per quota file) amount of time at system startup. An high id value of 2^25 (33 million) bumps it up to 2 mins per quota file. I'd be happy to change the code to use a new QUOTA_ID_MAX value. > (*) The id started as "int id" in the user API, but got punned in > several stages to "uid_t uid" in the kernel quotactl() and > vfs_quotactl_t. ufs_quotactl() now starts by punning it half way back > (uid_t id). As a result of this, any overflow for converting uids and > gids to ids has already occurred at the syscall boundary, and most > overflows in the kernel consist of overflowing back to the original > value (uid_t -> int -> uid_t kernel) and back to the already-overflowed > value (uid_t -> int -> uid_t -> int). Since uid_t is uint32_t and int > is 32 bits 2's complement on all supported systems, the only overflows > of ids that occur now are from huge positive to negative and back. > However, if the id is initially -1, then all overflows are in the > kernel (-1 -> u/gid -> ...). Also, an u/gid 0xffffffff overflows to > -1 at the syscall boundary and then gets corrrupted to the real u/gid > in the kernel. > > The user API is only correct if ints have more bits that u/gids, so > that conversion of u/gids to ints doesn't overflow and -1 isn't in-band. > It was correct before 4.4BSD on systems with >= 17-bit ints, since > u/gid_t was only 16 bits until then. > > The patch has some style bugs (misformatted comment, unlike the other > XXX comments about "negative" uids). Found it...will get it fixed. > Bruce -- Mike Pritchard mpp @ mppsystems.com or mpp @ FreeBSD.org "If tyranny and oppression come to this land, it will be in the guise of fighting a foreign enemy." - James Madison (1787)