On Wed, Jan 27, 2016 at 12:27:46PM +0100, Mike Belopuhov wrote:
> On 27 January 2016 at 09:52, Martin Natano <[email protected]> wrote:
> > In ufs, the calculation of i_modrev can produce signed overflow on 32
> > bit architectures (found on i386). The tv.tv_usec * 4294 calculation is
> > designed to move the microseconds part of a struct timeval to the upper
> > bits of an unsigned(!) 32 bit value to make room for simple i_modrev
> > increments, but the calculation is performed signed, causing overflow.
> > The diff below gets rid of the overflow by casting to unsigned first.
> >
> > While there I replaced the union _qcvt/SETHIGH/SETLOW dance with simple
> > bitshift operations.
> >
>
> NetBSD have fixed this a while ago with a different construct
> that produces different results compared to your code:
>
> http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/ufs/ufs/ufs_vnops.c.diff?r1=1.105&r2=1.107
>
> Basically the difference is that you propagate carry bits into the upper
> part while neither current code nor NetBSD code do. Can you please
> post an example you've used to test this?
There won't be any carry. tv_usec contains the microsecond part of a
timestamp, so the expected range is [0, 1,000,000). 999,999 * 4294 is
less than 2^32.
natano@watschnbaum:test$ cat test.c
#include <sys/time.h>
#include <stdio.h>
int
main(void)
{
struct timeval tv;
u_quad_t modrev;
tv.tv_sec = 0x42424242;
tv.tv_usec = 999999;
modrev = (u_quad_t)tv.tv_sec << 32;
printf("0x%llx\n", modrev);
modrev |= (u_quad_t)tv.tv_usec * 4294;
printf("0x%llx\n", modrev);
modrev++;
modrev++;
modrev++;
modrev++;
printf("0x%llx\n", modrev);
return 0;
}
natano@watschnbaum:test$ cc test.c
natano@watschnbaum:test$ ./a.out
0x4242424200000000
0x42424242fff12cba
0x42424242fff12cbe
natano@watschnbaum:test$
> But curiosity took the better of me. Why do we have to multiply
> microseconds by 4294? Does this number come from 0xffffffff/(1000*1000)?
Yes, 4294 is the largest factor for which the assumption that
tv_usec * factor < 2^32 holds true.
> FreeBSD did something completely different here:
>
> http://bxr.su/FreeBSD/sys/ufs/ffs/ffs_softdep.c#2681
> http://bxr.su/FreeBSD/sys/fs/ext2fs/ext2_vnops.c#1515
> http://bxr.su/FreeBSD/sys/kern/vfs_subr.c#4317
>
> Since it's an NFS revision thing the lower part can be anything,
> including as simple as (usec &~ 0xffff) to make some room for the
> counter... I don't think propagating carry bits into the upper
> part makes much sense.
I don't think (usec &~ 0xffff) would be a good idea. The purpose of
multiplying usec is to make room, such that it is unlikely that
i_modrev will jump backwards when it is set again after a couple of
i_modrev++. Also, &~ 0xffff would just waste the upper 12 bits of the
lower half of i_modrev (always zero).
cheers,
natano
