* Re: Extending clone_args for clone3()
From: Arnd Bergmann @ 2025-06-04 8:29 UTC (permalink / raw)
To: Yury Khrustalev, linux-kernel
Cc: Christian Brauner, Mark Brown, Mark Rutland, linux-api
In-Reply-To: <aCs65ccRQtJBnZ_5@arm.com>
On Mon, May 19, 2025, at 16:06, Yury Khrustalev wrote:
>
> This seems fragile and may call for writing cumbersome code. In essence,
> we will have to have clone30(), clone31(), clone32()... wrappers which
> probably defeats the point of why clone3 was added:
>
>
> if (clone32_supported && clone32(...) == -1 && errno == E2BIG)
> {
> clone32_supported = false;
> /* ... */
> }
> else if (clone31_supported && clone31(...) == -1 && errno == E2BIG)
> {
> clone12_supported = false;
> /* ... */
> }
> ...
>
> Is there a neat way to work around this? What was the idea for extending
> clone_args in practice?
>
> I suppose we can't rely on kernel version because support for extended
> clone_args can be backported. In any case, we'd have to do a syscall
> for this (it would probably be great to have kernel version in auxv).
I don't think there is a generic way to handle extended syscalls
from libc, it really depends on the specific feature it's trying
to use that requires the additional fields to be nonzero: some features
may have a reasonable fallback implementation in libc, other features
still require an error to be passed back to the caller.
As I understand the shadow stack feature, we want this to be enabled
whenever the kernel and hardware supports it, completely transparent
to an application, right?
I think ideally we'd check for HWCAP_GCS on arm64 or the equivalent
feature on other architectures and expect clone3 to support the
longer argument whenever that is set, but it looks like that would
break on current kernels that already support HWCAP_GCS but not
the clone3 argument.
Adding one more hwcap flag would be ugly, but that seems to be
the easiest way. That way, glibc can just test for the new hwcap
flag only use the extra clone3 word if all prerequisites (hardware
support, kernel gcs support, clone3 argument support) are there.
Arnd
^ permalink raw reply
* Re: [DISCUSSION] proposed mctl() API
From: Matthew Wilcox @ 2025-06-02 18:01 UTC (permalink / raw)
To: Johannes Weiner
Cc: Lorenzo Stoakes, Andrew Morton, Shakeel Butt, Liam R . Howlett,
David Hildenbrand, Vlastimil Babka, Jann Horn, Arnd Bergmann,
Christian Brauner, SeongJae Park, Usama Arif, Mike Rapoport,
Barry Song, linux-mm, linux-arch, linux-kernel, linux-api,
Pedro Falcato
In-Reply-To: <20250529211423.GA1271329@cmpxchg.org>
On Thu, May 29, 2025 at 05:14:23PM -0400, Johannes Weiner wrote:
> On Thu, May 29, 2025 at 04:28:46PM +0100, Matthew Wilcox wrote:
> > Barry's problem is that we're all nervous about possibly regressing
> > performance on some unknown workloads. Just try Barry's proposal, see
> > if anyone actually compains or if we're just afraid of our own shadows.
>
> I actually explained why I think this is a terrible idea. But okay, I
> tried the patch anyway.
Sorry, I must've missed that one ;-(
> This is 'git log' on a hot kernel repo after a large IO stream:
>
> VANILLA BARRY
> Real time 49.93 ( +0.00%) 60.36 ( +20.48%)
> User time 32.10 ( +0.00%) 32.09 ( -0.04%)
> System time 14.41 ( +0.00%) 14.64 ( +1.50%)
> pgmajfault 9227.00 ( +0.00%) 18390.00 ( +99.30%)
> workingset_refault_file 184.00 ( +0.00%) 236899.00 (+127954.05%)
>
> Clearly we can't generally ignore page cache hits just because the
> mmaps() are intermittent.
>
> The whole point is to cache across processes and their various
> apertures into a common, long-lived filesystem space.
>
> Barry knows something about the relationship between certain processes
> and certain files that he could exploit with MADV_COLD-on-exit
> semantics. But that's not something the kernel can safely assume. Not
> without defeating the page cache for an entire class of file accesses.
So what about distinguishing between exited-normally processes (ie git
log) vs killed-by-oom processes (ie Barry's usecase)? Update the
referenced bit in the first case and not the second?
^ permalink raw reply
* [PATCH 5.4 193/204] pidfd: check pid has attached task in fdinfo
From: Greg Kroah-Hartman @ 2025-06-02 13:48 UTC (permalink / raw)
To: stable
Cc: Greg Kroah-Hartman, patches, Jann Horn, Christian Kellner,
linux-api, Christian Brauner, Oleg Nesterov, Christian Brauner
In-Reply-To: <20250602134255.449974357@linuxfoundation.org>
5.4-stable review patch. If anyone has any objections, please let me know.
------------------
From: Christian Brauner <christian.brauner@ubuntu.com>
commit 3d6d8da48d0b214d65ea0227d47228abc75d7c88 upstream.
Currently, when a task is dead we still print the pid it used to use in
the fdinfo files of its pidfds. This doesn't make much sense since the
pid may have already been reused. So verify that the task is still alive
by introducing the pid_has_task() helper which will be used by other
callers in follow-up patches.
If the task is not alive anymore, we will print -1. This allows us to
differentiate between a task not being present in a given pid namespace
- in which case we already print 0 - and a task having been reaped.
Note that this uses PIDTYPE_PID for the check. Technically, we could've
checked PIDTYPE_TGID since pidfds currently only refer to thread-group
leaders but if they won't anymore in the future then this check becomes
problematic without it being immediately obvious to non-experts imho. If
a thread is created via clone(CLONE_THREAD) than struct pid has a single
non-empty list pid->tasks[PIDTYPE_PID] and this pid can't be used as a
PIDTYPE_TGID meaning pid->tasks[PIDTYPE_TGID] will return NULL even
though the thread-group leader might still be very much alive. So
checking PIDTYPE_PID is fine and is easier to maintain should we ever
allow pidfds to refer to threads.
Cc: Jann Horn <jannh@google.com>
Cc: Christian Kellner <christian@kellner.me>
Cc: linux-api@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Link: https://lore.kernel.org/r/20191017101832.5985-1-christian.brauner@ubuntu.com
Signed-off-by: Christian Brauner <brauner@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
include/linux/pid.h | 4 ++++
kernel/fork.c | 10 ++++++++--
2 files changed, 12 insertions(+), 2 deletions(-)
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -85,6 +85,10 @@ static inline struct pid *get_pid(struct
extern void put_pid(struct pid *pid);
extern struct task_struct *pid_task(struct pid *pid, enum pid_type);
+static inline bool pid_has_task(struct pid *pid, enum pid_type type)
+{
+ return !hlist_empty(&pid->tasks[type]);
+}
extern struct task_struct *get_pid_task(struct pid *pid, enum pid_type);
extern struct pid *get_task_pid(struct task_struct *task, enum pid_type type);
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1703,10 +1703,16 @@ static int pidfd_release(struct inode *i
#ifdef CONFIG_PROC_FS
static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
{
- struct pid_namespace *ns = proc_pid_ns(file_inode(m->file));
struct pid *pid = f->private_data;
+ struct pid_namespace *ns;
+ pid_t nr = -1;
- seq_put_decimal_ull(m, "Pid:\t", pid_nr_ns(pid, ns));
+ if (likely(pid_has_task(pid, PIDTYPE_PID))) {
+ ns = proc_pid_ns(file_inode(m->file));
+ nr = pid_nr_ns(pid, ns);
+ }
+
+ seq_put_decimal_ll(m, "Pid:\t", nr);
seq_putc(m, '\n');
}
#endif
^ permalink raw reply
* Re: Extending clone_args for clone3()
From: Yury Khrustalev @ 2025-06-02 14:10 UTC (permalink / raw)
To: linux-kernel
Cc: Christian Brauner, Arnd Bergmann, Mark Brown, Mark Rutland,
linux-api
In-Reply-To: <aCs65ccRQtJBnZ_5@arm.com>
Hi everyone,
A gentle ping :)
On Mon, May 19, 2025 at 03:06:29PM +0100, Yury Khrustalev wrote:
> Hi,
>
> I'm working on an RFC patch for Glibc to make use of the newly added
> shadow_stack_token field in struct clone_args in [1] on arm64 targets.
>
> I encountered the following problem. Glibc might be built with newer
> version of struct clone_args than the currently running kernel. In
> this case, we may attempt to use a non-zero value in the new field
> in args (and pass size bigger than expected by the kernel) and the
> kernel will reject the syscall with E2BIG error.
>
> This seems to be due to a fail-early approach. The unexpected non-
> zero values beyond what's supported by the kernel may indicate that
> userspace expects something to happen (and may even have allocated
> some resources). So it's better to indicate a problem rather than
> silently ignore this and have userspace encounter an error later.
>
> However, it creates difficulty with using extended "versions" of
> the clone3 syscall. AFAIK, there is no way to ask kernel about
> the supported size of struct clone_args except for making syscalls
> with decreasing value of size until we stop getting E2BIG.
>
> This seems fragile and may call for writing cumbersome code. In essence,
> we will have to have clone30(), clone31(), clone32()... wrappers which
> probably defeats the point of why clone3 was added:
>
>
> if (clone32_supported && clone32(...) == -1 && errno == E2BIG)
> {
> clone32_supported = false;
> /* ... */
> }
> else if (clone31_supported && clone31(...) == -1 && errno == E2BIG)
> {
> clone12_supported = false;
> /* ... */
> }
> ...
>
> Is there a neat way to work around this? What was the idea for extending
> clone_args in practice?
>
> I suppose we can't rely on kernel version because support for extended
> clone_args can be backported. In any case, we'd have to do a syscall
> for this (it would probably be great to have kernel version in auxv).
>
> I appreciate any advice here.
>
> Thanks,
> Yury
>
>
> [1]: https://lore.kernel.org/all/20250416-clone3-shadow-stack-v16-0-2ffc9ca3917b@kernel.org/
>
Kind regards,
Yury
^ permalink raw reply
* Re: [DISCUSSION] proposed mctl() API
From: Lorenzo Stoakes @ 2025-05-30 13:10 UTC (permalink / raw)
To: Usama Arif
Cc: Andrew Morton, Shakeel Butt, Liam R . Howlett, David Hildenbrand,
Vlastimil Babka, Jann Horn, Arnd Bergmann, Christian Brauner,
SeongJae Park, Mike Rapoport, Johannes Weiner, Barry Song,
linux-mm, linux-arch, linux-kernel, linux-api, Pedro Falcato
In-Reply-To: <e166592f-aeb3-4573-bb73-270a2eb90be3@gmail.com>
On Thu, May 29, 2025 at 06:21:55PM +0100, Usama Arif wrote:
>
>
> On 29/05/2025 15:43, Lorenzo Stoakes wrote:
> > ## INTRODUCTION
> >
> > After discussions in various threads (Usama's series adding a new prctl()
> > in [0], and a proposal to adapt process_madvise() to do the same -
> > conception in [1] and RFC in [2]), it seems fairly clear that it would make
> > sense to explore a dedicated API to explicitly allow for actions which
> > affect the virtual address space as a whole.
> >
> > Also, Barry is implementing a feature (currently under RFC) which could
> > additionally make use of this API (see [3]).
> >
> > [0]: https://lore.kernel.org/all/20250515133519.2779639-1-usamaarif642@gmail.com/
> > [1]: https://lore.kernel.org/linux-mm/c390dd7e-0770-4d29-bb0e-f410ff6678e3@lucifer.local/
> > [2]: https://lore.kernel.org/all/cover.1747686021.git.lorenzo.stoakes@oracle.com/
> > [3]: https://lore.kernel.org/all/20250514070820.51793-1-21cnbao@gmail.com/
> >
> > While madvise() and process_madvise() are useful for altering the
> > attributes of VMAs within a virtual address space, it isn't the right fit
> > for something that affects the whole address space.
> >
> > Additionally, a requirement of Usama's proposal (see [0]) is that we have
> > the ability to propagate the change in behaviour across fork/exec. This
> > further suggests the need for a dedicated interface, as this really sits
> > outside the ordinary behaviour of [process_]madvise().
> >
> > prctl() is too broad and encourages mm code to migrate to kernel/sys.c
> > where it is at risk of bit-rotting. It can make it harder/impossible to
> > isolate mm logic for testing and logic there might be missed in changes
> > moving forward.
> >
> > It also, like so many kernel interfaces, has 'grown roots out of its pot'
> > so to speak - while it started off as an ostensible 'process' manipulation
> > interface, prctl() operations manipulate a myriad of task, virtual
> > address space and even specific VMA attributes.
> >
> > At this stage it really is a 'catch-all' for things we simply couldn't fit
> > elsewhere.
> >
> > Therefore, as suggested by the rather excellent Liam Howlett, I propose an
> > mm-specific interface that _explicitly_ manipulates attributes of the
> > virtual address space as a whole.
> >
> > I think something that mimics the simplicity of [process_]madvise() makes
> > sense - have a limited set of actions that can be taken, and treat them as
> > a simple action - a user requests you do XXX to the virtual address space
> > (that is, across the mm_struct), and you do it.
> >
>
>
> Hi Lorenzo,
>
> Thanks for writing the proposal, this is awesome!
Thanks :)
>
> Whatever the community agrees with, whether its this or prctl, happy to
> move forward with either as both should accomplish the usecase proposed.
Awesome!
>
> I will just add some points over here in defence of prctl, this is just for
> discussion, and if the community disagrees, completely happy to move forward
> with new syscall as well.
Please do - it's vital we have all sides of the argument!
>
> When it comes to having mm code in kernel/sys.c, we can just do something
> like below that can actually clean it up?
>
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 3a2df1bd9f64..bfadc339e2c5 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2467,6 +2467,12 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>
> error = 0;
> switch (option) {
> + case PR_SET_MM:
> + case PR_GET_THP_DISABLE:
> + case PR_SET_THP_DISABLE:
> + case PR_NEW_MM_THING:
> + error = some_function_in_mm_folder(); // in mm/mctl.c ?
> + break;
> case PR_SET_PDEATHSIG:
> if (!valid_signal(arg2)) {
> error = -EINVAL;
>
> when it comes to prctl becoming a catch-all thing, with above clean up,
> we can be a lot more careful to what gets added to the mm side of prctl.
Sure, and I think we were to decide prctl() was the way to go this would be the
way it should look, or at least specifically for this case.
This addresses the bit rot issue but not how hidden the functionality is, what
prctl() generally represents de-facto (that is - a catch-all for obscure stuff),
depending on the interface of the function it might be harder/impossible to
userland-test and of course we have the beautiful vector-interface that means we
can't restrict behaviour in the API in the way we'd ideally like.
I mean overall there is no beautiful solution here.
As Matthew says, the reason we're struggling here is probably more so because
this is just showing up a flaw in how THP is these days - we really ideally need
something automagic where the kernel figures things out for us.
But in the meantime we have this frankly kinda terrible interface that's rather
off/on and yet want to establish policy _ourselves_ when the kernel should be
able to figure it out.
But with that said, we must trade that off with the reality that there is a need
for this kind of fine-grained control now, in this less than ideal world we
currently inhabit :)
>
> The advantage of this is it avoids having another syscall.
> My personal view (which can be wrong :)) is that a new syscall should be
> for something major,
> and I feel that PR_DEFAULT_MADV_HUGEPAGE and PR_DEFAULT_MADV_NOHUGEPAGE
> might be small enough to fit in prctl? but I completely understand
> your point of view as well!
See Andrew's point on the syscall thing, they're cheap ;)
I mean the idea of course is that we can put all future stuff like that here
too.
We could even port some existing prctl() stuff across, in theory. But having 2
ways of doing the same thing would probably suck.
>
> > ## INTERFACE
> >
> > The proposed interface is simply:
> >
> > int mctl(int pidfd, int action, unsigned int flags);
> >
> > Since PIDFD_SELF is now available, it is easy to invoke this for the
> > current process, while also adding the flexibility of being able to apply
> > actions to other processes also.
> >
> > The function will return 0 on success, -1 on failure, with errno set to the
> > error that arose, standard stuff.
> >
> > The behaviour will be tailored to each action taken.
> >
> > To begin with, I propose a single flag:
> >
> > - MCTL_SET_DEFAULT_EXEC - Persists this behaviour across fork/exec.
> >
> > This again will be tailored - only certain actions will be allowed to set
> > this flag, and we will of course assert appropriate capabilities, etc. upon
> > its use.
> >
>
> Sounds good to me. Just adding this here, the solution will be used in systemd
> in exec_invoke, similar to how KSM is done with prctl in [1], so for the THP
> solution, we would need MCTL_SET_DEFAULT_EXEC as it would need to be inherited
> across fork+exec.
Right yeah, this was the intent.
>
> [1] https://github.com/systemd/systemd/blob/2e72d3efafa88c1cb4d9b28dd4ade7c6ab7be29a/src/core/exec-invoke.c#L5046
>
> > All actions would, impact every VMA (if adjustments to VMAs are required).
> >
> > ## SECURITY
> >
> > Of course, security will be of utmost concern (Jann's input is important
> > here :)
> >
> > We can vary security requirements depending on the action taken.
> >
> > For an initial version I suggest we simply limit operations which:
> >
> > - Operate on a remote process
> > - Use the MCTL_SET_DEFAULT_EXEC flag
> >
> > To those tasks which possess the CAP_SYS_ADMIN capability.
> >
> > This may be too restrictive - be good to get some feedback on this.
> >
> > I know Jann raised concerns around privileged execution and perhaps it'd be
> > useful to see whether this would make more sense for the SET_DEFAULT_EXEC
> > case or not.
> >
> > Usama - would requiring CAP_SYS_ADMIN be egregious to your use case?
> >
>
> My knowledge is security is limited, so please bare with me, but I actually
> didn't understand the security issue and the need for CAP_SYS_ADMIN for
> doing VM_(NO)HUGEPAGE.
>
> A process can already madvise its own VMAs, and this is just doing that
> for the entire process. And VM_INIT_DEF_MASK is already set to VM_NOHUGEPAGE
> so it will be inherited by the parent. Just adding VM_HUGEPAGE shouldnt be
> a issue? Inheriting MMF_VM_HUGEPAGE will mean that khugepaged would enter
> for that process as well, which again doesnt seem like a security issue
> to me.
W.R.T. the current process, the Issue is one Jann raised, in relation to
propagation of behaviour to privileged (e.g. setuid) processes.
W.R.T. remote processes, obviously we want to make sure we are permitted to do
so.
>
> > ## IMPLEMENTATION
> >
> > I think that sensibly we'd need to add some new files here, mm/mctl.c,
> > include/linux/mctl.h (the latter of providing the MCTL_xxx actions and
> > flags).
> >
> > We could find ways to share code between mm files where appropriate to
> > avoid too much duplication.
> >
> > I suggest that the best way forward, if we were minded to examine how this
> > would look in practice, would be for me to implement an RFC that adds the
> > interface, and a simple MCTL_SET_NOHUGEPAGE, MCTL_CLEAR_NOHUGEPAGE
> > implementation as a proof of concept.
> >
> > If we wanted to then go ahead with a non-RFC version, this could then form
> > a foundation upon which Usama and Barry could implement their features,
> > with Usama then able to add MCTL_[SET/CLEAR]_HUGEPAGE and Barry
> > MCTL_[SET/CLEAR]_FADE_ON_DEATH.
> >
> > Obviously I don't mean to presume to suggest how we might proceed here -
> > only suggesting this might be a good way of moving forward and getting
> > things done as quickly as possible while allowing you guys to move forward
> > with your features.
> >
> > Let me know if this makes sense, alternatively I could try to find a
> > relatively benign action to implement as part of the base work, or we could
> > simply collaborate to do it all in one series with multiple authors?
> >
> > ## RFC
> >
> > The above is all only in effect 'putting ideas out there' so this is
> > entirely an RFC in spirit and intent - let me know if this makes sense in
> > whole or part :)
> >
> > Thanks!
> >
> > Lorenzo
>
> Again thanks for the proposal! Happy to move forward with this or prctl.
> Just adding my 2 cents in this email.
Thank you! And your input is most appreciated - it's important we get all sides
of the story here!
>
> Thanks
> Usama
>
Cheers, Lorenzo
^ permalink raw reply
* Re: [DISCUSSION] proposed mctl() API
From: Vlastimil Babka @ 2025-05-30 10:31 UTC (permalink / raw)
To: Johannes Weiner, Matthew Wilcox
Cc: Lorenzo Stoakes, Andrew Morton, Shakeel Butt, Liam R . Howlett,
David Hildenbrand, Jann Horn, Arnd Bergmann, Christian Brauner,
SeongJae Park, Usama Arif, Mike Rapoport, Barry Song, linux-mm,
linux-arch, linux-kernel, linux-api, Pedro Falcato
In-Reply-To: <20250529211423.GA1271329@cmpxchg.org>
On 5/29/25 23:14, Johannes Weiner wrote:
> On Thu, May 29, 2025 at 04:28:46PM +0100, Matthew Wilcox wrote:
>> Barry's problem is that we're all nervous about possibly regressing
>> performance on some unknown workloads. Just try Barry's proposal, see
>> if anyone actually compains or if we're just afraid of our own shadows.
>
> I actually explained why I think this is a terrible idea. But okay, I
> tried the patch anyway.
>
> This is 'git log' on a hot kernel repo after a large IO stream:
>
> VANILLA BARRY
> Real time 49.93 ( +0.00%) 60.36 ( +20.48%)
> User time 32.10 ( +0.00%) 32.09 ( -0.04%)
> System time 14.41 ( +0.00%) 14.64 ( +1.50%)
> pgmajfault 9227.00 ( +0.00%) 18390.00 ( +99.30%)
> workingset_refault_file 184.00 ( +0.00%) 236899.00 (+127954.05%)
>
> Clearly we can't generally ignore page cache hits just because the
> mmaps() are intermittent.
>
> The whole point is to cache across processes and their various
> apertures into a common, long-lived filesystem space.
>
> Barry knows something about the relationship between certain processes
> and certain files that he could exploit with MADV_COLD-on-exit
> semantics. But that's not something the kernel can safely assume. Not
> without defeating the page cache for an entire class of file accesses.
I've just read the previous threads about Barry's proposal and if doing this
always isn't feasible, I'm wondering if memcg would be a better interface to
opt-in for this kind of behavior than both prctl or mctl. I think at least
conceptually it fits what memcg is doing? The question is if the
implementation would be feasible, and if android puts apps in separate memcgs...
^ permalink raw reply
* Re: [DISCUSSION] proposed mctl() API
From: Barry Song @ 2025-05-30 7:52 UTC (permalink / raw)
To: Johannes Weiner
Cc: Matthew Wilcox, Lorenzo Stoakes, Andrew Morton, Shakeel Butt,
Liam R . Howlett, David Hildenbrand, Vlastimil Babka, Jann Horn,
Arnd Bergmann, Christian Brauner, SeongJae Park, Usama Arif,
Mike Rapoport, linux-mm, linux-arch, linux-kernel, linux-api,
Pedro Falcato
In-Reply-To: <20250529211423.GA1271329@cmpxchg.org>
On Fri, May 30, 2025 at 9:14 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Thu, May 29, 2025 at 04:28:46PM +0100, Matthew Wilcox wrote:
> > Barry's problem is that we're all nervous about possibly regressing
> > performance on some unknown workloads. Just try Barry's proposal, see
> > if anyone actually compains or if we're just afraid of our own shadows.
>
> I actually explained why I think this is a terrible idea. But okay, I
> tried the patch anyway.
>
> This is 'git log' on a hot kernel repo after a large IO stream:
>
> VANILLA BARRY
> Real time 49.93 ( +0.00%) 60.36 ( +20.48%)
> User time 32.10 ( +0.00%) 32.09 ( -0.04%)
> System time 14.41 ( +0.00%) 14.64 ( +1.50%)
> pgmajfault 9227.00 ( +0.00%) 18390.00 ( +99.30%)
> workingset_refault_file 184.00 ( +0.00%) 236899.00 (+127954.05%)
>
> Clearly we can't generally ignore page cache hits just because the
> mmaps() are intermittent.
Hi Johannes,
Thanks!
Are you on v1, which lacks folio demotion[1], or v2, which includes it [2]?
[1] https://lore.kernel.org/linux-mm/20250412085852.48524-1-21cnbao@gmail.com/
[2] https://lore.kernel.org/linux-mm/20250514070820.51793-1-21cnbao@gmail.com/
>
> The whole point is to cache across processes and their various
> apertures into a common, long-lived filesystem space.
>
> Barry knows something about the relationship between certain processes
> and certain files that he could exploit with MADV_COLD-on-exit
> semantics. But that's not something the kernel can safely assume. Not
> without defeating the page cache for an entire class of file accesses.
Best Regards
Barry
^ permalink raw reply
* Re: [DISCUSSION] proposed mctl() API
From: Johannes Weiner @ 2025-05-29 23:14 UTC (permalink / raw)
To: Liam R. Howlett
Cc: Matthew Wilcox, Lorenzo Stoakes, Andrew Morton, Shakeel Butt,
David Hildenbrand, Vlastimil Babka, Jann Horn, Arnd Bergmann,
Christian Brauner, SeongJae Park, Usama Arif, Mike Rapoport,
Barry Song, linux-mm, linux-arch, linux-kernel, linux-api,
Pedro Falcato
In-Reply-To: <klrw3rjymes6phs5erz7eqkjji5oe3bg4j4jbqpjv3qzuw4vra@k6ei5pssfany>
On Thu, May 29, 2025 at 05:24:23PM -0400, Liam R. Howlett wrote:
> * Johannes Weiner <hannes@cmpxchg.org> [250529 17:14]:
> > On Thu, May 29, 2025 at 04:28:46PM +0100, Matthew Wilcox wrote:
> > > Barry's problem is that we're all nervous about possibly regressing
> > > performance on some unknown workloads. Just try Barry's proposal, see
> > > if anyone actually compains or if we're just afraid of our own shadows.
> >
> > I actually explained why I think this is a terrible idea. But okay, I
> > tried the patch anyway.
> >
> > This is 'git log' on a hot kernel repo after a large IO stream:
>
> Can you clarify this benchmark please?
>
> Is this running 'git log', then stream IO, then running 'git log' again?
Yes, but it's running git log twice first. On the vanilla kernel this
is the number of references when we usually activate.
You can substitute any sequence of commands that would interact with
the git objects repeatedly before a pause where programmer thinks.
You can probably get similar mmapIO patterns with sqlite, lmdb, etc.
Periodically running executables and scripts are another case. They
tend to be less latency-sensitive I suppose, but would still
unnecessarily eat into the available IO bandwidth.
^ permalink raw reply
* Re: [DISCUSSION] proposed mctl() API
From: Andrew Morton @ 2025-05-29 21:31 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Shakeel Butt, Liam R . Howlett, David Hildenbrand,
Vlastimil Babka, Jann Horn, Arnd Bergmann, Christian Brauner,
SeongJae Park, Usama Arif, Mike Rapoport, Johannes Weiner,
Barry Song, linux-mm, linux-arch, linux-kernel, linux-api,
Pedro Falcato
In-Reply-To: <85778a76-7dc8-4ea8-8827-acb45f74ee05@lucifer.local>
On Thu, 29 May 2025 15:43:26 +0100 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
>
> madvise()
>
> ...
>
> process_madvise()
>
> ...
>
> prctl()
>
> ...
>
Yeah. I think there has always been an attitude "ooh, new syscalls are
scary and I probably need permission from someone so let's graft it
onto something which is already there and hope nobody notices".
New syscalls are super easy and are cheap - it's just a table entry!
If it makes sense as a standalone thing, do that.
> The proposed interface is simply:
>
> int mctl(int pidfd, int action, unsigned int flags);
Well, why `flags'? One could even add a syscall per operation.
Debatable.
> Of course, security will be of utmost concern (Jann's input is important
> here :)
>
> We can vary security requirements depending on the action taken.
>
> For an initial version I suggest we simply limit operations which:
>
> - Operate on a remote process
> - Use the MCTL_SET_DEFAULT_EXEC flag
>
> To those tasks which possess the CAP_SYS_ADMIN capability.
>
> This may be too restrictive - be good to get some feedback on this.
Permissions needs careful consideration on a case-by-case basis.
Clearly in many cases, user A should be able to manipulate user A's
mm's.
And if different modes of mctl() end up with different permission
structures, one wonders why everything was clumped under the same
syscall! mctl() becomes "prctl() for memory".
Anyway, fun discussion. I'm with willy ;)
^ permalink raw reply
* Re: [DISCUSSION] proposed mctl() API
From: Liam R. Howlett @ 2025-05-29 21:24 UTC (permalink / raw)
To: Johannes Weiner
Cc: Matthew Wilcox, Lorenzo Stoakes, Andrew Morton, Shakeel Butt,
David Hildenbrand, Vlastimil Babka, Jann Horn, Arnd Bergmann,
Christian Brauner, SeongJae Park, Usama Arif, Mike Rapoport,
Barry Song, linux-mm, linux-arch, linux-kernel, linux-api,
Pedro Falcato
In-Reply-To: <20250529211423.GA1271329@cmpxchg.org>
* Johannes Weiner <hannes@cmpxchg.org> [250529 17:14]:
> On Thu, May 29, 2025 at 04:28:46PM +0100, Matthew Wilcox wrote:
> > Barry's problem is that we're all nervous about possibly regressing
> > performance on some unknown workloads. Just try Barry's proposal, see
> > if anyone actually compains or if we're just afraid of our own shadows.
>
> I actually explained why I think this is a terrible idea. But okay, I
> tried the patch anyway.
>
> This is 'git log' on a hot kernel repo after a large IO stream:
Can you clarify this benchmark please?
Is this running 'git log', then stream IO, then running 'git log' again?
>
> VANILLA BARRY
> Real time 49.93 ( +0.00%) 60.36 ( +20.48%)
> User time 32.10 ( +0.00%) 32.09 ( -0.04%)
> System time 14.41 ( +0.00%) 14.64 ( +1.50%)
> pgmajfault 9227.00 ( +0.00%) 18390.00 ( +99.30%)
> workingset_refault_file 184.00 ( +0.00%) 236899.00 (+127954.05%)
>
> Clearly we can't generally ignore page cache hits just because the
> mmaps() are intermittent.
>
> The whole point is to cache across processes and their various
> apertures into a common, long-lived filesystem space.
>
> Barry knows something about the relationship between certain processes
> and certain files that he could exploit with MADV_COLD-on-exit
> semantics. But that's not something the kernel can safely assume. Not
> without defeating the page cache for an entire class of file accesses.
^ permalink raw reply
* Re: [DISCUSSION] proposed mctl() API
From: Johannes Weiner @ 2025-05-29 21:14 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Lorenzo Stoakes, Andrew Morton, Shakeel Butt, Liam R . Howlett,
David Hildenbrand, Vlastimil Babka, Jann Horn, Arnd Bergmann,
Christian Brauner, SeongJae Park, Usama Arif, Mike Rapoport,
Barry Song, linux-mm, linux-arch, linux-kernel, linux-api,
Pedro Falcato
In-Reply-To: <aDh9LtSLCiTLjg2X@casper.infradead.org>
On Thu, May 29, 2025 at 04:28:46PM +0100, Matthew Wilcox wrote:
> Barry's problem is that we're all nervous about possibly regressing
> performance on some unknown workloads. Just try Barry's proposal, see
> if anyone actually compains or if we're just afraid of our own shadows.
I actually explained why I think this is a terrible idea. But okay, I
tried the patch anyway.
This is 'git log' on a hot kernel repo after a large IO stream:
VANILLA BARRY
Real time 49.93 ( +0.00%) 60.36 ( +20.48%)
User time 32.10 ( +0.00%) 32.09 ( -0.04%)
System time 14.41 ( +0.00%) 14.64 ( +1.50%)
pgmajfault 9227.00 ( +0.00%) 18390.00 ( +99.30%)
workingset_refault_file 184.00 ( +0.00%) 236899.00 (+127954.05%)
Clearly we can't generally ignore page cache hits just because the
mmaps() are intermittent.
The whole point is to cache across processes and their various
apertures into a common, long-lived filesystem space.
Barry knows something about the relationship between certain processes
and certain files that he could exploit with MADV_COLD-on-exit
semantics. But that's not something the kernel can safely assume. Not
without defeating the page cache for an entire class of file accesses.
^ permalink raw reply
* Re: [DISCUSSION] proposed mctl() API
From: Andy Lutomirski @ 2025-05-29 18:50 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Andrew Morton, Shakeel Butt, Liam R . Howlett, David Hildenbrand,
Vlastimil Babka, Jann Horn, Arnd Bergmann, Christian Brauner,
SeongJae Park, Usama Arif, Mike Rapoport, Johannes Weiner,
Barry Song, linux-mm, linux-arch, linux-kernel, linux-api,
Pedro Falcato
In-Reply-To: <85778a76-7dc8-4ea8-8827-acb45f74ee05@lucifer.local>
On Thu, May 29, 2025 at 7:44 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> The behaviour will be tailored to each action taken.
>
> To begin with, I propose a single flag:
>
> - MCTL_SET_DEFAULT_EXEC - Persists this behaviour across fork/exec.
It's hard to comment without a more complete proposal (*what* behavior
is persisted?), but off the top of my head, this isn't so great.
First, the name means nothing to me. What's "default exec"? Even
aside from that, why are fork and exec the same flag?
Beyond this, persisting anything across exec is a giant can of worms.
We have PR_SET_NO_NEW_PRIVS to make it less hazardous, but, in
general, it's risky and potentially quite confusing to do anything
that affects exec'd processes.
Oh, and this whole scheme is also potentially nasty for a different
reason: it's not thread safe. If one thread wants to spawn a process,
it should not interfere with another thread doing something else. So
making an mm flag that persists across close can interfere a bit, and
persisting it across clone + exec is even gnarlier.
For any of these, there should be matching query features -- CRIU,
debugging, etc should not be an afterthought.
^ permalink raw reply
* Re: [DISCUSSION] proposed mctl() API
From: Usama Arif @ 2025-05-29 18:32 UTC (permalink / raw)
To: Matthew Wilcox, Shakeel Butt
Cc: Lorenzo Stoakes, Andrew Morton, Liam R . Howlett,
David Hildenbrand, Vlastimil Babka, Jann Horn, Arnd Bergmann,
Christian Brauner, SeongJae Park, Mike Rapoport, Johannes Weiner,
Barry Song, linux-mm, linux-arch, linux-kernel, linux-api,
Pedro Falcato
In-Reply-To: <aDij5brAsGJVHCFK@casper.infradead.org>
On 29/05/2025 19:13, Matthew Wilcox wrote:
> On Thu, May 29, 2025 at 10:54:34AM -0700, Shakeel Butt wrote:
>> On Thu, May 29, 2025 at 04:28:46PM +0100, Matthew Wilcox wrote:
>>> People should put more effort into allocating THPs automatically and
>>> monitoring where they're helping performance and where they're hurting
>>> performance,
>>
>> Can you please expand on people putting more effort? Is it about
>> workloads or maybe malloc implementations (tcmalloc, jemalloc) being
>> more intelligent in managing their allocations/frees to keep more used
>> memory in hugepage aligned regions? And conveying to kernel which
>> regions they prefer hugepage backed and which they do not? Or something
>> else?
>
> We need infrastructure inside the kernel to monitor whether a task is
> making effective use of the THPs that it has, and if it's not then move
> those THPs over to where they will be better used.
>
I think this is the really difficult part.
If we have 2 workloads on the same server, For e.g. one is database where THPs
just dont do well, but the other one is AI where THPs do really well. How
will the kernel monitor that the database workload is performing worse
and the AI one isnt?
I added THP shrinker to hopefully try and do this automatically, and it does
really help. But unfortunately it is not a complete solution.
There are severely memory bound workloads where even a tiny increase
in memory will lead to an OOM. And if you colocate the container thats running
that workload with one in which we will benefit with THPs, we unfortunately
can't just rely on the system doing the right thing.
It would be awesome if THPs are truly transparent and don't require
any input, but unfortunately I don't think that there is a solution
for this with just kernel monitoring.
This is just a big hint from the user. If the global system policy is madvise
and the workload owner has done their own benchmarks and see benefits
with always, they set DEFAULT_MADV_HUGEPAGE for the process to optin as "always".
If the global system policy is always and the workload owner has done their own
benchmarks and see worse results with always, they set DEFAULT_MADV_NOHUGEPAGE for
the process to optin as "madvise".
> I don't necessarily object to userspace giving hints like "I think I'm
> going to use all of this 20MB region quite heavily", but the kernel should
> treat those hints with the appropriate skepticism, otherwise it's just
> a turbo button that nobody would ever _not_ press.
>
>>> instead of coming up with these baroque reasons to blame
>>> the sysadmin for not having tweaked some magic knob.
>>
>> To me this is not about blaming sysadmin but more about sysadmin wanting
>> more fine grained control on THP allocation policies for different
>> workloads running in a multi-tenant environment.
>
> That's the same thing. Linux should be auto-tuning, not relying on some
> omniscient sysadmin to fix it up.
^ permalink raw reply
* Re: [DISCUSSION] proposed mctl() API
From: Matthew Wilcox @ 2025-05-29 18:13 UTC (permalink / raw)
To: Shakeel Butt
Cc: Lorenzo Stoakes, Andrew Morton, Liam R . Howlett,
David Hildenbrand, Vlastimil Babka, Jann Horn, Arnd Bergmann,
Christian Brauner, SeongJae Park, Usama Arif, Mike Rapoport,
Johannes Weiner, Barry Song, linux-mm, linux-arch, linux-kernel,
linux-api, Pedro Falcato
In-Reply-To: <c7fqqny5yv7smhxnxe5o4rc2wepmc6uei76teymfhxoana34nk@sfqnc2qclf23>
On Thu, May 29, 2025 at 10:54:34AM -0700, Shakeel Butt wrote:
> On Thu, May 29, 2025 at 04:28:46PM +0100, Matthew Wilcox wrote:
> > People should put more effort into allocating THPs automatically and
> > monitoring where they're helping performance and where they're hurting
> > performance,
>
> Can you please expand on people putting more effort? Is it about
> workloads or maybe malloc implementations (tcmalloc, jemalloc) being
> more intelligent in managing their allocations/frees to keep more used
> memory in hugepage aligned regions? And conveying to kernel which
> regions they prefer hugepage backed and which they do not? Or something
> else?
We need infrastructure inside the kernel to monitor whether a task is
making effective use of the THPs that it has, and if it's not then move
those THPs over to where they will be better used.
I don't necessarily object to userspace giving hints like "I think I'm
going to use all of this 20MB region quite heavily", but the kernel should
treat those hints with the appropriate skepticism, otherwise it's just
a turbo button that nobody would ever _not_ press.
> > instead of coming up with these baroque reasons to blame
> > the sysadmin for not having tweaked some magic knob.
>
> To me this is not about blaming sysadmin but more about sysadmin wanting
> more fine grained control on THP allocation policies for different
> workloads running in a multi-tenant environment.
That's the same thing. Linux should be auto-tuning, not relying on some
omniscient sysadmin to fix it up.
^ permalink raw reply
* Re: [DISCUSSION] proposed mctl() API
From: Shakeel Butt @ 2025-05-29 17:54 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Lorenzo Stoakes, Andrew Morton, Liam R . Howlett,
David Hildenbrand, Vlastimil Babka, Jann Horn, Arnd Bergmann,
Christian Brauner, SeongJae Park, Usama Arif, Mike Rapoport,
Johannes Weiner, Barry Song, linux-mm, linux-arch, linux-kernel,
linux-api, Pedro Falcato
In-Reply-To: <aDh9LtSLCiTLjg2X@casper.infradead.org>
Hi Willy,
On Thu, May 29, 2025 at 04:28:46PM +0100, Matthew Wilcox wrote:
> On Thu, May 29, 2025 at 03:43:26PM +0100, Lorenzo Stoakes wrote:
> > After discussions in various threads (Usama's series adding a new prctl()
> > in [0], and a proposal to adapt process_madvise() to do the same -
> > conception in [1] and RFC in [2]), it seems fairly clear that it would make
> > sense to explore a dedicated API to explicitly allow for actions which
> > affect the virtual address space as a whole.
> >
> > Also, Barry is implementing a feature (currently under RFC) which could
> > additionally make use of this API (see [3]).
>
> I think the reason that you're having trouble coming up with a good
> place to put these ideas is because they are all bad ideas. Do none of
> them. Problem solved.
>
> People should put more effort into allocating THPs automatically and
> monitoring where they're helping performance and where they're hurting
> performance,
Can you please expand on people putting more effort? Is it about
workloads or maybe malloc implementations (tcmalloc, jemalloc) being
more intelligent in managing their allocations/frees to keep more used
memory in hugepage aligned regions? And conveying to kernel which
regions they prefer hugepage backed and which they do not? Or something
else?
> instead of coming up with these baroque reasons to blame
> the sysadmin for not having tweaked some magic knob.
To me this is not about blaming sysadmin but more about sysadmin wanting
more fine grained control on THP allocation policies for different
workloads running in a multi-tenant environment.
^ permalink raw reply
* Re: [DISCUSSION] proposed mctl() API
From: Usama Arif @ 2025-05-29 17:21 UTC (permalink / raw)
To: Lorenzo Stoakes, Andrew Morton, Shakeel Butt, Liam R . Howlett,
David Hildenbrand, Vlastimil Babka, Jann Horn, Arnd Bergmann,
Christian Brauner, SeongJae Park, Mike Rapoport, Johannes Weiner,
Barry Song, linux-mm, linux-arch, linux-kernel, linux-api,
Pedro Falcato
In-Reply-To: <85778a76-7dc8-4ea8-8827-acb45f74ee05@lucifer.local>
On 29/05/2025 15:43, Lorenzo Stoakes wrote:
> ## INTRODUCTION
>
> After discussions in various threads (Usama's series adding a new prctl()
> in [0], and a proposal to adapt process_madvise() to do the same -
> conception in [1] and RFC in [2]), it seems fairly clear that it would make
> sense to explore a dedicated API to explicitly allow for actions which
> affect the virtual address space as a whole.
>
> Also, Barry is implementing a feature (currently under RFC) which could
> additionally make use of this API (see [3]).
>
> [0]: https://lore.kernel.org/all/20250515133519.2779639-1-usamaarif642@gmail.com/
> [1]: https://lore.kernel.org/linux-mm/c390dd7e-0770-4d29-bb0e-f410ff6678e3@lucifer.local/
> [2]: https://lore.kernel.org/all/cover.1747686021.git.lorenzo.stoakes@oracle.com/
> [3]: https://lore.kernel.org/all/20250514070820.51793-1-21cnbao@gmail.com/
>
> While madvise() and process_madvise() are useful for altering the
> attributes of VMAs within a virtual address space, it isn't the right fit
> for something that affects the whole address space.
>
> Additionally, a requirement of Usama's proposal (see [0]) is that we have
> the ability to propagate the change in behaviour across fork/exec. This
> further suggests the need for a dedicated interface, as this really sits
> outside the ordinary behaviour of [process_]madvise().
>
> prctl() is too broad and encourages mm code to migrate to kernel/sys.c
> where it is at risk of bit-rotting. It can make it harder/impossible to
> isolate mm logic for testing and logic there might be missed in changes
> moving forward.
>
> It also, like so many kernel interfaces, has 'grown roots out of its pot'
> so to speak - while it started off as an ostensible 'process' manipulation
> interface, prctl() operations manipulate a myriad of task, virtual
> address space and even specific VMA attributes.
>
> At this stage it really is a 'catch-all' for things we simply couldn't fit
> elsewhere.
>
> Therefore, as suggested by the rather excellent Liam Howlett, I propose an
> mm-specific interface that _explicitly_ manipulates attributes of the
> virtual address space as a whole.
>
> I think something that mimics the simplicity of [process_]madvise() makes
> sense - have a limited set of actions that can be taken, and treat them as
> a simple action - a user requests you do XXX to the virtual address space
> (that is, across the mm_struct), and you do it.
>
Hi Lorenzo,
Thanks for writing the proposal, this is awesome!
Whatever the community agrees with, whether its this or prctl, happy to
move forward with either as both should accomplish the usecase proposed.
I will just add some points over here in defence of prctl, this is just for
discussion, and if the community disagrees, completely happy to move forward
with new syscall as well.
When it comes to having mm code in kernel/sys.c, we can just do something
like below that can actually clean it up?
diff --git a/kernel/sys.c b/kernel/sys.c
index 3a2df1bd9f64..bfadc339e2c5 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2467,6 +2467,12 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
error = 0;
switch (option) {
+ case PR_SET_MM:
+ case PR_GET_THP_DISABLE:
+ case PR_SET_THP_DISABLE:
+ case PR_NEW_MM_THING:
+ error = some_function_in_mm_folder(); // in mm/mctl.c ?
+ break;
case PR_SET_PDEATHSIG:
if (!valid_signal(arg2)) {
error = -EINVAL;
when it comes to prctl becoming a catch-all thing, with above clean up,
we can be a lot more careful to what gets added to the mm side of prctl.
The advantage of this is it avoids having another syscall.
My personal view (which can be wrong :)) is that a new syscall should be
for something major,
and I feel that PR_DEFAULT_MADV_HUGEPAGE and PR_DEFAULT_MADV_NOHUGEPAGE
might be small enough to fit in prctl? but I completely understand
your point of view as well!
> ## INTERFACE
>
> The proposed interface is simply:
>
> int mctl(int pidfd, int action, unsigned int flags);
>
> Since PIDFD_SELF is now available, it is easy to invoke this for the
> current process, while also adding the flexibility of being able to apply
> actions to other processes also.
>
> The function will return 0 on success, -1 on failure, with errno set to the
> error that arose, standard stuff.
>
> The behaviour will be tailored to each action taken.
>
> To begin with, I propose a single flag:
>
> - MCTL_SET_DEFAULT_EXEC - Persists this behaviour across fork/exec.
>
> This again will be tailored - only certain actions will be allowed to set
> this flag, and we will of course assert appropriate capabilities, etc. upon
> its use.
>
Sounds good to me. Just adding this here, the solution will be used in systemd
in exec_invoke, similar to how KSM is done with prctl in [1], so for the THP
solution, we would need MCTL_SET_DEFAULT_EXEC as it would need to be inherited
across fork+exec.
[1] https://github.com/systemd/systemd/blob/2e72d3efafa88c1cb4d9b28dd4ade7c6ab7be29a/src/core/exec-invoke.c#L5046
> All actions would, impact every VMA (if adjustments to VMAs are required).
>
> ## SECURITY
>
> Of course, security will be of utmost concern (Jann's input is important
> here :)
>
> We can vary security requirements depending on the action taken.
>
> For an initial version I suggest we simply limit operations which:
>
> - Operate on a remote process
> - Use the MCTL_SET_DEFAULT_EXEC flag
>
> To those tasks which possess the CAP_SYS_ADMIN capability.
>
> This may be too restrictive - be good to get some feedback on this.
>
> I know Jann raised concerns around privileged execution and perhaps it'd be
> useful to see whether this would make more sense for the SET_DEFAULT_EXEC
> case or not.
>
> Usama - would requiring CAP_SYS_ADMIN be egregious to your use case?
>
My knowledge is security is limited, so please bare with me, but I actually
didn't understand the security issue and the need for CAP_SYS_ADMIN for
doing VM_(NO)HUGEPAGE.
A process can already madvise its own VMAs, and this is just doing that
for the entire process. And VM_INIT_DEF_MASK is already set to VM_NOHUGEPAGE
so it will be inherited by the parent. Just adding VM_HUGEPAGE shouldnt be
a issue? Inheriting MMF_VM_HUGEPAGE will mean that khugepaged would enter
for that process as well, which again doesnt seem like a security issue
to me.
> ## IMPLEMENTATION
>
> I think that sensibly we'd need to add some new files here, mm/mctl.c,
> include/linux/mctl.h (the latter of providing the MCTL_xxx actions and
> flags).
>
> We could find ways to share code between mm files where appropriate to
> avoid too much duplication.
>
> I suggest that the best way forward, if we were minded to examine how this
> would look in practice, would be for me to implement an RFC that adds the
> interface, and a simple MCTL_SET_NOHUGEPAGE, MCTL_CLEAR_NOHUGEPAGE
> implementation as a proof of concept.
>
> If we wanted to then go ahead with a non-RFC version, this could then form
> a foundation upon which Usama and Barry could implement their features,
> with Usama then able to add MCTL_[SET/CLEAR]_HUGEPAGE and Barry
> MCTL_[SET/CLEAR]_FADE_ON_DEATH.
>
> Obviously I don't mean to presume to suggest how we might proceed here -
> only suggesting this might be a good way of moving forward and getting
> things done as quickly as possible while allowing you guys to move forward
> with your features.
>
> Let me know if this makes sense, alternatively I could try to find a
> relatively benign action to implement as part of the base work, or we could
> simply collaborate to do it all in one series with multiple authors?
>
> ## RFC
>
> The above is all only in effect 'putting ideas out there' so this is
> entirely an RFC in spirit and intent - let me know if this makes sense in
> whole or part :)
>
> Thanks!
>
> Lorenzo
Again thanks for the proposal! Happy to move forward with this or prctl.
Just adding my 2 cents in this email.
Thanks
Usama
^ permalink raw reply related
* Re: [DISCUSSION] proposed mctl() API
From: Matthew Wilcox @ 2025-05-29 15:28 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Andrew Morton, Shakeel Butt, Liam R . Howlett, David Hildenbrand,
Vlastimil Babka, Jann Horn, Arnd Bergmann, Christian Brauner,
SeongJae Park, Usama Arif, Mike Rapoport, Johannes Weiner,
Barry Song, linux-mm, linux-arch, linux-kernel, linux-api,
Pedro Falcato
In-Reply-To: <85778a76-7dc8-4ea8-8827-acb45f74ee05@lucifer.local>
On Thu, May 29, 2025 at 03:43:26PM +0100, Lorenzo Stoakes wrote:
> After discussions in various threads (Usama's series adding a new prctl()
> in [0], and a proposal to adapt process_madvise() to do the same -
> conception in [1] and RFC in [2]), it seems fairly clear that it would make
> sense to explore a dedicated API to explicitly allow for actions which
> affect the virtual address space as a whole.
>
> Also, Barry is implementing a feature (currently under RFC) which could
> additionally make use of this API (see [3]).
I think the reason that you're having trouble coming up with a good
place to put these ideas is because they are all bad ideas. Do none of
them. Problem solved.
People should put more effort into allocating THPs automatically and
monitoring where they're helping performance and where they're hurting
performance, instead of coming up with these baroque reasons to blame
the sysadmin for not having tweaked some magic knob.
Barry's problem is that we're all nervous about possibly regressing
performance on some unknown workloads. Just try Barry's proposal, see
if anyone actually compains or if we're just afraid of our own shadows.
^ permalink raw reply
* [DISCUSSION] proposed mctl() API
From: Lorenzo Stoakes @ 2025-05-29 14:43 UTC (permalink / raw)
To: Andrew Morton, Shakeel Butt, Liam R . Howlett, David Hildenbrand,
Vlastimil Babka, Jann Horn, Arnd Bergmann, Christian Brauner,
SeongJae Park, Usama Arif, Mike Rapoport, Johannes Weiner,
Barry Song, linux-mm, linux-arch, linux-kernel, linux-api,
Pedro Falcato
## INTRODUCTION
After discussions in various threads (Usama's series adding a new prctl()
in [0], and a proposal to adapt process_madvise() to do the same -
conception in [1] and RFC in [2]), it seems fairly clear that it would make
sense to explore a dedicated API to explicitly allow for actions which
affect the virtual address space as a whole.
Also, Barry is implementing a feature (currently under RFC) which could
additionally make use of this API (see [3]).
[0]: https://lore.kernel.org/all/20250515133519.2779639-1-usamaarif642@gmail.com/
[1]: https://lore.kernel.org/linux-mm/c390dd7e-0770-4d29-bb0e-f410ff6678e3@lucifer.local/
[2]: https://lore.kernel.org/all/cover.1747686021.git.lorenzo.stoakes@oracle.com/
[3]: https://lore.kernel.org/all/20250514070820.51793-1-21cnbao@gmail.com/
While madvise() and process_madvise() are useful for altering the
attributes of VMAs within a virtual address space, it isn't the right fit
for something that affects the whole address space.
Additionally, a requirement of Usama's proposal (see [0]) is that we have
the ability to propagate the change in behaviour across fork/exec. This
further suggests the need for a dedicated interface, as this really sits
outside the ordinary behaviour of [process_]madvise().
prctl() is too broad and encourages mm code to migrate to kernel/sys.c
where it is at risk of bit-rotting. It can make it harder/impossible to
isolate mm logic for testing and logic there might be missed in changes
moving forward.
It also, like so many kernel interfaces, has 'grown roots out of its pot'
so to speak - while it started off as an ostensible 'process' manipulation
interface, prctl() operations manipulate a myriad of task, virtual
address space and even specific VMA attributes.
At this stage it really is a 'catch-all' for things we simply couldn't fit
elsewhere.
Therefore, as suggested by the rather excellent Liam Howlett, I propose an
mm-specific interface that _explicitly_ manipulates attributes of the
virtual address space as a whole.
I think something that mimics the simplicity of [process_]madvise() makes
sense - have a limited set of actions that can be taken, and treat them as
a simple action - a user requests you do XXX to the virtual address space
(that is, across the mm_struct), and you do it.
## INTERFACE
The proposed interface is simply:
int mctl(int pidfd, int action, unsigned int flags);
Since PIDFD_SELF is now available, it is easy to invoke this for the
current process, while also adding the flexibility of being able to apply
actions to other processes also.
The function will return 0 on success, -1 on failure, with errno set to the
error that arose, standard stuff.
The behaviour will be tailored to each action taken.
To begin with, I propose a single flag:
- MCTL_SET_DEFAULT_EXEC - Persists this behaviour across fork/exec.
This again will be tailored - only certain actions will be allowed to set
this flag, and we will of course assert appropriate capabilities, etc. upon
its use.
All actions would, impact every VMA (if adjustments to VMAs are required).
## SECURITY
Of course, security will be of utmost concern (Jann's input is important
here :)
We can vary security requirements depending on the action taken.
For an initial version I suggest we simply limit operations which:
- Operate on a remote process
- Use the MCTL_SET_DEFAULT_EXEC flag
To those tasks which possess the CAP_SYS_ADMIN capability.
This may be too restrictive - be good to get some feedback on this.
I know Jann raised concerns around privileged execution and perhaps it'd be
useful to see whether this would make more sense for the SET_DEFAULT_EXEC
case or not.
Usama - would requiring CAP_SYS_ADMIN be egregious to your use case?
## IMPLEMENTATION
I think that sensibly we'd need to add some new files here, mm/mctl.c,
include/linux/mctl.h (the latter of providing the MCTL_xxx actions and
flags).
We could find ways to share code between mm files where appropriate to
avoid too much duplication.
I suggest that the best way forward, if we were minded to examine how this
would look in practice, would be for me to implement an RFC that adds the
interface, and a simple MCTL_SET_NOHUGEPAGE, MCTL_CLEAR_NOHUGEPAGE
implementation as a proof of concept.
If we wanted to then go ahead with a non-RFC version, this could then form
a foundation upon which Usama and Barry could implement their features,
with Usama then able to add MCTL_[SET/CLEAR]_HUGEPAGE and Barry
MCTL_[SET/CLEAR]_FADE_ON_DEATH.
Obviously I don't mean to presume to suggest how we might proceed here -
only suggesting this might be a good way of moving forward and getting
things done as quickly as possible while allowing you guys to move forward
with your features.
Let me know if this makes sense, alternatively I could try to find a
relatively benign action to implement as part of the base work, or we could
simply collaborate to do it all in one series with multiple authors?
## RFC
The above is all only in effect 'putting ideas out there' so this is
entirely an RFC in spirit and intent - let me know if this makes sense in
whole or part :)
Thanks!
Lorenzo
^ permalink raw reply
* Re: [RFC v2 00/16] Live Update Orchestrator
From: Mike Rapoport @ 2025-05-26 6:32 UTC (permalink / raw)
To: Pasha Tatashin
Cc: pratyush, jasonmiu, graf, changyuanl, dmatlack, rientjes, corbet,
rdunlap, ilpo.jarvinen, kanie, ojeda, aliceryhl, masahiroy, akpm,
tj, yoann.congal, mmaurer, roman.gushchin, chenridong, axboe,
mark.rutland, jannh, vincent.guittot, hannes, dan.j.williams,
david, joel.granados, rostedt, anna.schumaker, song, zhangguopeng,
linux, linux-kernel, linux-doc, linux-mm, gregkh, tglx, mingo, bp,
dave.hansen, x86, hpa, rafael, dakr, bartosz.golaszewski,
cw00.choi, myungjoo.ham, yesanishhere, Jonathan.Cameron,
quic_zijuhu, aleksander.lobakin, ira.weiny, andriy.shevchenko,
leon, lukas, bhelgaas, wagi, djeffery, stuart.w.hayes, ptyadav,
linux-api
In-Reply-To: <20250515182322.117840-1-pasha.tatashin@soleen.com>
(cc'ing linux-api)
On Thu, May 15, 2025 at 06:23:04PM +0000, Pasha Tatashin wrote:
> This v2 series introduces the LUO, a kernel subsystem designed to
> facilitate live kernel updates with minimal downtime,
> particularly in cloud delplyoments aiming to update without fully
> disrupting running virtual machines.
>
> This series builds upon KHO framework [1] by adding programmatic
> control over KHO's lifecycle and leveraging KHO for persisting LUO's
> own metadata across the kexec boundary. The git branch for this series
> can be found at:
> https://github.com/googleprodkernel/linux-liveupdate/tree/luo/rfc-v2
>
> Changelog from v1:
> - Control Interface: Shifted from sysfs-based control
> (/sys/kernel/liveupdate/{prepare,finish}) to an ioctl interface
> (/dev/liveupdate). Sysfs is now primarily for monitoring the state.
> - Event/State Renaming: LIVEUPDATE_REBOOT event/phase is now
> LIVEUPDATE_FREEZE.
> - FD Preservation: A new component for preserving file descriptors.
> Subsystem Registration: A formal mechanism for kernel subsystems
> to participate.
> - Device Layer: removed device list handling from this series, it is
> going to be added separately.
> - Selftests: Kernel-side selftest hooks and userspace selftests are
> now included.
> KHO Enhancements:
> - KHO debugfs became optional, and kernel APIs for finalize/abort
> were added (driven by LUO's needs).
> - KHO unpreserve functions were also added.
>
> What is Live Update?
> Live Update is a specialized reboot process where selected kernel
> resources (memory, file descriptors, and eventually devices) are kept
> operational or their state preserved across a kernel transition (e.g.,
> via kexec). For certain resources, DMA and interrupt activity might
> continue with minimal interruption during the kernel reboot.
>
> LUO v2 Overview:
> LUO v2 provides a framework for coordinating live updates. It features:
> State Machine: Manages the live update process through states:
> NORMAL, PREPARED, FROZEN, UPDATED.
>
> KHO Integration:
>
> LUO programmatically drives KHO's finalization and abort sequences.
> KHO's debugfs interface is now optional configured via
> CONFIG_KEXEC_HANDOVER_DEBUG.
>
> LUO preserves its own metadata via KHO's kho_add_subtree and
> kho_preserve_phys() mechanisms.
>
> Subsystem Participation: A callback API liveupdate_register_subsystem()
> allows kernel subsystems (e.g., KVM, IOMMU, VFIO, PCI) to register
> handlers for LUO events (PREPARE, FREEZE, FINISH, CANCEL) and persist a
> u64 payload via the LUO FDT.
>
> File Descriptor Preservation: Infrastructure
> liveupdate_register_filesystem, luo_register_file, luo_retrieve_file to
> allow specific types of file descriptors (e.g., memfd, vfio) to be
> preserved and restored.
>
> Handlers for specific file types can be registered to manage their
> preservation and restoration, storing a u64 payload in the LUO FDT.
>
> Example WIP for memfd preservation can be found here [2].
>
> User-space Interface:
>
> ioctl (/dev/liveupdate): The primary control interface for
> triggering LUO state transitions (prepare, freeze, finish, cancel)
> and managing the preservation/restoration of file descriptors.
> Access requires CAP_SYS_ADMIN.
>
> sysfs (/sys/kernel/liveupdate/state): A read-only interface for
> monitoring the current LUO state. This allows userspace services to
> track progress and coordinate actions.
>
> Selftests: Includes kernel-side hooks and userspace selftests to
> verify core LUO functionality, particularly subsystem registration and
> basic state transitions.
>
> LUO State Machine and Events:
>
> NORMAL: Default operational state.
> PREPARED: Initial preparation complete after LIVEUPDATE_PREPARE
> event. Subsystems have saved initial state.
> FROZEN: Final "blackout window" state after LIVEUPDATE_FREEZE
> event, just before kexec. Workloads must be suspended.
> UPDATED: Next kernel has booted via live update. Awaiting restoration
> and LIVEUPDATE_FINISH.
>
> Events:
> LIVEUPDATE_PREPARE: Prepare for reboot, serialize state.
> LIVEUPDATE_FREEZE: Final opportunity to save state before kexec.
> LIVEUPDATE_FINISH: Post-reboot cleanup in the next kernel.
> LIVEUPDATE_CANCEL: Abort prepare or freeze, revert changes.
>
> [1] https://lore.kernel.org/all/20250509074635.3187114-1-changyuanl@google.com
> https://github.com/googleprodkernel/linux-liveupdate/tree/luo/kho-v8
> [2] https://github.com/googleprodkernel/linux-liveupdate/tree/luo/memfd-v0.1
>
> RFC v1: https://lore.kernel.org/all/20250320024011.2995837-1-pasha.tatashin@soleen.com
>
> Changyuan Lyu (1):
> kho: add kho_unpreserve_folio/phys
>
> Pasha Tatashin (15):
> kho: make debugfs interface optional
> kho: allow to drive kho from within kernel
> luo: luo_core: Live Update Orchestrator
> luo: luo_core: integrate with KHO
> luo: luo_subsystems: add subsystem registration
> luo: luo_subsystems: implement subsystem callbacks
> luo: luo_files: add infrastructure for FDs
> luo: luo_files: implement file systems callbacks
> luo: luo_ioctl: add ioctl interface
> luo: luo_sysfs: add sysfs state monitoring
> reboot: call liveupdate_reboot() before kexec
> luo: add selftests for subsystems un/registration
> selftests/liveupdate: add subsystem/state tests
> docs: add luo documentation
> MAINTAINERS: add liveupdate entry
>
> .../ABI/testing/sysfs-kernel-liveupdate | 51 ++
> Documentation/admin-guide/index.rst | 1 +
> Documentation/admin-guide/liveupdate.rst | 62 ++
> .../userspace-api/ioctl/ioctl-number.rst | 1 +
> MAINTAINERS | 14 +-
> drivers/misc/Kconfig | 1 +
> drivers/misc/Makefile | 1 +
> drivers/misc/liveupdate/Kconfig | 60 ++
> drivers/misc/liveupdate/Makefile | 7 +
> drivers/misc/liveupdate/luo_core.c | 547 +++++++++++++++
> drivers/misc/liveupdate/luo_files.c | 664 ++++++++++++++++++
> drivers/misc/liveupdate/luo_internal.h | 59 ++
> drivers/misc/liveupdate/luo_ioctl.c | 203 ++++++
> drivers/misc/liveupdate/luo_selftests.c | 283 ++++++++
> drivers/misc/liveupdate/luo_selftests.h | 23 +
> drivers/misc/liveupdate/luo_subsystems.c | 413 +++++++++++
> drivers/misc/liveupdate/luo_sysfs.c | 92 +++
> include/linux/kexec_handover.h | 27 +
> include/linux/liveupdate.h | 214 ++++++
> include/uapi/linux/liveupdate.h | 324 +++++++++
> kernel/Kconfig.kexec | 10 +
> kernel/Makefile | 1 +
> kernel/kexec_handover.c | 343 +++------
> kernel/kexec_handover_debug.c | 237 +++++++
> kernel/kexec_handover_internal.h | 74 ++
> kernel/reboot.c | 4 +
> tools/testing/selftests/Makefile | 1 +
> tools/testing/selftests/liveupdate/.gitignore | 1 +
> tools/testing/selftests/liveupdate/Makefile | 7 +
> tools/testing/selftests/liveupdate/config | 6 +
> .../testing/selftests/liveupdate/liveupdate.c | 440 ++++++++++++
> 31 files changed, 3933 insertions(+), 238 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-kernel-liveupdate
> create mode 100644 Documentation/admin-guide/liveupdate.rst
> create mode 100644 drivers/misc/liveupdate/Kconfig
> create mode 100644 drivers/misc/liveupdate/Makefile
> create mode 100644 drivers/misc/liveupdate/luo_core.c
> create mode 100644 drivers/misc/liveupdate/luo_files.c
> create mode 100644 drivers/misc/liveupdate/luo_internal.h
> create mode 100644 drivers/misc/liveupdate/luo_ioctl.c
> create mode 100644 drivers/misc/liveupdate/luo_selftests.c
> create mode 100644 drivers/misc/liveupdate/luo_selftests.h
> create mode 100644 drivers/misc/liveupdate/luo_subsystems.c
> create mode 100644 drivers/misc/liveupdate/luo_sysfs.c
> create mode 100644 include/linux/liveupdate.h
> create mode 100644 include/uapi/linux/liveupdate.h
> create mode 100644 kernel/kexec_handover_debug.c
> create mode 100644 kernel/kexec_handover_internal.h
> create mode 100644 tools/testing/selftests/liveupdate/.gitignore
> create mode 100644 tools/testing/selftests/liveupdate/Makefile
> create mode 100644 tools/testing/selftests/liveupdate/config
> create mode 100644 tools/testing/selftests/liveupdate/liveupdate.c
>
> --
> 2.49.0.1101.gccaa498523-goog
>
--
Sincerely yours,
Mike.
^ permalink raw reply
* Re: [RFC v1] man/man2/close.2: CAVEATS: Document divergence from POSIX.1-2024
From: Florian Weimer @ 2025-05-24 19:25 UTC (permalink / raw)
To: Zack Weinberg
Cc: Alejandro Colomar, Rich Felker, Vincent Lefevre, Jan Kara,
Alexander Viro, Christian Brauner, linux-fsdevel, linux-api,
GNU libc development
In-Reply-To: <8c47e10a-be82-4d5b-a45e-2526f6e95123@app.fastmail.com>
* Zack Weinberg:
> BUGS
> Prior to POSIX.1-2024, there was no official guarantee that
> close() would always close the file descriptor, even on error.
> Linux has always closed the file descriptor, even on error,
> but other implementations might not have.
>
> The only such implementation we have heard of is HP-UX; at least
> some versions of HP-UX’s man page for close() said it should be
> retried if it returned -1 with errno set to EINTR. (If you know
> exactly which versions of HP-UX are affected, or of any other
> Unix where close() doesn’t always close the file descriptor,
> please contact us about it.)
The AIX documentation also says this:
| The success of the close subroutine is undetermined if the following
| is true:
|
| EINTR The state of the FileDescriptor is undetermined. Retry the
| close routine to ensure that the FileDescriptor is closed.
<https://www.ibm.com/docs/en/aix/7.2.0?topic=c-close-subroutine>
So it's not just HP-UX.
For z/OS, it looks like some other errors leave the descriptor open:
| EAGAIN
|
| The call did not complete because the specified socket descriptor
| is currently being used by another thread in the same process.
|
| For example, in a multithreaded environment, close() fails and
| returns EAGAIN when the following sequence of events occurs (1)
| thread is blocked in a read() or select() call on a given file or
| socket descriptor and (2) another thread issues a simultaneous
| close() call for the same descriptor.
| […]
| EBADF
| fildes is not a valid open file descriptor, or the socket
| parameter is not a valid socket descriptor.
<https://www.ibm.com/docs/en/zos/2.1.0?topic=functions-close-close-file>
^ permalink raw reply
* Re: [RFC v1] man/man2/close.2: CAVEATS: Document divergence from POSIX.1-2024
From: Rich Felker @ 2025-05-24 2:24 UTC (permalink / raw)
To: Zack Weinberg
Cc: Alejandro Colomar, Vincent Lefevre, Jan Kara, Alexander Viro,
Christian Brauner, linux-fsdevel, linux-api, GNU libc development
In-Reply-To: <8c47e10a-be82-4d5b-a45e-2526f6e95123@app.fastmail.com>
On Fri, May 23, 2025 at 02:10:57PM -0400, Zack Weinberg wrote:
> Taking everything said in this thread into account, I have attempted to
> wordsmith new language for the close(2) manpage. Please let me know
> what you think, and please help me with the bits marked in square
> brackets. I can make this into a proper patch for the manpages
> when everyone is happy with it.
>
> zw
>
> ---
>
> DESCRIPTION
> ... existing text ...
>
> close() always succeeds. That is, after it returns, _fd_ has
> always been disconnected from the open file it formerly referred
> to, and its number can be recycled to refer to some other file.
> Furthermore, if _fd_ was the last reference to the underlying
> open file description, the resources associated with the open file
> description will always have been scheduled to be released.
>
> However, close may report _delayed errors_ from a previous I/O
> operation. Therefore, its return value should not be ignored.
>
> RETURN VALUE
> close() returns zero if there are no delayed errors to report,
> or -1 if there _might_ be delayed errors.
>
> When close() returns -1, check _errno_ to see what the situation
> actually is. Most, but not all, _errno_ codes indicate a delayed
> I/O error that should be reported to the user. See ERRORS and
> NOTES for more detail.
>
> [QUERY: Is it ever possible to get delayed errors on close() from
> a file that was opened with O_RDONLY? What about a file that was
> opened with O_RDWR but never actually written to? If people only
> have to worry about delayed errors if the file was actually
> written to, we should say so at this point.
>
> It would also be good to mention whether it is possible to get a
> delayed error on close() even if a previous call to fsync() or
> fdatasync() succeeded and there haven’t been any more writes to
> that file *description* (not necessarily via the fd being closed)
> since.]
>
> ERRORS
> EBADF _fd_ wasn’t open in the first place, or is outside the
> valid numeric range for file descriptors.
>
> EINPROGRESS
> EINTR
> There are no delayed errors to report, but the kernel is
> still doing some clean-up work in the background. This
> situation should be treated the same as if close() had
> returned zero. Do not retry the close(), and do not report
> an error to the user.
Since this behavior for EINTR is non-conforming (and even prior to the
POSIX 2024 update, it was contrary to the general semantics for EINTR,
that no non-ignoreable side-effects have taken place), it should be
noted that it's Linux/glibc-specific.
> EDQUOT
> EFBIG
> EIO
> ENOSPC
> These are the most common errno codes associated with
> delayed I/O errors. They should be treated as a hard
> failure to write to the file that was formerly associated
> with _fd_, the same as if an earlier write(2) had failed
> with one of these codes. The file has still been closed!
> Do not retry the close(). But do report an error to the user.
>
> Depending on the underlying file, close() may return other errno
> codes; these should generally also be treated as delayed I/O errors.
>
> NOTES
> Dealing with error returns from close()
>
> As discussed above, close() always closes the file. Except when
> errno is set to EBADF, EINPROGRESS, or EINTR, an error return from
> close() reports a _delayed I/O error_ from a previous write()
> operation.
>
> It is vital to report delayed I/O errors to the user; failing to
> check the return value of close() can cause _silent_ loss of data.
> The most common situations where this actually happens involve
> networked filesystems, where, in the name of throughput, write()
> often returns success before the server has actually confirmed a
> successful write.
>
> However, it is also vital to understand that _no matter what_
> close() returns, and _no matter what_ it sets errno to, when it
> returns, _the file descriptor passed to close() has been closed_,
> and its number is _immediately_ available for reuse by open(2),
> dup(2), etc. Therefore, one should never retry a close(), not
> even if it set errno to a value that normally indicates the
> operation needs to be retried (e.g. EINTR). Retrying a close()
> is a serious bug, particularly in a multithreaded program; if
> the file descriptor number has already been reused, _that file_
> will get closed out from under whatever other thread opened it.
>
> [Possibly something about fsync/fdatasync here?]
While I agree with all of this, I think the tone is way too
proscriptive. The man pages are to document the behaviors, not tell
people how to program. And again, it should be noted that the standard
behavior is that you *do* have to retry on EINTR, or arrange to ensure
it never happens (e.g. by not installing interrupting signal handlers,
or blocking signals across calls to close), and that treating EINTR as
"fd has been closed" is something you should only do on
known-nonconforming systems.
Aside: the reason EINTR *has to* be specified this way is that pthread
cancellation is aligned with EINTR. If EINTR were defined to have
closed the fd, then acting on cancellation during close would also
have closed the fd, but the cancellation handler would have no way to
distinguish this, leading to a situation where you're forced to either
leak fds or introduce a double-close vuln.
> BUGS
> Prior to POSIX.1-2024, there was no official guarantee that
> close() would always close the file descriptor, even on error.
> Linux has always closed the file descriptor, even on error,
> but other implementations might not have.
>
> The only such implementation we have heard of is HP-UX; at least
> some versions of HP-UX’s man page for close() said it should be
> retried if it returned -1 with errno set to EINTR. (If you know
> exactly which versions of HP-UX are affected, or of any other
> Unix where close() doesn’t always close the file descriptor,
> please contact us about it.)
>
> Portable code should nonetheless never retry a failed close(); the
> consequences of a file descriptor leak are far less dangerous than
> the consequences of closing a file out from under another thread.
This is explicitly the opposite of what's specified for portable code.
It sounds like you are intentionally omitting that POSIX says the
opposite of what you want it to, and treating the standard behavior as
a historical HP-UX quirk/bug. This is polemic, not the sort of
documentation that belongs in a man page.
An outline of what I'd like to see instead:
- Clear explanation of why double-close is a serious bug that must
always be avoided. (I think we all agree on this.)
- Statement that the historical Linux/glibc behavior and current POSIX
requirement differ, without language that tries to paint the POSIX
behavior as a HP-UX bug/quirk. Possibly citing real sources/history
of the issue (Austin Group tracker items 529, 614; maybe others).
- Consequence of just assuming the Linux behavior (fd leaks on
conforming systems).
- Consequences of assuming the POSIX behavior (double-close vulns on
GNU/Linux, maybe others).
- Survey of methods for avoiding the problem (ways to preclude EINTR,
possibly ways to infer behavior, etc).
Rich
^ permalink raw reply
* Re: [RFC v1] man/man2/close.2: CAVEATS: Document divergence from POSIX.1-2024
From: Zack Weinberg @ 2025-05-23 18:10 UTC (permalink / raw)
To: Alejandro Colomar, Rich Felker
Cc: Vincent Lefevre, Jan Kara, Alexander Viro, Christian Brauner,
linux-fsdevel, linux-api, GNU libc development
In-Reply-To: <5jm7pblkwkhh4frqjptrw4ll4nwncn22ep2v7sli6kz5wxg5ik@pbnj6wfv66af>
Taking everything said in this thread into account, I have attempted to
wordsmith new language for the close(2) manpage. Please let me know
what you think, and please help me with the bits marked in square
brackets. I can make this into a proper patch for the manpages
when everyone is happy with it.
zw
---
DESCRIPTION
... existing text ...
close() always succeeds. That is, after it returns, _fd_ has
always been disconnected from the open file it formerly referred
to, and its number can be recycled to refer to some other file.
Furthermore, if _fd_ was the last reference to the underlying
open file description, the resources associated with the open file
description will always have been scheduled to be released.
However, close may report _delayed errors_ from a previous I/O
operation. Therefore, its return value should not be ignored.
RETURN VALUE
close() returns zero if there are no delayed errors to report,
or -1 if there _might_ be delayed errors.
When close() returns -1, check _errno_ to see what the situation
actually is. Most, but not all, _errno_ codes indicate a delayed
I/O error that should be reported to the user. See ERRORS and
NOTES for more detail.
[QUERY: Is it ever possible to get delayed errors on close() from
a file that was opened with O_RDONLY? What about a file that was
opened with O_RDWR but never actually written to? If people only
have to worry about delayed errors if the file was actually
written to, we should say so at this point.
It would also be good to mention whether it is possible to get a
delayed error on close() even if a previous call to fsync() or
fdatasync() succeeded and there haven’t been any more writes to
that file *description* (not necessarily via the fd being closed)
since.]
ERRORS
EBADF _fd_ wasn’t open in the first place, or is outside the
valid numeric range for file descriptors.
EINPROGRESS
EINTR
There are no delayed errors to report, but the kernel is
still doing some clean-up work in the background. This
situation should be treated the same as if close() had
returned zero. Do not retry the close(), and do not report
an error to the user.
EDQUOT
EFBIG
EIO
ENOSPC
These are the most common errno codes associated with
delayed I/O errors. They should be treated as a hard
failure to write to the file that was formerly associated
with _fd_, the same as if an earlier write(2) had failed
with one of these codes. The file has still been closed!
Do not retry the close(). But do report an error to the user.
Depending on the underlying file, close() may return other errno
codes; these should generally also be treated as delayed I/O errors.
NOTES
Dealing with error returns from close()
As discussed above, close() always closes the file. Except when
errno is set to EBADF, EINPROGRESS, or EINTR, an error return from
close() reports a _delayed I/O error_ from a previous write()
operation.
It is vital to report delayed I/O errors to the user; failing to
check the return value of close() can cause _silent_ loss of data.
The most common situations where this actually happens involve
networked filesystems, where, in the name of throughput, write()
often returns success before the server has actually confirmed a
successful write.
However, it is also vital to understand that _no matter what_
close() returns, and _no matter what_ it sets errno to, when it
returns, _the file descriptor passed to close() has been closed_,
and its number is _immediately_ available for reuse by open(2),
dup(2), etc. Therefore, one should never retry a close(), not
even if it set errno to a value that normally indicates the
operation needs to be retried (e.g. EINTR). Retrying a close()
is a serious bug, particularly in a multithreaded program; if
the file descriptor number has already been reused, _that file_
will get closed out from under whatever other thread opened it.
[Possibly something about fsync/fdatasync here?]
BUGS
Prior to POSIX.1-2024, there was no official guarantee that
close() would always close the file descriptor, even on error.
Linux has always closed the file descriptor, even on error,
but other implementations might not have.
The only such implementation we have heard of is HP-UX; at least
some versions of HP-UX’s man page for close() said it should be
retried if it returned -1 with errno set to EINTR. (If you know
exactly which versions of HP-UX are affected, or of any other
Unix where close() doesn’t always close the file descriptor,
please contact us about it.)
Portable code should nonetheless never retry a failed close(); the
consequences of a file descriptor leak are far less dangerous than
the consequences of closing a file out from under another thread.
^ permalink raw reply
* Re: [PATCH v5 3/7] selinux: implement inode_file_[g|s]etattr hooks
From: Paul Moore @ 2025-05-22 22:26 UTC (permalink / raw)
To: Andrey Albershteyn, Richard Henderson, Matt Turner, Russell King,
Catalin Marinas, Will Deacon, Geert Uytterhoeven, Michal Simek,
Thomas Bogendoerfer, James E.J. Bottomley, Helge Deller,
Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Christophe Leroy, Naveen N Rao, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Yoshinori Sato, Rich Felker, John Paul Adrian Glaubitz,
David S. Miller, Andreas Larsson, Andy Lutomirski,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Chris Zankel, Max Filippov, Alexander Viro,
Christian Brauner, Jan Kara, Mickaël Salaün,
Günther Noack, Arnd Bergmann, Pali Rohár, James Morris,
Serge E. Hallyn, Stephen Smalley, Ondrej Mosnacek, Tyler Hicks,
Miklos Szeredi, Amir Goldstein
Cc: linux-alpha, linux-kernel, linux-arm-kernel, linux-m68k,
linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, linux-fsdevel, linux-security-module, linux-api,
linux-arch, selinux, ecryptfs, linux-unionfs, linux-xfs,
Andrey Albershteyn
In-Reply-To: <20250513-xattrat-syscall-v5-3-22bb9c6c767f@kernel.org>
On May 13, 2025 Andrey Albershteyn <aalbersh@redhat.com> wrote:
>
> These hooks are called on inode extended attribute retrieval/change.
>
> Cc: selinux@vger.kernel.org
> Cc: Paul Moore <paul@paul-moore.com>
>
> Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
> ---
> security/selinux/hooks.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
Acked-by: Paul Moore <paul@paul-moore.com>
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v5 2/7] lsm: introduce new hooks for setting/getting inode fsxattr
From: Paul Moore @ 2025-05-22 22:26 UTC (permalink / raw)
To: Andrey Albershteyn, Richard Henderson, Matt Turner, Russell King,
Catalin Marinas, Will Deacon, Geert Uytterhoeven, Michal Simek,
Thomas Bogendoerfer, James E.J. Bottomley, Helge Deller,
Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Christophe Leroy, Naveen N Rao, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Yoshinori Sato, Rich Felker, John Paul Adrian Glaubitz,
David S. Miller, Andreas Larsson, Andy Lutomirski,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Chris Zankel, Max Filippov, Alexander Viro,
Christian Brauner, Jan Kara, Mickaël Salaün,
Günther Noack, Arnd Bergmann, Pali Rohár, James Morris,
Serge E. Hallyn, Stephen Smalley, Ondrej Mosnacek, Tyler Hicks,
Miklos Szeredi, Amir Goldstein
Cc: linux-alpha, linux-kernel, linux-arm-kernel, linux-m68k,
linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, linux-fsdevel, linux-security-module, linux-api,
linux-arch, selinux, ecryptfs, linux-unionfs, linux-xfs,
Andrey Albershteyn
In-Reply-To: <20250513-xattrat-syscall-v5-2-22bb9c6c767f@kernel.org>
On May 13, 2025 Andrey Albershteyn <aalbersh@redhat.com> wrote:
>
> Introduce new hooks for setting and getting filesystem extended
> attributes on inode (FS_IOC_FSGETXATTR).
>
> Cc: selinux@vger.kernel.org
> Cc: Paul Moore <paul@paul-moore.com>
>
> Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
> ---
> fs/file_attr.c | 19 ++++++++++++++++---
> include/linux/lsm_hook_defs.h | 2 ++
> include/linux/security.h | 16 ++++++++++++++++
> security/security.c | 30 ++++++++++++++++++++++++++++++
> 4 files changed, 64 insertions(+), 3 deletions(-)
The only thing that gives me a slight pause is that on a set operation
we are going to hit both the get and set LSM hooks, but since the code
does call into the getter on a set operation this is arguably the right
thing.
Acked-by: Paul Moore <paul@paul-moore.com>
--
paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH 0/5] add process_madvise() flags to modify behaviour
From: Mike Rapoport @ 2025-05-22 12:12 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Andrew Morton, Liam R . Howlett, David Hildenbrand,
Vlastimil Babka, Jann Horn, Arnd Bergmann, Christian Brauner,
linux-mm, linux-arch, linux-kernel, SeongJae Park, Usama Arif,
linux-api
In-Reply-To: <cover.1747686021.git.lorenzo.stoakes@oracle.com>
(cc'ing linux-api)
On Mon, May 19, 2025 at 09:52:37PM +0100, Lorenzo Stoakes wrote:
> REVIEWERS NOTES:
> ================
>
> This is a VERY EARLY version of the idea, it's relatively untested, and I'm
> 'putting it out there' for feedback. Any serious version of this will add a
> bunch of self-tests to assert correct behaviour and I will more carefully
> confirm everything's working.
>
> This is based on discussion arising from Usama's series [0], SJ's input on
> the thread around process_madvise() behaviour [1] (and a subsequent
> response by me [2]) and prior discussion about a new madvise() interface
> [3].
>
> [0]: https://lore.kernel.org/linux-mm/20250515133519.2779639-1-usamaarif642@gmail.com/
> [1]: https://lore.kernel.org/linux-mm/20250517162048.36347-1-sj@kernel.org/
> [2]: https://lore.kernel.org/linux-mm/e3ba284c-3cb1-42c1-a0ba-9c59374d0541@lucifer.local/
> [3]: https://lore.kernel.org/linux-mm/c390dd7e-0770-4d29-bb0e-f410ff6678e3@lucifer.local/
>
> ================
>
> Currently, we are rather restricted in how madvise() operations
> proceed. While effort has been put in to expanding what process_madvise()
> can do (that is - unrestricted application of advice to the local process
> alongside recent improvements on the efficiency of TLB operations over
> these batvches), we are still constrained by existing madvise() limitations
> and default behaviours.
>
> This series makes use of the currently unused flags field in
> process_madvise() to provide more flexiblity.
>
> It introduces four flags:
>
> 1. PMADV_SKIP_ERRORS
>
> Currently, when an error arises applying advice in any individual VMA
> (keeping in mind that a range specified to madvise() or as part of the
> iovec passed to process_madvise()), the operation stops where it is and
> returns an error.
>
> This might not be the desired behaviour of the user, who may wish instead
> for the operation to be 'best effort'. By setting this flag, that behaviour
> is obtained.
>
> Since process_madvise() would trivially, if skipping errors, simply return
> the input vector size, we instead return the number of entries in the
> vector which completed successfully without error.
>
> The PMADV_SKIP_ERRORS flag implicitly implies PMADV_NO_ERROR_ON_UNMAPPED.
>
> 2. PMADV_NO_ERROR_ON_UNMAPPED
>
> Currently madvise() has the peculiar behaviour of, if the range specified
> to it contains unmapped range(s), completing the full operation, but
> ultimately returning -ENOMEM.
>
> In the case of process_madvise(), this is fatal, as the operation will stop
> immediately upon this occurring.
>
> By setting PMADV_NO_ERROR_ON_UNMAPPED, the user can indicate that it wishes
> unmapped areas to simply be entirely ignored.
>
> 3. PMADV_SET_FORK_EXEC_DEFAULT
>
> It may be desirable for a user to specify that all VMAs mapped in a process
> address space default to having an madvise() behaviour established by
> default, in such a fashion as that this persists across fork/exec.
>
> Since this is a very powerful option that would make no sense for many
> advice modes, we explicitly only permit known-safe flags here (currently
> MADV_HUGEPAGE and MADV_NOHUGEPAGE only).
>
> 4. PMADV_ENTIRE_ADDRESS_SPACE
>
> It can be annoying, should a user wish to apply madvise() to all VMAs in an
> address space, to have to add a singular large entry to the input iovec.
>
> So provide sugar to permit this - PMADV_ENTIRE_ADDRESS_SPACE. If specified,
> we expect the user to pass NULL and -1 to the vec and vlen parameters
> respectively so they explicitly acknowledge that these will be ignored,
> e.g.:
>
> process_madvise(PIDFD_SELF, NULL, -1, MADV_HUGEPAGE,
> PMADV_ENTIRE_ADDRESS_SPACE | PMADV_SKIP_ERRORS);
>
> Usually a user ought to prefer setting PMADV_SKIP_ERRORS here as it may
> well be the case that incompatible VMAs will be encountered that ought to
> be skipped.
>
> If this is not set, the PMADV_NO_ERROR_ON_UNMAPPED (which was otherwise
> implicitly implied by PMADV_SKIP_ERRORS) ought to be set as of course, the
> entire address space spans at least some gaps.
>
> Lorenzo Stoakes (5):
> mm: madvise: refactor madvise_populate()
> mm/madvise: add PMADV_SKIP_ERRORS process_madvise() flag
> mm/madvise: add PMADV_NO_ERROR_ON_UNMAPPED process_madvise() flag
> mm/madvise: add PMADV_SET_FORK_EXEC_DEFAULT process_madvise() flag
> mm/madvise: add PMADV_ENTIRE_ADDRESS_SPACE process_madvise() flag
>
> include/uapi/asm-generic/mman-common.h | 6 +
> mm/madvise.c | 206 +++++++++++++++++++------
> 2 files changed, 168 insertions(+), 44 deletions(-)
>
> --
> 2.49.0
>
--
Sincerely yours,
Mike.
^ 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