* Re: [PATCH net-next v6 4/4] eventpoll: Add epoll ioctl for epoll_params
From: Joe Damato @ 2024-02-07 19:16 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, linux-kernel, chuck.lever, jlayton, linux-api, brauner,
edumazet, davem, alexander.duyck, sridhar.samudrala,
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: <20240207110726.68c07188@kernel.org>
On Wed, Feb 07, 2024 at 11:07:26AM -0800, Jakub Kicinski wrote:
> On Wed, 7 Feb 2024 10:50:15 -0800 Joe Damato wrote:
> > > 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.
>
> Yes, I think that's it.
OK, thanks for letting me know. I wasn't sure if it was because of the
netdev/source_inline which marked 1/4 as "fail" because of the inlines
added.
Does that need to be changed, as well?
^ permalink raw reply
* Re: [PATCH net-next v6 1/4] eventpoll: support busy poll per epoll instance
From: Joe Damato @ 2024-02-07 19:14 UTC (permalink / raw)
To: Jakub Kicinski
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: <20240207110413.0cfedc37@kernel.org>
On Wed, Feb 07, 2024 at 11:04:13AM -0800, Jakub Kicinski wrote:
> 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.
In patch 4/4 the value is limited to U32_MAX, but if you prefer I use a u32
here instead, I can make that change.
^ permalink raw reply
* Re: [PATCH net-next v6 4/4] eventpoll: Add epoll ioctl for epoll_params
From: Jakub Kicinski @ 2024-02-07 19:07 UTC (permalink / raw)
To: Joe Damato
Cc: netdev, linux-kernel, chuck.lever, jlayton, linux-api, brauner,
edumazet, davem, alexander.duyck, sridhar.samudrala,
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: <20240207185014.GA1221@fastly.com>
On Wed, 7 Feb 2024 10:50:15 -0800 Joe Damato wrote:
> > 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.
Yes, I think that's it.
^ permalink raw reply
* 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
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