svn commit: r201773 - head/sys/fs/tmpfs

[Bruce Evans]

On Fri, 8 Jan 2010, Jaakko Heinonen wrote:

> Author: jh
> Date: Fri Jan  8 07:57:43 2010
> New Revision: 201773
> URL: http://svn.freebsd.org/changeset/base/201773
>
> Log:
>  - Change the type of size_max to u_quad_t because its value is converted
>    with vfs_scanopt(9) using the "%qu" format string.

%qu should never be used, since there are no correct uses of u_quad_t.
However, there seems to be nothing much better, due to API design and
implementation bugs.  (%lu and u_long would work, but is a bit fragile.
I thought at first that size_max was a page count, so that it should
have type u_int like all page counts in vm, and thus easily scanned using
%u.)

>  - Limit the maximum value of size_max to (SIZE_MAX - PAGE_SIZE) to
>    prevent overflow in howmany() macro.

Overflow still occurs, since `pages' remains "infinite" when howmany()
would overflow.

> Modified: head/sys/fs/tmpfs/tmpfs_vfsops.c
> ==============================================================================
> --- head/sys/fs/tmpfs/tmpfs_vfsops.c	Fri Jan  8 07:42:01 2010	(r201772)
> +++ head/sys/fs/tmpfs/tmpfs_vfsops.c	Fri Jan  8 07:57:43 2010	(r201773)
> @@ -185,8 +185,8 @@ tmpfs_mount(struct mount *mp)
> 	ino_t nodes;
> 	int error;
> 	/* Size counters. */
> -	ino_t	nodes_max;

Indentation error in old version.  Local variables are not indented
in KNF.  The local variables before here are normally indented.

> -	size_t	size_max;

Indentation error, as above.

> +	ino_t		nodes_max;

Even larger indentation error in new version.  Now the variable name
doesn't even line up with the other 1-fold-excessively indented variables.

There are also many type errors associated with this variable.  ino_t is
a reasonable type for a limit on the number of inodes (unless you want to
support inodes numbered 0 to the maximum -- then you need a larger type
to hold the maximum plus 1), but there is no scanf letter for ino_t and
thus no correct way to scan an ino_t in vfs_scanopt().  The current
incorrect way is to use %d.  Since ino_t happens to have type uint32_t
and u_int happens to have type uint32_t on all supported machines, %u
would work but %d gives undefined behaviour when the value exceeds INT_MAX.

The simplest fix is to use u_int and %u.

> +	u_quad_t	size_max;

Even larger indentation error, as above.

The technically correct type to use here is vm_size_t, but this cannot
be used either since it cannot be scanned, as for ino_t.  vm types and
size_t are also more machine-dependent than ino_t, so u_int would not
work here.

There is a scanf letter for size_t in scanf(3); however, this is not
supported by scanf(9) (only printf(9) supports the C99 letters).
scanf(9) does support u_quad_t, though this depends on undefined or
implementation-defined behaviour when the value exceeds QUAD_MAX.
scanf also doesn't support the most general type and easist to use
correctly type that could be used here -- uintmax_t.  Most uses of
u_quad_t and %qu are for when the user really wants the largest integral
type but hasn't caught up with C99.

The bugs in scanf(9) should be fixed by removing scanf(9) from the kernel
and only using strto*() and something special for string scanning in the
couple of places it is needed.  It is unfortunately too late to remove
scanf(3).  scanf is unusable for almost all purposes since it gives
undefined behaviour on overflow, unlike strto*().

kenv and its uses have similar type problems, but at least it doesn't
use scanf.  It hasn't caught up with C99 either, and uses the quad
mistake via strtoq() for all numeric input, with null error handling
to get implementation-defined instead of undefined behaviour on
overflow (large values get truncated to QUAD_MAX, which is even less
than ULONG_MAX on 64-bit machines).

>
> 	/* Root node attributes. */
> 	uid_t	root_uid;

This and the remaining loal variables have typedefed types, so they too
cannot be scanned correctly.

> @@ -239,7 +239,7 @@ tmpfs_mount(struct mount *mp)
> 	 * allowed to use, based on the maximum size the user passed in
> 	 * the mount structure.  A value of zero is treated as if the
> 	 * maximum available space was requested. */

The comment doesn't match the code.  Values between 1 and PAGE_SIZE-1
are also treated as if the maximum available space were requested.

The comment is not formatted in KNF (leading and trailing delimiters).

> -	if (size_max < PAGE_SIZE || size_max >= SIZE_MAX)
> +	if (size_max < PAGE_SIZE || size_max > (SIZE_MAX - PAGE_SIZE))

Style bug: extra ().

> 		pages = SIZE_MAX;

This is the "infinite" setting of `pages'

> 	else
> 		pages = howmany(size_max, PAGE_SIZE);
>

Page counts are normally divided by PAGE_SIZE like they are here, so they
are far from infinite.

tmpfs consistently uses the bogus type size_t for page counts.  This at
least allows it to represent the above "infinite" value.  But using this
value risks overflow.  Overflow first occurs a few lines later:

% 	if (nodes_max <= 3)
% 		nodes = 3 + pages * PAGE_SIZE / 1024;
   			    ^^^^^^^^^^^^^^^^^
% 	else
% 		nodes = nodes_max;

Elsewhere, `pages' seems to only be used as a limit, so overflow doesn't
occur but the limit has no effect, unlike the limit given by a size_max
of SIZE_MAX - PAGE_SIZE.  However, this limit probably has no effect
either, since it is impossible to use use all pages except 1 in an address
space for tmpfs on any supported machine.

Apart from overflow problems, SIZE_MAX is a better limit on the number
of pages than howmany(SIZE_MAX - PAGE_SIZE, PAGE_SIZE), since there
might be more pages than can be mapped.  vm already supports this (for
PAE).  It uses u_int for all page counts and APIs (struct vmmeter and
too many sysctls).  This barely works for PAE, since 2^32 pages is
4096 times as many as can be mapped and no one can afford that many
and/or PAE hardware is limited to that many.

Bruce