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? But curiosity took the better of me. Why do we have to multiply microseconds by 4294? Does this number come from 0xffffffff/(1000*1000)? 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.
