This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation
- From: Chris Metcalf <cmetcalf at mellanox dot com>
- To: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>, <libc-alpha at sourceware dot org>, Mike Frysinger <vapier at gentoo dot org>
- Date: Thu, 16 Jun 2016 11:25:24 -0400
- Subject: Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation
- Authentication-results: sourceware.org; auth=none
- Authentication-results: spf=none (sender IP is ) smtp dot mailfrom=cmetcalf at mellanox dot com;
- References: <1465941275-3459-1-git-send-email-adhemerval dot zanella at linaro dot org> <20160615053714 dot GM4053 at vapier dot lan> <57615BFB dot 2050101 at linaro dot org> <e9d7c072-9fd7-e4ab-5b65-6974a7597c77 at mellanox dot com> <5761B7ED dot 6040102 at linaro dot org>
- Spamdiagnosticmetadata: NSPM
- Spamdiagnosticoutput: 1:99
On 6/15/2016 4:17 PM, Adhemerval Zanella wrote:
On 15/06/2016 15:21, Chris Metcalf wrote:
On 6/15/2016 9:45 AM, Adhemerval Zanella wrote:
On 15/06/2016 02:37, Mike Frysinger wrote:
On 14 Jun 2016 18:54, Adhemerval Zanella wrote:
This patch fixes the p{readv,writev}{64} consolidation implementation
from commits 4e77815 and af5fdf5. Different from pread/pwrite
implementation, preadv/pwritev implementation does not require
__ALIGNMENT_ARG because kernel syscall prototypes define
the high and low part of the off_t, if it is the case, directly
(different from pread/pwrite where the architecture ABI for passing
64-bit values must be in consideration for passsing the arguments).
i had looked at that specifically but thought it ok because the old code
was using the alignment arg. was the old code broken too ?
this is what the preadv code looked like:
-ssize_t
-__libc_preadv (int fd, const struct iovec *vector, int count, off_t offset)
-{
- assert (sizeof (offset) == 4);
- return SYSCALL_CANCEL (preadv, fd,
- vector, count, __ALIGNMENT_ARG
- __LONG_LONG_PAIR (offset >> 31, offset));
-}
although i guess this isn't too surprising as this code was in the
generic sysdeps dir which currently doesn't have as many users as
we wish it did :).
The idea is not really to align the argument to zero pass, but rather to split
the possible 64-bits argument in high and low as required (as the default
implementation was doing [2]). On tile, it is working because the preadv.c
offset is 32-bits and thus the high word is indeed zero, but it is passing
one superfluous argument.
No, what happens is that instead of passing r3=pos_low=lo, r4=pos_high=0, we
are passing r3=pos_low=0, r4=pos_high=lo, r5=0. This means that we get a crazy
high offset and either fail or (for a sufficiently large file) get the wrong data.
Filed as bug 20261.
I was referring to *old* behaviour (pre-consolidation) implementation
(the sysdeps/unix/sysv/linux/generic/wordsize-32/preadv.c), which did:
--
return SYSCALL_CANCEL (preadv, fd,
vector, count, __ALIGNMENT_ARG
__LONG_LONG_PAIR (offset >> 31, offset));
--
It has 2 issue:
1. It passed one superfluous argument to preadv. On tilepro build
I noted that is using internal_syscall6, which means it is passing
{ fd, vector, cound, 0, offset, offset>>31 }. It is not wrong,
since for this code off_t will be the old 32-bit value, but the
semantic is wrong.
No, it's definitely wrong :-)
We pass "offset" as the high part, and "0" as the low part. Accordingly, what
the kernel sees is a request for "offset << 32" rather than "offset". The bug
report I filed contains a repro that does fail on tilegx32.
2. __LONG_LONG_PAIR is not correct for big-endian.
Yes, this needs to be fixed on the kernel side for consistency. Many other
syscalls pass their arguments with __LONG_LONG_PAIR. Handling it this way
in the kernel seems pretty standard at this point, e.g. regs_to_64 in
arch/arm64/include/asm/assembler.h, or merge_64 in
arch/mips/kernel/linux32.c. Even the fixed-endian platforms do it
consistently, e.g. "hi, lo" on parisc/s390/sparc and "lo, hi" on metag/x86.
It's possible little-endian compat powerpc shares this bug since it seems to
take fixed "hi, lo" arguments, e.g. in compat_sys_readahead(). I'll
forward this email on to the powerpc kernel maintainers to make sure
they have a chance to think about it.
See https://lkml.kernel.org/g/1466019219-10462-1-git-send-email-cmetcalf@mellanox.com
Now for BZ#20261 I do not think it applicable since this is a fix for
consolidation done in development phase, it does not appear in any
released version.
It breaks on all versions, as far as I can tell. I tested it on our RHEL 6 platform
(based on backported glibc 2.12) and it fails there.
Now, your analysis is correct for *current* code and it is contains the
__LONG_LONG_PAIR issue due the SYSCALL_LL usage. I will change it
and post a second version.
I think you should revert that part of the change and stick with __LONG_LONG_PAIR.
--
Chris Metcalf, Mellanox Technologies
http://www.mellanox.com