* [PATCH 1/6] selftests/filesystems: add initial select and poll selftest
2021-01-12 0:30 [PATCH 0/6] fs: deduplicate compat logic Willem de Bruijn
@ 2021-01-12 0:30 ` Willem de Bruijn
2021-01-12 0:30 ` [PATCH 2/6] select: deduplicate compat logic Willem de Bruijn
` (5 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Willem de Bruijn @ 2021-01-12 0:30 UTC (permalink / raw)
To: linux-fsdevel; +Cc: linux-kernel, viro, akpm, willy, arnd, Willem de Bruijn
From: Willem de Bruijn <willemb@google.com>
Add initial code coverage for select, pselect, poll and ppoll.
Open a socketpair and wait for a read event.
1. run with data waiting
2. run to timeout, if a (short) timeout is specified.
Also optionally pass sigset to pselect and ppoll, to exercise
all datapaths. Build with -m32, -mx32 and -m64 to cover all the
various compat and 32/64-bit time syscall implementations.
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
.../testing/selftests/filesystems/.gitignore | 1 +
tools/testing/selftests/filesystems/Makefile | 2 +-
.../selftests/filesystems/selectpoll.c | 207 ++++++++++++++++++
3 files changed, 209 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/filesystems/selectpoll.c
diff --git a/tools/testing/selftests/filesystems/.gitignore b/tools/testing/selftests/filesystems/.gitignore
index f0c0ff20d6cf..d4a2e50475ea 100644
--- a/tools/testing/selftests/filesystems/.gitignore
+++ b/tools/testing/selftests/filesystems/.gitignore
@@ -1,3 +1,4 @@
# SPDX-License-Identifier: GPL-2.0-only
dnotify_test
devpts_pts
+selectpoll
diff --git a/tools/testing/selftests/filesystems/Makefile b/tools/testing/selftests/filesystems/Makefile
index 129880fb42d3..8de184865fa4 100644
--- a/tools/testing/selftests/filesystems/Makefile
+++ b/tools/testing/selftests/filesystems/Makefile
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
CFLAGS += -I../../../../usr/include/
-TEST_GEN_PROGS := devpts_pts
+TEST_GEN_PROGS := devpts_pts selectpoll
TEST_GEN_PROGS_EXTENDED := dnotify_test
include ../lib.mk
diff --git a/tools/testing/selftests/filesystems/selectpoll.c b/tools/testing/selftests/filesystems/selectpoll.c
new file mode 100644
index 000000000000..315da0786a6c
--- /dev/null
+++ b/tools/testing/selftests/filesystems/selectpoll.c
@@ -0,0 +1,207 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define _GNU_SOURCE
+#include <asm/unistd.h>
+#include <poll.h>
+#include <unistd.h>
+#include <assert.h>
+#include <signal.h>
+#include <stdbool.h>
+#include <sys/select.h>
+#include <sys/socket.h>
+#include "../kselftest_harness.h"
+
+const unsigned long timeout_us = 5UL * 1000;
+const unsigned long timeout_ns = timeout_us * 1000;
+
+/* (p)select: basic invocation, optionally with data waiting */
+
+FIXTURE(select_basic)
+{
+ fd_set readfds;
+ int sfd[2];
+};
+
+FIXTURE_SETUP(select_basic)
+{
+ ASSERT_EQ(socketpair(AF_UNIX, SOCK_STREAM, 0, self->sfd), 0);
+
+ FD_ZERO(&self->readfds);
+ FD_SET(self->sfd[0], &self->readfds);
+ FD_SET(self->sfd[1], &self->readfds);
+}
+
+FIXTURE_TEARDOWN(select_basic)
+{
+ /* FD_ISSET(self->sfd[0] tested in TEST_F: depends on timeout */
+ ASSERT_EQ(FD_ISSET(self->sfd[1], &self->readfds), 0);
+
+ EXPECT_EQ(close(self->sfd[0]), 0);
+ EXPECT_EQ(close(self->sfd[1]), 0);
+}
+
+TEST_F(select_basic, select)
+{
+ ASSERT_EQ(write(self->sfd[1], "w", 1), 1);
+ ASSERT_EQ(select(self->sfd[1] + 1, &self->readfds,
+ NULL, NULL, NULL), 1);
+ ASSERT_NE(FD_ISSET(self->sfd[0], &self->readfds), 0);
+}
+
+TEST_F(select_basic, select_with_timeout)
+{
+ struct timeval tv = { .tv_usec = timeout_us };
+
+ ASSERT_EQ(write(self->sfd[1], "w", 1), 1);
+ ASSERT_EQ(select(self->sfd[1] + 1, &self->readfds,
+ NULL, NULL, &tv), 1);
+ ASSERT_GE(tv.tv_usec, 1000);
+ ASSERT_NE(FD_ISSET(self->sfd[0], &self->readfds), 0);
+}
+
+TEST_F(select_basic, select_timeout)
+{
+ struct timeval tv = { .tv_usec = timeout_us };
+
+ ASSERT_EQ(select(self->sfd[1] + 1, &self->readfds,
+ NULL, NULL, &tv), 0);
+ ASSERT_EQ(FD_ISSET(self->sfd[0], &self->readfds), 0);
+}
+
+TEST_F(select_basic, pselect)
+{
+ ASSERT_EQ(write(self->sfd[1], "w", 1), 1);
+ ASSERT_EQ(pselect(self->sfd[1] + 1, &self->readfds,
+ NULL, NULL, NULL, NULL), 1);
+ ASSERT_NE(FD_ISSET(self->sfd[0], &self->readfds), 0);
+}
+
+TEST_F(select_basic, pselect_with_timeout)
+{
+ struct timespec ts = { .tv_nsec = timeout_ns };
+
+ ASSERT_EQ(write(self->sfd[1], "w", 1), 1);
+ ASSERT_EQ(pselect(self->sfd[1] + 1, &self->readfds,
+ NULL, NULL, &ts, NULL), 1);
+ ASSERT_GE(ts.tv_nsec, 1000);
+ ASSERT_NE(FD_ISSET(self->sfd[0], &self->readfds), 0);
+}
+
+TEST_F(select_basic, pselect_timeout)
+{
+ struct timespec ts = { .tv_nsec = timeout_ns };
+
+ ASSERT_EQ(pselect(self->sfd[1] + 1, &self->readfds,
+ NULL, NULL, &ts, NULL), 0);
+ ASSERT_EQ(FD_ISSET(self->sfd[0], &self->readfds), 0);
+}
+
+TEST_F(select_basic, pselect_sigset_with_timeout)
+{
+ struct timespec ts = { .tv_nsec = timeout_ns };
+ sigset_t sigmask;
+
+ sigemptyset(&sigmask);
+ sigaddset(&sigmask, SIGUSR1);
+ sigprocmask(SIG_SETMASK, &sigmask, NULL);
+ sigemptyset(&sigmask);
+
+ ASSERT_EQ(write(self->sfd[1], "w", 1), 1);
+ ASSERT_EQ(pselect(self->sfd[1] + 1, &self->readfds,
+ NULL, NULL, &ts, &sigmask), 1);
+ ASSERT_GE(ts.tv_nsec, 1000);
+ ASSERT_NE(FD_ISSET(self->sfd[0], &self->readfds), 0);
+}
+
+/* (p)poll: basic invocation with data waiting */
+
+FIXTURE(poll_basic)
+{
+ struct pollfd pfds[2];
+ int sfd[2];
+};
+
+FIXTURE_SETUP(poll_basic)
+{
+ ASSERT_EQ(socketpair(AF_UNIX, SOCK_STREAM, 0, self->sfd), 0);
+
+ self->pfds[0].events = POLLIN;
+ self->pfds[0].revents = 0;
+ self->pfds[0].fd = self->sfd[0];
+
+ self->pfds[1].events = POLLIN;
+ self->pfds[1].revents = 0;
+ self->pfds[1].fd = self->sfd[1];
+}
+
+FIXTURE_TEARDOWN(poll_basic)
+{
+ /* FD_ISSET(self->pfds[0] tested in TEST_F: depends on timeout */
+ EXPECT_EQ(self->pfds[1].revents & POLLIN, 0);
+
+ EXPECT_EQ(close(self->sfd[0]), 0);
+ EXPECT_EQ(close(self->sfd[1]), 0);
+}
+
+TEST_F(poll_basic, poll)
+{
+ ASSERT_EQ(write(self->sfd[1], "w", 1), 1);
+ EXPECT_EQ(poll(self->pfds, ARRAY_SIZE(self->pfds), 0), 1);
+ EXPECT_EQ(self->pfds[0].revents & POLLIN, POLLIN);
+}
+
+TEST_F(poll_basic, poll_with_timeout)
+{
+ ASSERT_EQ(write(self->sfd[1], "w", 1), 1);
+ EXPECT_EQ(poll(self->pfds, ARRAY_SIZE(self->pfds), 1001), 1);
+ EXPECT_EQ(self->pfds[0].revents & POLLIN, POLLIN);
+}
+
+TEST_F(poll_basic, poll_timeout)
+{
+ EXPECT_EQ(poll(self->pfds, ARRAY_SIZE(self->pfds), 1001), 0);
+ EXPECT_EQ(self->pfds[0].revents & POLLIN, 0);
+}
+
+TEST_F(poll_basic, ppoll)
+{
+ ASSERT_EQ(write(self->sfd[1], "w", 1), 1);
+ EXPECT_EQ(ppoll(self->pfds, ARRAY_SIZE(self->pfds), NULL, NULL), 1);
+ EXPECT_EQ(self->pfds[0].revents & POLLIN, POLLIN);
+}
+
+TEST_F(poll_basic, ppoll_with_timeout)
+{
+ struct timespec ts = { .tv_nsec = timeout_ns };
+
+ ASSERT_EQ(write(self->sfd[1], "w", 1), 1);
+ EXPECT_EQ(ppoll(self->pfds, ARRAY_SIZE(self->pfds), &ts, NULL), 1);
+ ASSERT_GE(ts.tv_nsec, 1000);
+ EXPECT_EQ(self->pfds[0].revents & POLLIN, POLLIN);
+}
+
+TEST_F(poll_basic, ppoll_timeout)
+{
+ struct timespec ts = { .tv_nsec = timeout_ns };
+
+ EXPECT_EQ(ppoll(self->pfds, ARRAY_SIZE(self->pfds), &ts, NULL), 0);
+ EXPECT_EQ(self->pfds[0].revents & POLLIN, 0);
+}
+
+TEST_F(poll_basic, ppoll_sigset_with_timeout)
+{
+ struct timespec ts = { .tv_nsec = timeout_ns };
+ sigset_t sigmask;
+
+ sigemptyset(&sigmask);
+ sigaddset(&sigmask, SIGUSR1);
+ sigprocmask(SIG_SETMASK, &sigmask, NULL);
+ sigemptyset(&sigmask);
+
+ ASSERT_EQ(write(self->sfd[1], "w", 1), 1);
+ EXPECT_EQ(ppoll(self->pfds, ARRAY_SIZE(self->pfds), &ts, &sigmask), 1);
+ ASSERT_GE(ts.tv_nsec, 1000);
+ EXPECT_EQ(self->pfds[0].revents & POLLIN, POLLIN);
+}
+
+TEST_HARNESS_MAIN
--
2.30.0.284.gd98b1dd5eaa7-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 2/6] select: deduplicate compat logic
2021-01-12 0:30 [PATCH 0/6] fs: deduplicate compat logic Willem de Bruijn
2021-01-12 0:30 ` [PATCH 1/6] selftests/filesystems: add initial select and poll selftest Willem de Bruijn
@ 2021-01-12 0:30 ` Willem de Bruijn
2021-01-12 0:30 ` [PATCH 3/6] ppoll: " Willem de Bruijn
` (4 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Willem de Bruijn @ 2021-01-12 0:30 UTC (permalink / raw)
To: linux-fsdevel; +Cc: linux-kernel, viro, akpm, willy, arnd, Willem de Bruijn
From: Willem de Bruijn <willemb@google.com>
Select and pselect have multiple syscall implementations to handle
compat and 32-bit time variants.
Deduplicate core logic, which can cause divergence over time as
changes may not be applied consistently. See vmalloc support in
select, for one example.
Handle compat differences using in_compat_syscall() where needed.
Specifically, fd_set and sigmask may be compat variants. Handle
the !in_compat_syscall() case first, for branch prediction.
Handle timeval/timespec differences by passing along the type to
where the pointer is used.
Compat variants of select and old_select can now call standard
kern_select, removing all callers to do_compat_select.
Compat variants of pselect6 (time32 and time64) can now call standard
do_pselect, removing all callers to do_compat_pselect.
That removes both callers to compat_core_sys_select. And with that
callers to compat_[gs]et_fd_set.
Also move up zero_fd_set, to avoid one open-coded variant.
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
fs/select.c | 254 ++++++++++++----------------------------------------
1 file changed, 57 insertions(+), 197 deletions(-)
diff --git a/fs/select.c b/fs/select.c
index 37aaa8317f3a..dee7dfc5217b 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -382,32 +382,39 @@ typedef struct {
#define FDS_LONGS(nr) (((nr)+FDS_BITPERLONG-1)/FDS_BITPERLONG)
#define FDS_BYTES(nr) (FDS_LONGS(nr)*sizeof(long))
+static inline
+void zero_fd_set(unsigned long nr, unsigned long *fdset)
+{
+ memset(fdset, 0, FDS_BYTES(nr));
+}
+
/*
* Use "unsigned long" accesses to let user-mode fd_set's be long-aligned.
*/
static inline
int get_fd_set(unsigned long nr, void __user *ufdset, unsigned long *fdset)
{
- nr = FDS_BYTES(nr);
- if (ufdset)
- return copy_from_user(fdset, ufdset, nr) ? -EFAULT : 0;
+ if (!ufdset) {
+ zero_fd_set(nr, fdset);
+ return 0;
+ }
- memset(fdset, 0, nr);
- return 0;
+ if (!in_compat_syscall())
+ return copy_from_user(fdset, ufdset, FDS_BYTES(nr)) ? -EFAULT : 0;
+ else
+ return compat_get_bitmap(fdset, ufdset, nr);
}
static inline unsigned long __must_check
set_fd_set(unsigned long nr, void __user *ufdset, unsigned long *fdset)
{
- if (ufdset)
- return __copy_to_user(ufdset, fdset, FDS_BYTES(nr));
- return 0;
-}
+ if (!ufdset)
+ return 0;
-static inline
-void zero_fd_set(unsigned long nr, unsigned long *fdset)
-{
- memset(fdset, 0, FDS_BYTES(nr));
+ if (!in_compat_syscall())
+ return __copy_to_user(ufdset, fdset, FDS_BYTES(nr));
+ else
+ return compat_put_bitmap(ufdset, fdset, nr);
}
#define FDS_IN(fds, n) (fds->in + n)
@@ -698,15 +705,29 @@ int core_sys_select(int n, fd_set __user *inp, fd_set __user *outp,
}
static int kern_select(int n, fd_set __user *inp, fd_set __user *outp,
- fd_set __user *exp, struct __kernel_old_timeval __user *tvp)
+ fd_set __user *exp, void __user *tvp,
+ enum poll_time_type type)
{
struct timespec64 end_time, *to = NULL;
struct __kernel_old_timeval tv;
+ struct old_timeval32 otv;
int ret;
if (tvp) {
- if (copy_from_user(&tv, tvp, sizeof(tv)))
- return -EFAULT;
+ switch (type) {
+ case PT_TIMEVAL:
+ if (copy_from_user(&tv, tvp, sizeof(tv)))
+ return -EFAULT;
+ break;
+ case PT_OLD_TIMEVAL:
+ if (copy_from_user(&otv, tvp, sizeof(otv)))
+ return -EFAULT;
+ tv.tv_sec = otv.tv_sec;
+ tv.tv_usec = otv.tv_usec;
+ break;
+ default:
+ BUG();
+ }
to = &end_time;
if (poll_select_set_timeout(to,
@@ -716,18 +737,18 @@ static int kern_select(int n, fd_set __user *inp, fd_set __user *outp,
}
ret = core_sys_select(n, inp, outp, exp, to);
- return poll_select_finish(&end_time, tvp, PT_TIMEVAL, ret);
+ return poll_select_finish(&end_time, tvp, type, ret);
}
SYSCALL_DEFINE5(select, int, n, fd_set __user *, inp, fd_set __user *, outp,
fd_set __user *, exp, struct __kernel_old_timeval __user *, tvp)
{
- return kern_select(n, inp, outp, exp, tvp);
+ return kern_select(n, inp, outp, exp, tvp, PT_TIMEVAL);
}
static long do_pselect(int n, fd_set __user *inp, fd_set __user *outp,
fd_set __user *exp, void __user *tsp,
- const sigset_t __user *sigmask, size_t sigsetsize,
+ const void __user *sigmask, size_t sigsetsize,
enum poll_time_type type)
{
struct timespec64 ts, end_time, *to = NULL;
@@ -752,7 +773,10 @@ static long do_pselect(int n, fd_set __user *inp, fd_set __user *outp,
return -EINVAL;
}
- ret = set_user_sigmask(sigmask, sigsetsize);
+ if (!in_compat_syscall())
+ ret = set_user_sigmask(sigmask, sigsetsize);
+ else
+ ret = set_compat_user_sigmask(sigmask, sigsetsize);
if (ret)
return ret;
@@ -829,7 +853,7 @@ SYSCALL_DEFINE1(old_select, struct sel_arg_struct __user *, arg)
if (copy_from_user(&a, arg, sizeof(a)))
return -EFAULT;
- return kern_select(a.n, a.inp, a.outp, a.exp, a.tvp);
+ return kern_select(a.n, a.inp, a.outp, a.exp, a.tvp, PT_TIMEVAL);
}
#endif
@@ -1148,146 +1172,14 @@ SYSCALL_DEFINE5(ppoll_time32, struct pollfd __user *, ufds, unsigned int, nfds,
#endif
#ifdef CONFIG_COMPAT
-#define __COMPAT_NFDBITS (8 * sizeof(compat_ulong_t))
-
-/*
- * Ooo, nasty. We need here to frob 32-bit unsigned longs to
- * 64-bit unsigned longs.
- */
-static
-int compat_get_fd_set(unsigned long nr, compat_ulong_t __user *ufdset,
- unsigned long *fdset)
-{
- if (ufdset) {
- return compat_get_bitmap(fdset, ufdset, nr);
- } else {
- zero_fd_set(nr, fdset);
- return 0;
- }
-}
-
-static
-int compat_set_fd_set(unsigned long nr, compat_ulong_t __user *ufdset,
- unsigned long *fdset)
-{
- if (!ufdset)
- return 0;
- return compat_put_bitmap(ufdset, fdset, nr);
-}
-
-
-/*
- * This is a virtual copy of sys_select from fs/select.c and probably
- * should be compared to it from time to time
- */
-
-/*
- * We can actually return ERESTARTSYS instead of EINTR, but I'd
- * like to be certain this leads to no problems. So I return
- * EINTR just for safety.
- *
- * Update: ERESTARTSYS breaks at least the xview clock binary, so
- * I'm trying ERESTARTNOHAND which restart only when you want to.
- */
-static int compat_core_sys_select(int n, compat_ulong_t __user *inp,
- compat_ulong_t __user *outp, compat_ulong_t __user *exp,
- struct timespec64 *end_time)
-{
- fd_set_bits fds;
- void *bits;
- int size, max_fds, ret = -EINVAL;
- struct fdtable *fdt;
- long stack_fds[SELECT_STACK_ALLOC/sizeof(long)];
-
- if (n < 0)
- goto out_nofds;
-
- /* max_fds can increase, so grab it once to avoid race */
- rcu_read_lock();
- fdt = files_fdtable(current->files);
- max_fds = fdt->max_fds;
- rcu_read_unlock();
- if (n > max_fds)
- n = max_fds;
-
- /*
- * We need 6 bitmaps (in/out/ex for both incoming and outgoing),
- * since we used fdset we need to allocate memory in units of
- * long-words.
- */
- size = FDS_BYTES(n);
- bits = stack_fds;
- if (size > sizeof(stack_fds) / 6) {
- bits = kmalloc_array(6, size, GFP_KERNEL);
- ret = -ENOMEM;
- if (!bits)
- goto out_nofds;
- }
- fds.in = (unsigned long *) bits;
- fds.out = (unsigned long *) (bits + size);
- fds.ex = (unsigned long *) (bits + 2*size);
- fds.res_in = (unsigned long *) (bits + 3*size);
- fds.res_out = (unsigned long *) (bits + 4*size);
- fds.res_ex = (unsigned long *) (bits + 5*size);
-
- if ((ret = compat_get_fd_set(n, inp, fds.in)) ||
- (ret = compat_get_fd_set(n, outp, fds.out)) ||
- (ret = compat_get_fd_set(n, exp, fds.ex)))
- goto out;
- zero_fd_set(n, fds.res_in);
- zero_fd_set(n, fds.res_out);
- zero_fd_set(n, fds.res_ex);
-
- ret = do_select(n, &fds, end_time);
-
- if (ret < 0)
- goto out;
- if (!ret) {
- ret = -ERESTARTNOHAND;
- if (signal_pending(current))
- goto out;
- ret = 0;
- }
-
- if (compat_set_fd_set(n, inp, fds.res_in) ||
- compat_set_fd_set(n, outp, fds.res_out) ||
- compat_set_fd_set(n, exp, fds.res_ex))
- ret = -EFAULT;
-out:
- if (bits != stack_fds)
- kfree(bits);
-out_nofds:
- return ret;
-}
-
-static int do_compat_select(int n, compat_ulong_t __user *inp,
- compat_ulong_t __user *outp, compat_ulong_t __user *exp,
- struct old_timeval32 __user *tvp)
-{
- struct timespec64 end_time, *to = NULL;
- struct old_timeval32 tv;
- int ret;
-
- if (tvp) {
- if (copy_from_user(&tv, tvp, sizeof(tv)))
- return -EFAULT;
-
- to = &end_time;
- if (poll_select_set_timeout(to,
- tv.tv_sec + (tv.tv_usec / USEC_PER_SEC),
- (tv.tv_usec % USEC_PER_SEC) * NSEC_PER_USEC))
- return -EINVAL;
- }
-
- ret = compat_core_sys_select(n, inp, outp, exp, to);
- return poll_select_finish(&end_time, tvp, PT_OLD_TIMEVAL, ret);
-}
COMPAT_SYSCALL_DEFINE5(select, int, n, compat_ulong_t __user *, inp,
compat_ulong_t __user *, outp, compat_ulong_t __user *, exp,
struct old_timeval32 __user *, tvp)
{
- return do_compat_select(n, inp, outp, exp, tvp);
+ return kern_select(n, (void __user *)inp, (void __user *)outp,
+ (void __user *)exp, (void __user *)tvp,
+ PT_OLD_TIMEVAL);
}
struct compat_sel_arg_struct {
@@ -1304,43 +1196,9 @@ COMPAT_SYSCALL_DEFINE1(old_select, struct compat_sel_arg_struct __user *, arg)
if (copy_from_user(&a, arg, sizeof(a)))
return -EFAULT;
- return do_compat_select(a.n, compat_ptr(a.inp), compat_ptr(a.outp),
- compat_ptr(a.exp), compat_ptr(a.tvp));
-}
-
-static long do_compat_pselect(int n, compat_ulong_t __user *inp,
- compat_ulong_t __user *outp, compat_ulong_t __user *exp,
- void __user *tsp, compat_sigset_t __user *sigmask,
- compat_size_t sigsetsize, enum poll_time_type type)
-{
- struct timespec64 ts, end_time, *to = NULL;
- int ret;
-
- if (tsp) {
- switch (type) {
- case PT_OLD_TIMESPEC:
- if (get_old_timespec32(&ts, tsp))
- return -EFAULT;
- break;
- case PT_TIMESPEC:
- if (get_timespec64(&ts, tsp))
- return -EFAULT;
- break;
- default:
- BUG();
- }
-
- to = &end_time;
- if (poll_select_set_timeout(to, ts.tv_sec, ts.tv_nsec))
- return -EINVAL;
- }
-
- ret = set_compat_user_sigmask(sigmask, sigsetsize);
- if (ret)
- return ret;
-
- ret = compat_core_sys_select(n, inp, outp, exp, to);
- return poll_select_finish(&end_time, tsp, type, ret);
+ return kern_select(a.n, compat_ptr(a.inp), compat_ptr(a.outp),
+ compat_ptr(a.exp), compat_ptr(a.tvp),
+ PT_OLD_TIMEVAL);
}
struct compat_sigset_argpack {
@@ -1372,8 +1230,9 @@ COMPAT_SYSCALL_DEFINE6(pselect6_time64, int, n, compat_ulong_t __user *, inp,
if (get_compat_sigset_argpack(&x, sig))
return -EFAULT;
- return do_compat_pselect(n, inp, outp, exp, tsp, compat_ptr(x.p),
- x.size, PT_TIMESPEC);
+ return do_pselect(n, (void __user *)inp, (void __user *)outp,
+ (void __user *)exp, (void __user *)tsp,
+ compat_ptr(x.p), x.size, PT_TIMESPEC);
}
#if defined(CONFIG_COMPAT_32BIT_TIME)
@@ -1387,8 +1246,9 @@ COMPAT_SYSCALL_DEFINE6(pselect6_time32, int, n, compat_ulong_t __user *, inp,
if (get_compat_sigset_argpack(&x, sig))
return -EFAULT;
- return do_compat_pselect(n, inp, outp, exp, tsp, compat_ptr(x.p),
- x.size, PT_OLD_TIMESPEC);
+ return do_pselect(n, (void __user *)inp, (void __user *)outp,
+ (void __user *)exp, (void __user *)tsp,
+ compat_ptr(x.p), x.size, PT_OLD_TIMESPEC);
}
#endif
--
2.30.0.284.gd98b1dd5eaa7-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 3/6] ppoll: deduplicate compat logic
2021-01-12 0:30 [PATCH 0/6] fs: deduplicate compat logic Willem de Bruijn
2021-01-12 0:30 ` [PATCH 1/6] selftests/filesystems: add initial select and poll selftest Willem de Bruijn
2021-01-12 0:30 ` [PATCH 2/6] select: deduplicate compat logic Willem de Bruijn
@ 2021-01-12 0:30 ` Willem de Bruijn
2021-01-12 0:30 ` [PATCH 4/6] epoll: " Willem de Bruijn
` (3 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Willem de Bruijn @ 2021-01-12 0:30 UTC (permalink / raw)
To: linux-fsdevel; +Cc: linux-kernel, viro, akpm, willy, arnd, Willem de Bruijn
From: Willem de Bruijn <willemb@google.com>
Apply the same compat deduplication strategy to ppoll that was
previously applied to select and pselect.
Like pselect, ppoll has timespec and sigmask arguments, which have
compat variants. poll has neither, so is not modified.
Convert the ppoll syscall to a do_ppoll() helper that branches on
timespec and sigmask variants internally.
This allows calling the same implementation for all syscall variants:
standard, time32, compat, compat + time32.
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
fs/select.c | 91 ++++++++++++++++++-----------------------------------
1 file changed, 30 insertions(+), 61 deletions(-)
diff --git a/fs/select.c b/fs/select.c
index dee7dfc5217b..27567795a892 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -1120,28 +1120,48 @@ SYSCALL_DEFINE3(poll, struct pollfd __user *, ufds, unsigned int, nfds,
return ret;
}
-SYSCALL_DEFINE5(ppoll, struct pollfd __user *, ufds, unsigned int, nfds,
- struct __kernel_timespec __user *, tsp, const sigset_t __user *, sigmask,
- size_t, sigsetsize)
+static int do_ppoll(struct pollfd __user *ufds, unsigned int nfds,
+ void __user *tsp, const void __user *sigmask,
+ size_t sigsetsize, enum poll_time_type type)
{
struct timespec64 ts, end_time, *to = NULL;
int ret;
if (tsp) {
- if (get_timespec64(&ts, tsp))
- return -EFAULT;
+ switch (type) {
+ case PT_TIMESPEC:
+ if (get_timespec64(&ts, tsp))
+ return -EFAULT;
+ break;
+ case PT_OLD_TIMESPEC:
+ if (get_old_timespec32(&ts, tsp))
+ return -EFAULT;
+ break;
+ default:
+ BUG();
+ }
to = &end_time;
if (poll_select_set_timeout(to, ts.tv_sec, ts.tv_nsec))
return -EINVAL;
}
- ret = set_user_sigmask(sigmask, sigsetsize);
+ if (!in_compat_syscall())
+ ret = set_user_sigmask(sigmask, sigsetsize);
+ else
+ ret = set_compat_user_sigmask(sigmask, sigsetsize);
if (ret)
return ret;
ret = do_sys_poll(ufds, nfds, to);
- return poll_select_finish(&end_time, tsp, PT_TIMESPEC, ret);
+ return poll_select_finish(&end_time, tsp, type, ret);
+}
+
+SYSCALL_DEFINE5(ppoll, struct pollfd __user *, ufds, unsigned int, nfds,
+ struct __kernel_timespec __user *, tsp, const sigset_t __user *, sigmask,
+ size_t, sigsetsize)
+{
+ return do_ppoll(ufds, nfds, tsp, sigmask, sigsetsize, PT_TIMESPEC);
}
#if defined(CONFIG_COMPAT_32BIT_TIME) && !defined(CONFIG_64BIT)
@@ -1150,24 +1170,7 @@ SYSCALL_DEFINE5(ppoll_time32, struct pollfd __user *, ufds, unsigned int, nfds,
struct old_timespec32 __user *, tsp, const sigset_t __user *, sigmask,
size_t, sigsetsize)
{
- struct timespec64 ts, end_time, *to = NULL;
- int ret;
-
- if (tsp) {
- if (get_old_timespec32(&ts, tsp))
- return -EFAULT;
-
- to = &end_time;
- if (poll_select_set_timeout(to, ts.tv_sec, ts.tv_nsec))
- return -EINVAL;
- }
-
- ret = set_user_sigmask(sigmask, sigsetsize);
- if (ret)
- return ret;
-
- ret = do_sys_poll(ufds, nfds, to);
- return poll_select_finish(&end_time, tsp, PT_OLD_TIMESPEC, ret);
+ return do_ppoll(ufds, nfds, tsp, sigmask, sigsetsize, PT_OLD_TIMESPEC);
}
#endif
@@ -1258,24 +1261,7 @@ COMPAT_SYSCALL_DEFINE5(ppoll_time32, struct pollfd __user *, ufds,
unsigned int, nfds, struct old_timespec32 __user *, tsp,
const compat_sigset_t __user *, sigmask, compat_size_t, sigsetsize)
{
- struct timespec64 ts, end_time, *to = NULL;
- int ret;
-
- if (tsp) {
- if (get_old_timespec32(&ts, tsp))
- return -EFAULT;
-
- to = &end_time;
- if (poll_select_set_timeout(to, ts.tv_sec, ts.tv_nsec))
- return -EINVAL;
- }
-
- ret = set_compat_user_sigmask(sigmask, sigsetsize);
- if (ret)
- return ret;
-
- ret = do_sys_poll(ufds, nfds, to);
- return poll_select_finish(&end_time, tsp, PT_OLD_TIMESPEC, ret);
+ return do_ppoll(ufds, nfds, tsp, sigmask, sigsetsize, PT_OLD_TIMESPEC);
}
#endif
@@ -1284,24 +1270,7 @@ COMPAT_SYSCALL_DEFINE5(ppoll_time64, struct pollfd __user *, ufds,
unsigned int, nfds, struct __kernel_timespec __user *, tsp,
const compat_sigset_t __user *, sigmask, compat_size_t, sigsetsize)
{
- struct timespec64 ts, end_time, *to = NULL;
- int ret;
-
- if (tsp) {
- if (get_timespec64(&ts, tsp))
- return -EFAULT;
-
- to = &end_time;
- if (poll_select_set_timeout(to, ts.tv_sec, ts.tv_nsec))
- return -EINVAL;
- }
-
- ret = set_compat_user_sigmask(sigmask, sigsetsize);
- if (ret)
- return ret;
-
- ret = do_sys_poll(ufds, nfds, to);
- return poll_select_finish(&end_time, tsp, PT_TIMESPEC, ret);
+ return do_ppoll(ufds, nfds, tsp, sigmask, sigsetsize, PT_TIMESPEC);
}
#endif
--
2.30.0.284.gd98b1dd5eaa7-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 4/6] epoll: deduplicate compat logic
2021-01-12 0:30 [PATCH 0/6] fs: deduplicate compat logic Willem de Bruijn
` (2 preceding siblings ...)
2021-01-12 0:30 ` [PATCH 3/6] ppoll: " Willem de Bruijn
@ 2021-01-12 0:30 ` Willem de Bruijn
2021-01-12 0:30 ` [PATCH 5/6] compat: add set_maybe_compat_user_sigmask helper Willem de Bruijn
` (2 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Willem de Bruijn @ 2021-01-12 0:30 UTC (permalink / raw)
To: linux-fsdevel; +Cc: linux-kernel, viro, akpm, willy, arnd, Willem de Bruijn
From: Willem de Bruijn <willemb@google.com>
Apply the same compat deduplication strategy to epoll that was
previously applied to (p)select and ppoll.
Make do_epoll_wait handle both variants of sigmask. This removes
the need for near duplicate do_compat_epoll_pwait.
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
fs/eventpoll.c | 38 +++++++++-----------------------------
1 file changed, 9 insertions(+), 29 deletions(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index a829af074eb5..c9dcffba2da1 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -2239,7 +2239,7 @@ SYSCALL_DEFINE4(epoll_wait, int, epfd, struct epoll_event __user *, events,
*/
static int do_epoll_pwait(int epfd, struct epoll_event __user *events,
int maxevents, struct timespec64 *to,
- const sigset_t __user *sigmask, size_t sigsetsize)
+ const void __user *sigmask, size_t sigsetsize)
{
int error;
@@ -2247,7 +2247,10 @@ static int do_epoll_pwait(int epfd, struct epoll_event __user *events,
* If the caller wants a certain signal mask to be set during the wait,
* we apply it here.
*/
- error = set_user_sigmask(sigmask, sigsetsize);
+ if (!in_compat_syscall())
+ error = set_user_sigmask(sigmask, sigsetsize);
+ else
+ error = set_compat_user_sigmask(sigmask, sigsetsize);
if (error)
return error;
@@ -2288,28 +2291,6 @@ SYSCALL_DEFINE6(epoll_pwait2, int, epfd, struct epoll_event __user *, events,
}
#ifdef CONFIG_COMPAT
-static int do_compat_epoll_pwait(int epfd, struct epoll_event __user *events,
- int maxevents, struct timespec64 *timeout,
- const compat_sigset_t __user *sigmask,
- compat_size_t sigsetsize)
-{
- long err;
-
- /*
- * If the caller wants a certain signal mask to be set during the wait,
- * we apply it here.
- */
- err = set_compat_user_sigmask(sigmask, sigsetsize);
- if (err)
- return err;
-
- err = do_epoll_wait(epfd, events, maxevents, timeout);
-
- restore_saved_sigmask_unless(err == -EINTR);
-
- return err;
-}
-
COMPAT_SYSCALL_DEFINE6(epoll_pwait, int, epfd,
struct epoll_event __user *, events,
int, maxevents, int, timeout,
@@ -2318,9 +2299,9 @@ COMPAT_SYSCALL_DEFINE6(epoll_pwait, int, epfd,
{
struct timespec64 to;
- return do_compat_epoll_pwait(epfd, events, maxevents,
- ep_timeout_to_timespec(&to, timeout),
- sigmask, sigsetsize);
+ return do_epoll_pwait(epfd, events, maxevents,
+ ep_timeout_to_timespec(&to, timeout),
+ sigmask, sigsetsize);
}
COMPAT_SYSCALL_DEFINE6(epoll_pwait2, int, epfd,
@@ -2340,8 +2321,7 @@ COMPAT_SYSCALL_DEFINE6(epoll_pwait2, int, epfd,
return -EINVAL;
}
- return do_compat_epoll_pwait(epfd, events, maxevents, to,
- sigmask, sigsetsize);
+ return do_epoll_pwait(epfd, events, maxevents, to, sigmask, sigsetsize);
}
#endif
--
2.30.0.284.gd98b1dd5eaa7-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 5/6] compat: add set_maybe_compat_user_sigmask helper
2021-01-12 0:30 [PATCH 0/6] fs: deduplicate compat logic Willem de Bruijn
` (3 preceding siblings ...)
2021-01-12 0:30 ` [PATCH 4/6] epoll: " Willem de Bruijn
@ 2021-01-12 0:30 ` Willem de Bruijn
2021-01-12 7:11 ` kernel test robot
2021-01-12 0:30 ` [PATCH 6/6] io_pgetevents: deduplicate compat logic Willem de Bruijn
2021-01-12 0:58 ` [PATCH 0/6] fs: " Al Viro
6 siblings, 1 reply; 10+ messages in thread
From: Willem de Bruijn @ 2021-01-12 0:30 UTC (permalink / raw)
To: linux-fsdevel
Cc: linux-kernel, viro, akpm, willy, arnd, Willem de Bruijn,
Jens Axboe
From: Willem de Bruijn <willemb@google.com>
Deduplicate the open coded branch on sigmask compat handling.
Signed-off-by: Willem de Bruijn <willemb@google.com>
Cc: Jens Axboe <axboe@kernel.dk>
---
fs/eventpoll.c | 5 +----
fs/io_uring.c | 9 +--------
fs/select.c | 10 ++--------
include/linux/compat.h | 10 ++++++++++
4 files changed, 14 insertions(+), 20 deletions(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index c9dcffba2da1..c011327c8402 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -2247,10 +2247,7 @@ static int do_epoll_pwait(int epfd, struct epoll_event __user *events,
* If the caller wants a certain signal mask to be set during the wait,
* we apply it here.
*/
- if (!in_compat_syscall())
- error = set_user_sigmask(sigmask, sigsetsize);
- else
- error = set_compat_user_sigmask(sigmask, sigsetsize);
+ error = set_maybe_compat_user_sigmask(sigmask, sigsetsize);
if (error)
return error;
diff --git a/fs/io_uring.c b/fs/io_uring.c
index fdc923e53873..abc88bc738ce 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -7190,14 +7190,7 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
} while (1);
if (sig) {
-#ifdef CONFIG_COMPAT
- if (in_compat_syscall())
- ret = set_compat_user_sigmask((const compat_sigset_t __user *)sig,
- sigsz);
- else
-#endif
- ret = set_user_sigmask(sig, sigsz);
-
+ ret = set_maybe_compat_user_sigmask(sig, sigsz);
if (ret)
return ret;
}
diff --git a/fs/select.c b/fs/select.c
index 27567795a892..c013662bbf51 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -773,10 +773,7 @@ static long do_pselect(int n, fd_set __user *inp, fd_set __user *outp,
return -EINVAL;
}
- if (!in_compat_syscall())
- ret = set_user_sigmask(sigmask, sigsetsize);
- else
- ret = set_compat_user_sigmask(sigmask, sigsetsize);
+ ret = set_maybe_compat_user_sigmask(sigmask, sigsetsize);
if (ret)
return ret;
@@ -1146,10 +1143,7 @@ static int do_ppoll(struct pollfd __user *ufds, unsigned int nfds,
return -EINVAL;
}
- if (!in_compat_syscall())
- ret = set_user_sigmask(sigmask, sigsetsize);
- else
- ret = set_compat_user_sigmask(sigmask, sigsetsize);
+ ret = set_maybe_compat_user_sigmask(sigmask, sigsetsize);
if (ret)
return ret;
diff --git a/include/linux/compat.h b/include/linux/compat.h
index 6e65be753603..4a9b740496b4 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -18,6 +18,7 @@
#include <linux/aio_abi.h> /* for aio_context_t */
#include <linux/uaccess.h>
#include <linux/unistd.h>
+#include <linux/sched/signal.h>
#include <asm/compat.h>
@@ -942,6 +943,15 @@ static inline bool in_compat_syscall(void) { return false; }
#endif /* CONFIG_COMPAT */
+static inline int set_maybe_compat_user_sigmask(const void __user *umask,
+ size_t sigsetsize)
+{
+ if (!in_compat_syscall())
+ return set_user_sigmask(umask, sigsetsize);
+ else
+ return set_compat_user_sigmask(umask, sigsetsize);
+}
+
/*
* Some legacy ABIs like the i386 one use less than natural alignment for 64-bit
* types, and will need special compat treatment for that. Most architectures
--
2.30.0.284.gd98b1dd5eaa7-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 5/6] compat: add set_maybe_compat_user_sigmask helper
2021-01-12 0:30 ` [PATCH 5/6] compat: add set_maybe_compat_user_sigmask helper Willem de Bruijn
@ 2021-01-12 7:11 ` kernel test robot
0 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2021-01-12 7:11 UTC (permalink / raw)
To: Willem de Bruijn, linux-fsdevel
Cc: kbuild-all, linux-kernel, viro, akpm, willy, arnd,
Willem de Bruijn, Jens Axboe
[-- Attachment #1: Type: text/plain, Size: 4552 bytes --]
Hi Willem,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on kselftest/next]
[also build test ERROR on linus/master v5.11-rc3 next-20210111]
[cannot apply to linux/master hnaz-linux-mm/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Willem-de-Bruijn/fs-deduplicate-compat-logic/20210112-083603
base: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git next
config: powerpc64-randconfig-r034-20210111 (attached as .config)
compiler: powerpc-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/b49b48b376694f2a27d3c494f2bf638d0c8f7650
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Willem-de-Bruijn/fs-deduplicate-compat-logic/20210112-083603
git checkout b49b48b376694f2a27d3c494f2bf638d0c8f7650
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from arch/powerpc/kernel/asm-offsets.c:14:
include/linux/compat.h: In function 'set_maybe_compat_user_sigmask':
>> include/linux/compat.h:952:10: error: implicit declaration of function 'set_compat_user_sigmask'; did you mean 'set_maybe_compat_user_sigmask'? [-Werror=implicit-function-declaration]
952 | return set_compat_user_sigmask(umask, sigsetsize);
| ^~~~~~~~~~~~~~~~~~~~~~~
| set_maybe_compat_user_sigmask
cc1: some warnings being treated as errors
--
WARNING: unmet direct dependencies detected for NET_DEVLINK
Depends on NET
Selected by
- BNXT && NETDEVICES && ETHERNET && NET_VENDOR_BROADCOM && PCI
- NFP && NETDEVICES && ETHERNET && NET_VENDOR_NETRONOME && PCI && PCI_MSI && (VXLAN || VXLAN && (TLS && TLS_DEVICE || !TLS_DEVICE
In file included from arch/powerpc/kernel/asm-offsets.c:14:
include/linux/compat.h: In function 'set_maybe_compat_user_sigmask':
>> include/linux/compat.h:952:10: error: implicit declaration of function 'set_compat_user_sigmask'; did you mean
952 | return set_compat_user_sigmask(umask, sigsetsize);
| ^~~~~~~~~~~~~~~~~~~~~~~
| set_maybe_compat_user_sigmask
cc1: some warnings being treated as errors
Makefile arch include kernel scripts source usr [scripts/Makefile.build:117: arch/powerpc/kernel/asm-offsets.s] Error 1
Target '__build' not remade because of errors.
Makefile arch include kernel scripts source usr [Makefile:1206: prepare0] Error 2
Target 'prepare' not remade because of errors.
make: Makefile arch include kernel scripts source usr [Makefile:185: __sub-make] Error 2
make: Target 'prepare' not remade because of errors.
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for NETDEVICES
Depends on NET
Selected by
- AKEBONO && PPC_47x
WARNING: unmet direct dependencies detected for ETHERNET
Depends on NETDEVICES && NET
Selected by
- AKEBONO && PPC_47x
WARNING: unmet direct dependencies detected for FAILOVER
Depends on NET
Selected by
- NET_FAILOVER && NETDEVICES
WARNING: unmet direct dependencies detected for PAGE_POOL
Depends on NET
Selected by
- BNXT && NETDEVICES && ETHERNET && NET_VENDOR_BROADCOM && PCI
WARNING: unmet direct dependencies detected for NET_DEVLINK
Depends on NET
Selected by
- BNXT && NETDEVICES && ETHERNET && NET_VENDOR_BROADCOM && PCI
- NFP && NETDEVICES && ETHERNET && NET_VENDOR_NETRONOME && PCI && PCI_MSI && (VXLAN || VXLAN && (TLS && TLS_DEVICE || !TLS_DEVICE
vim +/set_compat_user_sigmask +952 include/linux/compat.h
945
946 static inline int set_maybe_compat_user_sigmask(const void __user *umask,
947 size_t sigsetsize)
948 {
949 if (!in_compat_syscall())
950 return set_user_sigmask(umask, sigsetsize);
951 else
> 952 return set_compat_user_sigmask(umask, sigsetsize);
953 }
954
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26622 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 6/6] io_pgetevents: deduplicate compat logic
2021-01-12 0:30 [PATCH 0/6] fs: deduplicate compat logic Willem de Bruijn
` (4 preceding siblings ...)
2021-01-12 0:30 ` [PATCH 5/6] compat: add set_maybe_compat_user_sigmask helper Willem de Bruijn
@ 2021-01-12 0:30 ` Willem de Bruijn
2021-01-12 0:58 ` [PATCH 0/6] fs: " Al Viro
6 siblings, 0 replies; 10+ messages in thread
From: Willem de Bruijn @ 2021-01-12 0:30 UTC (permalink / raw)
To: linux-fsdevel
Cc: linux-kernel, viro, akpm, willy, arnd, Willem de Bruijn,
Benjamin LaHaise
From: Willem de Bruijn <willemb@google.com>
io_pgetevents has four variants, including compat variants of both
timespec and sigmask.
With set_maybe_compat_user_sigmask helper, the latter can be
deduplicated. Move the shared logic to new do_io_pgetevents,
analogous to do_io_getevents.
Signed-off-by: Willem de Bruijn <willemb@google.com>
Cc: Benjamin LaHaise <bcrl@kvack.org>
---
fs/aio.c | 94 ++++++++++++++++++++++----------------------------------
1 file changed, 37 insertions(+), 57 deletions(-)
diff --git a/fs/aio.c b/fs/aio.c
index d213be7b8a7e..56460ab47d64 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -2101,6 +2101,31 @@ struct __aio_sigset {
size_t sigsetsize;
};
+static long do_io_pgetevents(aio_context_t ctx_id,
+ long min_nr,
+ long nr,
+ struct io_event __user *events,
+ struct timespec64 *ts,
+ const void __user *umask,
+ size_t sigsetsize)
+{
+ bool interrupted;
+ int ret;
+
+ ret = set_maybe_compat_user_sigmask(umask, sigsetsize);
+ if (ret)
+ return ret;
+
+ ret = do_io_getevents(ctx_id, min_nr, nr, events, ts);
+
+ interrupted = signal_pending(current);
+ restore_saved_sigmask_unless(interrupted);
+ if (interrupted && !ret)
+ ret = -ERESTARTNOHAND;
+
+ return ret;
+}
+
SYSCALL_DEFINE6(io_pgetevents,
aio_context_t, ctx_id,
long, min_nr,
@@ -2111,8 +2136,6 @@ SYSCALL_DEFINE6(io_pgetevents,
{
struct __aio_sigset ksig = { NULL, };
struct timespec64 ts;
- bool interrupted;
- int ret;
if (timeout && unlikely(get_timespec64(&ts, timeout)))
return -EFAULT;
@@ -2120,18 +2143,9 @@ SYSCALL_DEFINE6(io_pgetevents,
if (usig && copy_from_user(&ksig, usig, sizeof(ksig)))
return -EFAULT;
- ret = set_user_sigmask(ksig.sigmask, ksig.sigsetsize);
- if (ret)
- return ret;
-
- ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &ts : NULL);
-
- interrupted = signal_pending(current);
- restore_saved_sigmask_unless(interrupted);
- if (interrupted && !ret)
- ret = -ERESTARTNOHAND;
-
- return ret;
+ return do_io_pgetevents(ctx_id, min_nr, nr, events,
+ timeout ? &ts : NULL,
+ ksig.sigmask, ksig.sigsetsize);
}
#if defined(CONFIG_COMPAT_32BIT_TIME) && !defined(CONFIG_64BIT)
@@ -2146,8 +2160,6 @@ SYSCALL_DEFINE6(io_pgetevents_time32,
{
struct __aio_sigset ksig = { NULL, };
struct timespec64 ts;
- bool interrupted;
- int ret;
if (timeout && unlikely(get_old_timespec32(&ts, timeout)))
return -EFAULT;
@@ -2155,19 +2167,9 @@ SYSCALL_DEFINE6(io_pgetevents_time32,
if (usig && copy_from_user(&ksig, usig, sizeof(ksig)))
return -EFAULT;
-
- ret = set_user_sigmask(ksig.sigmask, ksig.sigsetsize);
- if (ret)
- return ret;
-
- ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &ts : NULL);
-
- interrupted = signal_pending(current);
- restore_saved_sigmask_unless(interrupted);
- if (interrupted && !ret)
- ret = -ERESTARTNOHAND;
-
- return ret;
+ return do_io_pgetevents(ctx_id, min_nr, nr, events,
+ timeout ? &ts : NULL,
+ ksig.sigmask, ksig.sigsetsize);
}
#endif
@@ -2213,8 +2215,6 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents,
{
struct __compat_aio_sigset ksig = { 0, };
struct timespec64 t;
- bool interrupted;
- int ret;
if (timeout && get_old_timespec32(&t, timeout))
return -EFAULT;
@@ -2222,18 +2222,9 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents,
if (usig && copy_from_user(&ksig, usig, sizeof(ksig)))
return -EFAULT;
- ret = set_compat_user_sigmask(compat_ptr(ksig.sigmask), ksig.sigsetsize);
- if (ret)
- return ret;
-
- ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &t : NULL);
-
- interrupted = signal_pending(current);
- restore_saved_sigmask_unless(interrupted);
- if (interrupted && !ret)
- ret = -ERESTARTNOHAND;
-
- return ret;
+ return do_io_pgetevents(ctx_id, min_nr, nr, events,
+ timeout ? &t : NULL,
+ compat_ptr(ksig.sigmask), ksig.sigsetsize);
}
#endif
@@ -2248,8 +2239,6 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents_time64,
{
struct __compat_aio_sigset ksig = { 0, };
struct timespec64 t;
- bool interrupted;
- int ret;
if (timeout && get_timespec64(&t, timeout))
return -EFAULT;
@@ -2257,17 +2246,8 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents_time64,
if (usig && copy_from_user(&ksig, usig, sizeof(ksig)))
return -EFAULT;
- ret = set_compat_user_sigmask(compat_ptr(ksig.sigmask), ksig.sigsetsize);
- if (ret)
- return ret;
-
- ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &t : NULL);
-
- interrupted = signal_pending(current);
- restore_saved_sigmask_unless(interrupted);
- if (interrupted && !ret)
- ret = -ERESTARTNOHAND;
-
- return ret;
+ return do_io_pgetevents(ctx_id, min_nr, nr, events,
+ timeout ? &t : NULL,
+ compat_ptr(ksig.sigmask), ksig.sigsetsize);
}
#endif
--
2.30.0.284.gd98b1dd5eaa7-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 0/6] fs: deduplicate compat logic
2021-01-12 0:30 [PATCH 0/6] fs: deduplicate compat logic Willem de Bruijn
` (5 preceding siblings ...)
2021-01-12 0:30 ` [PATCH 6/6] io_pgetevents: deduplicate compat logic Willem de Bruijn
@ 2021-01-12 0:58 ` Al Viro
2021-01-12 1:16 ` Willem de Bruijn
6 siblings, 1 reply; 10+ messages in thread
From: Al Viro @ 2021-01-12 0:58 UTC (permalink / raw)
To: Willem de Bruijn
Cc: linux-fsdevel, linux-kernel, akpm, willy, arnd, Willem de Bruijn
On Mon, Jan 11, 2021 at 07:30:11PM -0500, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
>
> Use in_compat_syscall() to differentiate compat handling exactly
> where needed, including in nested function calls. Then remove
> duplicated code in callers.
IMO it's a bad idea. Use of in_compat_syscall() is hard to avoid
in some cases, but let's not use it without a good reason. It
makes the code harder to reason about.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 0/6] fs: deduplicate compat logic
2021-01-12 0:58 ` [PATCH 0/6] fs: " Al Viro
@ 2021-01-12 1:16 ` Willem de Bruijn
0 siblings, 0 replies; 10+ messages in thread
From: Willem de Bruijn @ 2021-01-12 1:16 UTC (permalink / raw)
To: Al Viro
Cc: Willem de Bruijn, Linux FS-devel Mailing List, linux-kernel,
Andrew Morton, Matthew Wilcox, Arnd Bergmann
On Mon, Jan 11, 2021 at 7:58 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Mon, Jan 11, 2021 at 07:30:11PM -0500, Willem de Bruijn wrote:
> > From: Willem de Bruijn <willemb@google.com>
> >
> > Use in_compat_syscall() to differentiate compat handling exactly
> > where needed, including in nested function calls. Then remove
> > duplicated code in callers.
>
> IMO it's a bad idea. Use of in_compat_syscall() is hard to avoid
> in some cases, but let's not use it without a good reason. It
> makes the code harder to reason about.
In the specific cases of select, poll and epoll, this removes quite a
bit of duplicate code that may diverge over time. Indeed, for select
already has. Reduction of duplication may also make subsequent changes
more feasible. We discussed avoiding in epoll an unnecessary
ktime_get_ts64 in select_estimate_accuracy, which requires plumbing a
variable through these intermediate helpers.
I also personally find the code simpler to understand without the
various near duplicates. The change exposes their differences
more clearly. select is the best example of this.
The last two patches I added based on earlier comments. Perhaps
the helper in 5 adds more churn than it's worth.
^ permalink raw reply [flat|nested] 10+ messages in thread