* Re: [PATCH net-next v6 3/4] eventpoll: Add per-epoll prefer busy poll option
From: Jakub Kicinski @ 2024-02-07 19:04 UTC (permalink / raw)
To: Joe Damato
Cc: linux-kernel, netdev, chuck.lever, jlayton, linux-api, brauner,
edumazet, davem, alexander.duyck, sridhar.samudrala,
willemdebruijn.kernel, weiwan, David.Laight, arnd, sdf,
amritha.nambiar, Alexander Viro, Jan Kara,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240205210453.11301-4-jdamato@fastly.com>
On Mon, 5 Feb 2024 21:04:48 +0000 Joe Damato wrote:
> When using epoll-based busy poll, the prefer_busy_poll option is hardcoded
> to false. Users may want to enable prefer_busy_poll to be used in
> conjunction with gro_flush_timeout and defer_hard_irqs_count to keep device
> IRQs masked.
>
> Other busy poll methods allow enabling or disabling prefer busy poll via
> SO_PREFER_BUSY_POLL, but epoll-based busy polling uses a hardcoded value.
>
> Fix this edge case by adding support for a per-epoll context
> prefer_busy_poll option. The default is false, as it was hardcoded before
> this change.
Reviewed-by: Jakub Kicinski <kuba@kernel.org>
^ permalink raw reply
* Re: [PATCH net-next v6 2/4] eventpoll: Add per-epoll busy poll packet budget
From: Jakub Kicinski @ 2024-02-07 19:04 UTC (permalink / raw)
To: Joe Damato
Cc: linux-kernel, netdev, chuck.lever, jlayton, linux-api, brauner,
edumazet, davem, alexander.duyck, sridhar.samudrala,
willemdebruijn.kernel, weiwan, David.Laight, arnd, sdf,
amritha.nambiar, Alexander Viro, Jan Kara,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240205210453.11301-3-jdamato@fastly.com>
On Mon, 5 Feb 2024 21:04:47 +0000 Joe Damato wrote:
> When using epoll-based busy poll, the packet budget is hardcoded to
> BUSY_POLL_BUDGET (8). Users may desire larger busy poll budgets, which
> can potentially increase throughput when busy polling under high network
> load.
>
> Other busy poll methods allow setting the busy poll budget via
> SO_BUSY_POLL_BUDGET, but epoll-based busy polling uses a hardcoded
> value.
>
> Fix this edge case by adding support for a per-epoll context busy poll
> packet budget. If not specified, the default value (BUSY_POLL_BUDGET) is
> used.
Reviewed-by: Jakub Kicinski <kuba@kernel.org>
^ permalink raw reply
* Re: [PATCH net-next v6 1/4] eventpoll: support busy poll per epoll instance
From: Jakub Kicinski @ 2024-02-07 19:04 UTC (permalink / raw)
To: Joe Damato
Cc: linux-kernel, netdev, chuck.lever, jlayton, linux-api, brauner,
edumazet, davem, alexander.duyck, sridhar.samudrala,
willemdebruijn.kernel, weiwan, David.Laight, arnd, sdf,
amritha.nambiar, Alexander Viro, Jan Kara,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240205210453.11301-2-jdamato@fastly.com>
On Mon, 5 Feb 2024 21:04:46 +0000 Joe Damato wrote:
> Allow busy polling on a per-epoll context basis. The per-epoll context
> usec timeout value is preferred, but the pre-existing system wide sysctl
> value is still supported if it specified.
Why do we need u64 for usecs? I think u16 would do, and u32 would give
a very solid "engineering margin". If it was discussed in previous
versions I think it's worth explaining in the commit message.
^ permalink raw reply
* Re: [PATCH net-next v6 4/4] eventpoll: Add epoll ioctl for epoll_params
From: Joe Damato @ 2024-02-07 18:50 UTC (permalink / raw)
To: kuba, netdev, linux-kernel
Cc: linux-kernel, netdev, chuck.lever, jlayton, linux-api, brauner,
edumazet, davem, alexander.duyck, sridhar.samudrala, kuba,
willemdebruijn.kernel, weiwan, David.Laight, arnd, sdf,
amritha.nambiar, Jonathan Corbet, Alexander Viro, Jan Kara,
Nathan Lynch, Michael Ellerman, Greg Kroah-Hartman, Namjae Jeon,
Steve French, Thomas Zimmermann, Julien Panis, Andrew Waterman,
Palmer Dabbelt, Albert Ou, open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <ec9791cf-d0a2-4d75-a7d6-00bcab92e823@kernel.org>
On Wed, Feb 07, 2024 at 09:37:14AM +0100, Jiri Slaby wrote:
> On 05. 02. 24, 22:04, Joe Damato wrote:
> >Add an ioctl for getting and setting epoll_params. User programs can use
> >this ioctl to get and set the busy poll usec time, packet budget, and
> >prefer busy poll params for a specific epoll context.
> >
> >Parameters are limited:
> > - busy_poll_usecs is limited to <= u32_max
> > - busy_poll_budget is limited to <= NAPI_POLL_WEIGHT by unprivileged
> > users (!capable(CAP_NET_ADMIN))
> > - prefer_busy_poll must be 0 or 1
> > - __pad must be 0
> >
> >Signed-off-by: Joe Damato <jdamato@fastly.com>
> ...
> >--- a/fs/eventpoll.c
> >+++ b/fs/eventpoll.c
> ...
> >@@ -497,6 +498,50 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
> > ep->napi_id = napi_id;
> > }
> >+static long ep_eventpoll_bp_ioctl(struct file *file, unsigned int cmd,
> >+ unsigned long arg)
> >+{
> >+ struct eventpoll *ep;
> >+ struct epoll_params epoll_params;
> >+ void __user *uarg = (void __user *) arg;
> >+
> >+ ep = file->private_data;
>
> This might have been on the ep declaration line.
>
> >+ switch (cmd) {
> >+ case EPIOCSPARAMS:
> >+ if (copy_from_user(&epoll_params, uarg, sizeof(epoll_params)))
> >+ return -EFAULT;
> >+
> >+ if (memchr_inv(epoll_params.__pad, 0, sizeof(epoll_params.__pad)))
> >+ return -EINVAL;
> >+
> >+ if (epoll_params.busy_poll_usecs > U32_MAX)
> >+ return -EINVAL;
> >+
> >+ if (epoll_params.prefer_busy_poll > 1)
> >+ return -EINVAL;
> >+
> >+ if (epoll_params.busy_poll_budget > NAPI_POLL_WEIGHT &&
> >+ !capable(CAP_NET_ADMIN))
> >+ return -EPERM;
> >+
> >+ ep->busy_poll_usecs = epoll_params.busy_poll_usecs;
> >+ ep->busy_poll_budget = epoll_params.busy_poll_budget;
> >+ ep->prefer_busy_poll = !!epoll_params.prefer_busy_poll;
>
> This !! is unnecessary. Nonzero values shall be "converted" to true.
>
> But FWIW, the above is nothing which should be blocking, so:
">
> Reviewed-by: Jiri Slaby <jirislaby@kernel.org>
netdev maintainers: Jiri marked this with Reviewed-by, but was this review
what caused "Changes Requested" to be the status set for this patch set in
patchwork?
If needed, I'll send a v7 with the changes Jiri suggested and add the
"Reviewed-by" since the changes are cosmetic, but I wanted to make sure
this was the reason.
Thanks.
^ permalink raw reply
* Re: [PATCH v2] pidfd: getfd should always report ESRCH if a task is exiting
From: Tycho Andersen @ 2024-02-07 13:38 UTC (permalink / raw)
To: Christian Brauner
Cc: Oleg Nesterov, Eric W . Biederman, linux-kernel, linux-api,
Tycho Andersen
In-Reply-To: <20240207-blumen-neulich-f1507e0c5cc0@brauner>
On Wed, Feb 07, 2024 at 10:50:26AM +0100, Christian Brauner wrote:
> So this is what I have applied currently where I moved the check into
> __pidfd_fget() where it makes more sense imho. But please double check
> that I didn't mess anything up:
Oof, good catch. Looks good to me.
Thanks,
Tycho
^ permalink raw reply
* [PATCH] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Oleg Nesterov @ 2024-02-07 11:45 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
linux-kernel
Turn kill_pid_info() into kill_pid_info_type(), this allows to pass any
pid_type to group_send_sig_info(), despite its name it should work fine
even if type = PIDTYPE_PID.
Change pidfd_send_signal() to use PIDTYPE_PID or PIDTYPE_TGID depending
on PIDFD_THREAD.
While at it kill another TODO comment in pidfd_show_fdinfo(). As Christian
expains fdinfo reports f_flags, userspace can already detect PIDFD_THREAD.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
kernel/fork.c | 2 --
kernel/signal.c | 18 ++++++++++++------
2 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/kernel/fork.c b/kernel/fork.c
index cd61ca87d0e6..47b565598063 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2051,8 +2051,6 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
seq_put_decimal_ll(m, "Pid:\t", nr);
- /* TODO: report PIDFD_THREAD */
-
#ifdef CONFIG_PID_NS
seq_put_decimal_ll(m, "\nNSpid:\t", nr);
if (nr > 0) {
diff --git a/kernel/signal.c b/kernel/signal.c
index c3fac06937e2..e3edcd784e45 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -47,6 +47,7 @@
#include <linux/cgroup.h>
#include <linux/audit.h>
#include <linux/sysctl.h>
+#include <uapi/linux/pidfd.h>
#define CREATE_TRACE_POINTS
#include <trace/events/signal.h>
@@ -1478,7 +1479,8 @@ int __kill_pgrp_info(int sig, struct kernel_siginfo *info, struct pid *pgrp)
return ret;
}
-int kill_pid_info(int sig, struct kernel_siginfo *info, struct pid *pid)
+static int kill_pid_info_type(int sig, struct kernel_siginfo *info,
+ struct pid *pid, enum pid_type type)
{
int error = -ESRCH;
struct task_struct *p;
@@ -1487,11 +1489,10 @@ int kill_pid_info(int sig, struct kernel_siginfo *info, struct pid *pid)
rcu_read_lock();
p = pid_task(pid, PIDTYPE_PID);
if (p)
- error = group_send_sig_info(sig, info, p, PIDTYPE_TGID);
+ error = group_send_sig_info(sig, info, p, type);
rcu_read_unlock();
if (likely(!p || error != -ESRCH))
return error;
-
/*
* The task was unhashed in between, try again. If it
* is dead, pid_task() will return NULL, if we race with
@@ -1500,6 +1501,11 @@ int kill_pid_info(int sig, struct kernel_siginfo *info, struct pid *pid)
}
}
+int kill_pid_info(int sig, struct kernel_siginfo *info, struct pid *pid)
+{
+ return kill_pid_info_type(sig, info, pid, PIDTYPE_TGID);
+}
+
static int kill_proc_info(int sig, struct kernel_siginfo *info, pid_t pid)
{
int error;
@@ -3890,6 +3896,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
struct fd f;
struct pid *pid;
kernel_siginfo_t kinfo;
+ enum pid_type type;
/* Enforce flags be set to 0 until we add an extension. */
if (flags)
@@ -3928,9 +3935,8 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
prepare_kill_siginfo(sig, &kinfo);
}
- /* TODO: respect PIDFD_THREAD */
- ret = kill_pid_info(sig, &kinfo, pid);
-
+ type = (f.file->f_flags & PIDFD_THREAD) ? PIDTYPE_PID : PIDTYPE_TGID;
+ ret = kill_pid_info_type(sig, &kinfo, pid, type);
err:
fdput(f);
return ret;
--
2.25.1.362.g51ebf55
^ permalink raw reply related
* Re: [PATCH v2] pidfd: getfd should always report ESRCH if a task is exiting
From: Christian Brauner @ 2024-02-07 11:17 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Tycho Andersen, Eric W . Biederman, linux-kernel, linux-api,
Tycho Andersen
In-Reply-To: <20240207103201.GB6627@redhat.com>
On Wed, Feb 07, 2024 at 11:32:01AM +0100, Oleg Nesterov wrote:
> On 02/07, Christian Brauner wrote:
> >
> > So this is what I have applied currently where I moved the check into
> > __pidfd_fget() where it makes more sense imho. But please double check
> > that I didn't mess anything up:
>
> LGTM.
>
> You have already applied this patch, no need to add my ack, but FWIW
>
> Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Added. Thank you!
^ permalink raw reply
* Re: [PATCH v2] pidfd: getfd should always report ESRCH if a task is exiting
From: Oleg Nesterov @ 2024-02-07 10:32 UTC (permalink / raw)
To: Christian Brauner
Cc: Tycho Andersen, Eric W . Biederman, linux-kernel, linux-api,
Tycho Andersen
In-Reply-To: <20240207-blumen-neulich-f1507e0c5cc0@brauner>
On 02/07, Christian Brauner wrote:
>
> So this is what I have applied currently where I moved the check into
> __pidfd_fget() where it makes more sense imho. But please double check
> that I didn't mess anything up:
LGTM.
You have already applied this patch, no need to add my ack, but FWIW
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
^ permalink raw reply
* Re: [PATCH] pidfd: getfd should always report ESRCH if a task is exiting
From: Oleg Nesterov @ 2024-02-07 10:28 UTC (permalink / raw)
To: Christian Brauner
Cc: Tycho Andersen, Eric W . Biederman, linux-kernel, linux-api,
Tycho Andersen
In-Reply-To: <20240207-beseitigen-ausfliegen-b2b95de67c4f@brauner>
On 02/07, Christian Brauner wrote:
>
> On Tue, Feb 06, 2024 at 08:25:54PM +0100, Oleg Nesterov wrote:
> > On 02/06, Tycho Andersen wrote:
> >
> > > On Tue, Feb 06, 2024 at 07:06:07PM +0100, Oleg Nesterov wrote:
> > > > Or we can check task->files != NULL rather than PF_EXITING.
> > > >
> > > > To me this looks even better, but looks more confusing without a comment.
> > > > OTOH, imo this needs a comment anyway ;)
> > >
> > > I thought about this, but I didn't really understand the null check in
> > > exit_files();
> >
> > I guess task->files can be NULL at least if it was cloned with
> > kernel_clone_args->no_files == T
>
> Won't this give false positives for vhost workers which do set
> ->no_files but are user workers? IOW, return -ESRCH even though -EBADF
> would be correct in this scenario?
OK, agreed. Lets check PF_EXITING or exit_state.
Oleg.
^ permalink raw reply
* Re: [PATCH v2] pidfd: getfd should always report ESRCH if a task is exiting
From: Christian Brauner @ 2024-02-07 9:50 UTC (permalink / raw)
To: Tycho Andersen, Oleg Nesterov
Cc: Eric W . Biederman, linux-kernel, linux-api, Tycho Andersen
In-Reply-To: <20240207-daran-fliesen-6039a2e36f39@brauner>
On Wed, Feb 07, 2024 at 10:29:51AM +0100, Christian Brauner wrote:
> On Wed, Feb 07, 2024 at 10:15:25AM +0100, Christian Brauner wrote:
> > On Tue, Feb 06, 2024 at 12:23:57PM -0700, Tycho Andersen wrote:
> > > From: Tycho Andersen <tandersen@netflix.com>
> > >
> > > We can get EBADF from __pidfd_fget() if a task is currently exiting, which
> > > might be confusing. Let's check PF_EXITING, and just report ESRCH if so.
> > >
> > > I chose PF_EXITING, because it is set in exit_signals(), which is called
> > > before exit_files(). Since ->exit_status is mostly set after exit_files()
> > > in exit_notify(), using that still leaves a window open for the race.
> > >
> > > Signed-off-by: Tycho Andersen <tandersen@netflix.com>
> > > v2: fix a race in the check by putting the check after __pidfd_fget()
> > > (thanks Oleg)
> > > ---
> > > kernel/pid.c | 17 +++++++++-
> > > .../selftests/pidfd/pidfd_getfd_test.c | 31 ++++++++++++++++++-
> > > 2 files changed, 46 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/kernel/pid.c b/kernel/pid.c
> > > index de0bf2f8d18b..a8cd6296ed6d 100644
> > > --- a/kernel/pid.c
> > > +++ b/kernel/pid.c
> > > @@ -693,8 +693,23 @@ static int pidfd_getfd(struct pid *pid, int fd)
> > >
> > > file = __pidfd_fget(task, fd);
> > > put_task_struct(task);
> > > - if (IS_ERR(file))
> > > + if (IS_ERR(file)) {
> > > + /*
> > > + * It is possible that the target thread is exiting; it can be
> > > + * either:
> > > + * 1. before exit_signals(), which gives a real fd
> > > + * 2. before exit_files() takes the task_lock() gives a real fd
> > > + * 3. after exit_files() releases task_lock(), ->files is NULL;
> > > + * this has PF_EXITING, since it was set in exit_signals(),
> > > + * __pidfd_fget() returns EBADF.
> > > + * In case 3 we get EBADF, but that really means ESRCH, since
> > > + * the task is currently exiting and has freed its files
> > > + * struct, so we fix it up.
> > > + */
> > > + if (task->flags & PF_EXITING && PTR_ERR(file) == -EBADF)
> > > + return -ESRCH;
> >
> > Isn't that a potential UAF because we called put_task_struct() above but
> > this is exiting task->flags afterwards?
>
> s/exiting/accessing/
So this is what I have applied currently where I moved the check into
__pidfd_fget() where it makes more sense imho. But please double check
that I didn't mess anything up:
From 7ab8f833aceb11c78627f4ea5d7e354314efa385 Mon Sep 17 00:00:00 2001
From: Tycho Andersen <tandersen@netflix.com>
Date: Wed, 7 Feb 2024 10:19:29 +0100
Subject: [PATCH 1/2] pidfd: getfd should always report ESRCH if a task is
exiting
We can get EBADF from pidfd_getfd() if a task is currently exiting,
which might be confusing. Let's check PF_EXITING, and just report ESRCH
if so.
I chose PF_EXITING, because it is set in exit_signals(), which is called
before exit_files(). Since ->exit_status is mostly set after
exit_files() in exit_notify(), using that still leaves a window open for
the race.
Signed-off-by: Tycho Andersen <tandersen@netflix.com>
Link: https://lore.kernel.org/r/20240206192357.81942-1-tycho@tycho.pizza
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
kernel/pid.c | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/kernel/pid.c b/kernel/pid.c
index de0bf2f8d18b..c1d940fbd314 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -678,7 +678,26 @@ static struct file *__pidfd_fget(struct task_struct *task, int fd)
up_read(&task->signal->exec_update_lock);
- return file ?: ERR_PTR(-EBADF);
+ if (!file) {
+ /*
+ * It is possible that the target thread is exiting; it can be
+ * either:
+ * 1. before exit_signals(), which gives a real fd
+ * 2. before exit_files() takes the task_lock() gives a real fd
+ * 3. after exit_files() releases task_lock(), ->files is NULL;
+ * this has PF_EXITING, since it was set in exit_signals(),
+ * __pidfd_fget() returns EBADF.
+ * In case 3 we get EBADF, but that really means ESRCH, since
+ * the task is currently exiting and has freed its files
+ * struct, so we fix it up.
+ */
+ if (task->flags & PF_EXITING)
+ file = ERR_PTR(-ESRCH);
+ else
+ file = ERR_PTR(-EBADF);
+ }
+
+ return file;
}
static int pidfd_getfd(struct pid *pid, int fd)
--
2.43.0
From 43316ed070cd8fb02a51ea9577c5fc1fcf639652 Mon Sep 17 00:00:00 2001
From: Tycho Andersen <tandersen@netflix.com>
Date: Wed, 7 Feb 2024 10:19:44 +0100
Subject: [PATCH 2/2] selftests: add ESRCH tests for pidfd_getfd()
Ensure that pidfd_getfd() reports -ESRCH if the task is already exiting.
Signed-off-by: Tycho Andersen <tandersen@netflix.com>
Link: https://lore.kernel.org/r/20240206192357.81942-1-tycho@tycho.pizza
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
.../selftests/pidfd/pidfd_getfd_test.c | 31 ++++++++++++++++++-
1 file changed, 30 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/pidfd/pidfd_getfd_test.c b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
index 0930e2411dfb..cd51d547b751 100644
--- a/tools/testing/selftests/pidfd/pidfd_getfd_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
@@ -5,6 +5,7 @@
#include <fcntl.h>
#include <limits.h>
#include <linux/types.h>
+#include <poll.h>
#include <sched.h>
#include <signal.h>
#include <stdio.h>
@@ -129,6 +130,7 @@ FIXTURE(child)
* When it is closed, the child will exit.
*/
int sk;
+ bool ignore_child_result;
};
FIXTURE_SETUP(child)
@@ -165,10 +167,14 @@ FIXTURE_SETUP(child)
FIXTURE_TEARDOWN(child)
{
+ int ret;
+
EXPECT_EQ(0, close(self->pidfd));
EXPECT_EQ(0, close(self->sk));
- EXPECT_EQ(0, wait_for_pid(self->pid));
+ ret = wait_for_pid(self->pid);
+ if (!self->ignore_child_result)
+ EXPECT_EQ(0, ret);
}
TEST_F(child, disable_ptrace)
@@ -235,6 +241,29 @@ TEST(flags_set)
EXPECT_EQ(errno, EINVAL);
}
+TEST_F(child, no_strange_EBADF)
+{
+ struct pollfd fds;
+
+ self->ignore_child_result = true;
+
+ fds.fd = self->pidfd;
+ fds.events = POLLIN;
+
+ ASSERT_EQ(kill(self->pid, SIGKILL), 0);
+ ASSERT_EQ(poll(&fds, 1, 5000), 1);
+
+ /*
+ * It used to be that pidfd_getfd() could race with the exiting thread
+ * between exit_files() and release_task(), and get a non-null task
+ * with a NULL files struct, and you'd get EBADF, which was slightly
+ * confusing.
+ */
+ errno = 0;
+ EXPECT_EQ(sys_pidfd_getfd(self->pidfd, self->remote_fd, 0), -1);
+ EXPECT_EQ(errno, ESRCH);
+}
+
#if __NR_pidfd_getfd == -1
int main(void)
{
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v2] pidfd: getfd should always report ESRCH if a task is exiting
From: Christian Brauner @ 2024-02-07 9:29 UTC (permalink / raw)
To: Tycho Andersen
Cc: Oleg Nesterov, Eric W . Biederman, linux-kernel, linux-api,
Tycho Andersen
In-Reply-To: <20240207-vibrieren-waldarbeiten-30eeade05203@brauner>
On Wed, Feb 07, 2024 at 10:15:25AM +0100, Christian Brauner wrote:
> On Tue, Feb 06, 2024 at 12:23:57PM -0700, Tycho Andersen wrote:
> > From: Tycho Andersen <tandersen@netflix.com>
> >
> > We can get EBADF from __pidfd_fget() if a task is currently exiting, which
> > might be confusing. Let's check PF_EXITING, and just report ESRCH if so.
> >
> > I chose PF_EXITING, because it is set in exit_signals(), which is called
> > before exit_files(). Since ->exit_status is mostly set after exit_files()
> > in exit_notify(), using that still leaves a window open for the race.
> >
> > Signed-off-by: Tycho Andersen <tandersen@netflix.com>
> > v2: fix a race in the check by putting the check after __pidfd_fget()
> > (thanks Oleg)
> > ---
> > kernel/pid.c | 17 +++++++++-
> > .../selftests/pidfd/pidfd_getfd_test.c | 31 ++++++++++++++++++-
> > 2 files changed, 46 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/pid.c b/kernel/pid.c
> > index de0bf2f8d18b..a8cd6296ed6d 100644
> > --- a/kernel/pid.c
> > +++ b/kernel/pid.c
> > @@ -693,8 +693,23 @@ static int pidfd_getfd(struct pid *pid, int fd)
> >
> > file = __pidfd_fget(task, fd);
> > put_task_struct(task);
> > - if (IS_ERR(file))
> > + if (IS_ERR(file)) {
> > + /*
> > + * It is possible that the target thread is exiting; it can be
> > + * either:
> > + * 1. before exit_signals(), which gives a real fd
> > + * 2. before exit_files() takes the task_lock() gives a real fd
> > + * 3. after exit_files() releases task_lock(), ->files is NULL;
> > + * this has PF_EXITING, since it was set in exit_signals(),
> > + * __pidfd_fget() returns EBADF.
> > + * In case 3 we get EBADF, but that really means ESRCH, since
> > + * the task is currently exiting and has freed its files
> > + * struct, so we fix it up.
> > + */
> > + if (task->flags & PF_EXITING && PTR_ERR(file) == -EBADF)
> > + return -ESRCH;
>
> Isn't that a potential UAF because we called put_task_struct() above but
> this is exiting task->flags afterwards?
s/exiting/accessing/
^ permalink raw reply
* Re: [PATCH v2] pidfd: getfd should always report ESRCH if a task is exiting
From: Christian Brauner @ 2024-02-07 9:15 UTC (permalink / raw)
To: Tycho Andersen
Cc: Oleg Nesterov, Eric W . Biederman, linux-kernel, linux-api,
Tycho Andersen
In-Reply-To: <20240206192357.81942-1-tycho@tycho.pizza>
On Tue, Feb 06, 2024 at 12:23:57PM -0700, Tycho Andersen wrote:
> From: Tycho Andersen <tandersen@netflix.com>
>
> We can get EBADF from __pidfd_fget() if a task is currently exiting, which
> might be confusing. Let's check PF_EXITING, and just report ESRCH if so.
>
> I chose PF_EXITING, because it is set in exit_signals(), which is called
> before exit_files(). Since ->exit_status is mostly set after exit_files()
> in exit_notify(), using that still leaves a window open for the race.
>
> Signed-off-by: Tycho Andersen <tandersen@netflix.com>
> v2: fix a race in the check by putting the check after __pidfd_fget()
> (thanks Oleg)
> ---
> kernel/pid.c | 17 +++++++++-
> .../selftests/pidfd/pidfd_getfd_test.c | 31 ++++++++++++++++++-
> 2 files changed, 46 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/pid.c b/kernel/pid.c
> index de0bf2f8d18b..a8cd6296ed6d 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -693,8 +693,23 @@ static int pidfd_getfd(struct pid *pid, int fd)
>
> file = __pidfd_fget(task, fd);
> put_task_struct(task);
> - if (IS_ERR(file))
> + if (IS_ERR(file)) {
> + /*
> + * It is possible that the target thread is exiting; it can be
> + * either:
> + * 1. before exit_signals(), which gives a real fd
> + * 2. before exit_files() takes the task_lock() gives a real fd
> + * 3. after exit_files() releases task_lock(), ->files is NULL;
> + * this has PF_EXITING, since it was set in exit_signals(),
> + * __pidfd_fget() returns EBADF.
> + * In case 3 we get EBADF, but that really means ESRCH, since
> + * the task is currently exiting and has freed its files
> + * struct, so we fix it up.
> + */
> + if (task->flags & PF_EXITING && PTR_ERR(file) == -EBADF)
> + return -ESRCH;
Isn't that a potential UAF because we called put_task_struct() above but
this is exiting task->flags afterwards?
^ permalink raw reply
* Re: [PATCH] pidfd: getfd should always report ESRCH if a task is exiting
From: Christian Brauner @ 2024-02-07 9:11 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Tycho Andersen, Eric W . Biederman, linux-kernel, linux-api,
Tycho Andersen
In-Reply-To: <20240206192553.GC3593@redhat.com>
On Tue, Feb 06, 2024 at 08:25:54PM +0100, Oleg Nesterov wrote:
> On 02/06, Tycho Andersen wrote:
>
> > On Tue, Feb 06, 2024 at 07:06:07PM +0100, Oleg Nesterov wrote:
> > > Or we can check task->files != NULL rather than PF_EXITING.
> > >
> > > To me this looks even better, but looks more confusing without a comment.
> > > OTOH, imo this needs a comment anyway ;)
> >
> > I thought about this, but I didn't really understand the null check in
> > exit_files();
>
> I guess task->files can be NULL at least if it was cloned with
> kernel_clone_args->no_files == T
Won't this give false positives for vhost workers which do set
->no_files but are user workers? IOW, return -ESRCH even though -EBADF
would be correct in this scenario?
^ permalink raw reply
* Re: [PATCH net-next v6 4/4] eventpoll: Add epoll ioctl for epoll_params
From: Jiri Slaby @ 2024-02-07 8:37 UTC (permalink / raw)
To: Joe Damato, linux-kernel, netdev
Cc: chuck.lever, jlayton, linux-api, brauner, edumazet, davem,
alexander.duyck, sridhar.samudrala, kuba, willemdebruijn.kernel,
weiwan, David.Laight, arnd, sdf, amritha.nambiar, Jonathan Corbet,
Alexander Viro, Jan Kara, Nathan Lynch, Michael Ellerman,
Greg Kroah-Hartman, Namjae Jeon, Steve French, Thomas Zimmermann,
Julien Panis, Andrew Waterman, Palmer Dabbelt, Albert Ou,
open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240205210453.11301-5-jdamato@fastly.com>
On 05. 02. 24, 22:04, Joe Damato wrote:
> Add an ioctl for getting and setting epoll_params. User programs can use
> this ioctl to get and set the busy poll usec time, packet budget, and
> prefer busy poll params for a specific epoll context.
>
> Parameters are limited:
> - busy_poll_usecs is limited to <= u32_max
> - busy_poll_budget is limited to <= NAPI_POLL_WEIGHT by unprivileged
> users (!capable(CAP_NET_ADMIN))
> - prefer_busy_poll must be 0 or 1
> - __pad must be 0
>
> Signed-off-by: Joe Damato <jdamato@fastly.com>
...
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
...
> @@ -497,6 +498,50 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
> ep->napi_id = napi_id;
> }
>
> +static long ep_eventpoll_bp_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct eventpoll *ep;
> + struct epoll_params epoll_params;
> + void __user *uarg = (void __user *) arg;
> +
> + ep = file->private_data;
This might have been on the ep declaration line.
> + switch (cmd) {
> + case EPIOCSPARAMS:
> + if (copy_from_user(&epoll_params, uarg, sizeof(epoll_params)))
> + return -EFAULT;
> +
> + if (memchr_inv(epoll_params.__pad, 0, sizeof(epoll_params.__pad)))
> + return -EINVAL;
> +
> + if (epoll_params.busy_poll_usecs > U32_MAX)
> + return -EINVAL;
> +
> + if (epoll_params.prefer_busy_poll > 1)
> + return -EINVAL;
> +
> + if (epoll_params.busy_poll_budget > NAPI_POLL_WEIGHT &&
> + !capable(CAP_NET_ADMIN))
> + return -EPERM;
> +
> + ep->busy_poll_usecs = epoll_params.busy_poll_usecs;
> + ep->busy_poll_budget = epoll_params.busy_poll_budget;
> + ep->prefer_busy_poll = !!epoll_params.prefer_busy_poll;
This !! is unnecessary. Nonzero values shall be "converted" to true.
But FWIW, the above is nothing which should be blocking, so:
Reviewed-by: Jiri Slaby <jirislaby@kernel.org>
> + return 0;
> + case EPIOCGPARAMS:
> + memset(&epoll_params, 0, sizeof(epoll_params));
> + epoll_params.busy_poll_usecs = ep->busy_poll_usecs;
> + epoll_params.busy_poll_budget = ep->busy_poll_budget;
> + epoll_params.prefer_busy_poll = ep->prefer_busy_poll;
> + if (copy_to_user(uarg, &epoll_params, sizeof(epoll_params)))
> + return -EFAULT;
> + return 0;
> + default:
> + return -ENOIOCTLCMD;
> + }
> +}
...
thanks,
--
js
suse labs
^ permalink raw reply
* Re: [PATCH] pidfd: getfd should always report ESRCH if a task is exiting
From: Tycho Andersen @ 2024-02-06 19:35 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Christian Brauner, Eric W . Biederman, linux-kernel, linux-api,
Tycho Andersen
In-Reply-To: <20240206192553.GC3593@redhat.com>
On Tue, Feb 06, 2024 at 08:25:54PM +0100, Oleg Nesterov wrote:
> But Tycho, I won't insist. If you prefer to check PF_EXITING, I am fine.
Looks like we raced, I sent a v2 with PF_EXITING, mostly because I
didn't want to run into weird things I didn't understand. I'm happy to
fix it up to check ->files if that's what you prefer Christian?
Tycho
^ permalink raw reply
* Re: [PATCH] pidfd: getfd should always report ESRCH if a task is exiting
From: Oleg Nesterov @ 2024-02-06 19:25 UTC (permalink / raw)
To: Tycho Andersen
Cc: Christian Brauner, Eric W . Biederman, linux-kernel, linux-api,
Tycho Andersen
In-Reply-To: <ZcJ13uLxD6rTqqZZ@tycho.pizza>
On 02/06, Tycho Andersen wrote:
> On Tue, Feb 06, 2024 at 07:06:07PM +0100, Oleg Nesterov wrote:
> > Or we can check task->files != NULL rather than PF_EXITING.
> >
> > To me this looks even better, but looks more confusing without a comment.
> > OTOH, imo this needs a comment anyway ;)
>
> I thought about this, but I didn't really understand the null check in
> exit_files();
I guess task->files can be NULL at least if it was cloned with
kernel_clone_args->no_files == T
> if it can really be called more than once,
I don't think this is possible. Well, unless the exiting task hits
a BUG() after exit_files() and calls do_exit() recursively.
> are there
> other cases where task->files == NULL that we really should report
> EBADF?
I don't think so...
If nothing else, sys_close() dereferences current->files without any
checks, so I think task->files == NULL is simply impossible if this
task is a userspace process/thread until it exits.
But Tycho, I won't insist. If you prefer to check PF_EXITING, I am fine.
Oleg.
^ permalink raw reply
* [PATCH v2] pidfd: getfd should always report ESRCH if a task is exiting
From: Tycho Andersen @ 2024-02-06 19:23 UTC (permalink / raw)
To: Christian Brauner
Cc: Oleg Nesterov, Eric W . Biederman, linux-kernel, linux-api,
Tycho Andersen, Tycho Andersen
From: Tycho Andersen <tandersen@netflix.com>
We can get EBADF from __pidfd_fget() if a task is currently exiting, which
might be confusing. Let's check PF_EXITING, and just report ESRCH if so.
I chose PF_EXITING, because it is set in exit_signals(), which is called
before exit_files(). Since ->exit_status is mostly set after exit_files()
in exit_notify(), using that still leaves a window open for the race.
Signed-off-by: Tycho Andersen <tandersen@netflix.com>
v2: fix a race in the check by putting the check after __pidfd_fget()
(thanks Oleg)
---
kernel/pid.c | 17 +++++++++-
.../selftests/pidfd/pidfd_getfd_test.c | 31 ++++++++++++++++++-
2 files changed, 46 insertions(+), 2 deletions(-)
diff --git a/kernel/pid.c b/kernel/pid.c
index de0bf2f8d18b..a8cd6296ed6d 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -693,8 +693,23 @@ static int pidfd_getfd(struct pid *pid, int fd)
file = __pidfd_fget(task, fd);
put_task_struct(task);
- if (IS_ERR(file))
+ if (IS_ERR(file)) {
+ /*
+ * It is possible that the target thread is exiting; it can be
+ * either:
+ * 1. before exit_signals(), which gives a real fd
+ * 2. before exit_files() takes the task_lock() gives a real fd
+ * 3. after exit_files() releases task_lock(), ->files is NULL;
+ * this has PF_EXITING, since it was set in exit_signals(),
+ * __pidfd_fget() returns EBADF.
+ * In case 3 we get EBADF, but that really means ESRCH, since
+ * the task is currently exiting and has freed its files
+ * struct, so we fix it up.
+ */
+ if (task->flags & PF_EXITING && PTR_ERR(file) == -EBADF)
+ return -ESRCH;
return PTR_ERR(file);
+ }
ret = receive_fd(file, NULL, O_CLOEXEC);
fput(file);
diff --git a/tools/testing/selftests/pidfd/pidfd_getfd_test.c b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
index 0930e2411dfb..cd51d547b751 100644
--- a/tools/testing/selftests/pidfd/pidfd_getfd_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
@@ -5,6 +5,7 @@
#include <fcntl.h>
#include <limits.h>
#include <linux/types.h>
+#include <poll.h>
#include <sched.h>
#include <signal.h>
#include <stdio.h>
@@ -129,6 +130,7 @@ FIXTURE(child)
* When it is closed, the child will exit.
*/
int sk;
+ bool ignore_child_result;
};
FIXTURE_SETUP(child)
@@ -165,10 +167,14 @@ FIXTURE_SETUP(child)
FIXTURE_TEARDOWN(child)
{
+ int ret;
+
EXPECT_EQ(0, close(self->pidfd));
EXPECT_EQ(0, close(self->sk));
- EXPECT_EQ(0, wait_for_pid(self->pid));
+ ret = wait_for_pid(self->pid);
+ if (!self->ignore_child_result)
+ EXPECT_EQ(0, ret);
}
TEST_F(child, disable_ptrace)
@@ -235,6 +241,29 @@ TEST(flags_set)
EXPECT_EQ(errno, EINVAL);
}
+TEST_F(child, no_strange_EBADF)
+{
+ struct pollfd fds;
+
+ self->ignore_child_result = true;
+
+ fds.fd = self->pidfd;
+ fds.events = POLLIN;
+
+ ASSERT_EQ(kill(self->pid, SIGKILL), 0);
+ ASSERT_EQ(poll(&fds, 1, 5000), 1);
+
+ /*
+ * It used to be that pidfd_getfd() could race with the exiting thread
+ * between exit_files() and release_task(), and get a non-null task
+ * with a NULL files struct, and you'd get EBADF, which was slightly
+ * confusing.
+ */
+ errno = 0;
+ EXPECT_EQ(sys_pidfd_getfd(self->pidfd, self->remote_fd, 0), -1);
+ EXPECT_EQ(errno, ESRCH);
+}
+
#if __NR_pidfd_getfd == -1
int main(void)
{
base-commit: 082d11c164aef02e51bcd9c7cbf1554a8e42d9b5
--
2.34.1
^ permalink raw reply related
* Re: [PATCH net-next v6 0/4] Per epoll context busy poll support
From: Stanislav Fomichev @ 2024-02-06 18:51 UTC (permalink / raw)
To: Joe Damato
Cc: linux-kernel, netdev, chuck.lever, jlayton, linux-api, brauner,
edumazet, davem, alexander.duyck, sridhar.samudrala, kuba,
willemdebruijn.kernel, weiwan, David.Laight, arnd,
amritha.nambiar, Albert Ou, Alexander Viro, Andrew Waterman,
Greg Kroah-Hartman, Jan Kara, Jiri Slaby, Jonathan Corbet,
Julien Panis, open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure), Maik Broemme,
Michael Ellerman, Namjae Jeon, Nathan Lynch, Palmer Dabbelt,
Steve French, Thomas Huth, Thomas Zimmermann
In-Reply-To: <20240205210453.11301-1-jdamato@fastly.com>
On 02/05, Joe Damato wrote:
> Greetings:
>
> Welcome to v6.
>
> TL;DR This builds on commit bf3b9f6372c4 ("epoll: Add busy poll support to
> epoll with socket fds.") by allowing user applications to enable
> epoll-based busy polling, set a busy poll packet budget, and enable or
> disable prefer busy poll on a per epoll context basis.
>
> This makes epoll-based busy polling much more usable for user
> applications than the current system-wide sysctl and hardcoded budget.
>
> To allow for this, two ioctls have been added for epoll contexts for
> getting and setting a new struct, struct epoll_params.
>
> ioctl was chosen vs a new syscall after reviewing a suggestion by Willem
> de Bruijn [1]. I am open to using a new syscall instead of an ioctl, but it
> seemed that:
> - Busy poll affects all existing epoll_wait and epoll_pwait variants in
> the same way, so new verions of many syscalls might be needed. It
> seems much simpler for users to use the correct
> epoll_wait/epoll_pwait for their app and add a call to ioctl to enable
> or disable busy poll as needed. This also probably means less work to
> get an existing epoll app using busy poll.
>
> - previously added epoll_pwait2 helped to bring epoll closer to
> existing syscalls (like pselect and ppoll) and this busy poll change
> reflected as a new syscall would not have the same effect.
>
> Note: patch 1/4 as of v4 uses an or (||) instead of an xor. I thought about
> it some more and I realized that if the user enables both the per-epoll
> context setting and the system wide sysctl, then busy poll should be
> enabled and not disabled. Using xor doesn't seem to make much sense after
> thinking through this a bit.
>
> Longer explanation:
>
> Presently epoll has support for a very useful form of busy poll based on
> the incoming NAPI ID (see also: SO_INCOMING_NAPI_ID [2]).
>
> This form of busy poll allows epoll_wait to drive NAPI packet processing
> which allows for a few interesting user application designs which can
> reduce latency and also potentially improve L2/L3 cache hit rates by
> deferring NAPI until userland has finished its work.
>
> The documentation available on this is, IMHO, a bit confusing so please
> allow me to explain how one might use this:
>
> 1. Ensure each application thread has its own epoll instance mapping
> 1-to-1 with NIC RX queues. An n-tuple filter would likely be used to
> direct connections with specific dest ports to these queues.
>
> 2. Optionally: Setup IRQ coalescing for the NIC RX queues where busy
> polling will occur. This can help avoid the userland app from being
> pre-empted by a hard IRQ while userland is running. Note this means that
> userland must take care to call epoll_wait and not take too long in
> userland since it now drives NAPI via epoll_wait.
>
> 3. Optionally: Consider using napi_defer_hard_irqs and gro_flush_timeout to
> further restrict IRQ generation from the NIC. These settings are
> system-wide so their impact must be carefully weighed against the running
> applications.
>
> 4. Ensure that all incoming connections added to an epoll instance
> have the same NAPI ID. This can be done with a BPF filter when
> SO_REUSEPORT is used or getsockopt + SO_INCOMING_NAPI_ID when a single
> accept thread is used which dispatches incoming connections to threads.
>
> 5. Lastly, busy poll must be enabled via a sysctl
> (/proc/sys/net/core/busy_poll).
>
> Please see Eric Dumazet's paper about busy polling [3] and a recent
> academic paper about measured performance improvements of busy polling [4]
> (albeit with a modification that is not currently present in the kernel)
> for additional context.
>
> The unfortunate part about step 5 above is that this enables busy poll
> system-wide which affects all user applications on the system,
> including epoll-based network applications which were not intended to
> be used this way or applications where increased CPU usage for lower
> latency network processing is unnecessary or not desirable.
>
> If the user wants to run one low latency epoll-based server application
> with epoll-based busy poll, but would like to run the rest of the
> applications on the system (which may also use epoll) without busy poll,
> this system-wide sysctl presents a significant problem.
>
> This change preserves the system-wide sysctl, but adds a mechanism (via
> ioctl) to enable or disable busy poll for epoll contexts as needed by
> individual applications, making epoll-based busy poll more usable.
>
> Note that this change includes an or (as of v4) instead of an xor. If the
> user has enabled both the system-wide sysctl and also the per epoll-context
> busy poll settings, then epoll should probably busy poll (vs being
> disabled).
>
> Thanks,
> Joe
>
> v5 -> v6:
> - patch 1/3 no functional change, but commit message corrected to explain
> that an or (||) is being used instead of xor.
>
> - patch 3/4 is a new patch which adds support for per epoll context
> prefer busy poll setting.
>
> - patch 4/4 updated to allow getting/setting per epoll context prefer
> busy poll setting; this setting is limited to either 0 or 1.
>
> v4 -> v5:
> - patch 3/3 updated to use memchr_inv to ensure that __pad is zero for
> the EPIOCSPARAMS ioctl. Recommended by Greg K-H [5], Dave Chinner [6],
> and Jiri Slaby [7].
>
> v3 -> v4:
> - patch 1/3 was updated to include an important functional change:
> ep_busy_loop_on was updated to use or (||) instead of xor (^). After
> thinking about it a bit more, I thought xor didn't make much sense.
> Enabling both the per-epoll context and the system-wide sysctl should
> probably enable busy poll, not disable it. So, or (||) makes more
> sense, I think.
>
> - patch 3/3 was updated:
> - to change the epoll_params fields to be __u64, __u16, and __u8 and
> to pad the struct to a multiple of 64bits. Suggested by Greg K-H [8]
> and Arnd Bergmann [9].
> - remove an unused pr_fmt, left over from the previous revision.
> - ioctl now returns -EINVAL when epoll_params.busy_poll_usecs >
> U32_MAX.
>
> v2 -> v3:
> - cover letter updated to mention why ioctl seems (to me) like a better
> choice vs a new syscall.
>
> - patch 3/4 was modified in 3 ways:
> - when an unknown ioctl is received, -ENOIOCTLCMD is returned instead
> of -EINVAL as the ioctl documentation requires.
> - epoll_params.busy_poll_budget can only be set to a value larger than
> NAPI_POLL_WEIGHT if code is run by privileged (CAP_NET_ADMIN) users.
> Otherwise, -EPERM is returned.
> - busy poll specific ioctl code moved out to its own function. On
> kernels without busy poll support, -EOPNOTSUPP is returned. This also
> makes the kernel build robot happier without littering the code with
> more #ifdefs.
>
> - dropped patch 4/4 after Eric Dumazet's review of it when it was sent
> independently to the list [10].
>
> v1 -> v2:
> - cover letter updated to make a mention of napi_defer_hard_irqs and
> gro_flush_timeout as an added step 3 and to cite both Eric Dumazet's
> busy polling paper and a paper from University of Waterloo for
> additional context. Specifically calling out the xor in patch 1/4
> incase it is missed by reviewers.
>
> - Patch 2/4 has its commit message updated, but no functional changes.
> Commit message now describes that allowing for a settable budget helps
> to improve throughput and is more consistent with other busy poll
> mechanisms that allow a settable budget via SO_BUSY_POLL_BUDGET.
>
> - Patch 3/4 was modified to check if the epoll_params.busy_poll_budget
> exceeds NAPI_POLL_WEIGHT. The larger value is allowed, but an error is
> printed. This was done for consistency with netif_napi_add_weight,
> which does the same.
>
> - Patch 3/4 the struct epoll_params was updated to fix the type of the
> data field; it was uint8_t and was changed to u8.
>
> - Patch 4/4 added to check if SO_BUSY_POLL_BUDGET exceeds
> NAPI_POLL_WEIGHT. The larger value is allowed, but an error is
> printed. This was done for consistency with netif_napi_add_weight,
> which does the same.
>
> [1]: https://lore.kernel.org/lkml/65b1cb7f73a6a_250560294bd@willemb.c.googlers.com.notmuch/
> [2]: https://lore.kernel.org/lkml/20170324170836.15226.87178.stgit@localhost.localdomain/
> [3]: https://netdevconf.info/2.1/papers/BusyPollingNextGen.pdf
> [4]: https://dl.acm.org/doi/pdf/10.1145/3626780
> [5]: https://lore.kernel.org/lkml/2024013001-prison-strum-899d@gregkh/
> [6]: https://lore.kernel.org/lkml/Zbm3AXgcwL9D6TNM@dread.disaster.area/
> [7]: https://lore.kernel.org/lkml/efee9789-4f05-4202-9a95-21d88f6307b0@kernel.org/
> [8]: https://lore.kernel.org/lkml/2024012551-anyone-demeaning-867b@gregkh/
> [9]: https://lore.kernel.org/lkml/57b62135-2159-493d-a6bb-47d5be55154a@app.fastmail.com/
> [10]: https://lore.kernel.org/lkml/CANn89i+uXsdSVFiQT9fDfGw+h_5QOcuHwPdWi9J=5U6oLXkQTA@mail.gmail.com/
>
> Joe Damato (4):
> eventpoll: support busy poll per epoll instance
> eventpoll: Add per-epoll busy poll packet budget
> eventpoll: Add per-epoll prefer busy poll option
> eventpoll: Add epoll ioctl for epoll_params
>
> .../userspace-api/ioctl/ioctl-number.rst | 1 +
> fs/eventpoll.c | 136 +++++++++++++++++-
> include/uapi/linux/eventpoll.h | 13 ++
> 3 files changed, 144 insertions(+), 6 deletions(-)
Coincidentally, we were looking into the same area and your patches are
super useful :-) Thank you for plumbing in prefer_busy_poll.
Acked-by: Stanislav Fomichev <sdf@google.com>
^ permalink raw reply
* Re: [PATCH] pidfd: getfd should always report ESRCH if a task is exiting
From: Tycho Andersen @ 2024-02-06 18:09 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Christian Brauner, Eric W . Biederman, linux-kernel, linux-api,
Tycho Andersen
In-Reply-To: <20240206180607.GB3593@redhat.com>
On Tue, Feb 06, 2024 at 07:06:07PM +0100, Oleg Nesterov wrote:
> Or we can check task->files != NULL rather than PF_EXITING.
>
> To me this looks even better, but looks more confusing without a comment.
> OTOH, imo this needs a comment anyway ;)
I thought about this, but I didn't really understand the null check in
exit_files(); if it can really be called more than once, are there
other cases where task->files == NULL that we really should report
EBADF?
Tycho
^ permalink raw reply
* Re: [PATCH] pidfd: getfd should always report ESRCH if a task is exiting
From: Oleg Nesterov @ 2024-02-06 18:06 UTC (permalink / raw)
To: Tycho Andersen
Cc: Christian Brauner, Eric W . Biederman, linux-kernel, linux-api,
Tycho Andersen
In-Reply-To: <20240206173722.GA3593@redhat.com>
Sorry for noise, forgot to mention...
On 02/06, Oleg Nesterov wrote:
>
> On 02/06, Tycho Andersen wrote:
> >
> > From: Tycho Andersen <tandersen@netflix.com>
> >
> > We can get EBADF from __pidfd_fget() if a task is currently exiting, which
> > might be confusing.
>
> agreed, because EBADF looks as if the "fd" argument was wrong,
>
> > Let's check PF_EXITING, and just report ESRCH if so.
>
> agreed, we can pretend that the task has already exited,
>
> But:
>
> > --- a/kernel/pid.c
> > +++ b/kernel/pid.c
> > @@ -688,7 +688,7 @@ static int pidfd_getfd(struct pid *pid, int fd)
> > int ret;
> >
> > task = get_pid_task(pid, PIDTYPE_PID);
> > - if (!task)
> > + if (!task || task->flags & PF_EXITING)
> > return -ESRCH;
>
> This looks racy. Suppose that pidfd_getfd() races with the exiting task.
>
> It is possible that this task sets PF_EXITING and does exit_files()
> after the "task->flags & PF_EXITING" check above and before pidfd_getfd()
> does __pidfd_fget(), in this case pidfd_getfd() still returns the same
> EBADF we want to avoid.
>
> Perhaps we can change pidfd_getfd() to do
>
> if (IS_ERR(file))
> return (task->flags & PF_EXITING) ? -ESRCH : PTR_ERR(file);
Or we can check task->files != NULL rather than PF_EXITING.
To me this looks even better, but looks more confusing without a comment.
OTOH, imo this needs a comment anyway ;)
>
> instead?
>
> This needs a comment to explain the PF_EXITING check. And perhaps another
> comment to explain that we can't miss PF_EXITING if the target task has
> already passed exit_files, both exit_files() and fget_task() take the same
> task_lock(task).
>
> What do you think?
>
> Oleg.
^ permalink raw reply
* Re: [PATCH] pidfd: getfd should always report ESRCH if a task is exiting
From: Tycho Andersen @ 2024-02-06 17:55 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Christian Brauner, Eric W . Biederman, linux-kernel, linux-api,
Tycho Andersen
In-Reply-To: <20240206173722.GA3593@redhat.com>
On Tue, Feb 06, 2024 at 06:37:22PM +0100, Oleg Nesterov wrote:
> > --- a/kernel/pid.c
> > +++ b/kernel/pid.c
> > @@ -688,7 +688,7 @@ static int pidfd_getfd(struct pid *pid, int fd)
> > int ret;
> >
> > task = get_pid_task(pid, PIDTYPE_PID);
> > - if (!task)
> > + if (!task || task->flags & PF_EXITING)
> > return -ESRCH;
>
> This looks racy. Suppose that pidfd_getfd() races with the exiting task.
>
> It is possible that this task sets PF_EXITING and does exit_files()
> after the "task->flags & PF_EXITING" check above and before pidfd_getfd()
> does __pidfd_fget(), in this case pidfd_getfd() still returns the same
> EBADF we want to avoid.
>
> Perhaps we can change pidfd_getfd() to do
>
> if (IS_ERR(file))
> return (task->flags & PF_EXITING) ? -ESRCH : PTR_ERR(file);
>
> instead?
>
> This needs a comment to explain the PF_EXITING check. And perhaps another
> comment to explain that we can't miss PF_EXITING if the target task has
> already passed exit_files, both exit_files() and fget_task() take the same
> task_lock(task).
>
> What do you think?
Yes, you're absolutely right. Let me resend.
Tycho
^ permalink raw reply
* Re: [PATCH] pidfd: getfd should always report ESRCH if a task is exiting
From: Oleg Nesterov @ 2024-02-06 17:37 UTC (permalink / raw)
To: Tycho Andersen
Cc: Christian Brauner, Eric W . Biederman, linux-kernel, linux-api,
Tycho Andersen
In-Reply-To: <20240206164308.62620-1-tycho@tycho.pizza>
On 02/06, Tycho Andersen wrote:
>
> From: Tycho Andersen <tandersen@netflix.com>
>
> We can get EBADF from __pidfd_fget() if a task is currently exiting, which
> might be confusing.
agreed, because EBADF looks as if the "fd" argument was wrong,
> Let's check PF_EXITING, and just report ESRCH if so.
agreed, we can pretend that the task has already exited,
But:
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -688,7 +688,7 @@ static int pidfd_getfd(struct pid *pid, int fd)
> int ret;
>
> task = get_pid_task(pid, PIDTYPE_PID);
> - if (!task)
> + if (!task || task->flags & PF_EXITING)
> return -ESRCH;
This looks racy. Suppose that pidfd_getfd() races with the exiting task.
It is possible that this task sets PF_EXITING and does exit_files()
after the "task->flags & PF_EXITING" check above and before pidfd_getfd()
does __pidfd_fget(), in this case pidfd_getfd() still returns the same
EBADF we want to avoid.
Perhaps we can change pidfd_getfd() to do
if (IS_ERR(file))
return (task->flags & PF_EXITING) ? -ESRCH : PTR_ERR(file);
instead?
This needs a comment to explain the PF_EXITING check. And perhaps another
comment to explain that we can't miss PF_EXITING if the target task has
already passed exit_files, both exit_files() and fget_task() take the same
task_lock(task).
What do you think?
Oleg.
^ permalink raw reply
* Re: The sk_err mechanism is infuriating in userspace
From: Andy Lutomirski @ 2024-02-06 17:24 UTC (permalink / raw)
To: Paolo Abeni; +Cc: Network Development, Linux API
In-Reply-To: <852606cd9cbc8da9c6735b4ad6216ba55408b767.camel@redhat.com>
On Tue, Feb 6, 2024 at 12:43 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> What about 'destination/port unreachable' and many other similar errors
> reported by sk_err? Which specific errors reported by sk_err does not
> indicate that anything is wrong with the socket ?
Destination/port unreachable are *exactly* the primary offenders. Consider:
1. TCP socket. If the peer becomes unreachable, the connection is
unusable. Maybe reading previously queued data is reasonable; maybe
it's not, but one way or another the connection isn't working any
more. The current API seems okay.
2. UDP peer-to-peer connection. I have a socket and it's connected to
a peer. The peer sends an ICMP error or a route changes and the
kernel can't route to the peer. The connection is at least
temporarily dead. If we accept that temporarily dead equals
permanently dead, then returning errors codes makes sense. Even if we
expect the application to try to recover without making a new socket,
telling the application seems fine. The application will understand
that an error occurred communicating with its peer and can do
something about it.
3. UDP *server* with multiple clients. (Or unconnected UDP socket
communicating with multiple peers, etc.) Imagine a DNS server or a
QUIC server -- I hear QUIC is cool lately. A userspace server has a
socket, and it does sendto() or sendmsg() to a whole bunch of
addresses. One of them sends an ICMP error. There are multiple
things the server might do. It might ignore the error entirely and
treat it just like a timeout, because it probably already has
perfectly nice timeout handling. Or it might want to know that there
was an error communicating with a *specific* peer and release
resources sooner than it would for a timeout. Or it might want to
collect the entire ICMP error (via RECVERR) and do something useful
with it. But it gets no value whatsoever from knowing that an
unspecified peer sent an ICMP error, and it gets negative value from
having a call to recvfrom() or recvmsg() fail and needing to look up
in some hopefully-correct table whether the failure indicates an
actual problem (EFAULT, for example) or a completely useless return
value that should be ignored (EHOSTUNREACH).
(#3 is probably worse if the application uses one-shot notifications
-- the application needs to make a decision as to whether to call
recvfrom/recvmsg again or go back to polling.)
--Andy
^ permalink raw reply
* [PATCH] pidfd: getfd should always report ESRCH if a task is exiting
From: Tycho Andersen @ 2024-02-06 16:43 UTC (permalink / raw)
To: Christian Brauner
Cc: Oleg Nesterov, Eric W . Biederman, linux-kernel, linux-api,
Tycho Andersen, Tycho Andersen
From: Tycho Andersen <tandersen@netflix.com>
We can get EBADF from __pidfd_fget() if a task is currently exiting, which
might be confusing. Let's check PF_EXITING, and just report ESRCH if so.
I chose PF_EXITING, because it is set in exit_signals(), which is called
before exit_files(). Since ->exit_status is mostly set after exit_files()
in exit_notify(), using that still leaves a window open for the race.
Signed-off-by: Tycho Andersen <tandersen@netflix.com>
---
kernel/pid.c | 2 +-
.../selftests/pidfd/pidfd_getfd_test.c | 31 ++++++++++++++++++-
2 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/kernel/pid.c b/kernel/pid.c
index de0bf2f8d18b..db8731f0ee45 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -688,7 +688,7 @@ static int pidfd_getfd(struct pid *pid, int fd)
int ret;
task = get_pid_task(pid, PIDTYPE_PID);
- if (!task)
+ if (!task || task->flags & PF_EXITING)
return -ESRCH;
file = __pidfd_fget(task, fd);
diff --git a/tools/testing/selftests/pidfd/pidfd_getfd_test.c b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
index 0930e2411dfb..cd51d547b751 100644
--- a/tools/testing/selftests/pidfd/pidfd_getfd_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
@@ -5,6 +5,7 @@
#include <fcntl.h>
#include <limits.h>
#include <linux/types.h>
+#include <poll.h>
#include <sched.h>
#include <signal.h>
#include <stdio.h>
@@ -129,6 +130,7 @@ FIXTURE(child)
* When it is closed, the child will exit.
*/
int sk;
+ bool ignore_child_result;
};
FIXTURE_SETUP(child)
@@ -165,10 +167,14 @@ FIXTURE_SETUP(child)
FIXTURE_TEARDOWN(child)
{
+ int ret;
+
EXPECT_EQ(0, close(self->pidfd));
EXPECT_EQ(0, close(self->sk));
- EXPECT_EQ(0, wait_for_pid(self->pid));
+ ret = wait_for_pid(self->pid);
+ if (!self->ignore_child_result)
+ EXPECT_EQ(0, ret);
}
TEST_F(child, disable_ptrace)
@@ -235,6 +241,29 @@ TEST(flags_set)
EXPECT_EQ(errno, EINVAL);
}
+TEST_F(child, no_strange_EBADF)
+{
+ struct pollfd fds;
+
+ self->ignore_child_result = true;
+
+ fds.fd = self->pidfd;
+ fds.events = POLLIN;
+
+ ASSERT_EQ(kill(self->pid, SIGKILL), 0);
+ ASSERT_EQ(poll(&fds, 1, 5000), 1);
+
+ /*
+ * It used to be that pidfd_getfd() could race with the exiting thread
+ * between exit_files() and release_task(), and get a non-null task
+ * with a NULL files struct, and you'd get EBADF, which was slightly
+ * confusing.
+ */
+ errno = 0;
+ EXPECT_EQ(sys_pidfd_getfd(self->pidfd, self->remote_fd, 0), -1);
+ EXPECT_EQ(errno, ESRCH);
+}
+
#if __NR_pidfd_getfd == -1
int main(void)
{
base-commit: 082d11c164aef02e51bcd9c7cbf1554a8e42d9b5
--
2.34.1
^ permalink raw reply related
* Re: [PATCH] pidfd: clone: allow CLONE_THREAD | CLONE_PIDFD together
From: Tycho Andersen @ 2024-02-06 14:52 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Christian Brauner, Andy Lutomirski, Eric W. Biederman, linux-api,
linux-kernel
In-Reply-To: <20240205145532.GA28823@redhat.com>
On Mon, Feb 05, 2024 at 03:55:32PM +0100, Oleg Nesterov wrote:
> copy_process() just needs to pass PIDFD_THREAD to __pidfd_prepare()
> if clone_flags & CLONE_THREAD.
>
> We can also add another CLONE_ flag (or perhaps reuse CLONE_DETACHED)
> to enforce PIDFD_THREAD without CLONE_THREAD.
>
> Originally-from: Tycho Andersen <tycho@tycho.pizza>
Goes without saying I suppose, but,
Reviewed-by: Tycho Andersen <tandersen@netflix.com>
as well.
Tycho
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox