On 27 January 2016 at 14:21, Martin Natano <[email protected]> wrote: > 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. >
Fair enough. I didn't realize that there was an upper bound on the value of usec, but it makes sense. > >> 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. > I see now. Thanks for an explanation. >> 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
