| Message ID | 20180829212032.32507-1-adhemerval.zanella@linaro.org |
|---|---|
| State | Accepted |
| Commit | 7a16bdbb9ff4122af0a28dc20996c95352011fdd |
| Headers | show |
| Series | Fix misreported errno on preadv2/pwritev2 (BZ#23579) | expand |
Ping. On 29/08/2018 14:20, Adhemerval Zanella wrote: > The fallback code of Linux wrapper for preadv2/pwritev2 executes > regardless of the errno code for preadv2, instead of the case where > the syscall is not supported. > > This fixes it by calling the fallback code iff errno is ENOSYS. The > patch also adds tests for both invalid file descriptor and invalid > iov_len and vector count. > > The only discrepancy between preadv2 and fallback code regarding > error reporting is when an invalid flags are used. The fallback code > bails out earlier with ENOTSUP instead of EINVAL/EBADF when the syscall > is used. > > Checked on x86_64-linux-gnu on a 4.4.0 and 4.15.0 kernel. > > [BZ #23579] > * misc/tst-preadvwritev2-common.c (do_test_with_invalid_fd): New > test. > * misc/tst-preadvwritev2.c, misc/tst-preadvwritev64v2.c (do_test): > Call do_test_with_invalid_fd. > * sysdeps/unix/sysv/linux/preadv2.c (preadv2): Use fallback code iff > errno is ENOSYS. > * sysdeps/unix/sysv/linux/preadv64v2.c (preadv64v2): Likewise. > * sysdeps/unix/sysv/linux/pwritev2.c (pwritev2): Likewise. > * sysdeps/unix/sysv/linux/pwritev64v2.c (pwritev64v2): Likewise. > --- > ChangeLog | 13 ++++++ > misc/tst-preadvwritev2-common.c | 65 +++++++++++++++++++++++++-- > misc/tst-preadvwritev2.c | 2 + > misc/tst-preadvwritev64v2.c | 2 + > sysdeps/unix/sysv/linux/preadv2.c | 2 +- > sysdeps/unix/sysv/linux/preadv64v2.c | 2 +- > sysdeps/unix/sysv/linux/pwritev2.c | 2 +- > sysdeps/unix/sysv/linux/pwritev64v2.c | 2 +- > 8 files changed, 83 insertions(+), 7 deletions(-) > > diff --git a/misc/tst-preadvwritev2-common.c b/misc/tst-preadvwritev2-common.c > index f889a21544..50b9da3fea 100644 > --- a/misc/tst-preadvwritev2-common.c > +++ b/misc/tst-preadvwritev2-common.c > @@ -19,9 +19,6 @@ > #include <limits.h> > #include <support/check.h> > > -static void > -do_test_with_invalid_flags (void) > -{ > #ifndef RWF_HIPRI > # define RWF_HIPRI 0 > #endif > @@ -39,6 +36,68 @@ do_test_with_invalid_flags (void) > #endif > #define RWF_SUPPORTED (RWF_HIPRI | RWF_DSYNC | RWF_SYNC | RWF_NOWAIT \ > | RWF_APPEND) > + > +static void > +do_test_with_invalid_fd (void) > +{ > + char buf[256]; > + struct iovec iov = { buf, sizeof buf }; > + > + /* Check with flag being 0 to use the fallback code which calls pwritev > + or writev. */ > + TEST_VERIFY (preadv2 (-1, &iov, 1, -1, 0) == -1); > + TEST_COMPARE (errno, EBADF); > + TEST_VERIFY (pwritev2 (-1, &iov, 1, -1, 0) == -1); > + TEST_COMPARE (errno, EBADF); > + > + /* Same tests as before but with flags being different than 0. Since > + there is no emulation for any flag value, fallback code returns > + ENOTSUP. This is different running on a kernel with preadv2/pwritev2 > + support, where EBADF is returned). */ > + TEST_VERIFY (preadv2 (-1, &iov, 1, 0, RWF_HIPRI) == -1); > + TEST_VERIFY (errno == EBADF || errno == ENOTSUP); > + TEST_VERIFY (pwritev2 (-1, &iov, 1, 0, RWF_HIPRI) == -1); > + TEST_VERIFY (errno == EBADF || errno == ENOTSUP); > +} > + > +static void > +do_test_with_invalid_iov (void) > +{ > + { > + char buf[256]; > + struct iovec iov; > + > + iov.iov_base = buf; > + iov.iov_len = (size_t)SSIZE_MAX + 1; > + > + TEST_VERIFY (preadv2 (temp_fd, &iov, 1, 0, 0) == -1); > + TEST_COMPARE (errno, EINVAL); > + TEST_VERIFY (pwritev2 (temp_fd, &iov, 1, 0, 0) == -1); > + TEST_COMPARE (errno, EINVAL); > + > + /* Same as for invalid file descriptor tests, emulation fallback > + first checks for flag value and return ENOTSUP. */ > + TEST_VERIFY (preadv2 (temp_fd, &iov, 1, 0, RWF_HIPRI) == -1); > + TEST_VERIFY (errno == EINVAL || errno == ENOTSUP); > + TEST_VERIFY (pwritev2 (temp_fd, &iov, 1, 0, RWF_HIPRI) == -1); > + TEST_VERIFY (errno == EINVAL || errno == ENOTSUP); > + } > + > + { > + /* An invalid iovec buffer should trigger an invalid memory access > + or an error (Linux for instance returns EFAULT). */ > + struct iovec iov[IOV_MAX+1] = { 0 }; > + > + TEST_VERIFY (preadv2 (temp_fd, iov, IOV_MAX + 1, 0, RWF_HIPRI) == -1); > + TEST_VERIFY (errno == EINVAL || errno == ENOTSUP); > + TEST_VERIFY (pwritev2 (temp_fd, iov, IOV_MAX + 1, 0, RWF_HIPRI) == -1); > + TEST_VERIFY (errno == EINVAL || errno == ENOTSUP); > + } > +} > + > +static void > +do_test_with_invalid_flags (void) > +{ > /* Set the next bit from the mask of all supported flags. */ > int invalid_flag = RWF_SUPPORTED != 0 ? __builtin_clz (RWF_SUPPORTED) : 2; > invalid_flag = 0x1 << ((sizeof (int) * CHAR_BIT) - invalid_flag); > diff --git a/misc/tst-preadvwritev2.c b/misc/tst-preadvwritev2.c > index be22802dbe..cb58cbe41e 100644 > --- a/misc/tst-preadvwritev2.c > +++ b/misc/tst-preadvwritev2.c > @@ -30,6 +30,8 @@ do_test (void) > { > do_test_with_invalid_flags (); > do_test_without_offset (); > + do_test_with_invalid_fd (); > + do_test_with_invalid_iov (); > > return do_test_with_offset (0); > } > diff --git a/misc/tst-preadvwritev64v2.c b/misc/tst-preadvwritev64v2.c > index 8d3cc32b28..6a9de54c78 100644 > --- a/misc/tst-preadvwritev64v2.c > +++ b/misc/tst-preadvwritev64v2.c > @@ -32,6 +32,8 @@ do_test (void) > { > do_test_with_invalid_flags (); > do_test_without_offset (); > + do_test_with_invalid_fd (); > + do_test_with_invalid_iov (); > > return do_test_with_offset (0); > } > diff --git a/sysdeps/unix/sysv/linux/preadv2.c b/sysdeps/unix/sysv/linux/preadv2.c > index c8bf0764ef..bb08cbc5fd 100644 > --- a/sysdeps/unix/sysv/linux/preadv2.c > +++ b/sysdeps/unix/sysv/linux/preadv2.c > @@ -32,7 +32,7 @@ preadv2 (int fd, const struct iovec *vector, int count, off_t offset, > # ifdef __NR_preadv2 > ssize_t result = SYSCALL_CANCEL (preadv2, fd, vector, count, > LO_HI_LONG (offset), flags); > - if (result >= 0) > + if (result >= 0 || errno != ENOSYS) > return result; > # endif > /* Trying to emulate the preadv2 syscall flags is troublesome: > diff --git a/sysdeps/unix/sysv/linux/preadv64v2.c b/sysdeps/unix/sysv/linux/preadv64v2.c > index d7400a0252..b72a047347 100644 > --- a/sysdeps/unix/sysv/linux/preadv64v2.c > +++ b/sysdeps/unix/sysv/linux/preadv64v2.c > @@ -30,7 +30,7 @@ preadv64v2 (int fd, const struct iovec *vector, int count, off64_t offset, > #ifdef __NR_preadv64v2 > ssize_t result = SYSCALL_CANCEL (preadv64v2, fd, vector, count, > LO_HI_LONG (offset), flags); > - if (result >= 0) > + if (result >= 0 || errno != ENOSYS) > return result; > #endif > /* Trying to emulate the preadv2 syscall flags is troublesome: > diff --git a/sysdeps/unix/sysv/linux/pwritev2.c b/sysdeps/unix/sysv/linux/pwritev2.c > index 29c2264c8f..26333ebd43 100644 > --- a/sysdeps/unix/sysv/linux/pwritev2.c > +++ b/sysdeps/unix/sysv/linux/pwritev2.c > @@ -28,7 +28,7 @@ pwritev2 (int fd, const struct iovec *vector, int count, off_t offset, > # ifdef __NR_pwritev2 > ssize_t result = SYSCALL_CANCEL (pwritev2, fd, vector, count, > LO_HI_LONG (offset), flags); > - if (result >= 0) > + if (result >= 0 || errno != ENOSYS) > return result; > # endif > /* Trying to emulate the pwritev2 syscall flags is troublesome: > diff --git a/sysdeps/unix/sysv/linux/pwritev64v2.c b/sysdeps/unix/sysv/linux/pwritev64v2.c > index 42da321149..17ea905aa6 100644 > --- a/sysdeps/unix/sysv/linux/pwritev64v2.c > +++ b/sysdeps/unix/sysv/linux/pwritev64v2.c > @@ -30,7 +30,7 @@ pwritev64v2 (int fd, const struct iovec *vector, int count, off64_t offset, > #ifdef __NR_pwritev64v2 > ssize_t result = SYSCALL_CANCEL (pwritev64v2, fd, vector, count, > LO_HI_LONG (offset), flags); > - if (result >= 0) > + if (result >= 0 || errno != ENOSYS) > return result; > #endif > /* Trying to emulate the pwritev2 syscall flags is troublesome: >
I will commit this patch shortly if no one opposes it. On 20/09/2018 15:32, Adhemerval Zanella wrote: > Ping. > > On 29/08/2018 14:20, Adhemerval Zanella wrote: >> The fallback code of Linux wrapper for preadv2/pwritev2 executes >> regardless of the errno code for preadv2, instead of the case where >> the syscall is not supported. >> >> This fixes it by calling the fallback code iff errno is ENOSYS. The >> patch also adds tests for both invalid file descriptor and invalid >> iov_len and vector count. >> >> The only discrepancy between preadv2 and fallback code regarding >> error reporting is when an invalid flags are used. The fallback code >> bails out earlier with ENOTSUP instead of EINVAL/EBADF when the syscall >> is used. >> >> Checked on x86_64-linux-gnu on a 4.4.0 and 4.15.0 kernel. >> >> [BZ #23579] >> * misc/tst-preadvwritev2-common.c (do_test_with_invalid_fd): New >> test. >> * misc/tst-preadvwritev2.c, misc/tst-preadvwritev64v2.c (do_test): >> Call do_test_with_invalid_fd. >> * sysdeps/unix/sysv/linux/preadv2.c (preadv2): Use fallback code iff >> errno is ENOSYS. >> * sysdeps/unix/sysv/linux/preadv64v2.c (preadv64v2): Likewise. >> * sysdeps/unix/sysv/linux/pwritev2.c (pwritev2): Likewise. >> * sysdeps/unix/sysv/linux/pwritev64v2.c (pwritev64v2): Likewise. >> --- >> ChangeLog | 13 ++++++ >> misc/tst-preadvwritev2-common.c | 65 +++++++++++++++++++++++++-- >> misc/tst-preadvwritev2.c | 2 + >> misc/tst-preadvwritev64v2.c | 2 + >> sysdeps/unix/sysv/linux/preadv2.c | 2 +- >> sysdeps/unix/sysv/linux/preadv64v2.c | 2 +- >> sysdeps/unix/sysv/linux/pwritev2.c | 2 +- >> sysdeps/unix/sysv/linux/pwritev64v2.c | 2 +- >> 8 files changed, 83 insertions(+), 7 deletions(-) >> >> diff --git a/misc/tst-preadvwritev2-common.c b/misc/tst-preadvwritev2-common.c >> index f889a21544..50b9da3fea 100644 >> --- a/misc/tst-preadvwritev2-common.c >> +++ b/misc/tst-preadvwritev2-common.c >> @@ -19,9 +19,6 @@ >> #include <limits.h> >> #include <support/check.h> >> >> -static void >> -do_test_with_invalid_flags (void) >> -{ >> #ifndef RWF_HIPRI >> # define RWF_HIPRI 0 >> #endif >> @@ -39,6 +36,68 @@ do_test_with_invalid_flags (void) >> #endif >> #define RWF_SUPPORTED (RWF_HIPRI | RWF_DSYNC | RWF_SYNC | RWF_NOWAIT \ >> | RWF_APPEND) >> + >> +static void >> +do_test_with_invalid_fd (void) >> +{ >> + char buf[256]; >> + struct iovec iov = { buf, sizeof buf }; >> + >> + /* Check with flag being 0 to use the fallback code which calls pwritev >> + or writev. */ >> + TEST_VERIFY (preadv2 (-1, &iov, 1, -1, 0) == -1); >> + TEST_COMPARE (errno, EBADF); >> + TEST_VERIFY (pwritev2 (-1, &iov, 1, -1, 0) == -1); >> + TEST_COMPARE (errno, EBADF); >> + >> + /* Same tests as before but with flags being different than 0. Since >> + there is no emulation for any flag value, fallback code returns >> + ENOTSUP. This is different running on a kernel with preadv2/pwritev2 >> + support, where EBADF is returned). */ >> + TEST_VERIFY (preadv2 (-1, &iov, 1, 0, RWF_HIPRI) == -1); >> + TEST_VERIFY (errno == EBADF || errno == ENOTSUP); >> + TEST_VERIFY (pwritev2 (-1, &iov, 1, 0, RWF_HIPRI) == -1); >> + TEST_VERIFY (errno == EBADF || errno == ENOTSUP); >> +} >> + >> +static void >> +do_test_with_invalid_iov (void) >> +{ >> + { >> + char buf[256]; >> + struct iovec iov; >> + >> + iov.iov_base = buf; >> + iov.iov_len = (size_t)SSIZE_MAX + 1; >> + >> + TEST_VERIFY (preadv2 (temp_fd, &iov, 1, 0, 0) == -1); >> + TEST_COMPARE (errno, EINVAL); >> + TEST_VERIFY (pwritev2 (temp_fd, &iov, 1, 0, 0) == -1); >> + TEST_COMPARE (errno, EINVAL); >> + >> + /* Same as for invalid file descriptor tests, emulation fallback >> + first checks for flag value and return ENOTSUP. */ >> + TEST_VERIFY (preadv2 (temp_fd, &iov, 1, 0, RWF_HIPRI) == -1); >> + TEST_VERIFY (errno == EINVAL || errno == ENOTSUP); >> + TEST_VERIFY (pwritev2 (temp_fd, &iov, 1, 0, RWF_HIPRI) == -1); >> + TEST_VERIFY (errno == EINVAL || errno == ENOTSUP); >> + } >> + >> + { >> + /* An invalid iovec buffer should trigger an invalid memory access >> + or an error (Linux for instance returns EFAULT). */ >> + struct iovec iov[IOV_MAX+1] = { 0 }; >> + >> + TEST_VERIFY (preadv2 (temp_fd, iov, IOV_MAX + 1, 0, RWF_HIPRI) == -1); >> + TEST_VERIFY (errno == EINVAL || errno == ENOTSUP); >> + TEST_VERIFY (pwritev2 (temp_fd, iov, IOV_MAX + 1, 0, RWF_HIPRI) == -1); >> + TEST_VERIFY (errno == EINVAL || errno == ENOTSUP); >> + } >> +} >> + >> +static void >> +do_test_with_invalid_flags (void) >> +{ >> /* Set the next bit from the mask of all supported flags. */ >> int invalid_flag = RWF_SUPPORTED != 0 ? __builtin_clz (RWF_SUPPORTED) : 2; >> invalid_flag = 0x1 << ((sizeof (int) * CHAR_BIT) - invalid_flag); >> diff --git a/misc/tst-preadvwritev2.c b/misc/tst-preadvwritev2.c >> index be22802dbe..cb58cbe41e 100644 >> --- a/misc/tst-preadvwritev2.c >> +++ b/misc/tst-preadvwritev2.c >> @@ -30,6 +30,8 @@ do_test (void) >> { >> do_test_with_invalid_flags (); >> do_test_without_offset (); >> + do_test_with_invalid_fd (); >> + do_test_with_invalid_iov (); >> >> return do_test_with_offset (0); >> } >> diff --git a/misc/tst-preadvwritev64v2.c b/misc/tst-preadvwritev64v2.c >> index 8d3cc32b28..6a9de54c78 100644 >> --- a/misc/tst-preadvwritev64v2.c >> +++ b/misc/tst-preadvwritev64v2.c >> @@ -32,6 +32,8 @@ do_test (void) >> { >> do_test_with_invalid_flags (); >> do_test_without_offset (); >> + do_test_with_invalid_fd (); >> + do_test_with_invalid_iov (); >> >> return do_test_with_offset (0); >> } >> diff --git a/sysdeps/unix/sysv/linux/preadv2.c b/sysdeps/unix/sysv/linux/preadv2.c >> index c8bf0764ef..bb08cbc5fd 100644 >> --- a/sysdeps/unix/sysv/linux/preadv2.c >> +++ b/sysdeps/unix/sysv/linux/preadv2.c >> @@ -32,7 +32,7 @@ preadv2 (int fd, const struct iovec *vector, int count, off_t offset, >> # ifdef __NR_preadv2 >> ssize_t result = SYSCALL_CANCEL (preadv2, fd, vector, count, >> LO_HI_LONG (offset), flags); >> - if (result >= 0) >> + if (result >= 0 || errno != ENOSYS) >> return result; >> # endif >> /* Trying to emulate the preadv2 syscall flags is troublesome: >> diff --git a/sysdeps/unix/sysv/linux/preadv64v2.c b/sysdeps/unix/sysv/linux/preadv64v2.c >> index d7400a0252..b72a047347 100644 >> --- a/sysdeps/unix/sysv/linux/preadv64v2.c >> +++ b/sysdeps/unix/sysv/linux/preadv64v2.c >> @@ -30,7 +30,7 @@ preadv64v2 (int fd, const struct iovec *vector, int count, off64_t offset, >> #ifdef __NR_preadv64v2 >> ssize_t result = SYSCALL_CANCEL (preadv64v2, fd, vector, count, >> LO_HI_LONG (offset), flags); >> - if (result >= 0) >> + if (result >= 0 || errno != ENOSYS) >> return result; >> #endif >> /* Trying to emulate the preadv2 syscall flags is troublesome: >> diff --git a/sysdeps/unix/sysv/linux/pwritev2.c b/sysdeps/unix/sysv/linux/pwritev2.c >> index 29c2264c8f..26333ebd43 100644 >> --- a/sysdeps/unix/sysv/linux/pwritev2.c >> +++ b/sysdeps/unix/sysv/linux/pwritev2.c >> @@ -28,7 +28,7 @@ pwritev2 (int fd, const struct iovec *vector, int count, off_t offset, >> # ifdef __NR_pwritev2 >> ssize_t result = SYSCALL_CANCEL (pwritev2, fd, vector, count, >> LO_HI_LONG (offset), flags); >> - if (result >= 0) >> + if (result >= 0 || errno != ENOSYS) >> return result; >> # endif >> /* Trying to emulate the pwritev2 syscall flags is troublesome: >> diff --git a/sysdeps/unix/sysv/linux/pwritev64v2.c b/sysdeps/unix/sysv/linux/pwritev64v2.c >> index 42da321149..17ea905aa6 100644 >> --- a/sysdeps/unix/sysv/linux/pwritev64v2.c >> +++ b/sysdeps/unix/sysv/linux/pwritev64v2.c >> @@ -30,7 +30,7 @@ pwritev64v2 (int fd, const struct iovec *vector, int count, off64_t offset, >> #ifdef __NR_pwritev64v2 >> ssize_t result = SYSCALL_CANCEL (pwritev64v2, fd, vector, count, >> LO_HI_LONG (offset), flags); >> - if (result >= 0) >> + if (result >= 0 || errno != ENOSYS) >> return result; >> #endif >> /* Trying to emulate the pwritev2 syscall flags is troublesome: >>
* Adhemerval Zanella: > [BZ #23579] > * misc/tst-preadvwritev2-common.c (do_test_with_invalid_fd): New > test. > * misc/tst-preadvwritev2.c, misc/tst-preadvwritev64v2.c (do_test): > Call do_test_with_invalid_fd. > * sysdeps/unix/sysv/linux/preadv2.c (preadv2): Use fallback code iff > errno is ENOSYS. > * sysdeps/unix/sysv/linux/preadv64v2.c (preadv64v2): Likewise. > * sysdeps/unix/sysv/linux/pwritev2.c (pwritev2): Likewise. > * sysdeps/unix/sysv/linux/pwritev64v2.c (pwritev64v2): Likewise. Looks okay to me. I see that we return ENOTSUP for invalid flags, but the kernel returns EINVAL. But this is a preexisting problem, unrelated to this patch. Thanks, Florian
On 28/09/2018 11:10, Florian Weimer wrote: > * Adhemerval Zanella: > >> [BZ #23579] >> * misc/tst-preadvwritev2-common.c (do_test_with_invalid_fd): New >> test. >> * misc/tst-preadvwritev2.c, misc/tst-preadvwritev64v2.c (do_test): >> Call do_test_with_invalid_fd. >> * sysdeps/unix/sysv/linux/preadv2.c (preadv2): Use fallback code iff >> errno is ENOSYS. >> * sysdeps/unix/sysv/linux/preadv64v2.c (preadv64v2): Likewise. >> * sysdeps/unix/sysv/linux/pwritev2.c (pwritev2): Likewise. >> * sysdeps/unix/sysv/linux/pwritev64v2.c (pwritev64v2): Likewise. > > Looks okay to me. > > I see that we return ENOTSUP for invalid flags, but the kernel returns > EINVAL. But this is a preexisting problem, unrelated to this patch. Yeah, I described this discrepancy in commit message and I can't really see how to easy emulate it.
* Adhemerval Zanella: > On 28/09/2018 11:10, Florian Weimer wrote: >> * Adhemerval Zanella: >> >>> [BZ #23579] >>> * misc/tst-preadvwritev2-common.c (do_test_with_invalid_fd): New >>> test. >>> * misc/tst-preadvwritev2.c, misc/tst-preadvwritev64v2.c (do_test): >>> Call do_test_with_invalid_fd. >>> * sysdeps/unix/sysv/linux/preadv2.c (preadv2): Use fallback code iff >>> errno is ENOSYS. >>> * sysdeps/unix/sysv/linux/preadv64v2.c (preadv64v2): Likewise. >>> * sysdeps/unix/sysv/linux/pwritev2.c (pwritev2): Likewise. >>> * sysdeps/unix/sysv/linux/pwritev64v2.c (pwritev64v2): Likewise. >> >> Looks okay to me. >> >> I see that we return ENOTSUP for invalid flags, but the kernel returns >> EINVAL. But this is a preexisting problem, unrelated to this patch. > > Yeah, I described this discrepancy in commit message and I can't really see > how to easy emulate it. I think there are two issues: EBADF vs flag error, and that the flag error is ENOTSUP, not EINVAL as the kernel returns (according to the manual page at least). The second part would be fixable, but not the first part. Thanks, Florian
On 28/09/2018 11:37, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 28/09/2018 11:10, Florian Weimer wrote: >>> * Adhemerval Zanella: >>> >>>> [BZ #23579] >>>> * misc/tst-preadvwritev2-common.c (do_test_with_invalid_fd): New >>>> test. >>>> * misc/tst-preadvwritev2.c, misc/tst-preadvwritev64v2.c (do_test): >>>> Call do_test_with_invalid_fd. >>>> * sysdeps/unix/sysv/linux/preadv2.c (preadv2): Use fallback code iff >>>> errno is ENOSYS. >>>> * sysdeps/unix/sysv/linux/preadv64v2.c (preadv64v2): Likewise. >>>> * sysdeps/unix/sysv/linux/pwritev2.c (pwritev2): Likewise. >>>> * sysdeps/unix/sysv/linux/pwritev64v2.c (pwritev64v2): Likewise. >>> >>> Looks okay to me. >>> >>> I see that we return ENOTSUP for invalid flags, but the kernel returns >>> EINVAL. But this is a preexisting problem, unrelated to this patch. >> >> Yeah, I described this discrepancy in commit message and I can't really see >> how to easy emulate it. > > I think there are two issues: EBADF vs flag error, and that the flag > error is ENOTSUP, not EINVAL as the kernel returns (according to the > manual page at least). The second part would be fixable, but not the > first part. The EINVAL for invalid flags is not what I am seeing with 4.15.0: preadv2(3, [{iov_base=0x7ffcb69d1620, iov_len=32}], 1, 0, 0x20 /* RWF_??? */) = -1 EOPNOTSUPP (Operation not supported) pwritev2(3, [{iov_base="\20\0\0\0\0\0\0\0\254\205\256[\0\0\0\0P\255(\16\0\0\0\0\254\205\256[\0\0\0\0", iov_len=32}], 1, 0, 0x20 /* RWF_??? */) = -1 EOPNOTSUPP (Operation not supported) And this I confirmed that is what kernels is doing: pwritev2 \_ do_writev | do_pwritev \_ vfs_writev \_ do_iter_write \_ do_iter_readv_writev \_ kiocb_set_rw_flags 665 static ssize_t do_iter_readv_writev(struct file *filp, struct iov_iter *iter, 666 loff_t *ppos, int type, rwf_t flags) 667 { 668 struct kiocb kiocb; 669 ssize_t ret; 670 671 init_sync_kiocb(&kiocb, filp); 672 ret = kiocb_set_rw_flags(&kiocb, flags); 673 if (ret) 674 return ret; static inline int kiocb_set_rw_flags(struct kiocb *ki, rwf_t flags) { if (unlikely(flags & ~RWF_SUPPORTED)) return -EOPNOTSUPP; \_ do_loop_readv_writev 687 static ssize_t do_loop_readv_writev(struct file *filp, struct iov_iter *iter, 688 loff_t *ppos, int type, rwf_t flags) 689 { 690 ssize_t ret = 0; 691 692 if (flags & ~RWF_HIPRI) 693 return -EOPNOTSUPP; I think we should update man-pages regarding this behaviour.
On 29/08/18 22:20, Adhemerval Zanella wrote: > The fallback code of Linux wrapper for preadv2/pwritev2 executes > regardless of the errno code for preadv2, instead of the case where > the syscall is not supported. > > This fixes it by calling the fallback code iff errno is ENOSYS. The > patch also adds tests for both invalid file descriptor and invalid > iov_len and vector count. > > The only discrepancy between preadv2 and fallback code regarding > error reporting is when an invalid flags are used. The fallback code > bails out earlier with ENOTSUP instead of EINVAL/EBADF when the syscall > is used. > > Checked on x86_64-linux-gnu on a 4.4.0 and 4.15.0 kernel. > > [BZ #23579] > * misc/tst-preadvwritev2-common.c (do_test_with_invalid_fd): New > test. > * misc/tst-preadvwritev2.c, misc/tst-preadvwritev64v2.c (do_test): > Call do_test_with_invalid_fd. > * sysdeps/unix/sysv/linux/preadv2.c (preadv2): Use fallback code iff > errno is ENOSYS. > * sysdeps/unix/sysv/linux/preadv64v2.c (preadv64v2): Likewise. > * sysdeps/unix/sysv/linux/pwritev2.c (pwritev2): Likewise. > * sysdeps/unix/sysv/linux/pwritev64v2.c (pwritev64v2): Likewise. with build-many-glibcs.py in logs/glibcs/i686-gnu/010-glibcs-i686-gnu-check-log.txt i see In file included from tst-preadvwritev2.c:26: tst-preadvwritev2-common.c: In function ‘do_test_with_invalid_iov’: tst-preadvwritev2-common.c:89:22: error: ‘IOV_MAX’ undeclared (first use in this function); did you mean ‘INT_MAX’? struct iovec iov[IOV_MAX+1] = { 0 }; ^~~~~~~ INT_MAX tst-preadvwritev2-common.c:89:22: note: each undeclared identifier is reported only once for each function it appears in tst-preadvwritev2-common.c:89:18: error: unused variable ‘iov’ [-Werror=unused-variable] struct iovec iov[IOV_MAX+1] = { 0 }; ^~~ cc1: all warnings being treated as errors make[3]: *** [../o-iterator.mk:9: /data/glibc-build-j2/build/glibcs/i686-gnu/glibc/misc/tst-preadvwritev2.o] Error 1 ... > +static void > +do_test_with_invalid_iov (void) > +{ > + { > + char buf[256]; > + struct iovec iov; > + > + iov.iov_base = buf; > + iov.iov_len = (size_t)SSIZE_MAX + 1; > + > + TEST_VERIFY (preadv2 (temp_fd, &iov, 1, 0, 0) == -1); > + TEST_COMPARE (errno, EINVAL); > + TEST_VERIFY (pwritev2 (temp_fd, &iov, 1, 0, 0) == -1); > + TEST_COMPARE (errno, EINVAL); > + > + /* Same as for invalid file descriptor tests, emulation fallback > + first checks for flag value and return ENOTSUP. */ > + TEST_VERIFY (preadv2 (temp_fd, &iov, 1, 0, RWF_HIPRI) == -1); > + TEST_VERIFY (errno == EINVAL || errno == ENOTSUP); > + TEST_VERIFY (pwritev2 (temp_fd, &iov, 1, 0, RWF_HIPRI) == -1); > + TEST_VERIFY (errno == EINVAL || errno == ENOTSUP); > + } > + > + { > + /* An invalid iovec buffer should trigger an invalid memory access > + or an error (Linux for instance returns EFAULT). */ > + struct iovec iov[IOV_MAX+1] = { 0 }; > + > + TEST_VERIFY (preadv2 (temp_fd, iov, IOV_MAX + 1, 0, RWF_HIPRI) == -1); > + TEST_VERIFY (errno == EINVAL || errno == ENOTSUP); > + TEST_VERIFY (pwritev2 (temp_fd, iov, IOV_MAX + 1, 0, RWF_HIPRI) == -1); > + TEST_VERIFY (errno == EINVAL || errno == ENOTSUP); > + } > +}
On 23/10/2018 10:09, Szabolcs Nagy wrote: > On 29/08/18 22:20, Adhemerval Zanella wrote: >> The fallback code of Linux wrapper for preadv2/pwritev2 executes >> regardless of the errno code for preadv2, instead of the case where >> the syscall is not supported. >> >> This fixes it by calling the fallback code iff errno is ENOSYS. The >> patch also adds tests for both invalid file descriptor and invalid >> iov_len and vector count. >> >> The only discrepancy between preadv2 and fallback code regarding >> error reporting is when an invalid flags are used. The fallback code >> bails out earlier with ENOTSUP instead of EINVAL/EBADF when the syscall >> is used. >> >> Checked on x86_64-linux-gnu on a 4.4.0 and 4.15.0 kernel. >> >> [BZ #23579] >> * misc/tst-preadvwritev2-common.c (do_test_with_invalid_fd): New >> test. >> * misc/tst-preadvwritev2.c, misc/tst-preadvwritev64v2.c (do_test): >> Call do_test_with_invalid_fd. >> * sysdeps/unix/sysv/linux/preadv2.c (preadv2): Use fallback code iff >> errno is ENOSYS. >> * sysdeps/unix/sysv/linux/preadv64v2.c (preadv64v2): Likewise. >> * sysdeps/unix/sysv/linux/pwritev2.c (pwritev2): Likewise. >> * sysdeps/unix/sysv/linux/pwritev64v2.c (pwritev64v2): Likewise. > > > with build-many-glibcs.py in > logs/glibcs/i686-gnu/010-glibcs-i686-gnu-check-log.txt i see > > In file included from tst-preadvwritev2.c:26: > tst-preadvwritev2-common.c: In function ‘do_test_with_invalid_iov’: > tst-preadvwritev2-common.c:89:22: error: ‘IOV_MAX’ undeclared (first use in this function); did you mean ‘INT_MAX’? > struct iovec iov[IOV_MAX+1] = { 0 }; > ^~~~~~~ > INT_MAX > tst-preadvwritev2-common.c:89:22: note: each undeclared identifier is reported only once for each function it appears in > tst-preadvwritev2-common.c:89:18: error: unused variable ‘iov’ [-Werror=unused-variable] > struct iovec iov[IOV_MAX+1] = { 0 }; > ^~~ > cc1: all warnings being treated as errors > make[3]: *** [../o-iterator.mk:9: /data/glibc-build-j2/build/glibcs/i686-gnu/glibc/misc/tst-preadvwritev2.o] Error 1 > It seems HURD uses the generic bits/uio_lim.h which does not define IOV_MAX. Loos like we need to define it on test for Hurd. I will fix it, thanks to bring this up.
On Tue, 23 Oct 2018, Szabolcs Nagy wrote: > with build-many-glibcs.py in > logs/glibcs/i686-gnu/010-glibcs-i686-gnu-check-log.txt i see > > In file included from tst-preadvwritev2.c:26: > tst-preadvwritev2-common.c: In function ‘do_test_with_invalid_iov’: > tst-preadvwritev2-common.c:89:22: error: ‘IOV_MAX’ undeclared (first use in this function); did you mean ‘INT_MAX’? > struct iovec iov[IOV_MAX+1] = { 0 }; > ^~~~~~~ > INT_MAX > tst-preadvwritev2-common.c:89:22: note: each undeclared identifier is reported only once for each function it appears in > tst-preadvwritev2-common.c:89:18: error: unused variable ‘iov’ [-Werror=unused-variable] > struct iovec iov[IOV_MAX+1] = { 0 }; > ^~~ > cc1: all warnings being treated as errors > make[3]: *** [../o-iterator.mk:9: /data/glibc-build-j2/build/glibcs/i686-gnu/glibc/misc/tst-preadvwritev2.o] Error 1 I guess this wasn't readily visible to people running build-many-glibcs.py tests because the current expected baseline is that the compilation testsuite doesn't have clean results on i686-gnu, ever since the C11 threads support went in (so that this build failure appeared is only visible if you actually look into the log files, not if you just look at the summary results of build-many-glibcs.py). It would of course be good to have C11 threads support for Hurd (sharing as much as possible of the code to implement C11 threads on top of pthreads, and the associated tests for C11 threads, with those used with NPTL). Until we have that, maybe it would be possible to XFAIL the relevant conform/ tests in some Hurd sysdeps Makefile (with a comment referencing a bug open in Bugzilla about the lack of C11 threads support for Hurd)? Finding regressions with build-many-glibcs.py works much better if the normal expected state is no failures at all. (ia64 has also been broken with GCC mainline for over a month - <https://gcc.gnu.org/ml/gcc-patches/2018-09/msg01052.html>.) -- Joseph S. Myers joseph@codesourcery.com
On 23/10/2018 18:37, Joseph Myers wrote: > On Tue, 23 Oct 2018, Szabolcs Nagy wrote: > >> with build-many-glibcs.py in >> logs/glibcs/i686-gnu/010-glibcs-i686-gnu-check-log.txt i see >> >> In file included from tst-preadvwritev2.c:26: >> tst-preadvwritev2-common.c: In function ‘do_test_with_invalid_iov’: >> tst-preadvwritev2-common.c:89:22: error: ‘IOV_MAX’ undeclared (first use in this function); did you mean ‘INT_MAX’? >> struct iovec iov[IOV_MAX+1] = { 0 }; >> ^~~~~~~ >> INT_MAX >> tst-preadvwritev2-common.c:89:22: note: each undeclared identifier is reported only once for each function it appears in >> tst-preadvwritev2-common.c:89:18: error: unused variable ‘iov’ [-Werror=unused-variable] >> struct iovec iov[IOV_MAX+1] = { 0 }; >> ^~~ >> cc1: all warnings being treated as errors >> make[3]: *** [../o-iterator.mk:9: /data/glibc-build-j2/build/glibcs/i686-gnu/glibc/misc/tst-preadvwritev2.o] Error 1 > > I guess this wasn't readily visible to people running build-many-glibcs.py > tests because the current expected baseline is that the compilation > testsuite doesn't have clean results on i686-gnu, ever since the C11 > threads support went in (so that this build failure appeared is only > visible if you actually look into the log files, not if you just look at > the summary results of build-many-glibcs.py). > > It would of course be good to have C11 threads support for Hurd (sharing > as much as possible of the code to implement C11 threads on top of > pthreads, and the associated tests for C11 threads, with those used with > NPTL). Until we have that, maybe it would be possible to XFAIL the > relevant conform/ tests in some Hurd sysdeps Makefile (with a comment > referencing a bug open in Bugzilla about the lack of C11 threads support > for Hurd)? Finding regressions with build-many-glibcs.py works much > better if the normal expected state is no failures at all. I agree it would be better to just XFAIL C11 threads conformance on Hurd, I don't plan to work on it in near feature and I presume it is a low priority issue for Hurd developers. > > (ia64 has also been broken with GCC mainline for over a month - > <https://gcc.gnu.org/ml/gcc-patches/2018-09/msg01052.html>.) >
Hello, Adhemerval Zanella, le mer. 24 oct. 2018 08:27:22 -0300, a ecrit: > On 23/10/2018 18:37, Joseph Myers wrote: > > maybe it would be possible to XFAIL the relevant conform/ tests in > > some Hurd sysdeps Makefile (with a comment referencing a bug open in > > Bugzilla about the lack of C11 threads support for Hurd)? > > I agree it would be better to just XFAIL C11 threads conformance on Hurd, > I don't plan to work on it in near feature and I presume it is a low > priority issue for Hurd developers. Indeed, so I did so. Samuel
diff --git a/misc/tst-preadvwritev2-common.c b/misc/tst-preadvwritev2-common.c index f889a21544..50b9da3fea 100644 --- a/misc/tst-preadvwritev2-common.c +++ b/misc/tst-preadvwritev2-common.c @@ -19,9 +19,6 @@ #include <limits.h> #include <support/check.h> -static void -do_test_with_invalid_flags (void) -{ #ifndef RWF_HIPRI # define RWF_HIPRI 0 #endif @@ -39,6 +36,68 @@ do_test_with_invalid_flags (void) #endif #define RWF_SUPPORTED (RWF_HIPRI | RWF_DSYNC | RWF_SYNC | RWF_NOWAIT \ | RWF_APPEND) + +static void +do_test_with_invalid_fd (void) +{ + char buf[256]; + struct iovec iov = { buf, sizeof buf }; + + /* Check with flag being 0 to use the fallback code which calls pwritev + or writev. */ + TEST_VERIFY (preadv2 (-1, &iov, 1, -1, 0) == -1); + TEST_COMPARE (errno, EBADF); + TEST_VERIFY (pwritev2 (-1, &iov, 1, -1, 0) == -1); + TEST_COMPARE (errno, EBADF); + + /* Same tests as before but with flags being different than 0. Since + there is no emulation for any flag value, fallback code returns + ENOTSUP. This is different running on a kernel with preadv2/pwritev2 + support, where EBADF is returned). */ + TEST_VERIFY (preadv2 (-1, &iov, 1, 0, RWF_HIPRI) == -1); + TEST_VERIFY (errno == EBADF || errno == ENOTSUP); + TEST_VERIFY (pwritev2 (-1, &iov, 1, 0, RWF_HIPRI) == -1); + TEST_VERIFY (errno == EBADF || errno == ENOTSUP); +} + +static void +do_test_with_invalid_iov (void) +{ + { + char buf[256]; + struct iovec iov; + + iov.iov_base = buf; + iov.iov_len = (size_t)SSIZE_MAX + 1; + + TEST_VERIFY (preadv2 (temp_fd, &iov, 1, 0, 0) == -1); + TEST_COMPARE (errno, EINVAL); + TEST_VERIFY (pwritev2 (temp_fd, &iov, 1, 0, 0) == -1); + TEST_COMPARE (errno, EINVAL); + + /* Same as for invalid file descriptor tests, emulation fallback + first checks for flag value and return ENOTSUP. */ + TEST_VERIFY (preadv2 (temp_fd, &iov, 1, 0, RWF_HIPRI) == -1); + TEST_VERIFY (errno == EINVAL || errno == ENOTSUP); + TEST_VERIFY (pwritev2 (temp_fd, &iov, 1, 0, RWF_HIPRI) == -1); + TEST_VERIFY (errno == EINVAL || errno == ENOTSUP); + } + + { + /* An invalid iovec buffer should trigger an invalid memory access + or an error (Linux for instance returns EFAULT). */ + struct iovec iov[IOV_MAX+1] = { 0 }; + + TEST_VERIFY (preadv2 (temp_fd, iov, IOV_MAX + 1, 0, RWF_HIPRI) == -1); + TEST_VERIFY (errno == EINVAL || errno == ENOTSUP); + TEST_VERIFY (pwritev2 (temp_fd, iov, IOV_MAX + 1, 0, RWF_HIPRI) == -1); + TEST_VERIFY (errno == EINVAL || errno == ENOTSUP); + } +} + +static void +do_test_with_invalid_flags (void) +{ /* Set the next bit from the mask of all supported flags. */ int invalid_flag = RWF_SUPPORTED != 0 ? __builtin_clz (RWF_SUPPORTED) : 2; invalid_flag = 0x1 << ((sizeof (int) * CHAR_BIT) - invalid_flag); diff --git a/misc/tst-preadvwritev2.c b/misc/tst-preadvwritev2.c index be22802dbe..cb58cbe41e 100644 --- a/misc/tst-preadvwritev2.c +++ b/misc/tst-preadvwritev2.c @@ -30,6 +30,8 @@ do_test (void) { do_test_with_invalid_flags (); do_test_without_offset (); + do_test_with_invalid_fd (); + do_test_with_invalid_iov (); return do_test_with_offset (0); } diff --git a/misc/tst-preadvwritev64v2.c b/misc/tst-preadvwritev64v2.c index 8d3cc32b28..6a9de54c78 100644 --- a/misc/tst-preadvwritev64v2.c +++ b/misc/tst-preadvwritev64v2.c @@ -32,6 +32,8 @@ do_test (void) { do_test_with_invalid_flags (); do_test_without_offset (); + do_test_with_invalid_fd (); + do_test_with_invalid_iov (); return do_test_with_offset (0); } diff --git a/sysdeps/unix/sysv/linux/preadv2.c b/sysdeps/unix/sysv/linux/preadv2.c index c8bf0764ef..bb08cbc5fd 100644 --- a/sysdeps/unix/sysv/linux/preadv2.c +++ b/sysdeps/unix/sysv/linux/preadv2.c @@ -32,7 +32,7 @@ preadv2 (int fd, const struct iovec *vector, int count, off_t offset, # ifdef __NR_preadv2 ssize_t result = SYSCALL_CANCEL (preadv2, fd, vector, count, LO_HI_LONG (offset), flags); - if (result >= 0) + if (result >= 0 || errno != ENOSYS) return result; # endif /* Trying to emulate the preadv2 syscall flags is troublesome: diff --git a/sysdeps/unix/sysv/linux/preadv64v2.c b/sysdeps/unix/sysv/linux/preadv64v2.c index d7400a0252..b72a047347 100644 --- a/sysdeps/unix/sysv/linux/preadv64v2.c +++ b/sysdeps/unix/sysv/linux/preadv64v2.c @@ -30,7 +30,7 @@ preadv64v2 (int fd, const struct iovec *vector, int count, off64_t offset, #ifdef __NR_preadv64v2 ssize_t result = SYSCALL_CANCEL (preadv64v2, fd, vector, count, LO_HI_LONG (offset), flags); - if (result >= 0) + if (result >= 0 || errno != ENOSYS) return result; #endif /* Trying to emulate the preadv2 syscall flags is troublesome: diff --git a/sysdeps/unix/sysv/linux/pwritev2.c b/sysdeps/unix/sysv/linux/pwritev2.c index 29c2264c8f..26333ebd43 100644 --- a/sysdeps/unix/sysv/linux/pwritev2.c +++ b/sysdeps/unix/sysv/linux/pwritev2.c @@ -28,7 +28,7 @@ pwritev2 (int fd, const struct iovec *vector, int count, off_t offset, # ifdef __NR_pwritev2 ssize_t result = SYSCALL_CANCEL (pwritev2, fd, vector, count, LO_HI_LONG (offset), flags); - if (result >= 0) + if (result >= 0 || errno != ENOSYS) return result; # endif /* Trying to emulate the pwritev2 syscall flags is troublesome: diff --git a/sysdeps/unix/sysv/linux/pwritev64v2.c b/sysdeps/unix/sysv/linux/pwritev64v2.c index 42da321149..17ea905aa6 100644 --- a/sysdeps/unix/sysv/linux/pwritev64v2.c +++ b/sysdeps/unix/sysv/linux/pwritev64v2.c @@ -30,7 +30,7 @@ pwritev64v2 (int fd, const struct iovec *vector, int count, off64_t offset, #ifdef __NR_pwritev64v2 ssize_t result = SYSCALL_CANCEL (pwritev64v2, fd, vector, count, LO_HI_LONG (offset), flags); - if (result >= 0) + if (result >= 0 || errno != ENOSYS) return result; #endif /* Trying to emulate the pwritev2 syscall flags is troublesome: