* Re: [PATCH v4] ELF: AT_PAGE_SHIFT_MASK -- supply userspace with available page shifts
From: Kees Cook @ 2024-02-10 0:41 UTC (permalink / raw)
To: Alexey Dobriyan
Cc: Andrew Morton, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Florian Weimer, linux-kernel, linux-arch, linux-api,
x86, Eric Biederman, linux-mm
In-Reply-To: <acd02481-ca2e-412a-8c6b-d9dff1345139@p183>
On Fri, Feb 09, 2024 at 03:30:37PM +0300, Alexey Dobriyan wrote:
> On Mon, Feb 05, 2024 at 04:48:08AM -0800, Kees Cook wrote:
> > On Mon, Feb 05, 2024 at 12:51:43PM +0300, Alexey Dobriyan wrote:
> > > +#define ARCH_AT_PAGE_SHIFT_MASK \
> > > + do { \
> > > + u32 val = 1 << 12; \
> > > + if (boot_cpu_has(X86_FEATURE_PSE)) { \
> > > + val |= 1 << 21; \
> > > + } \
> > > + if (boot_cpu_has(X86_FEATURE_GBPAGES)) { \
> > > + val |= 1 << 30; \
> > > + } \
> >
> > Can we use something besides literal "12", "21", and "30" values here?
>
> Ehh, no, why? Inside x86_64 the page shifts are very specific numbers,
> they won't change.
Well, it's nicer to have meaningful words to describe these things. In
fact, PAGE_SHIFT already exists for 12, and HPAGE_SHIFT already exists
for 21. Please use those, and add another, perhaps GBPAGE_SHIFT, for 30.
-Kees
--
Kees Cook
^ permalink raw reply
* Re: [PATCH RFT v5 4/7] fork: Add shadow stack support to clone3()
From: Edgecombe, Rick P @ 2024-02-10 0:55 UTC (permalink / raw)
To: dietmar.eggemann@arm.com, broonie@kernel.org,
Szabolcs.Nagy@arm.com, brauner@kernel.org,
dave.hansen@linux.intel.com, debug@rivosinc.com, mgorman@suse.de,
vincent.guittot@linaro.org, fweimer@redhat.com, mingo@redhat.com,
rostedt@goodmis.org, hjl.tools@gmail.com, tglx@linutronix.de,
vschneid@redhat.com, shuah@kernel.org, bristot@redhat.com,
hpa@zytor.com, peterz@infradead.org, bp@alien8.de,
bsegall@google.com, x86@kernel.org, juri.lelli@redhat.com
Cc: keescook@chromium.org, jannh@google.com,
linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org,
catalin.marinas@arm.com, linux-api@vger.kernel.org,
will@kernel.org
In-Reply-To: <565ca9697cf26be5509ef4b3c1cc95fa4f692b9f.camel@intel.com>
On Fri, 2024-02-09 at 12:18 -0800, Rick Edgecombe wrote:
>
> So, don't we want to consume the token on the *new* task's MM, which
> was already duplicated but still unmapped? In which case I think the
> other arch's would need to GUP regardless of the existence of shadow
> stack atomic ops.
I mean for the !CLONE_VM case.
^ permalink raw reply
* Re: [PATCH RFT v5 4/7] fork: Add shadow stack support to clone3()
From: Edgecombe, Rick P @ 2024-02-10 0:56 UTC (permalink / raw)
To: dietmar.eggemann@arm.com, broonie@kernel.org,
Szabolcs.Nagy@arm.com, brauner@kernel.org,
dave.hansen@linux.intel.com, debug@rivosinc.com, mgorman@suse.de,
vincent.guittot@linaro.org, fweimer@redhat.com, mingo@redhat.com,
rostedt@goodmis.org, hjl.tools@gmail.com, tglx@linutronix.de,
vschneid@redhat.com, shuah@kernel.org, bristot@redhat.com,
hpa@zytor.com, peterz@infradead.org, bp@alien8.de,
bsegall@google.com, x86@kernel.org, juri.lelli@redhat.com
Cc: keescook@chromium.org, jannh@google.com,
linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org,
catalin.marinas@arm.com, linux-api@vger.kernel.org,
will@kernel.org
In-Reply-To: <20240203-clone3-shadow-stack-v5-4-322c69598e4b@kernel.org>
On Sat, 2024-02-03 at 00:05 +0000, Mark Brown wrote:
> + if (args->shadow_stack) {
> + addr = args->shadow_stack;
> + size = args->shadow_stack_size;
>
> - size = adjust_shstk_size(stack_size);
> - addr = alloc_shstk(0, size, 0, false);
> - if (IS_ERR_VALUE(addr))
> - return addr;
> + /* There should be a valid token at the top of the
> stack. */
> + if (!shstk_consume_token(tsk, addr + size -
> sizeof(u64)))
> + return (unsigned long)ERR_PTR(-EINVAL);
I think for this case, it needs:
shstk->base = 0;
shstk->size = 0;
To prevent trying to free the parents shadow stack when the child
exits.
^ permalink raw reply
* Re: [PATCH v2 2/2] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Christian Brauner @ 2024-02-10 10:23 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <20240209155644.GD3282@redhat.com>
> and I forgot to mention, if you want to add PIDFD_SIGNAL_PRGP you can
> look at __kill_pgrp_info().
Yeah, I did that and there's a semantical twist in the old kill(2)
system call that made me think:
(1) kill(-1234) => kill process group with id 1234
(2) kill(0) => kill process group of @current
which implementation wise is indicated by
__kill_pgrp_info(..., pid ? find_vpid(-pid) ? task_pgrp(current))
We're obviously not going to implement (2) as that doesn't really make a
sense for pidfd_send_signal().
But (1) is also wrong for pidfd_send_signal(). If we'd ever implement
(1) it should be via pidfd_open(1234, PIDFD_PROCESS_GROUP).
So if PIDFD_PROCESS_GROUP is set then we want to send a signal to the
process group that @pidfd is in. Unless there's reasons we can't do
this. I tried to draft this and what I have is a totally uncompiled
draft.
I was unsure how best to cleanly express how to take the process group
from the struct pid of that @pidfd. So I modeled it after how we do it
for PIDTYPE_TGID.
From 8d886b07cc1b17cc6dd3a9ebf19c51212282b6f5 Mon Sep 17 00:00:00 2001
From: Christian Brauner <brauner@kernel.org>
Date: Fri, 9 Feb 2024 15:49:45 +0100
Subject: [PATCH] [RFC] pidfd: allow to override signal scope in
pidfd_send_signal()
Right now we determine the scope of the signal based on the type of
pidfd. There are use-cases where it's useful to override the scope of
the signal. For example in [1]. Add flags to determine the scope of the
signal:
(1) PIDFD_SIGNAL_THREAD: send signal to specific thread reference by @pidfd
(2) PIDFD_SIGNAL_THREAD_GROUP: send signal to thread-group of @pidfd
(2) PIDFD_SIGNAL_PROCESS_GROUP: send signal to process-group of @pidfd
There's a semantical quirk in the old kill(2) system call that made me think:
(1) kill(-1234) => kill process group with id 1234
(2) kill(0) => kill process group of @current
as indicated by
__kill_pgrp_info(..., pid ? find_vpid(-pid) ? task_pgrp(current))
We're obviously not going to implement (2) as that doesn't really make a
lot of sense for pidfd_send_signal().
But (1) is also wrong for pidfd_send_signal(). If we'd ever implement
(1) it should be via pidfd_open(..., PIDFD_PROCESS_GROUP).
Link: https://github.com/systemd/systemd/issues/31093 [1]
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
include/uapi/linux/pidfd.h | 5 +++++
kernel/signal.c | 46 ++++++++++++++++++++++++++++++++++----
2 files changed, 47 insertions(+), 4 deletions(-)
diff --git a/include/uapi/linux/pidfd.h b/include/uapi/linux/pidfd.h
index 2e6461459877..72ec000a97cd 100644
--- a/include/uapi/linux/pidfd.h
+++ b/include/uapi/linux/pidfd.h
@@ -10,4 +10,9 @@
#define PIDFD_NONBLOCK O_NONBLOCK
#define PIDFD_THREAD O_EXCL
+/* Flags for pidfd_send_signal(). */
+#define PIDFD_SIGNAL_THREAD (1UL << 0)
+#define PIDFD_SIGNAL_THREAD_GROUP (1UL << 1)
+#define PIDFD_SIGNAL_PROCESS_GROUP (1UL << 2)
+
#endif /* _UAPI_LINUX_PIDFD_H */
diff --git a/kernel/signal.c b/kernel/signal.c
index 8b8169623850..f0f9a5a822b4 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3873,6 +3873,23 @@ static struct pid *pidfd_to_pid(const struct file *file)
return tgid_pidfd_to_pid(file);
}
+static int kill_pgrp_info(int sig, struct kernel_siginfo *info, struct pid *pid)
+{
+ struct task_struct *p;
+ int ret = -ESRCH;
+
+ read_lock(&tasklist_lock);
+ p = pid_task(pid, PIDTYPE_PID);
+ if (p)
+ ret = __kill_pgrp_info(sig, info, task_pgrp(p));
+ read_unlock(&tasklist_lock);
+ return ret;
+}
+
+#define PIDFD_SEND_SIGNAL_FLAGS \
+ (PIDFD_SIGNAL_THREAD | PIDFD_SIGNAL_THREAD_GROUP | \
+ PIDFD_SIGNAL_PROCESS_GROUP)
+
/**
* sys_pidfd_send_signal - Signal a process through a pidfd
* @pidfd: file descriptor of the process
@@ -3897,7 +3914,11 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
enum pid_type type;
/* Enforce flags be set to 0 until we add an extension. */
- if (flags)
+ if (flags & ~PIDFD_SEND_SIGNAL_FLAGS)
+ return -EINVAL;
+
+ /* Ensure that only a single signal scope determining flag is set. */
+ if (hweight32(flags & PIDFD_SEND_SIGNAL_FLAGS) > 1)
return -EINVAL;
f = fdget(pidfd);
@@ -3915,10 +3936,24 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
if (!access_pidfd_pidns(pid))
goto err;
- if (f.file->f_flags & PIDFD_THREAD)
+ switch (flags) {
+ case 0:
+ /* Infer scope from the type of pidfd. */
+ if (f.file->f_flags & PIDFD_THREAD)
+ type = PIDTYPE_PID;
+ else
+ type = PIDTYPE_TGID;
+ break;
+ case PIDFD_SIGNAL_THREAD:
type = PIDTYPE_PID;
- else
+ break;
+ case PIDFD_SIGNAL_THREAD_GROUP:
type = PIDTYPE_TGID;
+ break;
+ case PIDFD_SIGNAL_PROCESS_GROUP:
+ type = PIDTYPE_PGID;
+ break;
+ }
if (info) {
ret = copy_siginfo_from_user_any(&kinfo, info);
@@ -3938,7 +3973,10 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
prepare_kill_siginfo(sig, &kinfo, type);
}
- ret = kill_pid_info_type(sig, &kinfo, pid, type);
+ if (type == PIDFD_SIGNAL_PROCESS_GROUP)
+ ret = kill_pgrp_info(sig, &kinfo, pid);
+ else
+ ret = kill_pid_info_type(sig, &kinfo, pid, type);
err:
fdput(f);
return ret;
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v2 2/2] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Oleg Nesterov @ 2024-02-10 12:30 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <20240210-abfinden-beimessen-2dbfea59b0da@brauner>
Christian,
Thanks again! the last 2 commits in vfs.pidfd look good to me.
As for this patch, I am not sure I understand your concerns, and I
have another concern, please see below.
For the moment, please forget about PIDFD_THREAD.
On 02/10, Christian Brauner wrote:
>
> (1) kill(-1234) => kill process group with id 1234
> (2) kill(0) => kill process group of @current
>
> which implementation wise is indicated by
>
> __kill_pgrp_info(..., pid ? find_vpid(-pid) ? task_pgrp(current))
>
> We're obviously not going to implement (2) as that doesn't really make a
> sense for pidfd_send_signal().
Sure,
> But (1) is also wrong for pidfd_send_signal(). If we'd ever implement
> (1) it should be via pidfd_open(1234, PIDFD_PROCESS_GROUP).
Why do you think we need another flag for open() ?
To me it looks fine if we allow to send the signal to pgrp if
flags & PIDFD_SIGNAL_PROCESS_GROUP.
And pidfd_send_signal() can just do
if (PIDFD_SIGNAL_THREAD_GROUP)
ret = __kill_pgrp_info(sig, kinfo, pid);
else
ret = kill_pid_info_type(...);
(yes, yes, this needs tasklist, just a pseudo code to simpliy)
Now lets recall about PIDFD_THREAD.
If the target task is a group leader - there is no difference.
If it is not a leader - then __kill_pgrp_info() will always return
-ESRCH, do_each_pid_task(PIDTYPE_PGID) won't find any task.
And personally I think this is all we need.
------------------------------------------------------------------------------
But if you want to make PIDFD_SIGNAL_THREAD_GROUP work even if the
target task is not a leader, then yes, we need something like
task_pgrp(pid_task(pid, PIDTYPE_PID))
like you did in the new kill_pgrp_info() helper in this patch.
I won't argue, but do you think this makes a lot of sense?
Oleg.
^ permalink raw reply
* Re: [PATCH v2 2/2] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Oleg Nesterov @ 2024-02-10 12:47 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <20240210123033.GA27557@redhat.com>
On 02/10, Oleg Nesterov wrote:
>
> On 02/10, Christian Brauner wrote:
> >
> > (1) kill(-1234) => kill process group with id 1234
> > (2) kill(0) => kill process group of @current
> >
> > which implementation wise is indicated by
> >
> > __kill_pgrp_info(..., pid ? find_vpid(-pid) ? task_pgrp(current))
> >
> > We're obviously not going to implement (2) as that doesn't really make a
> > sense for pidfd_send_signal().
>
> Sure,
>
> > But (1) is also wrong for pidfd_send_signal(). If we'd ever implement
> > (1) it should be via pidfd_open(1234, PIDFD_PROCESS_GROUP).
>
> Why do you think we need another flag for open() ?
>
> To me it looks fine if we allow to send the signal to pgrp if
> flags & PIDFD_SIGNAL_PROCESS_GROUP.
>
> And pidfd_send_signal() can just do
>
> if (PIDFD_SIGNAL_THREAD_GROUP)
> ret = __kill_pgrp_info(sig, kinfo, pid);
> else
> ret = kill_pid_info_type(...);
>
> (yes, yes, this needs tasklist, just a pseudo code to simpliy)
>
> Now lets recall about PIDFD_THREAD.
>
> If the target task is a group leader - there is no difference.
>
> If it is not a leader - then __kill_pgrp_info() will always return
> -ESRCH, do_each_pid_task(PIDTYPE_PGID) won't find any task.
To clarify, __kill_pgrp_info() should send the signal to pgrp
identified by @pid, so it will return ESRCH if the target didn't
do setpgid/etc.
> And personally I think this is all we need.
Yes. I don't think we should send a signal to task_pgrp(target).
And this matches sys_kill(). I mean,
pidfd = pidfd_open(1234);
pidfd_send_signal(pidfd, PIDFD_PROCESS_GROUP);
should act as kill(-1234).
> ------------------------------------------------------------------------------
> But if you want to make PIDFD_SIGNAL_THREAD_GROUP work even if the
> target task is not a leader, then yes, we need something like
>
> task_pgrp(pid_task(pid, PIDTYPE_PID))
>
> like you did in the new kill_pgrp_info() helper in this patch.
>
> I won't argue, but do you think this makes a lot of sense?
>
> Oleg.
^ permalink raw reply
* Re: [PATCH v2 2/2] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Christian Brauner @ 2024-02-10 12:54 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <20240210123033.GA27557@redhat.com>
On Sat, Feb 10, 2024 at 01:30:33PM +0100, Oleg Nesterov wrote:
> Christian,
>
> Thanks again! the last 2 commits in vfs.pidfd look good to me.
>
> As for this patch, I am not sure I understand your concerns, and I
> have another concern, please see below.
>
> For the moment, please forget about PIDFD_THREAD.
>
> On 02/10, Christian Brauner wrote:
> >
> > (1) kill(-1234) => kill process group with id 1234
> > (2) kill(0) => kill process group of @current
> >
> > which implementation wise is indicated by
> >
> > __kill_pgrp_info(..., pid ? find_vpid(-pid) ? task_pgrp(current))
> >
> > We're obviously not going to implement (2) as that doesn't really make a
> > sense for pidfd_send_signal().
>
> Sure,
>
> > But (1) is also wrong for pidfd_send_signal(). If we'd ever implement
> > (1) it should be via pidfd_open(1234, PIDFD_PROCESS_GROUP).
>
> Why do you think we need another flag for open() ?
We don't need one. We could if we wanted to was my point. But let's
ignore that for now.
>
> To me it looks fine if we allow to send the signal to pgrp if
> flags & PIDFD_SIGNAL_PROCESS_GROUP.
Yes, that's what I want too, I just wonder about the semantics.
>
> And pidfd_send_signal() can just do
>
> if (PIDFD_SIGNAL_THREAD_GROUP)
> ret = __kill_pgrp_info(sig, kinfo, pid);
> else
> ret = kill_pid_info_type(...);
>
> (yes, yes, this needs tasklist, just a pseudo code to simpliy)
>
> Now lets recall about PIDFD_THREAD.
>
> If the target task is a group leader - there is no difference.
>
> If it is not a leader - then __kill_pgrp_info() will always return
> -ESRCH, do_each_pid_task(PIDTYPE_PGID) won't find any task.
>
> And personally I think this is all we need.
>
> ------------------------------------------------------------------------------
> But if you want to make PIDFD_SIGNAL_THREAD_GROUP work even if the
> target task is not a leader, then yes, we need something like
>
> task_pgrp(pid_task(pid, PIDTYPE_PID))
>
> like you did in the new kill_pgrp_info() helper in this patch.
>
> I won't argue, but do you think this makes a lot of sense?
The question is what is more useful for userspace when they do:
pidfd_send_signal(1234, PIDFD_SEND_PROCESS_GROUP)?
(1) They either mean to signal a process group that is headed by 1234.
(2) Or they want to signal a process group of which 1234 is a member or
the leader.
From a usability perspective (1) is a lot more restrictive because it
requires @pidfd to refer to a process group leader. Whereas (2) doesn't
require userspace to hold a @pidfd to a process group leader. It is
enough to just hold a @pidfd. In other words, (2) has wider scope.
And intuitively that is what I had thought is more useful. But by that
logic PIDFD_SEND_THREAD_GROUP would have to signal to the thread-group
that @pidfd is in regardless of whether @pidfd is actually a
thread-group leader.
Which is also what you're pointing out, afaict.
^ permalink raw reply
* Re: [PATCH v2 2/2] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Oleg Nesterov @ 2024-02-10 13:15 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <20240210-dackel-getan-619c70fefa62@brauner>
On 02/10, Christian Brauner wrote:
>
> The question is what is more useful for userspace when they do:
> pidfd_send_signal(1234, PIDFD_SEND_PROCESS_GROUP)?
>
> (1) They either mean to signal a process group that is headed by 1234.
Yes, this is what I had in mind, see also another email from me.
Simple, clear, and matches kill(-1234).
> (2) Or they want to signal a process group of which 1234 is a member or
> the leader.
Somehow I didn't even consider this option when I thought about
PIDFD_SIGNAL_PGRP...
> From a usability perspective (1) is a lot more restrictive because it
> requires @pidfd to refer to a process group leader.
Yes, but to be honest (2) doesn't fit my head. Probably simply because
I always had (1) in mind...
But I won't argue if you think that (2) has useful usecases.
Oleg.
^ permalink raw reply
* Re: [PATCH v2 2/2] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Christian Brauner @ 2024-02-10 14:26 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <20240210131518.GC27557@redhat.com>
On Sat, Feb 10, 2024 at 02:15:18PM +0100, Oleg Nesterov wrote:
> On 02/10, Christian Brauner wrote:
> >
> > The question is what is more useful for userspace when they do:
> > pidfd_send_signal(1234, PIDFD_SEND_PROCESS_GROUP)?
> >
> > (1) They either mean to signal a process group that is headed by 1234.
>
> Yes, this is what I had in mind, see also another email from me.
> Simple, clear, and matches kill(-1234).
I went for a walk and kept thinking about this and I agree with you.
It will require that 1234 will be a process group leader but I think
that this is ok to require that. The implementation becomes a lot
simpler of course.
From 6a9d6a6bd8b77cba210b9c28f0e6e047c7956e9f Mon Sep 17 00:00:00 2001
From: Christian Brauner <brauner@kernel.org>
Date: Fri, 9 Feb 2024 15:49:45 +0100
Subject: [PATCH v2] [RFC] pidfd: allow to override signal scope in
pidfd_send_signal()
Right now we determine the scope of the signal based on the type of
pidfd. There are use-cases where it's useful to override the scope of
the signal. For example in [1]. Add flags to determine the scope of the
signal:
(1) PIDFD_SIGNAL_THREAD: send signal to specific thread referenced by @pidfd
(2) PIDFD_SIGNAL_THREAD_GROUP: send signal to thread-group headed by @pidfd
(2) PIDFD_SIGNAL_PROCESS_GROUP: send signal to process-group headed by @pidfd
Link: https://github.com/systemd/systemd/issues/31093 [1]
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
include/uapi/linux/pidfd.h | 5 +++++
kernel/signal.c | 44 +++++++++++++++++++++++++++++++-------
2 files changed, 41 insertions(+), 8 deletions(-)
diff --git a/include/uapi/linux/pidfd.h b/include/uapi/linux/pidfd.h
index 2e6461459877..72ec000a97cd 100644
--- a/include/uapi/linux/pidfd.h
+++ b/include/uapi/linux/pidfd.h
@@ -10,4 +10,9 @@
#define PIDFD_NONBLOCK O_NONBLOCK
#define PIDFD_THREAD O_EXCL
+/* Flags for pidfd_send_signal(). */
+#define PIDFD_SIGNAL_THREAD (1UL << 0)
+#define PIDFD_SIGNAL_THREAD_GROUP (1UL << 1)
+#define PIDFD_SIGNAL_PROCESS_GROUP (1UL << 2)
+
#endif /* _UAPI_LINUX_PIDFD_H */
diff --git a/kernel/signal.c b/kernel/signal.c
index 8b8169623850..cee7cd27ec88 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1905,16 +1905,19 @@ int send_sig_fault_trapno(int sig, int code, void __user *addr, int trapno,
return send_sig_info(info.si_signo, &info, t);
}
-int kill_pgrp(struct pid *pid, int sig, int priv)
+static int kill_pgrp_info(int sig, struct kernel_siginfo *info, struct pid *pgrp)
{
int ret;
-
read_lock(&tasklist_lock);
- ret = __kill_pgrp_info(sig, __si_special(priv), pid);
+ ret = __kill_pgrp_info(sig, info, pgrp);
read_unlock(&tasklist_lock);
-
return ret;
}
+
+int kill_pgrp(struct pid *pid, int sig, int priv)
+{
+ return kill_pgrp_info(sig, __si_special(priv), pid);
+}
EXPORT_SYMBOL(kill_pgrp);
int kill_pid(struct pid *pid, int sig, int priv)
@@ -3873,6 +3876,10 @@ static struct pid *pidfd_to_pid(const struct file *file)
return tgid_pidfd_to_pid(file);
}
+#define PIDFD_SEND_SIGNAL_FLAGS \
+ (PIDFD_SIGNAL_THREAD | PIDFD_SIGNAL_THREAD_GROUP | \
+ PIDFD_SIGNAL_PROCESS_GROUP)
+
/**
* sys_pidfd_send_signal - Signal a process through a pidfd
* @pidfd: file descriptor of the process
@@ -3897,7 +3904,11 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
enum pid_type type;
/* Enforce flags be set to 0 until we add an extension. */
- if (flags)
+ if (flags & ~PIDFD_SEND_SIGNAL_FLAGS)
+ return -EINVAL;
+
+ /* Ensure that only a single signal scope determining flag is set. */
+ if (hweight32(flags & PIDFD_SEND_SIGNAL_FLAGS) > 1)
return -EINVAL;
f = fdget(pidfd);
@@ -3915,10 +3926,24 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
if (!access_pidfd_pidns(pid))
goto err;
- if (f.file->f_flags & PIDFD_THREAD)
+ switch (flags) {
+ case 0:
+ /* Infer scope from the type of pidfd. */
+ if (f.file->f_flags & PIDFD_THREAD)
+ type = PIDTYPE_PID;
+ else
+ type = PIDTYPE_TGID;
+ break;
+ case PIDFD_SIGNAL_THREAD:
type = PIDTYPE_PID;
- else
+ break;
+ case PIDFD_SIGNAL_THREAD_GROUP:
type = PIDTYPE_TGID;
+ break;
+ case PIDFD_SIGNAL_PROCESS_GROUP:
+ type = PIDTYPE_PGID;
+ break;
+ }
if (info) {
ret = copy_siginfo_from_user_any(&kinfo, info);
@@ -3938,7 +3963,10 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
prepare_kill_siginfo(sig, &kinfo, type);
}
- ret = kill_pid_info_type(sig, &kinfo, pid, type);
+ if (type == PIDFD_SIGNAL_PROCESS_GROUP)
+ ret = kill_pgrp_info(sig, &kinfo, pid);
+ else
+ ret = kill_pid_info_type(sig, &kinfo, pid, type);
err:
fdput(f);
return ret;
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v2 2/2] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Oleg Nesterov @ 2024-02-10 16:51 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <20240210-chihuahua-hinzog-3945b6abd44a@brauner>
On 02/10, Christian Brauner wrote:
>
> On Sat, Feb 10, 2024 at 02:15:18PM +0100, Oleg Nesterov wrote:
> > On 02/10, Christian Brauner wrote:
> > >
> > > The question is what is more useful for userspace when they do:
> > > pidfd_send_signal(1234, PIDFD_SEND_PROCESS_GROUP)?
> > >
> > > (1) They either mean to signal a process group that is headed by 1234.
> >
> > Yes, this is what I had in mind, see also another email from me.
> > Simple, clear, and matches kill(-1234).
>
> I went for a walk and kept thinking about this and I agree with you.
> It will require that 1234 will be a process group leader but I think
> that this is ok to require that.
Yes... but I am starting to understand why you mentioned the new
open PIDFD_PROCESS_GROUP flag... perhaps we can do something like
this later, but this needs more thinking.
> + if (type == PIDFD_SIGNAL_PROCESS_GROUP)
> + ret = kill_pgrp_info(sig, &kinfo, pid);
I guess you meant
if (type == PIDTYPE_PGID)
other than that,
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
^ permalink raw reply
* Re: [PATCH v2 2/2] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Christian Brauner @ 2024-02-10 17:22 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <20240210165133.GD27557@redhat.com>
On Sat, Feb 10, 2024 at 05:51:33PM +0100, Oleg Nesterov wrote:
> On 02/10, Christian Brauner wrote:
> >
> > On Sat, Feb 10, 2024 at 02:15:18PM +0100, Oleg Nesterov wrote:
> > > On 02/10, Christian Brauner wrote:
> > > >
> > > > The question is what is more useful for userspace when they do:
> > > > pidfd_send_signal(1234, PIDFD_SEND_PROCESS_GROUP)?
> > > >
> > > > (1) They either mean to signal a process group that is headed by 1234.
> > >
> > > Yes, this is what I had in mind, see also another email from me.
> > > Simple, clear, and matches kill(-1234).
> >
> > I went for a walk and kept thinking about this and I agree with you.
> > It will require that 1234 will be a process group leader but I think
> > that this is ok to require that.
>
> Yes... but I am starting to understand why you mentioned the new
> open PIDFD_PROCESS_GROUP flag... perhaps we can do something like
> this later, but this needs more thinking.
>
> > + if (type == PIDFD_SIGNAL_PROCESS_GROUP)
> > + ret = kill_pgrp_info(sig, &kinfo, pid);
>
> I guess you meant
>
> if (type == PIDTYPE_PGID)
>
> other than that,
Bahaa, yes of course.
>
> Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Thanks!
^ permalink raw reply
* Re: [PATCH net-next v7 1/4] eventpoll: support busy poll per epoll instance
From: Eric Dumazet @ 2024-02-11 9:35 UTC (permalink / raw)
To: Joe Damato
Cc: linux-kernel, netdev, chuck.lever, jlayton, linux-api, brauner,
davem, alexander.duyck, sridhar.samudrala, kuba,
willemdebruijn.kernel, weiwan, David.Laight, arnd, sdf,
amritha.nambiar, Alexander Viro, Jan Kara,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240209211528.51234-2-jdamato@fastly.com>
On Fri, Feb 9, 2024 at 10:15 PM Joe Damato <jdamato@fastly.com> 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.
>
> busy_poll_usecs is a u32, but in a follow up patch the ioctl provided to
> the user only allows setting a value from 0 to S32_MAX.
>
> Signed-off-by: Joe Damato <jdamato@fastly.com>
> Acked-by: Stanislav Fomichev <sdf@google.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* Re: [PATCH net-next v7 4/4] eventpoll: Add epoll ioctl for epoll_params
From: Eric Dumazet @ 2024-02-11 9:40 UTC (permalink / raw)
To: Joe Damato
Cc: linux-kernel, netdev, chuck.lever, jlayton, linux-api, brauner,
davem, alexander.duyck, sridhar.samudrala, kuba,
willemdebruijn.kernel, weiwan, David.Laight, arnd, sdf,
amritha.nambiar, Jiri Slaby, Jonathan Corbet, Alexander Viro,
Jan Kara, Michael Ellerman, Nathan Lynch, Greg Kroah-Hartman,
Maik Broemme, Steve French, Thomas Zimmermann, Julien Panis,
Thomas Huth, Albert Ou, Palmer Dabbelt, open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240209211528.51234-5-jdamato@fastly.com>
On Fri, Feb 9, 2024 at 10:15 PM Joe Damato <jdamato@fastly.com> 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 <= s32_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>
> Acked-by: Stanislav Fomichev <sdf@google.com>
> Reviewed-by: Jiri Slaby <jirislaby@kernel.org>
> ---
> .../userspace-api/ioctl/ioctl-number.rst | 1 +
> fs/eventpoll.c | 72 +++++++++++++++++++
> include/uapi/linux/eventpoll.h | 13 ++++
> 3 files changed, 86 insertions(+)
>
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index 457e16f06e04..b33918232f78 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -309,6 +309,7 @@ Code Seq# Include File Comments
> 0x89 0B-DF linux/sockios.h
> 0x89 E0-EF linux/sockios.h SIOCPROTOPRIVATE range
> 0x89 F0-FF linux/sockios.h SIOCDEVPRIVATE range
> +0x8A 00-1F linux/eventpoll.h
> 0x8B all linux/wireless.h
> 0x8C 00-3F WiNRADiO driver
> <http://www.winradio.com.au/>
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index 1b8d01af0c2c..aa58d42737e6 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -37,6 +37,7 @@
> #include <linux/seq_file.h>
> #include <linux/compat.h>
> #include <linux/rculist.h>
> +#include <linux/capability.h>
> #include <net/busy_poll.h>
>
> /*
> @@ -494,6 +495,49 @@ 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 = file->private_data;
> + void __user *uarg = (void __user *)arg;
> + struct epoll_params epoll_params;
> +
> + switch (cmd) {
> + case EPIOCSPARAMS:
> + if (copy_from_user(&epoll_params, uarg, sizeof(epoll_params)))
> + return -EFAULT;
> +
> + /* pad byte must be zero */
> + if (epoll_params.__pad)
> + return -EINVAL;
> +
> + if (epoll_params.busy_poll_usecs > S32_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;
You need WRITE_ONCE(ep->XXX, val); for all these settings.
> + ep->busy_poll_budget = epoll_params.busy_poll_budget;
> + ep->prefer_busy_poll = epoll_params.prefer_busy_poll;
> + return 0;
> + case EPIOCGPARAMS:
> + memset(&epoll_params, 0, sizeof(epoll_params));
> + epoll_params.busy_poll_usecs = ep->busy_poll_usecs;
You need to use READ_ONCE(ep->XXXXX) for the three reads.
> + 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;
> + }
> +}
>
^ permalink raw reply
* Re: [PATCH RFT v5 4/7] fork: Add shadow stack support to clone3()
From: Mark Brown @ 2024-02-12 15:38 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: dietmar.eggemann@arm.com, Szabolcs.Nagy@arm.com,
brauner@kernel.org, dave.hansen@linux.intel.com,
debug@rivosinc.com, mgorman@suse.de, vincent.guittot@linaro.org,
fweimer@redhat.com, mingo@redhat.com, rostedt@goodmis.org,
hjl.tools@gmail.com, tglx@linutronix.de, vschneid@redhat.com,
shuah@kernel.org, bristot@redhat.com, hpa@zytor.com,
peterz@infradead.org, bp@alien8.de, bsegall@google.com,
x86@kernel.org, juri.lelli@redhat.com, keescook@chromium.org,
jannh@google.com, linux-kselftest@vger.kernel.org,
linux-kernel@vger.kernel.org, catalin.marinas@arm.com,
linux-api@vger.kernel.org, will@kernel.org
In-Reply-To: <565ca9697cf26be5509ef4b3c1cc95fa4f692b9f.camel@intel.com>
[-- Attachment #1: Type: text/plain, Size: 771 bytes --]
On Fri, Feb 09, 2024 at 08:18:11PM +0000, Edgecombe, Rick P wrote:
> On Sat, 2024-02-03 at 00:05 +0000, Mark Brown wrote:
> > + if (write_user_shstk_64((u64 __user *)addr, 0))
> > + return false;
> > +
> > + return true;
> > +}
> So, don't we want to consume the token on the *new* task's MM, which
> was already duplicated but still unmapped? In which case I think the
> other arch's would need to GUP regardless of the existence of shadow
> stack atomic ops.
Yes, that would be better - if nothing else it allows reuse of the same
shadow stack for multiple !CLONE_VM clone3()s.
> I wonder about adding a shstk_post_fork() to make it easier to think
> about and maintain, even if there are no issues today.
I agree.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH net-next v8 0/4] Per epoll context busy poll support
From: Joe Damato @ 2024-02-13 6:16 UTC (permalink / raw)
To: 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, Joe Damato,
Alexander Viro, Greg Kroah-Hartman, Helge Deller, Jan Kara,
Jiri Slaby, Jonathan Corbet, Julien Panis,
open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure), Michael Ellerman,
Nathan Lynch, Palmer Dabbelt, Steve French, Thomas Huth,
Thomas Zimmermann
Greetings:
Welcome to v8.
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
v7 -> v8:
- Reviewed-by tag from Eric Dumazet applied to commit message of patch
1/4.
- patch 4/4:
- EPIOCSPARAMS and EPIOCGPARAMS updated to use WRITE_ONCE and
READ_ONCE, as requested by Eric Dumazet
- Wrapped a long line (via netdev/checkpatch)
v6 -> v7:
- Acked-by tags from Stanislav Fomichev applied to commit messages of
all patches.
- Reviewed-by tags from Jakub Kicinski, Eric Dumazet applied to commit
messages of patches 2 and 3. Jiri Slaby's Reviewed-by applied to patch
4.
- patch 1/4:
- busy_poll_usecs reduced from u64 to u32.
- Unnecessary parens removed (via netdev/checkpatch)
- Wrapped long line (via netdev/checkpatch)
- Remove inline from busy_loop_ep_timeout as objdump suggests the
function is already inlined
- Moved struct eventpoll assignment to declaration
- busy_loop_ep_timeout is moved within CONFIG_NET_RX_BUSY_POLL and the
ifdefs internally have been removed as per Eric Dumazet's review
- Removed ep_busy_loop_on from the !defined CONFIG_NET_RX_BUSY_POLL
section as it is only called when CONFIG_NET_RX_BUSY_POLL is
defined
- patch 3/4:
- Fix whitespace alignment issue (via netdev/checkpatch)
- patch 4/4:
- epoll_params.busy_poll_usecs has been reduced to u32
- epoll_params.busy_poll_usecs is now checked to ensure it is <=
S32_MAX
- __pad has been reduced to a single u8
- memchr_inv has been dropped and replaced with a simple check for the
single __pad byte
- Removed space after cast (via netdev/checkpatch)
- Wrap long line (via netdev/checkpatch)
- Move struct eventpoll *ep assignment to declaration as per Jiri
Slaby's review
- Remove unnecessary !! as per Jiri Slaby's review
- Reorganized variables to be reverse christmas tree order
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 | 131 +++++++++++++++++-
include/uapi/linux/eventpoll.h | 13 ++
3 files changed, 138 insertions(+), 7 deletions(-)
--
2.25.1
^ permalink raw reply
* [PATCH net-next v8 1/4] eventpoll: support busy poll per epoll instance
From: Joe Damato @ 2024-02-13 6:16 UTC (permalink / raw)
To: 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, Joe Damato,
Alexander Viro, Jan Kara,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240213061652.6342-1-jdamato@fastly.com>
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.
busy_poll_usecs is a u32, but in a follow up patch the ioctl provided to
the user only allows setting a value from 0 to S32_MAX.
Signed-off-by: Joe Damato <jdamato@fastly.com>
Acked-by: Stanislav Fomichev <sdf@google.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
---
fs/eventpoll.c | 44 +++++++++++++++++++++++++++++++++++++++-----
1 file changed, 39 insertions(+), 5 deletions(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 3534d36a1474..401f865eced9 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -227,6 +227,8 @@ struct eventpoll {
#ifdef CONFIG_NET_RX_BUSY_POLL
/* used to track busy poll napi_id */
unsigned int napi_id;
+ /* busy poll timeout */
+ u32 busy_poll_usecs;
#endif
#ifdef CONFIG_DEBUG_LOCK_ALLOC
@@ -387,11 +389,41 @@ static inline int ep_events_available(struct eventpoll *ep)
}
#ifdef CONFIG_NET_RX_BUSY_POLL
+/**
+ * busy_loop_ep_timeout - check if busy poll has timed out. The timeout value
+ * from the epoll instance ep is preferred, but if it is not set fallback to
+ * the system-wide global via busy_loop_timeout.
+ *
+ * @start_time: The start time used to compute the remaining time until timeout.
+ * @ep: Pointer to the eventpoll context.
+ *
+ * Return: true if the timeout has expired, false otherwise.
+ */
+static bool busy_loop_ep_timeout(unsigned long start_time,
+ struct eventpoll *ep)
+{
+ unsigned long bp_usec = READ_ONCE(ep->busy_poll_usecs);
+
+ if (bp_usec) {
+ unsigned long end_time = start_time + bp_usec;
+ unsigned long now = busy_loop_current_time();
+
+ return time_after(now, end_time);
+ } else {
+ return busy_loop_timeout(start_time);
+ }
+}
+
+static bool ep_busy_loop_on(struct eventpoll *ep)
+{
+ return !!ep->busy_poll_usecs || net_busy_loop_on();
+}
+
static bool ep_busy_loop_end(void *p, unsigned long start_time)
{
struct eventpoll *ep = p;
- return ep_events_available(ep) || busy_loop_timeout(start_time);
+ return ep_events_available(ep) || busy_loop_ep_timeout(start_time, ep);
}
/*
@@ -404,7 +436,7 @@ static bool ep_busy_loop(struct eventpoll *ep, int nonblock)
{
unsigned int napi_id = READ_ONCE(ep->napi_id);
- if ((napi_id >= MIN_NAPI_ID) && net_busy_loop_on()) {
+ if (napi_id >= MIN_NAPI_ID && ep_busy_loop_on(ep)) {
napi_busy_loop(napi_id, nonblock ? NULL : ep_busy_loop_end, ep, false,
BUSY_POLL_BUDGET);
if (ep_events_available(ep))
@@ -425,12 +457,12 @@ static bool ep_busy_loop(struct eventpoll *ep, int nonblock)
*/
static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
{
- struct eventpoll *ep;
+ struct eventpoll *ep = epi->ep;
unsigned int napi_id;
struct socket *sock;
struct sock *sk;
- if (!net_busy_loop_on())
+ if (!ep_busy_loop_on(ep))
return;
sock = sock_from_file(epi->ffd.file);
@@ -442,7 +474,6 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
return;
napi_id = READ_ONCE(sk->sk_napi_id);
- ep = epi->ep;
/* Non-NAPI IDs can be rejected
* or
@@ -2058,6 +2089,9 @@ static int do_epoll_create(int flags)
error = PTR_ERR(file);
goto out_free_fd;
}
+#ifdef CONFIG_NET_RX_BUSY_POLL
+ ep->busy_poll_usecs = 0;
+#endif
ep->file = file;
fd_install(fd, file);
return fd;
--
2.25.1
^ permalink raw reply related
* [PATCH net-next v8 2/4] eventpoll: Add per-epoll busy poll packet budget
From: Joe Damato @ 2024-02-13 6:16 UTC (permalink / raw)
To: 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, Joe Damato,
Alexander Viro, Jan Kara,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240213061652.6342-1-jdamato@fastly.com>
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.
Signed-off-by: Joe Damato <jdamato@fastly.com>
Acked-by: Stanislav Fomichev <sdf@google.com>
Reviewed-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Eric Dumazet <edumazet@google.com>
---
fs/eventpoll.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 401f865eced9..ed83ae33dd45 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -229,6 +229,8 @@ struct eventpoll {
unsigned int napi_id;
/* busy poll timeout */
u32 busy_poll_usecs;
+ /* busy poll packet budget */
+ u16 busy_poll_budget;
#endif
#ifdef CONFIG_DEBUG_LOCK_ALLOC
@@ -435,10 +437,14 @@ static bool ep_busy_loop_end(void *p, unsigned long start_time)
static bool ep_busy_loop(struct eventpoll *ep, int nonblock)
{
unsigned int napi_id = READ_ONCE(ep->napi_id);
+ u16 budget = READ_ONCE(ep->busy_poll_budget);
+
+ if (!budget)
+ budget = BUSY_POLL_BUDGET;
if (napi_id >= MIN_NAPI_ID && ep_busy_loop_on(ep)) {
napi_busy_loop(napi_id, nonblock ? NULL : ep_busy_loop_end, ep, false,
- BUSY_POLL_BUDGET);
+ budget);
if (ep_events_available(ep))
return true;
/*
@@ -2091,6 +2097,7 @@ static int do_epoll_create(int flags)
}
#ifdef CONFIG_NET_RX_BUSY_POLL
ep->busy_poll_usecs = 0;
+ ep->busy_poll_budget = 0;
#endif
ep->file = file;
fd_install(fd, file);
--
2.25.1
^ permalink raw reply related
* [PATCH net-next v8 3/4] eventpoll: Add per-epoll prefer busy poll option
From: Joe Damato @ 2024-02-13 6:16 UTC (permalink / raw)
To: 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, Joe Damato,
Alexander Viro, Jan Kara,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240213061652.6342-1-jdamato@fastly.com>
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.
Signed-off-by: Joe Damato <jdamato@fastly.com>
Acked-by: Stanislav Fomichev <sdf@google.com>
Reviewed-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Eric Dumazet <edumazet@google.com>
---
fs/eventpoll.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index ed83ae33dd45..1b8d01af0c2c 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -231,6 +231,7 @@ struct eventpoll {
u32 busy_poll_usecs;
/* busy poll packet budget */
u16 busy_poll_budget;
+ bool prefer_busy_poll;
#endif
#ifdef CONFIG_DEBUG_LOCK_ALLOC
@@ -438,13 +439,14 @@ static bool ep_busy_loop(struct eventpoll *ep, int nonblock)
{
unsigned int napi_id = READ_ONCE(ep->napi_id);
u16 budget = READ_ONCE(ep->busy_poll_budget);
+ bool prefer_busy_poll = READ_ONCE(ep->prefer_busy_poll);
if (!budget)
budget = BUSY_POLL_BUDGET;
if (napi_id >= MIN_NAPI_ID && ep_busy_loop_on(ep)) {
- napi_busy_loop(napi_id, nonblock ? NULL : ep_busy_loop_end, ep, false,
- budget);
+ napi_busy_loop(napi_id, nonblock ? NULL : ep_busy_loop_end,
+ ep, prefer_busy_poll, budget);
if (ep_events_available(ep))
return true;
/*
@@ -2098,6 +2100,7 @@ static int do_epoll_create(int flags)
#ifdef CONFIG_NET_RX_BUSY_POLL
ep->busy_poll_usecs = 0;
ep->busy_poll_budget = 0;
+ ep->prefer_busy_poll = false;
#endif
ep->file = file;
fd_install(fd, file);
--
2.25.1
^ permalink raw reply related
* [PATCH net-next v8 4/4] eventpoll: Add epoll ioctl for epoll_params
From: Joe Damato @ 2024-02-13 6:16 UTC (permalink / raw)
To: 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, Joe Damato,
Jiri Slaby, Jonathan Corbet, Alexander Viro, Jan Kara,
Michael Ellerman, Greg Kroah-Hartman, Nathan Lynch,
Thomas Zimmermann, Julien Panis, Steve French, Palmer Dabbelt,
open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240213061652.6342-1-jdamato@fastly.com>
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 <= s32_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>
Acked-by: Stanislav Fomichev <sdf@google.com>
Reviewed-by: Jiri Slaby <jirislaby@kernel.org>
---
.../userspace-api/ioctl/ioctl-number.rst | 1 +
fs/eventpoll.c | 73 +++++++++++++++++++
include/uapi/linux/eventpoll.h | 13 ++++
3 files changed, 87 insertions(+)
diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index 457e16f06e04..b33918232f78 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -309,6 +309,7 @@ Code Seq# Include File Comments
0x89 0B-DF linux/sockios.h
0x89 E0-EF linux/sockios.h SIOCPROTOPRIVATE range
0x89 F0-FF linux/sockios.h SIOCDEVPRIVATE range
+0x8A 00-1F linux/eventpoll.h
0x8B all linux/wireless.h
0x8C 00-3F WiNRADiO driver
<http://www.winradio.com.au/>
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 1b8d01af0c2c..df2ed3af486e 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -37,6 +37,7 @@
#include <linux/seq_file.h>
#include <linux/compat.h>
#include <linux/rculist.h>
+#include <linux/capability.h>
#include <net/busy_poll.h>
/*
@@ -494,6 +495,49 @@ 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 = file->private_data;
+ void __user *uarg = (void __user *)arg;
+ struct epoll_params epoll_params;
+
+ switch (cmd) {
+ case EPIOCSPARAMS:
+ if (copy_from_user(&epoll_params, uarg, sizeof(epoll_params)))
+ return -EFAULT;
+
+ /* pad byte must be zero */
+ if (epoll_params.__pad)
+ return -EINVAL;
+
+ if (epoll_params.busy_poll_usecs > S32_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;
+
+ WRITE_ONCE(ep->busy_poll_usecs, epoll_params.busy_poll_usecs);
+ WRITE_ONCE(ep->busy_poll_budget, epoll_params.busy_poll_budget);
+ WRITE_ONCE(ep->prefer_busy_poll, epoll_params.prefer_busy_poll);
+ return 0;
+ case EPIOCGPARAMS:
+ memset(&epoll_params, 0, sizeof(epoll_params));
+ epoll_params.busy_poll_usecs = READ_ONCE(ep->busy_poll_usecs);
+ epoll_params.busy_poll_budget = READ_ONCE(ep->busy_poll_budget);
+ epoll_params.prefer_busy_poll = READ_ONCE(ep->prefer_busy_poll);
+ if (copy_to_user(uarg, &epoll_params, sizeof(epoll_params)))
+ return -EFAULT;
+ return 0;
+ default:
+ return -ENOIOCTLCMD;
+ }
+}
+
#else
static inline bool ep_busy_loop(struct eventpoll *ep, int nonblock)
@@ -505,6 +549,12 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
{
}
+static long ep_eventpoll_bp_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ return -EOPNOTSUPP;
+}
+
#endif /* CONFIG_NET_RX_BUSY_POLL */
/*
@@ -864,6 +914,27 @@ static void ep_clear_and_put(struct eventpoll *ep)
ep_free(ep);
}
+static long ep_eventpoll_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ int ret;
+
+ if (!is_file_epoll(file))
+ return -EINVAL;
+
+ switch (cmd) {
+ case EPIOCSPARAMS:
+ case EPIOCGPARAMS:
+ ret = ep_eventpoll_bp_ioctl(file, cmd, arg);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ return ret;
+}
+
static int ep_eventpoll_release(struct inode *inode, struct file *file)
{
struct eventpoll *ep = file->private_data;
@@ -970,6 +1041,8 @@ static const struct file_operations eventpoll_fops = {
.release = ep_eventpoll_release,
.poll = ep_eventpoll_poll,
.llseek = noop_llseek,
+ .unlocked_ioctl = ep_eventpoll_ioctl,
+ .compat_ioctl = compat_ptr_ioctl,
};
/*
diff --git a/include/uapi/linux/eventpoll.h b/include/uapi/linux/eventpoll.h
index cfbcc4cc49ac..4f4b948ef381 100644
--- a/include/uapi/linux/eventpoll.h
+++ b/include/uapi/linux/eventpoll.h
@@ -85,4 +85,17 @@ struct epoll_event {
__u64 data;
} EPOLL_PACKED;
+struct epoll_params {
+ __u32 busy_poll_usecs;
+ __u16 busy_poll_budget;
+ __u8 prefer_busy_poll;
+
+ /* pad the struct to a multiple of 64bits */
+ __u8 __pad;
+};
+
+#define EPOLL_IOC_TYPE 0x8A
+#define EPIOCSPARAMS _IOW(EPOLL_IOC_TYPE, 0x01, struct epoll_params)
+#define EPIOCGPARAMS _IOR(EPOLL_IOC_TYPE, 0x02, struct epoll_params)
+
#endif /* _UAPI_LINUX_EVENTPOLL_H */
--
2.25.1
^ permalink raw reply related
* Re: [PATCH net-next v8 4/4] eventpoll: Add epoll ioctl for epoll_params
From: Eric Dumazet @ 2024-02-13 6:34 UTC (permalink / raw)
To: Joe Damato
Cc: linux-kernel, netdev, chuck.lever, jlayton, linux-api, brauner,
davem, alexander.duyck, sridhar.samudrala, kuba,
willemdebruijn.kernel, weiwan, David.Laight, arnd, sdf,
amritha.nambiar, Jiri Slaby, Jonathan Corbet, Alexander Viro,
Jan Kara, Michael Ellerman, Greg Kroah-Hartman, Nathan Lynch,
Thomas Zimmermann, Julien Panis, Steve French, Palmer Dabbelt,
open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240213061652.6342-5-jdamato@fastly.com>
On Tue, Feb 13, 2024 at 7:17 AM Joe Damato <jdamato@fastly.com> 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 <= s32_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>
> Acked-by: Stanislav Fomichev <sdf@google.com>
> Reviewed-by: Jiri Slaby <jirislaby@kernel.org>
Reviewed-by: Eric Dumazet <edumazet@google.com>
Thanks !
^ permalink raw reply
* Re: [PATCH v4] ELF: AT_PAGE_SHIFT_MASK -- supply userspace with available page shifts
From: Alexey Dobriyan @ 2024-02-13 18:51 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Florian Weimer, linux-kernel, linux-arch, linux-api,
x86, Eric Biederman, linux-mm
In-Reply-To: <202402091625.4DF63CDD0B@keescook>
On Fri, Feb 09, 2024 at 04:41:36PM -0800, Kees Cook wrote:
> On Fri, Feb 09, 2024 at 03:30:37PM +0300, Alexey Dobriyan wrote:
> > On Mon, Feb 05, 2024 at 04:48:08AM -0800, Kees Cook wrote:
> > > On Mon, Feb 05, 2024 at 12:51:43PM +0300, Alexey Dobriyan wrote:
> > > > +#define ARCH_AT_PAGE_SHIFT_MASK \
> > > > + do { \
> > > > + u32 val = 1 << 12; \
> > > > + if (boot_cpu_has(X86_FEATURE_PSE)) { \
> > > > + val |= 1 << 21; \
> > > > + } \
> > > > + if (boot_cpu_has(X86_FEATURE_GBPAGES)) { \
> > > > + val |= 1 << 30; \
> > > > + } \
> > >
> > > Can we use something besides literal "12", "21", and "30" values here?
> >
> > Ehh, no, why? Inside x86_64 the page shifts are very specific numbers,
> > they won't change.
>
> Well, it's nicer to have meaningful words to describe these things.
Not really. Inside specific arch page shifts are fixed, so using names
is just more macros one need to remember.
If I were to invent names (which I wouldn't), the best names are
PAGE_SHIFT
PAGE_SHIFT2
PAGE_SHIFT3
...
with PAGE_SHIFT2, PAGE_SHIFT3 being optional macros if arch doesn't support
multiple page sizes.
> In fact, PAGE_SHIFT already exists for 12, and HPAGE_SHIFT already exists
> for 21. Please use those, and add another, perhaps GBPAGE_SHIFT, for 30.
HPAGE_SHIFT is bad name, H doesn't describe anything unless arch is
known. Hugepages is marketing name. If GBPAGE_SHIFT is good name,
then HPAGE_SHIFT is bad name, it should've been MBPAGE_SHIFT, which
wrong because it is 2 MiB not 1 MiB.
BTW parisc has REAL_HPAGE_SHIFT !
^ permalink raw reply
* Re: [PATCH v4] ELF: AT_PAGE_SHIFT_MASK -- supply userspace with available page shifts
From: Kees Cook @ 2024-02-13 19:39 UTC (permalink / raw)
To: Alexey Dobriyan
Cc: Andrew Morton, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Florian Weimer, linux-kernel, linux-arch, linux-api,
x86, Eric Biederman, linux-mm
In-Reply-To: <54774e70-1e94-44f5-b318-fdfd5115041d@p183>
On Tue, Feb 13, 2024 at 09:51:01PM +0300, Alexey Dobriyan wrote:
> On Fri, Feb 09, 2024 at 04:41:36PM -0800, Kees Cook wrote:
> > On Fri, Feb 09, 2024 at 03:30:37PM +0300, Alexey Dobriyan wrote:
> > > On Mon, Feb 05, 2024 at 04:48:08AM -0800, Kees Cook wrote:
> > > > On Mon, Feb 05, 2024 at 12:51:43PM +0300, Alexey Dobriyan wrote:
> > > > > +#define ARCH_AT_PAGE_SHIFT_MASK \
> > > > > + do { \
> > > > > + u32 val = 1 << 12; \
> > > > > + if (boot_cpu_has(X86_FEATURE_PSE)) { \
> > > > > + val |= 1 << 21; \
> > > > > + } \
> > > > > + if (boot_cpu_has(X86_FEATURE_GBPAGES)) { \
> > > > > + val |= 1 << 30; \
> > > > > + } \
> > > >
> > > > Can we use something besides literal "12", "21", and "30" values here?
> > >
> > > Ehh, no, why? Inside x86_64 the page shifts are very specific numbers,
> > > they won't change.
> >
> > Well, it's nicer to have meaningful words to describe these things.
>
> Not really. Inside specific arch page shifts are fixed, so using names
> is just more macros one need to remember.
>
> If I were to invent names (which I wouldn't), the best names are
>
> PAGE_SHIFT
> PAGE_SHIFT2
> PAGE_SHIFT3
> ...
>
> with PAGE_SHIFT2, PAGE_SHIFT3 being optional macros if arch doesn't support
> multiple page sizes.
>
> > In fact, PAGE_SHIFT already exists for 12, and HPAGE_SHIFT already exists
> > for 21. Please use those, and add another, perhaps GBPAGE_SHIFT, for 30.
>
> HPAGE_SHIFT is bad name, H doesn't describe anything unless arch is
> known. Hugepages is marketing name. If GBPAGE_SHIFT is good name,
> then HPAGE_SHIFT is bad name, it should've been MBPAGE_SHIFT, which
> wrong because it is 2 MiB not 1 MiB.
>
> BTW parisc has REAL_HPAGE_SHIFT !
Sure, I mean, we've got an x86-specific function here, so let's use the
x86-specific macros we already have for 12 and 21, and then add the
missing one for 30.
--
Kees Cook
^ permalink raw reply
* Re: [PATCH net-next v8 0/4] Per epoll context busy poll support
From: patchwork-bot+netdevbpf @ 2024-02-14 11:10 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, sdf,
amritha.nambiar, viro, gregkh, deller, jack, jirislaby, corbet,
jpanis, linux-doc, linux-fsdevel, mpe, nathanl, palmer, stfrench,
thuth, tzimmermann
In-Reply-To: <20240213061652.6342-1-jdamato@fastly.com>
Hello:
This series was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:
On Tue, 13 Feb 2024 06:16:41 +0000 you wrote:
> Greetings:
>
> Welcome to v8.
>
> 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.
>
> [...]
Here is the summary with links:
- [net-next,v8,1/4] eventpoll: support busy poll per epoll instance
https://git.kernel.org/netdev/net-next/c/85455c795c07
- [net-next,v8,2/4] eventpoll: Add per-epoll busy poll packet budget
https://git.kernel.org/netdev/net-next/c/c6aa2a7778d8
- [net-next,v8,3/4] eventpoll: Add per-epoll prefer busy poll option
https://git.kernel.org/netdev/net-next/c/de57a2510822
- [net-next,v8,4/4] eventpoll: Add epoll ioctl for epoll_params
https://git.kernel.org/netdev/net-next/c/18e2bf0edf4d
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* Re: [PATCH v2 2/2] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Oleg Nesterov @ 2024-02-14 12:36 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <20240210165133.GD27557@redhat.com>
On 02/10, Oleg Nesterov wrote:
>
> On 02/10, Christian Brauner wrote:
> >
> > + if (type == PIDFD_SIGNAL_PROCESS_GROUP)
> > + ret = kill_pgrp_info(sig, &kinfo, pid);
>
> I guess you meant
>
> if (type == PIDTYPE_PGID)
>
> other than that,
>
> Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Yes, but there is another thing I hadn't thought of...
sys_pidfd_send_signal() does
/* Only allow sending arbitrary signals to yourself. */
ret = -EPERM;
if ((task_pid(current) != pid) &&
(kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL))
goto err;
and I am not sure that task_pid(current) == pid should allow
the "arbitrary signals" if PIDFD_SIGNAL_PROCESS_GROUP.
Perhaps
/* Only allow sending arbitrary signals to yourself. */
ret = -EPERM;
if ((task_pid(current) != pid || type == PIDTYPE_PGID) &&
(kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL)
goto err;
?
Oleg.
^ permalink raw reply
* [PATCH v2] uapi/auxvec: Define AT_HWCAP3 and AT_HWCAP4 aux vector, entries
From: Peter Bergner @ 2024-02-14 22:34 UTC (permalink / raw)
To: linux-api, linux-arch, linux-kernel@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org
Cc: Adhemerval Zanella, Szabolcs Nagy, Nick Piggin, Michael Ellerman,
Arnd Bergmann
Changes from v1:
- Add Acked-by lines.
The powerpc toolchain keeps a copy of the HWCAP bit masks in our TCB for fast
access by the __builtin_cpu_supports built-in function. The TCB space for
the HWCAP entries - which are created in pairs - is an ABI extension, so
waiting to create the space for HWCAP3 and HWCAP4 until we need them is
problematical. Define AT_HWCAP3 and AT_HWCAP4 in the generic uapi header
so they can be used in glibc to reserve space in the powerpc TCB for their
future use.
I scanned through the Linux and GLIBC source codes looking for unused AT_*
values and 29 and 30 did not seem to be used, so they are what I went
with. This has received Acked-by's from both GLIBC and Linux kernel
developers and no reservations or Nacks from anyone.
Arnd, we seem to have consensus on the patch below. Is this something
you could take and apply to your tree?
Peter
Signed-off-by: Peter Bergner <bergner@linux.ibm.com>
Acked-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> (glibc)
Acked-by: Nicholas Piggin <npiggin@gmail.com> (powerpc)
Acked-by: Szabolcs Nagy <szabolcs.nagy@arm.com> (arm)
---
include/uapi/linux/auxvec.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/uapi/linux/auxvec.h b/include/uapi/linux/auxvec.h
index 6991c4b8ab18..cc61cb9b3e9a 100644
--- a/include/uapi/linux/auxvec.h
+++ b/include/uapi/linux/auxvec.h
@@ -32,6 +32,8 @@
#define AT_HWCAP2 26 /* extension of AT_HWCAP */
#define AT_RSEQ_FEATURE_SIZE 27 /* rseq supported feature size */
#define AT_RSEQ_ALIGN 28 /* rseq allocation alignment */
+#define AT_HWCAP3 29 /* extension of AT_HWCAP */
+#define AT_HWCAP4 30 /* extension of AT_HWCAP */
#define AT_EXECFN 31 /* filename of program */
--
2.39.3
^ permalink raw reply related
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