[v2] Fix LO_HI_LONG definition

On Mon, Jul 11, 2016 at 2:28 PM, Chris Metcalf <cmetcalf@mellanox.com> wrote:
> On 7/11/2016 5:09 PM, H.J. Lu wrote:
>>
>> On Wed, Jul 6, 2016 at 12:52 PM, Adhemerval Zanella
>> <adhemerval.zanella@linaro.org> wrote:
>>>
>>> The p{read,write}v{64} consolidation patch [1] added a wrong guard
>>> for LO_HI_LONG definition.  It currently uses both
>>> '__WORDSIZE == 64' and 'defined __ASSUME_WORDSIZE64_ILP32' to set
>>> the value to be passed in one argument, otherwise it will be split
>>> in two.  Since kernel commit 601cc11d054 the non-compat
>>> preadv/pwritev in same order, so the LO_HI_LONG is the same regardless
>>> off/off64_t size or wordsize.
>>>
>>> The tests were passing on 64-bit because files were small enough so
>>> the high part of offset is 0 regardless.  This patch also changes the
>>> tst-preadvwritev64 test to check reads and writes on a sparse file
>>> larger than 4G.
>>>
>>> Checked on x86_64, i686, x32, aarch64, armhf, and s390.
>>>
>>>          * sysdeps/unix/sysv/linux/sysdep.h
>>>          [__WORDSIZE == 64 || __ASSUME_WORDSIZE64_ILP32] (LO_HI_LONG):
>>> Remove
>>>          guards.
>>>          * misc/tst-preadvwritev-common.c: New file.
>>>          * misc/tst-preadvwritev.c: Use tst-preadvwritev-common.c.
>>>          * misc/tst-preadvwritev64.c: Use tst-preadwritev-common.c and
>>> add
>>>          a check for files larger than 2GB.
>>>
>>> diff --git a/sysdeps/unix/sysv/linux/sysdep.h
>>> b/sysdeps/unix/sysv/linux/sysdep.h
>>> index 8c9e62e..a469f57 100644
>>> --- a/sysdeps/unix/sysv/linux/sysdep.h
>>> +++ b/sysdeps/unix/sysv/linux/sysdep.h
>>> @@ -49,10 +49,6 @@
>>>   #endif
>>>
>>>   /* Provide a macro to pass the off{64}_t argument on
>>> p{readv,writev}{64}.  */
>>> -#if __WORDSIZE == 64 || defined __ASSUME_WORDSIZE64_ILP32
>>> -# define LO_HI_LONG(val) (val)
>>> -#else
>>> -# define LO_HI_LONG(val) \
>>> -  (long) (val), \
>>> -  (long) (((uint64_t) (val)) >> 32)
>>> -#endif
>>> +#define LO_HI_LONG(val) \
>>> + (long) (val), \
>>> + (long) (((uint64_t) (val)) >> 32)
>>> --
>>
>> This change is incorrect for x32.  __ASSUME_WORDSIZE64_ILP32 seems
>> to mean different things for mips and x86-64. X86-64 always passes 64-bit
>> value in a 64-bit register.
>
>
> Also, I'm not convinced the old code was right either.

At least, the old one is correct for x86.

> In the 64-bit case (or HJ's 32-bit with 64-bit syscall args), shouldn't we
> have "#define LO_HI_LONG(val) (val), 0"?  The trailing zero will ensure that
> the kernel's pos_h argument gets a zero value so we don't end up
> or'ing in garbage with the pos_from_hilo() call.
>

Yes, we have garbage in pos_h (%r8):

0000000000000000 <pwritev>:
   0: 8b 05 00 00 00 00     mov    0x0(%rip),%eax        # 6
<pwritev+0x6> 2: R_X86_64_PC32 __libc_multiple_threads-0x4
   6: 49 89 ca             mov    %rcx,%r10
   9: 85 c0                 test   %eax,%eax
   b: 75 3b                 jne    48 <pwritev+0x48>
   d: 49 89 c8             mov    %rcx,%r8
  10: 48 63 d2             movslq %edx,%rdx
  13: 48 63 ff             movslq %edi,%rdi
  16: 49 c1 e8 20           shr    $0x20,%r8
  1a: b8 28 01 00 00       mov    $0x128,%eax
  1f: 0f 05                 syscall

Luckily, kernel removes the garbage:

/* Provide a macro to pass the off{64}_t argument on p{readv,writev}{64}.  */
#define LO_HI_LONG(val) \
 (long) (val), \
 (long) (((uint64_t) (val)) >> 32)

kernel has

static inline loff_t pos_from_hilo(unsigned long high, unsigned long low)
{
#define HALF_LONG_BITS (BITS_PER_LONG / 2)
        return (((loff_t)high << HALF_LONG_BITS) << HALF_LONG_BITS) | low;
}

SYSCALL_DEFINE5(preadv, unsigned long, fd, const struct iovec __user *, vec,
                unsigned long, vlen, unsigned long, pos_l, unsigned long, pos_h)
{
        loff_t pos = pos_from_hilo(pos_h, pos_l);

But we don't pass garbage nor zero in pos_h.  I am testing this patch: