* Re: [PATCH v5 2/5] pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process
From: Lorenzo Stoakes @ 2024-10-25 22:17 UTC (permalink / raw)
To: John Hubbard
Cc: Pedro Falcato, Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, linux-kselftest, linux-mm,
linux-fsdevel, linux-api, linux-kernel, Oliver Sang
In-Reply-To: <330c0dae-fa8a-49e5-94b4-25b915f74e37@nvidia.com>
On Fri, Oct 25, 2024 at 02:51:29PM -0700, John Hubbard wrote:
> On 10/25/24 2:09 PM, Lorenzo Stoakes wrote:
> > On Fri, Oct 25, 2024 at 01:31:49PM -0700, John Hubbard wrote:
> > > On 10/25/24 12:49 PM, Lorenzo Stoakes wrote:
> > > > On Fri, Oct 25, 2024 at 11:44:34AM -0700, John Hubbard wrote:
> > > > > On 10/25/24 11:38 AM, Pedro Falcato wrote:
> > > > > > On Fri, Oct 25, 2024 at 6:41 PM John Hubbard <jhubbard@nvidia.com> wrote:
> > > ...
> > > I'll admit to being easily cowed by "you're breaking userspace" arguments.
> > > Even when they start to get rather absurd. Because I can't easily tell where
> > > the line is.
> > >
> > > Maybe "-std=c89 -pedantic" is on the other side of the line. I'd like it
> > > to be! :)
> >
> > Well, apparently not...
>
> Why not? Your arguments are clear and reasonable. Why shouldn't they prevail?
>
> Please don't think that I have some sort of firm position here. I'm simply
> looking for the right answer. And if that's different than something I
> proposed earlier, no problem. The best answer should win.
>
> ...
> > > > The bike shed should be blue! Wait no no, it should be red... Hang on
> > > > yellow yes! Yellow's great!
> > >
> > > Putting a header in the right location, so as to avoid breakage here or
> > > there, is not bikeshedding. Sorry.
> >
> > There are 312 uses of "static inline" already in UAPI headers, not all
> > quite as obscure as claimed.
> >
>
> OK, good. Let's lead with that. It seems very clear, then, that a new one
> won't cause a problem.
Right, sorry I thought I had made this point earlier, perhaps in a sub-thread of
a thread of thread. It felt as if you guys were acting as if that were
immaterial, which is why I highlighted it again.
>
> > Specifically requiring me and only me to support ansi C89 for a theorised
> > scenario is in my opinion bikeshedding, but I don't want to get into an
> > argument about something so petty :)
>
> An argument about the definition of bikeshedding sounds delightfully
> recursive, but yes, let's not. :)
:)
>
> ...
> > > > ANyway if you guys feel strong enough about this, I'll respin again and
> > > > just open-code this trivial check where it's used.
> > >
> > > No strong feelings, just hoping to help make a choice that gets you
> > > closer to getting your patches committed.
> >
> > I mean, you are saying I am breaking things and implying the series is
> > blocked on this, that sounds like a strong opinion, but again I'm not going
> > to argue.
>
> Actually, Pedro's request kicked this off, and I was hoping to dismiss
> it--again, in order to help move things along. My opinion is that we
> should shun ancient toolchains and ancient systems whenever possible.
>
> Somehow that got turned into "I'm trying to block the patchset". Really,
> whatever works, follows The Rules (whatever we eventually understand
> them to be), and doesn't cause someone *else* to come out of the
> woodwork and claim a problem, is fine with me.
Text is a poor medium, sorry!
I don't mean to say you're doing that purposefully on any level, I mean to
say that, by arguing here over something that feels kind of unimportant, we
are inadvertently doing that.
>
> >
> > As with the requirement that I, only for my part of the change, must fix up
> > test header import, while I disagree I should be doing the fix, I did it
> > anyway as I am accommodating and reasonable.
>
> I agree that pre-existing problems in selftests should not be your
> problem.
>
> By the way, I'm occasionally involved in helping fix up various
> selftest-related problems, especially when they impact mm. Send me a
> note if you have anything in mind that ought to be fixed up, I might be
> able to help head off future grief in that area.
Sure, and I'm passionate about tests (I've written _thousands_ of lines of
tests recently!) I mention this as a related example of something that
feels out of scope.
Equally as the pidfd.h test header already had other instances of exactly
what I did and thus really should have been solved as a separate series
(one that I'd have been happy to do myself), I feel this issue, if truly a
problem should be considered separately.
>
> >
> > So fine - I'll respin and just open-code this as it's trivial and there's
> > no (other) sensible place to put it anyway.
> >
> > A P.S. though - a very NOT theoretical issue with userspace is the import
> > of linux/fcntl.h in pidfd.h which seems to me to have been imported solely
> > for the kernel's sake.
> >
> > A gentle suggestion (it seems I can't win - gentle suggestions are ignored,
> > tongue-in-cheek parody is taken to be mean... but anyway) is to do
>
> Actually, these come across as sarcasm, especially in the context of
> these emails that show you are becoming quite distraught.
Yes, I get that, rather a Brit element to this, to be clear - I am not
distraught, merely mildly perturbed. Again text is a bloody awful medium!
This genuinely was meant to be tongue in cheek, BUT I realise it's a me
issue on this kind of thing - obviously written down like that it comes off
as possibly dripping with a kind of venom that was ABSOLUTELY not intended.
Whereas my intent was a sort of wry smile, 'come on guys this doesn't
matter' thing.
But since this is the second time now that I've said something intended
that way and having been received as quite something different - this is a
me thing - and I will refrain from dalliances into the rhetorical like this
in future!
Apologies if either you or Pedro took offence and I'm fine! Other than this
damn cold that wont' go away...
>
> I've met you several times at the conferences. We get along well. And
> your work is top notch. So please consider that I'm very much supportive
> of you and your work here.
And I consider you one of the loveliest people in the kernel and very very
sharp, and have enjoyed meeting you and your erstwhile colleague Jason :)
To be clear - I also have high regard for Pedro who I consider very smart,
and I don't say that lightly.
I mean this _whole series_ is his idea, for instance. I don't just write
series based on an idea on review for just anyone ;)
So this is not at all intended to be a critique of either of you, as I have
the utmost regard for you both...!
>
> I'm still trying to understand why you are recently sending these very
> strong emails (Vlastimil also took some heat), but I see that you also
> mentioned some long hours.
Well some of higher 'strength' have more basis than others that may be my
failing to communicate things quite as intended. We'd have to speak on a
case-by-case basis :)
But in Vlastimil's case, that was absolutely not intended. Again text is a
bad medium!
>
> If my feedback is making things worse here, I'll try to adjust.
> Selftests in general are a frustrating area.
No it's fine, I think just a comms thing here.
Please do carry on reviewing it is all much appreciated... I promise!
>
>
> thanks,
> --
> John Hubbard
>
>
> > something like:
> >
> > #ifdef __KERNEL__
> > #include <linux/fcntl.h>
> > #else
> > #include <fcntl.h>
> > #endif
> >
> > At the top of the pidfd.h header. This must surely sting a _lot_ of people
> > in userland otherwise.
> >
> > But this is out of scope for this change.
>
Anyway on this issue, as I said, and meant - I will respin with this taken
out to alleviate concerns.
The _far_ more pressing issue I think is the one Pedro raised about the
actual PIDFD_SELF* values. I may simply choose some arbitrary ones in the
range specified by Pedro on respin.
Thanks! And I guess I owe you both beers ;)
^ permalink raw reply
* Re: [PATCH v3 1/5] mm: pagewalk: add the ability to install PTEs
From: Lorenzo Stoakes @ 2024-10-25 21:58 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Paul E . McKenney, Jann Horn, David Hildenbrand,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Matt Turner, Thomas Bogendoerfer, James E . J . Bottomley,
Helge Deller, Chris Zankel, Max Filippov, Arnd Bergmann,
linux-alpha, linux-mips, linux-parisc, linux-arch, Shuah Khan,
Christian Brauner, linux-kselftest, Sidhartha Kumar, Jeff Xu,
Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <8b1854b5-5c0a-43b1-aed7-aa4e8b8e8a1a@suse.cz>
On Fri, Oct 25, 2024 at 08:13:26PM +0200, Vlastimil Babka wrote:
> On 10/23/24 18:24, Lorenzo Stoakes wrote:
> > The existing generic pagewalk logic permits the walking of page tables,
> > invoking callbacks at individual page table levels via user-provided
> > mm_walk_ops callbacks.
> >
> > This is useful for traversing existing page table entries, but precludes
> > the ability to establish new ones.
> >
> > Existing mechanism for performing a walk which also installs page table
> > entries if necessary are heavily duplicated throughout the kernel, each
> > with semantic differences from one another and largely unavailable for use
> > elsewhere.
> >
> > Rather than add yet another implementation, we extend the generic pagewalk
> > logic to enable the installation of page table entries by adding a new
> > install_pte() callback in mm_walk_ops. If this is specified, then upon
> > encountering a missing page table entry, we allocate and install a new one
> > and continue the traversal.
> >
> > If a THP huge page is encountered at either the PMD or PUD level we split
> > it only if there are ops->pte_entry() (or ops->pmd_entry at PUD level),
> > otherwise if there is only an ops->install_pte(), we avoid the unnecessary
> > split.
> >
> > We do not support hugetlb at this stage.
> >
> > If this function returns an error, or an allocation fails during the
> > operation, we abort the operation altogether. It is up to the caller to
> > deal appropriately with partially populated page table ranges.
> >
> > If install_pte() is defined, the semantics of pte_entry() change - this
> > callback is then only invoked if the entry already exists. This is a useful
> > property, as it allows a caller to handle existing PTEs while installing
> > new ones where necessary in the specified range.
> >
> > If install_pte() is not defined, then there is no functional difference to
> > this patch, so all existing logic will work precisely as it did before.
> >
> > As we only permit the installation of PTEs where a mapping does not already
> > exist there is no need for TLB management, however we do invoke
> > update_mmu_cache() for architectures which require manual maintenance of
> > mappings for other CPUs.
> >
> > We explicitly do not allow the existing page walk API to expose this
> > feature as it is dangerous and intended for internal mm use only. Therefore
> > we provide a new walk_page_range_mm() function exposed only to
> > mm/internal.h.
> >
> > Reviewed-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
Thanks!
>
> Just a small subjective suggestion in case you agree and there's a respin or
> followups:
>
> > @@ -109,18 +131,19 @@ static int walk_pmd_range(pud_t *pud, unsigned long addr, unsigned long end,
> >
> > if (walk->action == ACTION_AGAIN)
> > goto again;
> > -
> > - /*
> > - * Check this here so we only break down trans_huge
> > - * pages when we _need_ to
> > - */
> > - if ((!walk->vma && (pmd_leaf(*pmd) || !pmd_present(*pmd))) ||
> > - walk->action == ACTION_CONTINUE ||
> > - !(ops->pte_entry))
> > + if (walk->action == ACTION_CONTINUE)
> > continue;
> > + if (!ops->install_pte && !ops->pte_entry)
> > + continue; /* Nothing to do. */
> > + if (!ops->pte_entry && ops->install_pte &&
> > + pmd_present(*pmd) &&
> > + (pmd_trans_huge(*pmd) || pmd_devmap(*pmd)))
> > + continue; /* Avoid unnecessary split. */
>
> Much better now, thanks, but maybe the last 2 parts could be:
>
> if (!ops->pte_entry) {
> if (!ops->install_pte)
> continue; /* Nothing to do. */
> else if (pmd_present(*pmd)
> && (pmd_trans_huge(*pmd) || pmd_devmap(*pmd)))
> continue; /* Avoid unnecessary split. */
> }
I quite liked separating out the 'nothing to do' vs. the 'unnecessary split'
cases, but I agree it makes it harder to see that the 2nd case is an 'install
pte ONLY' case.
Yeah so I think your version is better, but maybe we can find a way to be more
expressive somehow... if we could declare vars mid-way thhrough it'd be easier
:P
Will improve on respin if it comes up
>
> Or at least put !ops->pte_entry first in both conditions?
Ack yeah that'd be better!
>
> > if (walk->vma)
> > split_huge_pmd(walk->vma, pmd, addr);
> > + else if (pmd_leaf(*pmd) || !pmd_present(*pmd))
> > + continue; /* Nothing to do. */
> >
> > err = walk_pte_range(pmd, addr, next, walk);
> > if (err)
> > @@ -148,11 +171,14 @@ static int walk_pud_range(p4d_t *p4d, unsigned long addr, unsigned long end,
> > again:
> > next = pud_addr_end(addr, end);
> > if (pud_none(*pud)) {
> > - if (ops->pte_hole)
> > + if (ops->install_pte)
> > + err = __pmd_alloc(walk->mm, pud, addr);
> > + else if (ops->pte_hole)
> > err = ops->pte_hole(addr, next, depth, walk);
> > if (err)
> > break;
> > - continue;
> > + if (!ops->install_pte)
> > + continue;
> > }
> >
> > walk->action = ACTION_SUBTREE;
> > @@ -164,14 +190,20 @@ static int walk_pud_range(p4d_t *p4d, unsigned long addr, unsigned long end,
> >
> > if (walk->action == ACTION_AGAIN)
> > goto again;
> > -
> > - if ((!walk->vma && (pud_leaf(*pud) || !pud_present(*pud))) ||
> > - walk->action == ACTION_CONTINUE ||
> > - !(ops->pmd_entry || ops->pte_entry))
> > + if (walk->action == ACTION_CONTINUE)
> > continue;
> > + if (!ops->install_pte && !ops->pte_entry && !ops->pmd_entry)
> > + continue; /* Nothing to do. */
> > + if (!ops->pmd_entry && !ops->pte_entry && ops->install_pte &&
> > + pud_present(*pud) &&
> > + (pud_trans_huge(*pud) || pud_devmap(*pud)))
> > + continue; /* Avoid unnecessary split. */
>
> Ditto.
Ack!
>
> Thanks!
>
Cheers!
^ permalink raw reply
* Re: [PATCH v3 3/5] mm: madvise: implement lightweight guard page mechanism
From: Vlastimil Babka @ 2024-10-25 21:56 UTC (permalink / raw)
To: Lorenzo Stoakes, Andrew Morton
Cc: Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
Paul E . McKenney, Jann Horn, David Hildenbrand, linux-mm,
linux-kernel, Muchun Song, Richard Henderson, Matt Turner,
Thomas Bogendoerfer, James E . J . Bottomley, Helge Deller,
Chris Zankel, Max Filippov, Arnd Bergmann, linux-alpha,
linux-mips, linux-parisc, linux-arch, Shuah Khan,
Christian Brauner, linux-kselftest, Sidhartha Kumar, Jeff Xu,
Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <f09086cd-c49c-4eea-adff-d626c16083de@lucifer.local>
On 10/25/24 19:12, Lorenzo Stoakes wrote:
> On Wed, Oct 23, 2024 at 05:24:40PM +0100, Lorenzo Stoakes wrote:
>> Implement a new lightweight guard page feature, that is regions of userland
>> virtual memory that, when accessed, cause a fatal signal to arise.
>
> <snip>
>
> Hi Andrew - Could you apply the below fix-patch? I realise we must handle
> fatal signals and conditional rescheduling in the vector_madvise() special
> case.
>
> Thanks!
>
> ----8<----
> From 546d7e1831c71599fc733d589e0d75f52e84826d Mon Sep 17 00:00:00 2001
> From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Date: Fri, 25 Oct 2024 18:05:48 +0100
> Subject: [PATCH] mm: yield on fatal signal/cond_sched() in vector_madvise()
>
> While we have to treat -ERESTARTNOINTR specially here as we are looping
> through a vector of operations and can't simply restart the entire
> operation, we mustn't hold up fatal signals or RT kernels.
For plain madvise() syscall returning -ERESTARTNOINTR does the right thing
and checks fatal_signal_pending() before returning, right?
Uh actually can we be just returning -ERESTARTNOINTR or do we need to use
restart_syscall()?
> ---
> mm/madvise.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 48eba25e25fe..127aa5d86656 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -1713,8 +1713,14 @@ static ssize_t vector_madvise(struct mm_struct *mm, struct iov_iter *iter,
> * we have already rescinded locks, it should be no problem to
> * simply try again.
> */
> - if (ret == -ERESTARTNOINTR)
> + if (ret == -ERESTARTNOINTR) {
> + if (fatal_signal_pending(current)) {
> + ret = -EINTR;
> + break;
> + }
> + cond_resched();
Should be unnecessary as we're calling an operation that takes a rwsem so
there are reschedule points already. And with lazy preempt hopefully
cond_resched()s will become history, so let's not add more only to delete later.
> continue;
> + }
> if (ret < 0)
> break;
> iov_iter_advance(iter, iter_iov_len(iter));
> --
> 2.47.0
^ permalink raw reply
* Re: [PATCH v5 2/5] pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process
From: John Hubbard @ 2024-10-25 21:51 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Pedro Falcato, Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, linux-kselftest, linux-mm,
linux-fsdevel, linux-api, linux-kernel, Oliver Sang
In-Reply-To: <239456b7-4045-46cd-a2e7-8445dd6640c6@lucifer.local>
On 10/25/24 2:09 PM, Lorenzo Stoakes wrote:
> On Fri, Oct 25, 2024 at 01:31:49PM -0700, John Hubbard wrote:
>> On 10/25/24 12:49 PM, Lorenzo Stoakes wrote:
>>> On Fri, Oct 25, 2024 at 11:44:34AM -0700, John Hubbard wrote:
>>>> On 10/25/24 11:38 AM, Pedro Falcato wrote:
>>>>> On Fri, Oct 25, 2024 at 6:41 PM John Hubbard <jhubbard@nvidia.com> wrote:
>> ...
>> I'll admit to being easily cowed by "you're breaking userspace" arguments.
>> Even when they start to get rather absurd. Because I can't easily tell where
>> the line is.
>>
>> Maybe "-std=c89 -pedantic" is on the other side of the line. I'd like it
>> to be! :)
>
> Well, apparently not...
Why not? Your arguments are clear and reasonable. Why shouldn't they prevail?
Please don't think that I have some sort of firm position here. I'm simply
looking for the right answer. And if that's different than something I
proposed earlier, no problem. The best answer should win.
...
>>> The bike shed should be blue! Wait no no, it should be red... Hang on
>>> yellow yes! Yellow's great!
>>
>> Putting a header in the right location, so as to avoid breakage here or
>> there, is not bikeshedding. Sorry.
>
> There are 312 uses of "static inline" already in UAPI headers, not all
> quite as obscure as claimed.
>
OK, good. Let's lead with that. It seems very clear, then, that a new one
won't cause a problem.
> Specifically requiring me and only me to support ansi C89 for a theorised
> scenario is in my opinion bikeshedding, but I don't want to get into an
> argument about something so petty :)
An argument about the definition of bikeshedding sounds delightfully
recursive, but yes, let's not. :)
...
>>> ANyway if you guys feel strong enough about this, I'll respin again and
>>> just open-code this trivial check where it's used.
>>
>> No strong feelings, just hoping to help make a choice that gets you
>> closer to getting your patches committed.
>
> I mean, you are saying I am breaking things and implying the series is
> blocked on this, that sounds like a strong opinion, but again I'm not going
> to argue.
Actually, Pedro's request kicked this off, and I was hoping to dismiss
it--again, in order to help move things along. My opinion is that we
should shun ancient toolchains and ancient systems whenever possible.
Somehow that got turned into "I'm trying to block the patchset". Really,
whatever works, follows The Rules (whatever we eventually understand
them to be), and doesn't cause someone *else* to come out of the
woodwork and claim a problem, is fine with me.
>
> As with the requirement that I, only for my part of the change, must fix up
> test header import, while I disagree I should be doing the fix, I did it
> anyway as I am accommodating and reasonable.
I agree that pre-existing problems in selftests should not be your
problem.
By the way, I'm occasionally involved in helping fix up various
selftest-related problems, especially when they impact mm. Send me a
note if you have anything in mind that ought to be fixed up, I might be
able to help head off future grief in that area.
>
> So fine - I'll respin and just open-code this as it's trivial and there's
> no (other) sensible place to put it anyway.
>
> A P.S. though - a very NOT theoretical issue with userspace is the import
> of linux/fcntl.h in pidfd.h which seems to me to have been imported solely
> for the kernel's sake.
>
> A gentle suggestion (it seems I can't win - gentle suggestions are ignored,
> tongue-in-cheek parody is taken to be mean... but anyway) is to do
Actually, these come across as sarcasm, especially in the context of
these emails that show you are becoming quite distraught.
I've met you several times at the conferences. We get along well. And
your work is top notch. So please consider that I'm very much supportive
of you and your work here.
I'm still trying to understand why you are recently sending these very
strong emails (Vlastimil also took some heat), but I see that you also
mentioned some long hours.
If my feedback is making things worse here, I'll try to adjust.
Selftests in general are a frustrating area.
thanks,
--
John Hubbard
> something like:
>
> #ifdef __KERNEL__
> #include <linux/fcntl.h>
> #else
> #include <fcntl.h>
> #endif
>
> At the top of the pidfd.h header. This must surely sting a _lot_ of people
> in userland otherwise.
>
> But this is out of scope for this change.
^ permalink raw reply
* Re: [PATCH v3 3/5] mm: madvise: implement lightweight guard page mechanism
From: Lorenzo Stoakes @ 2024-10-25 21:49 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Paul E . McKenney, Jann Horn, David Hildenbrand,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Matt Turner, Thomas Bogendoerfer, James E . J . Bottomley,
Helge Deller, Chris Zankel, Max Filippov, Arnd Bergmann,
linux-alpha, linux-mips, linux-parisc, linux-arch, Shuah Khan,
Christian Brauner, linux-kselftest, Sidhartha Kumar, Jeff Xu,
Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <0ed7b766-1e7d-4f77-bf5a-bfa52d36ca8e@suse.cz>
On Fri, Oct 25, 2024 at 11:44:56PM +0200, Vlastimil Babka wrote:
> On 10/23/24 18:24, Lorenzo Stoakes wrote:
> > Implement a new lightweight guard page feature, that is regions of userland
> > virtual memory that, when accessed, cause a fatal signal to arise.
> >
> > Currently users must establish PROT_NONE ranges to achieve this.
> >
> > However this is very costly memory-wise - we need a VMA for each and every
> > one of these regions AND they become unmergeable with surrounding VMAs.
> >
> > In addition repeated mmap() calls require repeated kernel context switches
> > and contention of the mmap lock to install these ranges, potentially also
> > having to unmap memory if installed over existing ranges.
> >
> > The lightweight guard approach eliminates the VMA cost altogether - rather
> > than establishing a PROT_NONE VMA, it operates at the level of page table
> > entries - establishing PTE markers such that accesses to them cause a fault
> > followed by a SIGSGEV signal being raised.
> >
> > This is achieved through the PTE marker mechanism, which we have already
> > extended to provide PTE_MARKER_GUARD, which we installed via the generic
> > page walking logic which we have extended for this purpose.
> >
> > These guard ranges are established with MADV_GUARD_INSTALL. If the range in
> > which they are installed contain any existing mappings, they will be
> > zapped, i.e. free the range and unmap memory (thus mimicking the behaviour
> > of MADV_DONTNEED in this respect).
> >
> > Any existing guard entries will be left untouched. There is therefore no
> > nesting of guarded pages.
> >
> > Guarded ranges are NOT cleared by MADV_DONTNEED nor MADV_FREE (in both
> > instances the memory range may be reused at which point a user would expect
> > guards to still be in place), but they are cleared via MADV_GUARD_REMOVE,
> > process teardown or unmapping of memory ranges.
> >
> > The guard property can be removed from ranges via MADV_GUARD_REMOVE. The
> > ranges over which this is applied, should they contain non-guard entries,
> > will be untouched, with only guard entries being cleared.
> >
> > We permit this operation on anonymous memory only, and only VMAs which are
> > non-special, non-huge and not mlock()'d (if we permitted this we'd have to
> > drop locked pages which would be rather counterintuitive).
> >
> > Racing page faults can cause repeated attempts to install guard pages that
> > are interrupted, result in a zap, and this process can end up being
> > repeated. If this happens more than would be expected in normal operation,
> > we rescind locks and retry the whole thing, which avoids lock contention in
> > this scenario.
> >
> > Suggested-by: Vlastimil Babka <vbabka@suse.cz>
> > Suggested-by: Jann Horn <jannh@google.com>
> > Suggested-by: David Hildenbrand <david@redhat.com>
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
>
Thanks!
> > --- a/mm/internal.h
> > +++ b/mm/internal.h
> > @@ -423,6 +423,12 @@ extern unsigned long highest_memmap_pfn;
> > */
> > #define MAX_RECLAIM_RETRIES 16
> >
> > +/*
> > + * Maximum number of attempts we make to install guard pages before we give up
> > + * and return -ERESTARTNOINTR to have userspace try again.
> > + */
> > +#define MAX_MADVISE_GUARD_RETRIES 3
>
> Can't we simply put this in mm/madvise.c ? Didn't find usage elsewhere.
>
>
Sure, will move if respin/can send a quick fixpatch next week if otherwise
settled. Just felt vaguely 'neater' here for... spurious subjective squishy
brained reasons :)
^ permalink raw reply
* Re: [PATCH v3 3/5] mm: madvise: implement lightweight guard page mechanism
From: Vlastimil Babka @ 2024-10-25 21:44 UTC (permalink / raw)
To: Lorenzo Stoakes, Andrew Morton
Cc: Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
Paul E . McKenney, Jann Horn, David Hildenbrand, linux-mm,
linux-kernel, Muchun Song, Richard Henderson, Matt Turner,
Thomas Bogendoerfer, James E . J . Bottomley, Helge Deller,
Chris Zankel, Max Filippov, Arnd Bergmann, linux-alpha,
linux-mips, linux-parisc, linux-arch, Shuah Khan,
Christian Brauner, linux-kselftest, Sidhartha Kumar, Jeff Xu,
Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <415da1e6c5828d96db3af480d243a7f68ccabf6d.1729699916.git.lorenzo.stoakes@oracle.com>
On 10/23/24 18:24, Lorenzo Stoakes wrote:
> Implement a new lightweight guard page feature, that is regions of userland
> virtual memory that, when accessed, cause a fatal signal to arise.
>
> Currently users must establish PROT_NONE ranges to achieve this.
>
> However this is very costly memory-wise - we need a VMA for each and every
> one of these regions AND they become unmergeable with surrounding VMAs.
>
> In addition repeated mmap() calls require repeated kernel context switches
> and contention of the mmap lock to install these ranges, potentially also
> having to unmap memory if installed over existing ranges.
>
> The lightweight guard approach eliminates the VMA cost altogether - rather
> than establishing a PROT_NONE VMA, it operates at the level of page table
> entries - establishing PTE markers such that accesses to them cause a fault
> followed by a SIGSGEV signal being raised.
>
> This is achieved through the PTE marker mechanism, which we have already
> extended to provide PTE_MARKER_GUARD, which we installed via the generic
> page walking logic which we have extended for this purpose.
>
> These guard ranges are established with MADV_GUARD_INSTALL. If the range in
> which they are installed contain any existing mappings, they will be
> zapped, i.e. free the range and unmap memory (thus mimicking the behaviour
> of MADV_DONTNEED in this respect).
>
> Any existing guard entries will be left untouched. There is therefore no
> nesting of guarded pages.
>
> Guarded ranges are NOT cleared by MADV_DONTNEED nor MADV_FREE (in both
> instances the memory range may be reused at which point a user would expect
> guards to still be in place), but they are cleared via MADV_GUARD_REMOVE,
> process teardown or unmapping of memory ranges.
>
> The guard property can be removed from ranges via MADV_GUARD_REMOVE. The
> ranges over which this is applied, should they contain non-guard entries,
> will be untouched, with only guard entries being cleared.
>
> We permit this operation on anonymous memory only, and only VMAs which are
> non-special, non-huge and not mlock()'d (if we permitted this we'd have to
> drop locked pages which would be rather counterintuitive).
>
> Racing page faults can cause repeated attempts to install guard pages that
> are interrupted, result in a zap, and this process can end up being
> repeated. If this happens more than would be expected in normal operation,
> we rescind locks and retry the whole thing, which avoids lock contention in
> this scenario.
>
> Suggested-by: Vlastimil Babka <vbabka@suse.cz>
> Suggested-by: Jann Horn <jannh@google.com>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -423,6 +423,12 @@ extern unsigned long highest_memmap_pfn;
> */
> #define MAX_RECLAIM_RETRIES 16
>
> +/*
> + * Maximum number of attempts we make to install guard pages before we give up
> + * and return -ERESTARTNOINTR to have userspace try again.
> + */
> +#define MAX_MADVISE_GUARD_RETRIES 3
Can't we simply put this in mm/madvise.c ? Didn't find usage elsewhere.
^ permalink raw reply
* Re: [PATCH 2/6] futex: Implement FUTEX2_NUMA
From: André Almeida @ 2024-10-25 21:23 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, mingo, dvhart, dave, Andrew Morton, urezki, hch,
lstoakes, Arnd Bergmann, linux-api, linux-mm, linux-arch,
malteskarupke, cl, llong, tglx
In-Reply-To: <20241025093944.485691531@infradead.org>
Hey Peter,
Em 25/10/2024 06:03, Peter Zijlstra escreveu:
> Extend the futex2 interface to be numa aware.
>
> When FUTEX2_NUMA is specified for a futex, the user value is extended
> to two words (of the same size). The first is the user value we all
> know, the second one will be the node to place this futex on.
>
> struct futex_numa_32 {
> u32 val;
> u32 node;
> };
>
Maybe this should live at include/uapi/linux/futex.h.
> When node is set to ~0, WAIT will set it to the current node_id such
> that WAKE knows where to find it. If userspace corrupts the node value
> between WAIT and WAKE, the futex will not be found and no wakeup will
> happen.
>
> When FUTEX2_NUMA is not set, the node is simply an extention of the
> hash, such that traditional futexes are still interleaved over the
> nodes.
>
> This is done to avoid having to have a separate !numa hash-table.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Do you think some of those changes should be guarded with #ifdef
CONFIG_NUMA? Or is fine as it is? I see that most of NUMA_ values
defines to 1 anyway on !numa, but maybe the futex_init() and
futex_hash() would be a bit more simplified.
[...]
>
> +static int futex_get_value(u32 *val, u32 __user *from, unsigned int flags)
> +{
> + switch (futex_size(flags)) {
> + case 1: return __get_user(*val, (u8 __user *)from);
> + case 2: return __get_user(*val, (u16 __user *)from);
> + case 4: return __get_user(*val, (u32 __user *)from);
> + default: BUG();
> + }
> +}
> +
> +static int futex_put_value(u32 val, u32 __user *to, unsigned int flags)
> +{
> + switch (futex_size(flags)) {
> + case 1: return __put_user(val, (u8 __user *)to);
> + case 2: return __put_user(val, (u16 __user *)to);
> + case 4: return __put_user(val, (u32 __user *)to);
> + default: BUG();
> + }
> +}
> +
I found a bit confusing that this is here, shouldn't be at [PATCH 4/6]
futex: Enable FUTEX2_{8,16}?
^ permalink raw reply
* Re: [PATCH v5 2/5] pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process
From: Lorenzo Stoakes @ 2024-10-25 21:09 UTC (permalink / raw)
To: John Hubbard
Cc: Pedro Falcato, Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, linux-kselftest, linux-mm,
linux-fsdevel, linux-api, linux-kernel, Oliver Sang
In-Reply-To: <9de8d876-5729-454b-bf8c-8b0ec8f8ffc1@nvidia.com>
On Fri, Oct 25, 2024 at 01:31:49PM -0700, John Hubbard wrote:
> On 10/25/24 12:49 PM, Lorenzo Stoakes wrote:
> > On Fri, Oct 25, 2024 at 11:44:34AM -0700, John Hubbard wrote:
> > > On 10/25/24 11:38 AM, Pedro Falcato wrote:
> > > > On Fri, Oct 25, 2024 at 6:41 PM John Hubbard <jhubbard@nvidia.com> wrote:
> ...
> > > > That seems to only apply to the kernel internally, uapi headers are
> > >
> > > Yes.
> > >
> > > > included from userspace too (-std=c89 -pedantic doesn't know what a
> > > > gnu extension is). And uapi headers _generally_ keep to defining
> > > > constants and structs, nothing more.
> > >
> > > OK
> >
> > Because a lot of people using -ANSI- C89 are importing a very new linux
> > feature header.
>
> I'll admit to being easily cowed by "you're breaking userspace" arguments.
> Even when they start to get rather absurd. Because I can't easily tell where
> the line is.
>
> Maybe "-std=c89 -pedantic" is on the other side of the line. I'd like it
> to be! :)
Well, apparently not...
>
> >
> > And let's ignore the hundreds of existing uses... OK.
> >
> > The rules, unstated anywhere, are that we must support 1972-era C in an
> > optional header for a feature available only in new kernels because
> > somebody somewhere is using a VAX-11 and gosh darn it they can't change
> > their toolchain!
> >
> > And you had better make sure you don't wear out those tape drums...
> >
> > >
> > > > I don't know what the guidelines for uapi headers are nowadays, but we
> > > > generally want to not break userspace.
> > > >
> > > > >
> > > > > I think it's quite clear at this point, that we should not hold up new
> > > > > work, based on concerns about handling the inline keyword, nor about
> > > > > C89.
> > > >
> > > > Right, but the correct solution is probably to move
> > > > pidfd_is_self_sentinel to some other place, since it's not even
> > > > supposed to be used by userspace (it's semantically useless to
> > > > userspace, and it's only two users are in the kernel, kernel/pid.c and
> > > > exit.c).
> > > >
> > >
> > > Yes, if userspace absolutely doesn't need nor want this, then putting
> > > it in a non-uapi header does sound like the right move.
> >
> > The bike shed should be blue! Wait no no, it should be red... Hang on
> > yellow yes! Yellow's great!
>
> Putting a header in the right location, so as to avoid breakage here or
> there, is not bikeshedding. Sorry.
There are 312 uses of "static inline" already in UAPI headers, not all
quite as obscure as claimed.
Specifically requiring me and only me to support ansi C89 for a theorised
scenario is in my opinion bikeshedding, but I don't want to get into an
argument about something so petty :)
>
> >
> > No wait - did we _test_ yellow in the way I wanted...
> >
> > I mean for me this isn't a big deal - we declare the defines here, it makes
> > sense to have a very very simple inline function.
> >
> > It's not like userspace is overly hurt by this...
> >
> > Also I did explain there's no obvious header to put this in in the kernel
> > and I'm not introducing one sorry.
> >
> > ANyway if you guys feel strong enough about this, I'll respin again and
> > just open-code this trivial check where it's used.
>
> No strong feelings, just hoping to help make a choice that gets you
> closer to getting your patches committed.
I mean, you are saying I am breaking things and implying the series is
blocked on this, that sounds like a strong opinion, but again I'm not going
to argue.
As with the requirement that I, only for my part of the change, must fix up
test header import, while I disagree I should be doing the fix, I did it
anyway as I am accommodating and reasonable.
So fine - I'll respin and just open-code this as it's trivial and there's
no (other) sensible place to put it anyway.
A P.S. though - a very NOT theoretical issue with userspace is the import
of linux/fcntl.h in pidfd.h which seems to me to have been imported solely
for the kernel's sake.
A gentle suggestion (it seems I can't win - gentle suggestions are ignored,
tongue-in-cheek parody is taken to be mean... but anyway) is to do
something like:
#ifdef __KERNEL__
#include <linux/fcntl.h>
#else
#include <fcntl.h>
#endif
At the top of the pidfd.h header. This must surely sting a _lot_ of people
in userland otherwise.
But this is out of scope for this change.
^ permalink raw reply
* Re: [PATCH v5 2/5] pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process
From: John Hubbard @ 2024-10-25 20:31 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Pedro Falcato, Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, linux-kselftest, linux-mm,
linux-fsdevel, linux-api, linux-kernel, Oliver Sang
In-Reply-To: <31dd0e52-9868-4cb4-aec6-d8749cdd2560@lucifer.local>
On 10/25/24 12:49 PM, Lorenzo Stoakes wrote:
> On Fri, Oct 25, 2024 at 11:44:34AM -0700, John Hubbard wrote:
>> On 10/25/24 11:38 AM, Pedro Falcato wrote:
>>> On Fri, Oct 25, 2024 at 6:41 PM John Hubbard <jhubbard@nvidia.com> wrote:
...
>>> That seems to only apply to the kernel internally, uapi headers are
>>
>> Yes.
>>
>>> included from userspace too (-std=c89 -pedantic doesn't know what a
>>> gnu extension is). And uapi headers _generally_ keep to defining
>>> constants and structs, nothing more.
>>
>> OK
>
> Because a lot of people using -ANSI- C89 are importing a very new linux
> feature header.
I'll admit to being easily cowed by "you're breaking userspace" arguments.
Even when they start to get rather absurd. Because I can't easily tell where
the line is.
Maybe "-std=c89 -pedantic" is on the other side of the line. I'd like it
to be! :)
>
> And let's ignore the hundreds of existing uses... OK.
>
> The rules, unstated anywhere, are that we must support 1972-era C in an
> optional header for a feature available only in new kernels because
> somebody somewhere is using a VAX-11 and gosh darn it they can't change
> their toolchain!
>
> And you had better make sure you don't wear out those tape drums...
>
>>
>>> I don't know what the guidelines for uapi headers are nowadays, but we
>>> generally want to not break userspace.
>>>
>>>>
>>>> I think it's quite clear at this point, that we should not hold up new
>>>> work, based on concerns about handling the inline keyword, nor about
>>>> C89.
>>>
>>> Right, but the correct solution is probably to move
>>> pidfd_is_self_sentinel to some other place, since it's not even
>>> supposed to be used by userspace (it's semantically useless to
>>> userspace, and it's only two users are in the kernel, kernel/pid.c and
>>> exit.c).
>>>
>>
>> Yes, if userspace absolutely doesn't need nor want this, then putting
>> it in a non-uapi header does sound like the right move.
>
> The bike shed should be blue! Wait no no, it should be red... Hang on
> yellow yes! Yellow's great!
Putting a header in the right location, so as to avoid breakage here or
there, is not bikeshedding. Sorry.
>
> No wait - did we _test_ yellow in the way I wanted...
>
> I mean for me this isn't a big deal - we declare the defines here, it makes
> sense to have a very very simple inline function.
>
> It's not like userspace is overly hurt by this...
>
> Also I did explain there's no obvious header to put this in in the kernel
> and I'm not introducing one sorry.
>
> ANyway if you guys feel strong enough about this, I'll respin again and
> just open-code this trivial check where it's used.
No strong feelings, just hoping to help make a choice that gets you
closer to getting your patches committed.
thanks,
--
John Hubbard
^ permalink raw reply
* Re: [PATCH v5 2/5] pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process
From: Lorenzo Stoakes @ 2024-10-25 19:49 UTC (permalink / raw)
To: John Hubbard
Cc: Pedro Falcato, Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, linux-kselftest, linux-mm,
linux-fsdevel, linux-api, linux-kernel, Oliver Sang
In-Reply-To: <e5ac648b-88d7-4fa6-8eb4-d061a4b2baac@nvidia.com>
On Fri, Oct 25, 2024 at 11:44:34AM -0700, John Hubbard wrote:
> On 10/25/24 11:38 AM, Pedro Falcato wrote:
> > On Fri, Oct 25, 2024 at 6:41 PM John Hubbard <jhubbard@nvidia.com> wrote:
> > >
> > > On 10/25/24 5:50 AM, Pedro Falcato wrote:
> > > > On Fri, Oct 25, 2024 at 10:41 AM Lorenzo Stoakes
> > > > <lorenzo.stoakes@oracle.com> wrote:
> > > ...
> > > > > +static inline int pidfd_is_self_sentinel(pid_t pid)
> > > > > +{
> > > > > + return pid == PIDFD_SELF_THREAD || pid == PIDFD_SELF_THREAD_GROUP;
> > > > > +}
> > > >
> > > > Do we want this in the uapi header? Even if this is useful, it might
> > > > come with several drawbacks such as breaking scripts that parse kernel
> > > > headers (and a quick git grep suggests we do have static inlines in
> > > > headers, but in rather obscure ones) and breaking C89:
> > > >
> > >
> > > Let's please not say "C89" anymore, we've moved on! :)
> > >
> > > The notes in [1], which is now nearly 2.5 years old, discuss the move to
> > > C11, and specifically how to handle the inline keyword.
> >
> > That seems to only apply to the kernel internally, uapi headers are
>
> Yes.
>
> > included from userspace too (-std=c89 -pedantic doesn't know what a
> > gnu extension is). And uapi headers _generally_ keep to defining
> > constants and structs, nothing more.
>
> OK
Because a lot of people using -ANSI- C89 are importing a very new linux
feature header.
And let's ignore the hundreds of existing uses... OK.
The rules, unstated anywhere, are that we must support 1972-era C in an
optional header for a feature available only in new kernels because
somebody somewhere is using a VAX-11 and gosh darn it they can't change
their toolchain!
And you had better make sure you don't wear out those tape drums...
>
> > I don't know what the guidelines for uapi headers are nowadays, but we
> > generally want to not break userspace.
> >
> > >
> > > I think it's quite clear at this point, that we should not hold up new
> > > work, based on concerns about handling the inline keyword, nor about
> > > C89.
> >
> > Right, but the correct solution is probably to move
> > pidfd_is_self_sentinel to some other place, since it's not even
> > supposed to be used by userspace (it's semantically useless to
> > userspace, and it's only two users are in the kernel, kernel/pid.c and
> > exit.c).
> >
>
> Yes, if userspace absolutely doesn't need nor want this, then putting
> it in a non-uapi header does sound like the right move.
The bike shed should be blue! Wait no no, it should be red... Hang on
yellow yes! Yellow's great!
No wait - did we _test_ yellow in the way I wanted...
I mean for me this isn't a big deal - we declare the defines here, it makes
sense to have a very very simple inline function.
It's not like userspace is overly hurt by this...
Also I did explain there's no obvious header to put this in in the kernel
and I'm not introducing one sorry.
ANyway if you guys feel strong enough about this, I'll respin again and
just open-code this trivial check where it's used.
>
>
> thanks,
> --
> John Hubbard
>
^ permalink raw reply
* Re: [PATCH 2/6] futex: Implement FUTEX2_NUMA
From: Christoph Lameter (Ampere) @ 2024-10-25 19:28 UTC (permalink / raw)
To: Peter Zijlstra
Cc: tglx, linux-kernel, mingo, dvhart, dave, andrealmeid,
Andrew Morton, urezki, hch, lstoakes, Arnd Bergmann, linux-api,
linux-mm, linux-arch, malteskarupke, llong
In-Reply-To: <20241025093944.485691531@infradead.org>
On Fri, 25 Oct 2024, Peter Zijlstra wrote:
> Extend the futex2 interface to be numa aware.
>
> When FUTEX2_NUMA is specified for a futex, the user value is extended
> to two words (of the same size). The first is the user value we all
> know, the second one will be the node to place this futex on.
>
> struct futex_numa_32 {
> u32 val;
> u32 node;
> };
>
> When node is set to ~0, WAIT will set it to the current node_id such
> that WAKE knows where to find it. If userspace corrupts the node value
> between WAIT and WAKE, the futex will not be found and no wakeup will
> happen.
>
> When FUTEX2_NUMA is not set, the node is simply an extention of the
> hash, such that traditional futexes are still interleaved over the
> nodes.
Would it be possible to follow the NUMA memory policy set up for a task
when making these decisions? We may not need a separate FUTEX2_NUMA
option. There are supportive functions in mm/mempolicy.c that will yield
a node for the futex logic to use.
See f.e. linux/include/uapi/mempolicy.h for the types of memory policy
that can be set for a task in current->mempolicy.
MPOL_DEFAULT get local memory / use system default policy
MPOL_INTERLEAVE interleaved over nodes
MPOL_BIND use the node specified in the task policy.
MPOL_LOCAL get_local_memory
etc.
You will get a page or objects with the correct node by calling
alloc_pages() or kmalloc without GFP_THISNODE.
If you just need the node to use then use
mempolicy_slab_node()
and assign that to the node of the futex.
The function will determine which node to use depending on the active
memory policy.
^ permalink raw reply
* Re: [PATCH v1 11/14] futex: Implement FUTEX2_NUMA
From: Christoph Lameter (Ampere) @ 2024-10-25 19:36 UTC (permalink / raw)
To: Peter Zijlstra
Cc: tglx, axboe, linux-kernel, mingo, dvhart, dave, andrealmeid,
Andrew Morton, urezki, hch, lstoakes, Arnd Bergmann, linux-api,
linux-mm, linux-arch, malteskarupke
In-Reply-To: <20241025085815.GG14555@noisy.programming.kicks-ass.net>
Sorry saw this after the other email.
On Fri, 25 Oct 2024, Peter Zijlstra wrote:
> > Could we follow NUMA policies like with other metadata allocations during
> > systen call processing?
>
> I had a quick look at this, and since the mempolicy stuff is per vma,
> and we don't have the vma, this is going to be terribly expensive --
> mmap_lock and all that.
There is a memory policy for the task as a whole that is used for slab
allocations and allocations that are not vma bound in current->mempolicy.
Use that.
> Using memory policies is probably okay -- but still risky, since you get
> the extra failure case where if you change the mempolicy between WAIT
> and WAKE things will not match and sadness happens, but that *SHOULD*
> hopefully not happen a lot. Mempolicies are typically fairly static.
Right.
> > That way the placement of the futex can be controlled by the tasks memory
> > policy. We could skip the FUTEX2_NUMA option.
>
> That doesn't work. If we don't have storage for the node across
> WAIT/WAKE, then the node must be deterministic per futex_hash().
> Otherwise wake has no chance of finding the entry.
You can get a node number following the current task mempolicy by calling
mempolicy_slab_node() and keep using that node for the future.
It is also possible to check if the policy is interleave and then follow
the distributed hash scheme.
> The current scheme where we determine node based on hash bits is fully
> deterministic and WAIT/WAKE will agree on which node-hash to use. The
> interleave is no worse than the global hash today -- OTOH it also isn't
> better.
This is unexpected strange behavior for those familiar with NUMA. We have
tools to set memory policies for tasks and those policies should be used
throughout.
^ permalink raw reply
* Re: [PATCH v3 1/5] mm: pagewalk: add the ability to install PTEs
From: David Hildenbrand @ 2024-10-25 19:08 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Vlastimil Babka, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Matt Turner, Thomas Bogendoerfer, James E . J . Bottomley,
Helge Deller, Chris Zankel, Max Filippov, Arnd Bergmann,
linux-alpha, linux-mips, linux-parisc, linux-arch, Shuah Khan,
Christian Brauner, linux-kselftest, Sidhartha Kumar, Jeff Xu,
Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <e62ef664-1c71-48e0-a695-6a53b6d46922@lucifer.local>
>>
>> We already discussed in the past that we need a better and more efficient
>> way to walk page tables. I have part of that on my TODO list, but I'm
>> getting distracted.
>
> Yes I remember an LSF session on this, it's a really obvious area of improvement
> that stands out at the moment for sure.
>
> Having worked several 12+ hour days in a row though recently I can relate to
> workload making this difficult though :)
Yes :)
>
>>
>> *Inserting* (not walking/modifying existing things as most users to) as done
>> in this patch is slightly different though, likely "on thing that fits all"
>> will not apply to all page table walker user cases.
>
> Yeah, there's also replace scenarios which then have to do egregious amounts of
> work to make sure we do everything right, in fact there's duplicates of this in
> mm/madvise.c *grumble grumble*.
>
>>
>> --
>> Cheers,
>>
>> David / dhildenb
>>
>
> OK so I guess I'll hold off my TODOs on this as you are looking in this area and
> I trust you :)
It will probably take me a while until I get to it, though. I'd focus on
walking (and batching what we can) first, then on top modifying existing
entries.
The "install something where there is nothing yet" (incl. populating
fresh page tables etc.) case probably deserves a separate "walker".
If you end up having spare cycles and want to sync on a possible design
for some part of that bigger task -- removing the old pagewalk logic --
please do reach out! :)
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v5 2/5] pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process
From: John Hubbard @ 2024-10-25 18:44 UTC (permalink / raw)
To: Pedro Falcato
Cc: Lorenzo Stoakes, Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, linux-kselftest, linux-mm,
linux-fsdevel, linux-api, linux-kernel, Oliver Sang
In-Reply-To: <CAKbZUD0fxczjSJo34MnWRNT4M6HTfWN0DRXr9CFe_+cKJW_mog@mail.gmail.com>
On 10/25/24 11:38 AM, Pedro Falcato wrote:
> On Fri, Oct 25, 2024 at 6:41 PM John Hubbard <jhubbard@nvidia.com> wrote:
>>
>> On 10/25/24 5:50 AM, Pedro Falcato wrote:
>>> On Fri, Oct 25, 2024 at 10:41 AM Lorenzo Stoakes
>>> <lorenzo.stoakes@oracle.com> wrote:
>> ...
>>>> +static inline int pidfd_is_self_sentinel(pid_t pid)
>>>> +{
>>>> + return pid == PIDFD_SELF_THREAD || pid == PIDFD_SELF_THREAD_GROUP;
>>>> +}
>>>
>>> Do we want this in the uapi header? Even if this is useful, it might
>>> come with several drawbacks such as breaking scripts that parse kernel
>>> headers (and a quick git grep suggests we do have static inlines in
>>> headers, but in rather obscure ones) and breaking C89:
>>>
>>
>> Let's please not say "C89" anymore, we've moved on! :)
>>
>> The notes in [1], which is now nearly 2.5 years old, discuss the move to
>> C11, and specifically how to handle the inline keyword.
>
> That seems to only apply to the kernel internally, uapi headers are
Yes.
> included from userspace too (-std=c89 -pedantic doesn't know what a
> gnu extension is). And uapi headers _generally_ keep to defining
> constants and structs, nothing more.
OK
> I don't know what the guidelines for uapi headers are nowadays, but we
> generally want to not break userspace.
>
>>
>> I think it's quite clear at this point, that we should not hold up new
>> work, based on concerns about handling the inline keyword, nor about
>> C89.
>
> Right, but the correct solution is probably to move
> pidfd_is_self_sentinel to some other place, since it's not even
> supposed to be used by userspace (it's semantically useless to
> userspace, and it's only two users are in the kernel, kernel/pid.c and
> exit.c).
>
Yes, if userspace absolutely doesn't need nor want this, then putting
it in a non-uapi header does sound like the right move.
thanks,
--
John Hubbard
^ permalink raw reply
* Re: [PATCH v5 2/5] pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process
From: Pedro Falcato @ 2024-10-25 18:38 UTC (permalink / raw)
To: John Hubbard
Cc: Lorenzo Stoakes, Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, linux-kselftest, linux-mm,
linux-fsdevel, linux-api, linux-kernel, Oliver Sang
In-Reply-To: <828674d9-e862-4438-86b6-61977f4cf3b5@nvidia.com>
On Fri, Oct 25, 2024 at 6:41 PM John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 10/25/24 5:50 AM, Pedro Falcato wrote:
> > On Fri, Oct 25, 2024 at 10:41 AM Lorenzo Stoakes
> > <lorenzo.stoakes@oracle.com> wrote:
> ...
> >> +static inline int pidfd_is_self_sentinel(pid_t pid)
> >> +{
> >> + return pid == PIDFD_SELF_THREAD || pid == PIDFD_SELF_THREAD_GROUP;
> >> +}
> >
> > Do we want this in the uapi header? Even if this is useful, it might
> > come with several drawbacks such as breaking scripts that parse kernel
> > headers (and a quick git grep suggests we do have static inlines in
> > headers, but in rather obscure ones) and breaking C89:
> >
>
> Let's please not say "C89" anymore, we've moved on! :)
>
> The notes in [1], which is now nearly 2.5 years old, discuss the move to
> C11, and specifically how to handle the inline keyword.
That seems to only apply to the kernel internally, uapi headers are
included from userspace too (-std=c89 -pedantic doesn't know what a
gnu extension is). And uapi headers _generally_ keep to defining
constants and structs, nothing more.
I don't know what the guidelines for uapi headers are nowadays, but we
generally want to not break userspace.
>
> I think it's quite clear at this point, that we should not hold up new
> work, based on concerns about handling the inline keyword, nor about
> C89.
Right, but the correct solution is probably to move
pidfd_is_self_sentinel to some other place, since it's not even
supposed to be used by userspace (it's semantically useless to
userspace, and it's only two users are in the kernel, kernel/pid.c and
exit.c).
--
Pedro
^ permalink raw reply
* Re: [PATCH 2/6] futex: Implement FUTEX2_NUMA
From: Davidlohr Bueso @ 2024-10-25 18:30 UTC (permalink / raw)
To: Peter Zijlstra
Cc: tglx, linux-kernel, mingo, dvhart, andrealmeid, Andrew Morton,
urezki, hch, lstoakes, Arnd Bergmann, linux-api, linux-mm,
linux-arch, malteskarupke, cl, llong
In-Reply-To: <20241025093944.485691531@infradead.org>
On Fri, 25 Oct 2024, Peter Zijlstra wrote:\n
> static int __init futex_init(void)
> {
>- unsigned int futex_shift;
>- unsigned long i;
>+ unsigned int order, n;
>+ unsigned long size, i;
>
> #ifdef CONFIG_BASE_SMALL
> futex_hashsize = 16;
> #else
>- futex_hashsize = roundup_pow_of_two(256 * num_possible_cpus());
>+ futex_hashsize = 256 * num_possible_cpus();
>+ futex_hashsize /= num_possible_nodes();
>+ futex_hashsize = roundup_pow_of_two(futex_hashsize);
> #endif
>+ futex_hashshift = ilog2(futex_hashsize);
>+ size = sizeof(struct futex_hash_bucket) * futex_hashsize;
>+ order = get_order(size);
>+
>+ for_each_node(n) {
Probably want to skip nodes that don't have CPUs, those will never
have the remote for .node value.
^ permalink raw reply
* Re: [PATCH v3 1/5] mm: pagewalk: add the ability to install PTEs
From: Vlastimil Babka @ 2024-10-25 18:13 UTC (permalink / raw)
To: Lorenzo Stoakes, Andrew Morton
Cc: Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
Paul E . McKenney, Jann Horn, David Hildenbrand, linux-mm,
linux-kernel, Muchun Song, Richard Henderson, Matt Turner,
Thomas Bogendoerfer, James E . J . Bottomley, Helge Deller,
Chris Zankel, Max Filippov, Arnd Bergmann, linux-alpha,
linux-mips, linux-parisc, linux-arch, Shuah Khan,
Christian Brauner, linux-kselftest, Sidhartha Kumar, Jeff Xu,
Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <9be732fd0e897453116b433fe2f468ef7795602e.1729699916.git.lorenzo.stoakes@oracle.com>
On 10/23/24 18:24, Lorenzo Stoakes wrote:
> The existing generic pagewalk logic permits the walking of page tables,
> invoking callbacks at individual page table levels via user-provided
> mm_walk_ops callbacks.
>
> This is useful for traversing existing page table entries, but precludes
> the ability to establish new ones.
>
> Existing mechanism for performing a walk which also installs page table
> entries if necessary are heavily duplicated throughout the kernel, each
> with semantic differences from one another and largely unavailable for use
> elsewhere.
>
> Rather than add yet another implementation, we extend the generic pagewalk
> logic to enable the installation of page table entries by adding a new
> install_pte() callback in mm_walk_ops. If this is specified, then upon
> encountering a missing page table entry, we allocate and install a new one
> and continue the traversal.
>
> If a THP huge page is encountered at either the PMD or PUD level we split
> it only if there are ops->pte_entry() (or ops->pmd_entry at PUD level),
> otherwise if there is only an ops->install_pte(), we avoid the unnecessary
> split.
>
> We do not support hugetlb at this stage.
>
> If this function returns an error, or an allocation fails during the
> operation, we abort the operation altogether. It is up to the caller to
> deal appropriately with partially populated page table ranges.
>
> If install_pte() is defined, the semantics of pte_entry() change - this
> callback is then only invoked if the entry already exists. This is a useful
> property, as it allows a caller to handle existing PTEs while installing
> new ones where necessary in the specified range.
>
> If install_pte() is not defined, then there is no functional difference to
> this patch, so all existing logic will work precisely as it did before.
>
> As we only permit the installation of PTEs where a mapping does not already
> exist there is no need for TLB management, however we do invoke
> update_mmu_cache() for architectures which require manual maintenance of
> mappings for other CPUs.
>
> We explicitly do not allow the existing page walk API to expose this
> feature as it is dangerous and intended for internal mm use only. Therefore
> we provide a new walk_page_range_mm() function exposed only to
> mm/internal.h.
>
> Reviewed-by: Jann Horn <jannh@google.com>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
Just a small subjective suggestion in case you agree and there's a respin or
followups:
> @@ -109,18 +131,19 @@ static int walk_pmd_range(pud_t *pud, unsigned long addr, unsigned long end,
>
> if (walk->action == ACTION_AGAIN)
> goto again;
> -
> - /*
> - * Check this here so we only break down trans_huge
> - * pages when we _need_ to
> - */
> - if ((!walk->vma && (pmd_leaf(*pmd) || !pmd_present(*pmd))) ||
> - walk->action == ACTION_CONTINUE ||
> - !(ops->pte_entry))
> + if (walk->action == ACTION_CONTINUE)
> continue;
> + if (!ops->install_pte && !ops->pte_entry)
> + continue; /* Nothing to do. */
> + if (!ops->pte_entry && ops->install_pte &&
> + pmd_present(*pmd) &&
> + (pmd_trans_huge(*pmd) || pmd_devmap(*pmd)))
> + continue; /* Avoid unnecessary split. */
Much better now, thanks, but maybe the last 2 parts could be:
if (!ops->pte_entry) {
if (!ops->install_pte)
continue; /* Nothing to do. */
else if (pmd_present(*pmd)
&& (pmd_trans_huge(*pmd) || pmd_devmap(*pmd)))
continue; /* Avoid unnecessary split. */
}
Or at least put !ops->pte_entry first in both conditions?
> if (walk->vma)
> split_huge_pmd(walk->vma, pmd, addr);
> + else if (pmd_leaf(*pmd) || !pmd_present(*pmd))
> + continue; /* Nothing to do. */
>
> err = walk_pte_range(pmd, addr, next, walk);
> if (err)
> @@ -148,11 +171,14 @@ static int walk_pud_range(p4d_t *p4d, unsigned long addr, unsigned long end,
> again:
> next = pud_addr_end(addr, end);
> if (pud_none(*pud)) {
> - if (ops->pte_hole)
> + if (ops->install_pte)
> + err = __pmd_alloc(walk->mm, pud, addr);
> + else if (ops->pte_hole)
> err = ops->pte_hole(addr, next, depth, walk);
> if (err)
> break;
> - continue;
> + if (!ops->install_pte)
> + continue;
> }
>
> walk->action = ACTION_SUBTREE;
> @@ -164,14 +190,20 @@ static int walk_pud_range(p4d_t *p4d, unsigned long addr, unsigned long end,
>
> if (walk->action == ACTION_AGAIN)
> goto again;
> -
> - if ((!walk->vma && (pud_leaf(*pud) || !pud_present(*pud))) ||
> - walk->action == ACTION_CONTINUE ||
> - !(ops->pmd_entry || ops->pte_entry))
> + if (walk->action == ACTION_CONTINUE)
> continue;
> + if (!ops->install_pte && !ops->pte_entry && !ops->pmd_entry)
> + continue; /* Nothing to do. */
> + if (!ops->pmd_entry && !ops->pte_entry && ops->install_pte &&
> + pud_present(*pud) &&
> + (pud_trans_huge(*pud) || pud_devmap(*pud)))
> + continue; /* Avoid unnecessary split. */
Ditto.
Thanks!
^ permalink raw reply
* Re: [PATCH v5 2/5] pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process
From: John Hubbard @ 2024-10-25 17:41 UTC (permalink / raw)
To: Pedro Falcato, Lorenzo Stoakes
Cc: Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, linux-kselftest, linux-mm,
linux-fsdevel, linux-api, linux-kernel, Oliver Sang
In-Reply-To: <CAKbZUD2ZB+U3GKJftfRH_2ejNja26v38OLVE2Lbfn_1KSOKhNQ@mail.gmail.com>
On 10/25/24 5:50 AM, Pedro Falcato wrote:
> On Fri, Oct 25, 2024 at 10:41 AM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
...
>> +static inline int pidfd_is_self_sentinel(pid_t pid)
>> +{
>> + return pid == PIDFD_SELF_THREAD || pid == PIDFD_SELF_THREAD_GROUP;
>> +}
>
> Do we want this in the uapi header? Even if this is useful, it might
> come with several drawbacks such as breaking scripts that parse kernel
> headers (and a quick git grep suggests we do have static inlines in
> headers, but in rather obscure ones) and breaking C89:
>
Let's please not say "C89" anymore, we've moved on! :)
The notes in [1], which is now nearly 2.5 years old, discuss the move to
C11, and specifically how to handle the inline keyword.
I think it's quite clear at this point, that we should not hold up new
work, based on concerns about handling the inline keyword, nor about
C89.
[1] commit e8c07082a810 ("Kbuild: move to -std=gnu11")
thanks,
--
John Hubbard
> <source>:8:8: error: unknown type name 'inline'
> 8 | static inline int pidfd_is_self_sentinel(pid_t pid)
>
> :)
>
^ permalink raw reply
* Re: [PATCH v3 3/5] mm: madvise: implement lightweight guard page mechanism
From: Lorenzo Stoakes @ 2024-10-25 17:12 UTC (permalink / raw)
To: Andrew Morton
Cc: Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
Vlastimil Babka, Paul E . McKenney, Jann Horn, David Hildenbrand,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Matt Turner, Thomas Bogendoerfer, James E . J . Bottomley,
Helge Deller, Chris Zankel, Max Filippov, Arnd Bergmann,
linux-alpha, linux-mips, linux-parisc, linux-arch, Shuah Khan,
Christian Brauner, linux-kselftest, Sidhartha Kumar, Jeff Xu,
Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <415da1e6c5828d96db3af480d243a7f68ccabf6d.1729699916.git.lorenzo.stoakes@oracle.com>
On Wed, Oct 23, 2024 at 05:24:40PM +0100, Lorenzo Stoakes wrote:
> Implement a new lightweight guard page feature, that is regions of userland
> virtual memory that, when accessed, cause a fatal signal to arise.
<snip>
Hi Andrew - Could you apply the below fix-patch? I realise we must handle
fatal signals and conditional rescheduling in the vector_madvise() special
case.
Thanks!
----8<----
From 546d7e1831c71599fc733d589e0d75f52e84826d Mon Sep 17 00:00:00 2001
From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Date: Fri, 25 Oct 2024 18:05:48 +0100
Subject: [PATCH] mm: yield on fatal signal/cond_sched() in vector_madvise()
While we have to treat -ERESTARTNOINTR specially here as we are looping
through a vector of operations and can't simply restart the entire
operation, we mustn't hold up fatal signals or RT kernels.
---
mm/madvise.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/mm/madvise.c b/mm/madvise.c
index 48eba25e25fe..127aa5d86656 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1713,8 +1713,14 @@ static ssize_t vector_madvise(struct mm_struct *mm, struct iov_iter *iter,
* we have already rescinded locks, it should be no problem to
* simply try again.
*/
- if (ret == -ERESTARTNOINTR)
+ if (ret == -ERESTARTNOINTR) {
+ if (fatal_signal_pending(current)) {
+ ret = -EINTR;
+ break;
+ }
+ cond_resched();
continue;
+ }
if (ret < 0)
break;
iov_iter_advance(iter, iter_iov_len(iter));
--
2.47.0
^ permalink raw reply related
* Re: [PATCH 1/6] mm: Add vmalloc_huge_node()
From: Davidlohr Bueso @ 2024-10-25 16:00 UTC (permalink / raw)
To: Peter Zijlstra
Cc: tglx, linux-kernel, mingo, dvhart, andrealmeid, Andrew Morton,
urezki, hch, lstoakes, Arnd Bergmann, linux-api, linux-mm,
linux-arch, malteskarupke, cl, llong, Christoph Hellwig
In-Reply-To: <20241025093944.372391936@infradead.org>
On Fri, 25 Oct 2024, Peter Zijlstra wrote:
>To enable node specific hash-tables.
>
>Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
^ permalink raw reply
* Re: [PATCH v5 2/5] pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process
From: Lorenzo Stoakes @ 2024-10-25 13:08 UTC (permalink / raw)
To: Pedro Falcato
Cc: Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, linux-kselftest, linux-mm,
linux-fsdevel, linux-api, linux-kernel, Oliver Sang, John Hubbard
In-Reply-To: <CAKbZUD2ZB+U3GKJftfRH_2ejNja26v38OLVE2Lbfn_1KSOKhNQ@mail.gmail.com>
On Fri, Oct 25, 2024 at 01:50:12PM +0100, Pedro Falcato wrote:
> On Fri, Oct 25, 2024 at 10:41 AM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > It is useful to be able to utilise the pidfd mechanism to reference the
> > current thread or process (from a userland point of view - thread group
> > leader from the kernel's point of view).
> >
> > Therefore introduce PIDFD_SELF_THREAD to refer to the current thread, and
> > PIDFD_SELF_THREAD_GROUP to refer to the current thread group leader.
> >
> > For convenience and to avoid confusion from userland's perspective we alias
> > these:
> >
> > * PIDFD_SELF is an alias for PIDFD_SELF_THREAD - This is nearly always what
> > the user will want to use, as they would find it surprising if for
> > instance fd's were unshared()'d and they wanted to invoke pidfd_getfd()
> > and that failed.
> >
> > * PIDFD_SELF_PROCESS is an alias for PIDFD_SELF_THREAD_GROUP - Most users
> > have no concept of thread groups or what a thread group leader is, and
> > from userland's perspective and nomenclature this is what userland
> > considers to be a process.
> >
> > Due to the refactoring of the central __pidfd_get_pid() function we can
> > implement this functionality centrally, providing the use of this sentinel
> > in most functionality which utilises pidfd's.
> >
> > We need to explicitly adjust kernel_waitid_prepare() to permit this (though
> > it wouldn't really make sense to use this there, we provide the ability for
> > consistency).
> >
> > We explicitly disallow use of this in setns(), which would otherwise have
> > required explicit custom handling, as it doesn't make sense to set the
> > current calling thread to join the namespace of itself.
> >
> > As the callers of pidfd_get_pid() expect an increased reference count on
> > the pid we do so in the self case, reducing churn and avoiding any breakage
> > from existing logic which decrements this reference count.
> >
> > This change implicitly provides PIDFD_SELF_* support in the waitid(P_PIDFS,
> > ...), process_madvise(), process_mrelease(), pidfd_send_signal(), and
> > pidfd_getfd() system calls.
> >
> > Things such as polling a pidfs and general fd operations are not supported,
> > this strictly provides the sentinel for APIs which explicitly accept a
> > pidfd.
> >
> > Reviewed-by: Shakeel Butt <shakeel.butt@linux.dev>
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > ---
> > include/linux/pid.h | 8 ++++--
> > include/uapi/linux/pidfd.h | 15 +++++++++++
> > kernel/exit.c | 3 ++-
> > kernel/nsproxy.c | 1 +
> > kernel/pid.c | 51 ++++++++++++++++++++++++--------------
> > 5 files changed, 57 insertions(+), 21 deletions(-)
> >
> > diff --git a/include/linux/pid.h b/include/linux/pid.h
> > index d466890e1b35..3b2ac7567a88 100644
> > --- a/include/linux/pid.h
> > +++ b/include/linux/pid.h
> > @@ -78,11 +78,15 @@ struct file;
> > * __pidfd_get_pid() - Retrieve a pid associated with the specified pidfd.
> > *
> > * @pidfd: The pidfd whose pid we want, or the fd of a /proc/<pid> file if
> > - * @alloc_proc is also set.
> > + * @alloc_proc is also set, or PIDFD_SELF_* to refer to the current
> > + * thread or thread group leader.
> > * @allow_proc: If set, then an fd of a /proc/<pid> file can be passed instead
> > * of a pidfd, and this will be used to determine the pid.
> > +
> > * @flags: Output variable, if non-NULL, then the file->f_flags of the
> > - * pidfd will be set here.
> > + * pidfd will be set here or If PIDFD_SELF_THREAD is set, this is
> > + * set to PIDFD_THREAD, otherwise if PIDFD_SELF_THREAD_GROUP then
> > + * this is set to zero.
> > *
> > * Returns: If successful, the pid associated with the pidfd, otherwise an
> > * error.
> > diff --git a/include/uapi/linux/pidfd.h b/include/uapi/linux/pidfd.h
> > index 565fc0629fff..0ca2ebf906fd 100644
> > --- a/include/uapi/linux/pidfd.h
> > +++ b/include/uapi/linux/pidfd.h
> > @@ -29,4 +29,19 @@
> > #define PIDFD_GET_USER_NAMESPACE _IO(PIDFS_IOCTL_MAGIC, 9)
> > #define PIDFD_GET_UTS_NAMESPACE _IO(PIDFS_IOCTL_MAGIC, 10)
> >
> > +/*
> > + * Special sentinel values which can be used to refer to the current thread or
> > + * thread group leader (which from a userland perspective is the process).
> > + */
> > +#define PIDFD_SELF PIDFD_SELF_THREAD
> > +#define PIDFD_SELF_PROCESS PIDFD_SELF_THREAD_GROUP
> > +
> > +#define PIDFD_SELF_THREAD -100 /* Current thread. */
>
> This conflicts with AT_FDCWD, might be worth changing?
>
> > +#define PIDFD_SELF_THREAD_GROUP -200 /* Current thread group leader. */
>
> We might want to pick some range outside of the negative errno space
> (-4096 IIRC), since we have plenty of values to pick from (2^31 at
> least).
This is entirely up to Christian, I used the values he suggested in
review. But I agree we should probably find one that doesn't conflict and
is outside that range.
>
> > +static inline int pidfd_is_self_sentinel(pid_t pid)
> > +{
> > + return pid == PIDFD_SELF_THREAD || pid == PIDFD_SELF_THREAD_GROUP;
> > +}
>
> Do we want this in the uapi header? Even if this is useful, it might
> come with several drawbacks such as breaking scripts that parse kernel
> headers (and a quick git grep suggests we do have static inlines in
> headers, but in rather obscure ones) and breaking C89:
>
> <source>:8:8: error: unknown type name 'inline'
> 8 | static inline int pidfd_is_self_sentinel(pid_t pid)
>
> :)
It doesn't really make sense to put it anywhere else I don't think.
I'm not sure 'support compilers that don't know what inline is' is a
requirement for UAPI. Nor do I suspect people using such strict ansi-c89
compilers will be importing linux/pidfd.h... :)
Also:
[~/kerndev/kernels/mm/include/uapi/linux]$ ag inline | wc -l
382
I mean yeah 'obscure' or not it seems this is an acceptable thing to do :)
>
> > +
> > #endif /* _UAPI_LINUX_PIDFD_H */
> > diff --git a/kernel/exit.c b/kernel/exit.c
> > index 619f0014c33b..3eb20f8252ee 100644
> > --- a/kernel/exit.c
> > +++ b/kernel/exit.c
> > @@ -71,6 +71,7 @@
> > #include <linux/user_events.h>
> > #include <linux/uaccess.h>
> >
> > +#include <uapi/linux/pidfd.h>
> > #include <uapi/linux/wait.h>
> >
> > #include <asm/unistd.h>
> > @@ -1739,7 +1740,7 @@ int kernel_waitid_prepare(struct wait_opts *wo, int which, pid_t upid,
> > break;
> > case P_PIDFD:
> > type = PIDTYPE_PID;
> > - if (upid < 0)
> > + if (upid < 0 && !pidfd_is_self_sentinel(upid))
> > return -EINVAL;
> >
> > pid = pidfd_get_pid(upid, &f_flags);
> > diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> > index dc952c3b05af..d239f7eeaa1f 100644
> > --- a/kernel/nsproxy.c
> > +++ b/kernel/nsproxy.c
> > @@ -550,6 +550,7 @@ SYSCALL_DEFINE2(setns, int, fd, int, flags)
> > struct nsset nsset = {};
> > int err = 0;
> >
> > + /* If fd is PIDFD_SELF_*, implicitly fail here, as invalid. */
> > if (!fd_file(f))
> > return -EBADF;
> >
> > diff --git a/kernel/pid.c b/kernel/pid.c
> > index 94c97559e5c5..8742157b36f8 100644
> > --- a/kernel/pid.c
> > +++ b/kernel/pid.c
> > @@ -535,33 +535,48 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
> > }
> > EXPORT_SYMBOL_GPL(find_ge_pid);
> >
> > +static struct pid *pidfd_get_pid_self(unsigned int pidfd, unsigned int *flags)
> > +{
> > + bool is_thread = pidfd == PIDFD_SELF_THREAD;
> > + enum pid_type type = is_thread ? PIDTYPE_PID : PIDTYPE_TGID;
> > + struct pid *pid = *task_pid_ptr(current, type);
> > +
> > + /* The caller expects an elevated reference count. */
> > + get_pid(pid);
>
> It would be really really nice to avoid the get here, but I imagine
> it'll take some refactoring around put_pid's?
I cover this in the commit message and have addressed it on review already,
but to risk repeating myself :)
Yes it'd be nice, but then you would have to make sure you _always_
unpinned correctly _everywhere_ from here on in, and it makes the behaviour
different for these self modes.
You'd need to change how everyone everywhere puts and... yeah. It's not a
big deal to do a useless ref inc here I don't think, eliminates a class of
bug, and importantly it keeps behaviour identical to if you do a self-pidfd
in the 'manual' way.
I equally dislike this aspect, but doing it this way also enables us to
implement this in this one place and get self pidfd support 'for free'
everywhere.
So I think RoI-wise this is a better proposition than the alternative.
>
> > + return pid;
> > +}
> > +
> > struct pid *__pidfd_get_pid(unsigned int pidfd, bool allow_proc,
> > unsigned int *flags)
> > {
> > - struct pid *pid;
> > - struct fd f = fdget(pidfd);
> > - struct file *file = fd_file(f);
> > + if (pidfd_is_self_sentinel(pidfd)) {
> > + return pidfd_get_pid_self(pidfd, flags);
> > + } else {
>
> Skipping the else here might make the rest of the code more legible
> (since the sentinel branch returns anyway...).
This is so we can declare types for the other branch without having to
figure out how to assign the struct fd sensibly.
Normally I'm a big fan of the if (!...) { return ... } guard pattern, but
it's because of the 'types first' requirement of kernel code that I do this
here.
>
> --
> Pedro
^ permalink raw reply
* Re: [PATCH v5 2/5] pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process
From: Pedro Falcato @ 2024-10-25 12:50 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, linux-kselftest, linux-mm,
linux-fsdevel, linux-api, linux-kernel, Oliver Sang, John Hubbard
In-Reply-To: <788fdfcc9ef602b408951d68097918d6ae379395.1729848252.git.lorenzo.stoakes@oracle.com>
On Fri, Oct 25, 2024 at 10:41 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> It is useful to be able to utilise the pidfd mechanism to reference the
> current thread or process (from a userland point of view - thread group
> leader from the kernel's point of view).
>
> Therefore introduce PIDFD_SELF_THREAD to refer to the current thread, and
> PIDFD_SELF_THREAD_GROUP to refer to the current thread group leader.
>
> For convenience and to avoid confusion from userland's perspective we alias
> these:
>
> * PIDFD_SELF is an alias for PIDFD_SELF_THREAD - This is nearly always what
> the user will want to use, as they would find it surprising if for
> instance fd's were unshared()'d and they wanted to invoke pidfd_getfd()
> and that failed.
>
> * PIDFD_SELF_PROCESS is an alias for PIDFD_SELF_THREAD_GROUP - Most users
> have no concept of thread groups or what a thread group leader is, and
> from userland's perspective and nomenclature this is what userland
> considers to be a process.
>
> Due to the refactoring of the central __pidfd_get_pid() function we can
> implement this functionality centrally, providing the use of this sentinel
> in most functionality which utilises pidfd's.
>
> We need to explicitly adjust kernel_waitid_prepare() to permit this (though
> it wouldn't really make sense to use this there, we provide the ability for
> consistency).
>
> We explicitly disallow use of this in setns(), which would otherwise have
> required explicit custom handling, as it doesn't make sense to set the
> current calling thread to join the namespace of itself.
>
> As the callers of pidfd_get_pid() expect an increased reference count on
> the pid we do so in the self case, reducing churn and avoiding any breakage
> from existing logic which decrements this reference count.
>
> This change implicitly provides PIDFD_SELF_* support in the waitid(P_PIDFS,
> ...), process_madvise(), process_mrelease(), pidfd_send_signal(), and
> pidfd_getfd() system calls.
>
> Things such as polling a pidfs and general fd operations are not supported,
> this strictly provides the sentinel for APIs which explicitly accept a
> pidfd.
>
> Reviewed-by: Shakeel Butt <shakeel.butt@linux.dev>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
> include/linux/pid.h | 8 ++++--
> include/uapi/linux/pidfd.h | 15 +++++++++++
> kernel/exit.c | 3 ++-
> kernel/nsproxy.c | 1 +
> kernel/pid.c | 51 ++++++++++++++++++++++++--------------
> 5 files changed, 57 insertions(+), 21 deletions(-)
>
> diff --git a/include/linux/pid.h b/include/linux/pid.h
> index d466890e1b35..3b2ac7567a88 100644
> --- a/include/linux/pid.h
> +++ b/include/linux/pid.h
> @@ -78,11 +78,15 @@ struct file;
> * __pidfd_get_pid() - Retrieve a pid associated with the specified pidfd.
> *
> * @pidfd: The pidfd whose pid we want, or the fd of a /proc/<pid> file if
> - * @alloc_proc is also set.
> + * @alloc_proc is also set, or PIDFD_SELF_* to refer to the current
> + * thread or thread group leader.
> * @allow_proc: If set, then an fd of a /proc/<pid> file can be passed instead
> * of a pidfd, and this will be used to determine the pid.
> +
> * @flags: Output variable, if non-NULL, then the file->f_flags of the
> - * pidfd will be set here.
> + * pidfd will be set here or If PIDFD_SELF_THREAD is set, this is
> + * set to PIDFD_THREAD, otherwise if PIDFD_SELF_THREAD_GROUP then
> + * this is set to zero.
> *
> * Returns: If successful, the pid associated with the pidfd, otherwise an
> * error.
> diff --git a/include/uapi/linux/pidfd.h b/include/uapi/linux/pidfd.h
> index 565fc0629fff..0ca2ebf906fd 100644
> --- a/include/uapi/linux/pidfd.h
> +++ b/include/uapi/linux/pidfd.h
> @@ -29,4 +29,19 @@
> #define PIDFD_GET_USER_NAMESPACE _IO(PIDFS_IOCTL_MAGIC, 9)
> #define PIDFD_GET_UTS_NAMESPACE _IO(PIDFS_IOCTL_MAGIC, 10)
>
> +/*
> + * Special sentinel values which can be used to refer to the current thread or
> + * thread group leader (which from a userland perspective is the process).
> + */
> +#define PIDFD_SELF PIDFD_SELF_THREAD
> +#define PIDFD_SELF_PROCESS PIDFD_SELF_THREAD_GROUP
> +
> +#define PIDFD_SELF_THREAD -100 /* Current thread. */
This conflicts with AT_FDCWD, might be worth changing?
> +#define PIDFD_SELF_THREAD_GROUP -200 /* Current thread group leader. */
We might want to pick some range outside of the negative errno space
(-4096 IIRC), since we have plenty of values to pick from (2^31 at
least).
> +static inline int pidfd_is_self_sentinel(pid_t pid)
> +{
> + return pid == PIDFD_SELF_THREAD || pid == PIDFD_SELF_THREAD_GROUP;
> +}
Do we want this in the uapi header? Even if this is useful, it might
come with several drawbacks such as breaking scripts that parse kernel
headers (and a quick git grep suggests we do have static inlines in
headers, but in rather obscure ones) and breaking C89:
<source>:8:8: error: unknown type name 'inline'
8 | static inline int pidfd_is_self_sentinel(pid_t pid)
:)
> +
> #endif /* _UAPI_LINUX_PIDFD_H */
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 619f0014c33b..3eb20f8252ee 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -71,6 +71,7 @@
> #include <linux/user_events.h>
> #include <linux/uaccess.h>
>
> +#include <uapi/linux/pidfd.h>
> #include <uapi/linux/wait.h>
>
> #include <asm/unistd.h>
> @@ -1739,7 +1740,7 @@ int kernel_waitid_prepare(struct wait_opts *wo, int which, pid_t upid,
> break;
> case P_PIDFD:
> type = PIDTYPE_PID;
> - if (upid < 0)
> + if (upid < 0 && !pidfd_is_self_sentinel(upid))
> return -EINVAL;
>
> pid = pidfd_get_pid(upid, &f_flags);
> diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> index dc952c3b05af..d239f7eeaa1f 100644
> --- a/kernel/nsproxy.c
> +++ b/kernel/nsproxy.c
> @@ -550,6 +550,7 @@ SYSCALL_DEFINE2(setns, int, fd, int, flags)
> struct nsset nsset = {};
> int err = 0;
>
> + /* If fd is PIDFD_SELF_*, implicitly fail here, as invalid. */
> if (!fd_file(f))
> return -EBADF;
>
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 94c97559e5c5..8742157b36f8 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -535,33 +535,48 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
> }
> EXPORT_SYMBOL_GPL(find_ge_pid);
>
> +static struct pid *pidfd_get_pid_self(unsigned int pidfd, unsigned int *flags)
> +{
> + bool is_thread = pidfd == PIDFD_SELF_THREAD;
> + enum pid_type type = is_thread ? PIDTYPE_PID : PIDTYPE_TGID;
> + struct pid *pid = *task_pid_ptr(current, type);
> +
> + /* The caller expects an elevated reference count. */
> + get_pid(pid);
It would be really really nice to avoid the get here, but I imagine
it'll take some refactoring around put_pid's?
> + return pid;
> +}
> +
> struct pid *__pidfd_get_pid(unsigned int pidfd, bool allow_proc,
> unsigned int *flags)
> {
> - struct pid *pid;
> - struct fd f = fdget(pidfd);
> - struct file *file = fd_file(f);
> + if (pidfd_is_self_sentinel(pidfd)) {
> + return pidfd_get_pid_self(pidfd, flags);
> + } else {
Skipping the else here might make the rest of the code more legible
(since the sentinel branch returns anyway...).
--
Pedro
^ permalink raw reply
* Re: [PATCH 1/6] mm: Add vmalloc_huge_node()
From: Uladzislau Rezki @ 2024-10-25 9:58 UTC (permalink / raw)
To: Peter Zijlstra
Cc: tglx, linux-kernel, mingo, dvhart, dave, andrealmeid,
Andrew Morton, urezki, hch, lstoakes, Arnd Bergmann, linux-api,
linux-mm, linux-arch, malteskarupke, cl, llong, Christoph Hellwig
In-Reply-To: <20241025093944.372391936@infradead.org>
On Fri, Oct 25, 2024 at 11:03:48AM +0200, Peter Zijlstra wrote:
> To enable node specific hash-tables.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
> include/linux/vmalloc.h | 3 +++
> mm/vmalloc.c | 7 +++++++
> 2 files changed, 10 insertions(+)
>
> --- a/include/linux/vmalloc.h
> +++ b/include/linux/vmalloc.h
> @@ -177,6 +177,9 @@ void *__vmalloc_node_noprof(unsigned lon
> void *vmalloc_huge_noprof(unsigned long size, gfp_t gfp_mask) __alloc_size(1);
> #define vmalloc_huge(...) alloc_hooks(vmalloc_huge_noprof(__VA_ARGS__))
>
> +void *vmalloc_huge_node_noprof(unsigned long size, gfp_t gfp_mask, int node) __alloc_size(1);
> +#define vmalloc_huge_node(...) alloc_hooks(vmalloc_huge_node_noprof(__VA_ARGS__))
> +
> extern void *__vmalloc_array_noprof(size_t n, size_t size, gfp_t flags) __alloc_size(1, 2);
> #define __vmalloc_array(...) alloc_hooks(__vmalloc_array_noprof(__VA_ARGS__))
>
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -3948,6 +3948,13 @@ void *vmalloc_huge_noprof(unsigned long
> }
> EXPORT_SYMBOL_GPL(vmalloc_huge_noprof);
>
> +void *vmalloc_huge_node_noprof(unsigned long size, gfp_t gfp_mask, int node)
> +{
> + return __vmalloc_node_range(size, 1, VMALLOC_START, VMALLOC_END,
> + gfp_mask, PAGE_KERNEL, VM_ALLOW_HUGE_VMAP,
> + node, __builtin_return_address(0));
> +}
> +
> /**
> * vzalloc - allocate virtually contiguous memory with zero fill
> * @size: allocation size
>
>
Reviewed-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
--
Uladzislau Rezki
^ permalink raw reply
* [PATCH v5 5/5] selftests: pidfd: add tests for PIDFD_SELF_*
From: Lorenzo Stoakes @ 2024-10-25 9:41 UTC (permalink / raw)
To: Christian Brauner
Cc: Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Oliver Sang, John Hubbard
In-Reply-To: <cover.1729848252.git.lorenzo.stoakes@oracle.com>
Add tests to assert that PIDFD_SELF_* correctly refers to the current
thread and process.
This is only practically meaningful to pidfd_send_signal() and
pidfd_getfd(), but also explicitly test that we disallow this feature for
setns() where it would make no sense.
We cannot reasonably wait on ourself using waitid(P_PIDFD, ...) so while in
theory PIDFD_SELF_* would work here, we'd be left blocked if we tried it.
We defer testing of mm-specific functionality which uses pidfd, namely
process_madvise() and process_mrelease() to mm testing (though note the
latter can not be sensibly tested as it would require the testing process
to be dying).
Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
tools/testing/selftests/pidfd/pidfd.h | 2 +
.../selftests/pidfd/pidfd_getfd_test.c | 141 ++++++++++++++++++
.../selftests/pidfd/pidfd_setns_test.c | 11 ++
tools/testing/selftests/pidfd/pidfd_test.c | 76 ++++++++--
4 files changed, 218 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h
index 0f3fc51cec73..1dbe48c1cf46 100644
--- a/tools/testing/selftests/pidfd/pidfd.h
+++ b/tools/testing/selftests/pidfd/pidfd.h
@@ -16,6 +16,8 @@
#include <sys/types.h>
#include <sys/wait.h>
+#include <linux/pidfd.h>
+
#include "../kselftest.h"
#include "pidfd_helpers.h"
diff --git a/tools/testing/selftests/pidfd/pidfd_getfd_test.c b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
index cd51d547b751..48d224b13c01 100644
--- a/tools/testing/selftests/pidfd/pidfd_getfd_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
@@ -6,6 +6,7 @@
#include <limits.h>
#include <linux/types.h>
#include <poll.h>
+#include <pthread.h>
#include <sched.h>
#include <signal.h>
#include <stdio.h>
@@ -15,6 +16,7 @@
#include <sys/prctl.h>
#include <sys/wait.h>
#include <unistd.h>
+#include <sys/mman.h>
#include <sys/socket.h>
#include <linux/kcmp.h>
@@ -114,6 +116,94 @@ static int child(int sk)
return ret;
}
+static int __pidfd_self_thread_worker(unsigned long page_size)
+{
+ int memfd;
+ int newfd;
+ char *ptr;
+ int err = 0;
+
+ /*
+ * Unshare our FDs so we have our own set. This means
+ * PIDFD_SELF_THREAD_GROUP will fal.
+ */
+ if (unshare(CLONE_FILES) < 0) {
+ err = -errno;
+ goto exit;
+ }
+
+ /* Truncate, map in and write to our memfd. */
+ memfd = sys_memfd_create("test_self_child", 0);
+ if (memfd < 0) {
+ err = -errno;
+ goto exit;
+ }
+
+ if (ftruncate(memfd, page_size)) {
+ err = -errno;
+ goto exit_close_memfd;
+ }
+
+ ptr = mmap(NULL, page_size, PROT_READ | PROT_WRITE,
+ MAP_SHARED, memfd, 0);
+ if (ptr == MAP_FAILED) {
+ err = -errno;
+ goto exit_close_memfd;
+ }
+ ptr[0] = 'y';
+ if (munmap(ptr, page_size)) {
+ err = -errno;
+ goto exit_close_memfd;
+ }
+
+ /* Get a thread-local duplicate of our memfd. */
+ newfd = sys_pidfd_getfd(PIDFD_SELF_THREAD, memfd, 0);
+ if (newfd < 0) {
+ err = -errno;
+ goto exit_close_memfd;
+ }
+
+ if (memfd == newfd) {
+ err = -EINVAL;
+ goto exit_close_fds;
+ }
+
+ /* Map in new fd and make sure that the data is as expected. */
+ ptr = mmap(NULL, page_size, PROT_READ | PROT_WRITE,
+ MAP_SHARED, newfd, 0);
+ if (ptr == MAP_FAILED) {
+ err = -errno;
+ goto exit_close_fds;
+ }
+
+ if (ptr[0] != 'y') {
+ err = -EINVAL;
+ goto exit_close_fds;
+ }
+
+ if (munmap(ptr, page_size)) {
+ err = -errno;
+ goto exit_close_fds;
+ }
+
+exit_close_fds:
+ close(newfd);
+exit_close_memfd:
+ close(memfd);
+exit:
+ return err;
+}
+
+static void *pidfd_self_thread_worker(void *arg)
+{
+ unsigned long page_size = (unsigned long)arg;
+ int ret;
+
+ /* We forward any errors for the caller to handle. */
+ ret = __pidfd_self_thread_worker(page_size);
+ return (void *)(intptr_t)ret;
+}
+
FIXTURE(child)
{
/*
@@ -264,6 +354,57 @@ TEST_F(child, no_strange_EBADF)
EXPECT_EQ(errno, ESRCH);
}
+TEST(pidfd_self)
+{
+ int memfd = sys_memfd_create("test_self", 0);
+ unsigned long page_size = sysconf(_SC_PAGESIZE);
+ int newfd;
+ char *ptr;
+ pthread_t thread;
+ void *res;
+ int err;
+
+ ASSERT_GE(memfd, 0);
+ ASSERT_EQ(ftruncate(memfd, page_size), 0);
+
+ /*
+ * Map so we can assert that the duplicated fd references the same
+ * memory.
+ */
+ ptr = mmap(NULL, page_size, PROT_READ | PROT_WRITE,
+ MAP_SHARED, memfd, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+ ptr[0] = 'x';
+ ASSERT_EQ(munmap(ptr, page_size), 0);
+
+ /* Now get a duplicate of our memfd. */
+ newfd = sys_pidfd_getfd(PIDFD_SELF_THREAD_GROUP, memfd, 0);
+ ASSERT_GE(newfd, 0);
+ ASSERT_NE(memfd, newfd);
+
+ /* Now map duplicate fd and make sure it references the same memory. */
+ ptr = mmap(NULL, page_size, PROT_READ | PROT_WRITE,
+ MAP_SHARED, newfd, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+ ASSERT_EQ(ptr[0], 'x');
+ ASSERT_EQ(munmap(ptr, page_size), 0);
+
+ /* Cleanup. */
+ close(memfd);
+ close(newfd);
+
+ /*
+ * Fire up the thread and assert that we can lookup the thread-specific
+ * PIDFD_SELF_THREAD (also aliased by PIDFD_SELF).
+ */
+ ASSERT_EQ(pthread_create(&thread, NULL, pidfd_self_thread_worker,
+ (void *)page_size), 0);
+ ASSERT_EQ(pthread_join(thread, &res), 0);
+ err = (int)(intptr_t)res;
+
+ ASSERT_EQ(err, 0);
+}
+
#if __NR_pidfd_getfd == -1
int main(void)
{
diff --git a/tools/testing/selftests/pidfd/pidfd_setns_test.c b/tools/testing/selftests/pidfd/pidfd_setns_test.c
index 7c2a4349170a..bbd39dc5ceb7 100644
--- a/tools/testing/selftests/pidfd/pidfd_setns_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_setns_test.c
@@ -752,4 +752,15 @@ TEST(setns_einval)
close(fd);
}
+TEST(setns_pidfd_self_disallowed)
+{
+ ASSERT_EQ(setns(PIDFD_SELF_THREAD, 0), -1);
+ EXPECT_EQ(errno, EBADF);
+
+ errno = 0;
+
+ ASSERT_EQ(setns(PIDFD_SELF_THREAD_GROUP, 0), -1);
+ EXPECT_EQ(errno, EBADF);
+}
+
TEST_HARNESS_MAIN
diff --git a/tools/testing/selftests/pidfd/pidfd_test.c b/tools/testing/selftests/pidfd/pidfd_test.c
index 9faa686f90e4..440447cf89ba 100644
--- a/tools/testing/selftests/pidfd/pidfd_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_test.c
@@ -42,12 +42,41 @@ static pid_t pidfd_clone(int flags, int *pidfd, int (*fn)(void *))
#endif
}
-static int signal_received;
+static pthread_t signal_received;
static void set_signal_received_on_sigusr1(int sig)
{
if (sig == SIGUSR1)
- signal_received = 1;
+ signal_received = pthread_self();
+}
+
+static int send_signal(int pidfd)
+{
+ int ret = 0;
+
+ if (sys_pidfd_send_signal(pidfd, SIGUSR1, NULL, 0) < 0) {
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ if (signal_received != pthread_self()) {
+ ret = -EINVAL;
+ goto exit;
+ }
+
+exit:
+ signal_received = 0;
+ return ret;
+}
+
+static void *send_signal_worker(void *arg)
+{
+ int pidfd = (int)(intptr_t)arg;
+ int ret;
+
+ /* We forward any errors for the caller to handle. */
+ ret = send_signal(pidfd);
+ return (void *)(intptr_t)ret;
}
/*
@@ -56,8 +85,11 @@ static void set_signal_received_on_sigusr1(int sig)
*/
static int test_pidfd_send_signal_simple_success(void)
{
- int pidfd, ret;
+ int pidfd;
const char *test_name = "pidfd_send_signal send SIGUSR1";
+ pthread_t thread;
+ void *thread_res;
+ int err;
if (!have_pidfd_send_signal) {
ksft_test_result_skip(
@@ -66,25 +98,45 @@ static int test_pidfd_send_signal_simple_success(void)
return 0;
}
+ signal(SIGUSR1, set_signal_received_on_sigusr1);
+
+ /* Try sending a signal to ourselves via /proc/self. */
pidfd = open("/proc/self", O_DIRECTORY | O_CLOEXEC);
if (pidfd < 0)
ksft_exit_fail_msg(
"%s test: Failed to open process file descriptor\n",
test_name);
+ err = send_signal(pidfd);
+ if (err)
+ ksft_exit_fail_msg(
+ "%s test: Error %d on sending pidfd signal\n",
+ test_name, err);
+ close(pidfd);
- signal(SIGUSR1, set_signal_received_on_sigusr1);
+ /* Now try the same thing only using PIDFD_SELF_THREAD_GROUP. */
+ err = send_signal(PIDFD_SELF_THREAD_GROUP);
+ if (err)
+ ksft_exit_fail_msg(
+ "%s test: Error %d on PIDFD_SELF_THREAD_GROUP signal\n",
+ test_name, err);
- ret = sys_pidfd_send_signal(pidfd, SIGUSR1, NULL, 0);
- close(pidfd);
- if (ret < 0)
- ksft_exit_fail_msg("%s test: Failed to send signal\n",
+ /*
+ * Now try the same thing in a thread and assert thread ID is equal to
+ * worker thread ID.
+ */
+ if (pthread_create(&thread, NULL, send_signal_worker,
+ (void *)(intptr_t)PIDFD_SELF_THREAD))
+ ksft_exit_fail_msg("%s test: Failed to create thread\n",
test_name);
-
- if (signal_received != 1)
- ksft_exit_fail_msg("%s test: Failed to receive signal\n",
+ if (pthread_join(thread, &thread_res))
+ ksft_exit_fail_msg("%s test: Failed to join thread\n",
test_name);
+ err = (int)(intptr_t)thread_res;
+ if (err)
+ ksft_exit_fail_msg(
+ "%s test: Error %d on PIDFD_SELF_THREAD signal\n",
+ test_name, err);
- signal_received = 0;
ksft_test_result_pass("%s test: Sent signal\n", test_name);
return 0;
}
--
2.47.0
^ permalink raw reply related
* [PATCH v5 4/5] selftests: pidfd: add pidfd.h UAPI wrapper
From: Lorenzo Stoakes @ 2024-10-25 9:41 UTC (permalink / raw)
To: Christian Brauner
Cc: Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Oliver Sang, John Hubbard
In-Reply-To: <cover.1729848252.git.lorenzo.stoakes@oracle.com>
Conflicts can arise between system fcntl.h and linux/fcntl.h, imported by
the linux/pidfd.h UAPI header.
Work around this by adding a wrapper for linux/pidfd.h to
tools/include/ which sets the linux/fcntl.h header guard ahead of
importing the pidfd.h header file.
Adjust the pidfd selftests Makefile to reference this include directory and
put it at a higher precidence than any make header installed headers to
ensure the wrapper is preferred.
This way we can directly import the UAPI header file without issue, use the
latest system header file without having to duplicate anything.
Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
tools/include/linux/pidfd.h | 14 ++++++++++++++
tools/testing/selftests/pidfd/Makefile | 3 +--
2 files changed, 15 insertions(+), 2 deletions(-)
create mode 100644 tools/include/linux/pidfd.h
diff --git a/tools/include/linux/pidfd.h b/tools/include/linux/pidfd.h
new file mode 100644
index 000000000000..113c8023072d
--- /dev/null
+++ b/tools/include/linux/pidfd.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef _TOOLS_LINUX_PIDFD_H
+#define _TOOLS_LINUX_PIDFD_H
+
+/*
+ * Some systems have issues with the linux/fcntl.h import in linux/pidfd.h, so
+ * work around this by setting the header guard.
+ */
+#define _LINUX_FCNTL_H
+#include "../../../include/uapi/linux/pidfd.h"
+#undef _LINUX_FCNTL_H
+
+#endif /* _TOOLS_LINUX_PIDFD_H */
diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
index d731e3e76d5b..f5038c9dae14 100644
--- a/tools/testing/selftests/pidfd/Makefile
+++ b/tools/testing/selftests/pidfd/Makefile
@@ -1,8 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only
-CFLAGS += -g $(KHDR_INCLUDES) -pthread -Wall
+CFLAGS += -g -isystem $(top_srcdir)/tools/include $(KHDR_INCLUDES) -pthread -Wall
TEST_GEN_PROGS := pidfd_test pidfd_fdinfo_test pidfd_open_test \
pidfd_poll_test pidfd_wait pidfd_getfd_test pidfd_setns_test
include ../lib.mk
-
--
2.47.0
^ 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