Date: Sat, 9 Jan 2010 06:46:07 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Jaakko Heinonen <jh@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r201773 - head/sys/fs/tmpfs Message-ID: <20100109051536.R57595@delplex.bde.org> In-Reply-To: <201001080757.o087vhrr009799@svn.freebsd.org> References: <201001080757.o087vhrr009799@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100109051536.R57595>