cvs commit: src/sys/ufs/ufs ufs_quota.c

[Mike Pritchard]

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)