| Message ID | a5987ece-5351-346b-21fd-5a3e0092229e@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Zanella, On 06/15/2017 04:10 PM, Adhemerval Zanella wrote: > After the issue with LO_HI_LONG definition on x86_64-linux-gnu, I planed to add > this patch to check the above patch for correct check for invalid flags (which > would also have show this issue with LO_HI_LONG being used on p{read,write}v2). > > However it seems to trigger what I think it is a kernel bug on version that > provides p{read,write}v, where preadv2 does fails with EOPNOTSUPP but > pwritev2 does not. For instance, on x86_64-linux-gnu-x32 and i686-linux-gnu > (4.10.0-21-generic/x86_64): It looks like the issue is that you are going through the compat_writev path, which for some reason discards the flags parameter. Can you apply the patch below to your kernel?diff --git a/fs/read_write.c b/fs/read_write.c index c4f88af..f77eb22 100644 --- a/fs/read_write.c +++ b/fs/read_write.c @@ -1210,7 +1210,7 @@ static size_t compat_writev(struct file *file, if (!(file->f_mode & FMODE_CAN_WRITE)) goto out; - ret = compat_do_readv_writev(WRITE, file, vec, vlen, pos, 0); + ret = compat_do_readv_writev(WRITE, file, vec, vlen, pos, flags); out: if (ret > 0)
On Thu, Jun 01, 2017 at 11:52:25AM -0600, Jon Derrick wrote: > Can you apply the patch below to your kernel? I've already sent this patch to Al twice (including a stable tag), but it didn't seem to make it anywhere. > diff --git a/fs/read_write.c b/fs/read_write.c > index c4f88af..f77eb22 100644 > --- a/fs/read_write.c > +++ b/fs/read_write.c > @@ -1210,7 +1210,7 @@ static size_t compat_writev(struct file *file, > if (!(file->f_mode & FMODE_CAN_WRITE)) > goto out; > > - ret = compat_do_readv_writev(WRITE, file, vec, vlen, pos, 0); > + ret = compat_do_readv_writev(WRITE, file, vec, vlen, pos, flags); > > out: > if (ret > 0) ---end quoted text---
On 16/06/2017 03:04, Christoph Hellwig wrote: > On Thu, Jun 01, 2017 at 11:52:25AM -0600, Jon Derrick wrote: >> Can you apply the patch below to your kernel? > > I've already sent this patch to Al twice (including a stable tag), > but it didn't seem to make it anywhere. Right, thanks for the reply. So it is a kernel issue and I think it should not prevent us to use it a regression test. > >> diff --git a/fs/read_write.c b/fs/read_write.c >> index c4f88af..f77eb22 100644 >> --- a/fs/read_write.c >> +++ b/fs/read_write.c >> @@ -1210,7 +1210,7 @@ static size_t compat_writev(struct file *file, >> if (!(file->f_mode & FMODE_CAN_WRITE)) >> goto out; >> >> - ret = compat_do_readv_writev(WRITE, file, vec, vlen, pos, 0); >> + ret = compat_do_readv_writev(WRITE, file, vec, vlen, pos, flags); >> >> out: >> if (ret > 0) > ---end quoted text--- >
diff --git a/misc/tst-preadvwritev2-common.c b/misc/tst-preadvwritev2-common.c new file mode 100644 index 0000000000..a8073e6894 --- /dev/null +++ b/misc/tst-preadvwritev2-common.c @@ -0,0 +1,46 @@ +/* Common function for preadv2 and pwritev2 tests. + Copyright (C) 2017 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#include <support/check.h> + +static void +do_test_with_invalid_flags (void) +{ + int invalid_flag = 0x1; +#ifdef RWF_HIPRI + invalid_flag <<= 1; +#endif +#ifdef RWF_DSYNC + invalid_flag <<= 1; +#endif +#ifdef RWF_SYNC + invalid_flag <<= 1; +#endif + + char buf[32]; + const struct iovec vec = { .iov_base = buf, .iov_len = sizeof (buf) }; + if (preadv2 (temp_fd, &vec, 1, 0, invalid_flag) != -1) + FAIL_EXIT1 ("preadv2 did not fail with an invalid flag"); + if (errno != ENOTSUP) + FAIL_EXIT1 ("preadv2 failure did not set errno to ENOTSUP (%d)", errno); + + if (pwritev2 (temp_fd, &vec, 1, 0, invalid_flag) != -1) + FAIL_EXIT1 ("pwritev2 did not fail with an invalid flag"); + if (errno != ENOTSUP) + FAIL_EXIT1 ("pwritev2 failure did not set errno to ENOTSUP (%d)", errno); +} diff --git a/misc/tst-preadvwritev2.c b/misc/tst-preadvwritev2.c index cf36272dd3..682c7579da 100644 --- a/misc/tst-preadvwritev2.c +++ b/misc/tst-preadvwritev2.c @@ -23,9 +23,12 @@ pwritev2 (__fd, __iov, __iovcnt, __offset, 0) #include "tst-preadvwritev-common.c" +#include "tst-preadvwritev2-common.c" static int do_test (void) { + do_test_with_invalid_flags (); + return do_test_with_offset (0); } diff --git a/misc/tst-preadvwritev64v2.c b/misc/tst-preadvwritev64v2.c index 8d0c48ea78..9ddc7625f0 100644 --- a/misc/tst-preadvwritev64v2.c +++ b/misc/tst-preadvwritev64v2.c @@ -25,9 +25,12 @@ pwritev2 (__fd, __iov, __iovcnt, __offset, 0) #include "tst-preadvwritev-common.c" +#include "tst-preadvwritev2-common.c" static int do_test (void) { + do_test_with_invalid_flags (); + return do_test_with_offset (0); }
After the issue with LO_HI_LONG definition on x86_64-linux-gnu, I planed to add this patch to check the above patch for correct check for invalid flags (which would also have show this issue with LO_HI_LONG being used on p{read,write}v2). However it seems to trigger what I think it is a kernel bug on version that provides p{read,write}v, where preadv2 does fails with EOPNOTSUPP but pwritev2 does not. For instance, on x86_64-linux-gnu-x32 and i686-linux-gnu (4.10.0-21-generic/x86_64): [...] [pid 1027] preadv2(3, 0xff96d3d0, 1, 0, 0x8 /* RWF_??? */) = -1 EOPNOTSUPP (Operation not supported) [pid 1027] pwritev2(3, [{"\0\360bVl\206\4\10\0\0\0\0Z=l\367t\372\226\377\214\362`\367\f\0\0\0\0\320\4\10", 32}], 1, 0, 0x8 /* RWF_??? */) = 32 [...] And on sparcv9-linux-gnu (4.12.0-rc1-sparc64-smp): [...] [pid 139846] preadv2(3, [{iov_base=0xffefcd88, iov_len=32}], 1, 0, 0x8 /* RWF_??? */) = -1 EOPNOTSUPP (Operation not supported) [pid 139846] pwritev2(3, [{iov_base="\377\357\315\300p\1f\314\0\0\0\0\0\0\0\2p\3@\10\0\2B\260p\3+\330\0\1\21\220", iov_len=32}], 1, 0, 0x8 /* RWF_??? */) = 32 [...] For both on 64 bits syscalls works as intended. For instance on x86-64 aforementioned kernel: [...] [pid 24276] preadv2(3, 0x7ffd084c1ee0, 1, 0, 0x8 /* RWF_??? */) = -1 EOPNOTSUPP (Operation not supported) [pid 24276] pwritev2(3, [{"\0\0\0\0\0\0\0\0!`\331\311\304\177\0\0\220\37L\10\375\177\0\0\0\0\0\0\0\0\0\0", 32}], 1, 0, 0x8 /* RWF_??? */) = -1 EOPNOTSUPP (Operation not supported) [...] I haven't tested on newer kernels or on different architectures (these are the ones I have readily available). Any idea why kernel ignores invalid flags for pwritev2 on compatibility mode? Does it invalidate the testcase? PS: I am copying the ones on the original pwritev2/preadv2 email about the status of the syscall on glibc [1]. [1] https://sourceware.org/ml/libc-alpha/2017-04/msg00452.html --