Any OKs or objections to this diff? This looks solid to me, FWIW.
On Wed, Jan 27, 2016 at 09:52 +0100, Martin Natano 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.
>
> Index: ufs/ext2fs/ext2fs_subr.c
> ===================================================================
> RCS file: /cvs/src/sys/ufs/ext2fs/ext2fs_subr.c,v
> retrieving revision 1.33
> diff -u -p -u -r1.33 ext2fs_subr.c
> --- ufs/ext2fs/ext2fs_subr.c 14 Mar 2015 03:38:52 -0000 1.33
> +++ ufs/ext2fs/ext2fs_subr.c 27 Jan 2016 08:26:05 -0000
> @@ -49,25 +49,6 @@
> #include <ufs/ext2fs/ext2fs_extern.h>
> #include <ufs/ext2fs/ext2fs_extents.h>
>
> -union _qcvt {
> - int64_t qcvt;
> - int32_t val[2];
> -};
> -
> -#define SETHIGH(q, h) { \
> - union _qcvt tmp; \
> - tmp.qcvt = (q); \
> - tmp.val[_QUAD_HIGHWORD] = (h); \
> - (q) = tmp.qcvt; \
> -}
> -
> -#define SETLOW(q, l) { \
> - union _qcvt tmp; \
> - tmp.qcvt = (q); \
> - tmp.val[_QUAD_LOWWORD] = (l); \
> - (q) = tmp.qcvt; \
> -}
> -
> #ifdef _KERNEL
>
> /*
> @@ -220,8 +201,8 @@ ext2fs_vinit(struct mount *mp, struct vo
>
> /* Initialize modrev times */
> getmicrouptime(&tv);
> - SETHIGH(ip->i_modrev, tv.tv_sec);
> - SETLOW(ip->i_modrev, tv.tv_usec * 4294);
> + ip->i_modrev = (u_quad_t)tv.tv_sec << 32;
> + ip->i_modrev |= (u_quad_t)tv.tv_usec * 4294;
>
> *vpp = vp;
>
> Index: ufs/ufs/ufs_vnops.c
> ===================================================================
> RCS file: /cvs/src/sys/ufs/ufs/ufs_vnops.c,v
> retrieving revision 1.123
> diff -u -p -u -r1.123 ufs_vnops.c
> --- ufs/ufs/ufs_vnops.c 8 Dec 2015 15:31:01 -0000 1.123
> +++ ufs/ufs/ufs_vnops.c 27 Jan 2016 08:26:10 -0000
> @@ -78,24 +78,6 @@ int filt_ufswrite(struct knote *, long);
> int filt_ufsvnode(struct knote *, long);
> void filt_ufsdetach(struct knote *);
>
> -union _qcvt {
> - int64_t qcvt;
> - int32_t val[2];
> -};
> -
> -#define SETHIGH(q, h) { \
> - union _qcvt tmp; \
> - tmp.qcvt = (q); \
> - tmp.val[_QUAD_HIGHWORD] = (h); \
> - (q) = tmp.qcvt; \
> -}
> -#define SETLOW(q, l) { \
> - union _qcvt tmp; \
> - tmp.qcvt = (q); \
> - tmp.val[_QUAD_LOWWORD] = (l); \
> - (q) = tmp.qcvt; \
> -}
> -
> /*
> * A virgin directory (no blushing please).
> */
> @@ -1879,8 +1861,8 @@ ufs_vinit(struct mount *mntp, struct vop
> * Initialize modrev times
> */
> getmicrouptime(&mtv);
> - SETHIGH(ip->i_modrev, mtv.tv_sec);
> - SETLOW(ip->i_modrev, mtv.tv_usec * 4294);
> + ip->i_modrev = (u_quad_t)mtv.tv_sec << 32;
> + ip->i_modrev |= (u_quad_t)mtv.tv_usec * 4294;
> *vpp = vp;
> return (0);
> }
>
> cheers,
> natano
>
