* Re: [RFC PATCH 0/3] introduce PIDFD_SELF
From: Lorenzo Stoakes @ 2024-09-30 14:32 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Christian Brauner, Florian Weimer, Christian Brauner, Shuah Khan,
Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel
In-Reply-To: <20240930.141721-salted.birth.growing.forges-5Z29YNO700C@cyphar.com>
On Mon, Sep 30, 2024 at 04:21:23PM GMT, Aleksa Sarai wrote:
> On 2024-09-30, Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
> > On Mon, Sep 30, 2024 at 02:34:33PM GMT, Christian Brauner wrote:
> > > On Mon, Sep 30, 2024 at 11:39:49AM GMT, Lorenzo Stoakes wrote:
> > > > On Mon, Sep 30, 2024 at 12:33:18PM GMT, Florian Weimer wrote:
> > > > > * Lorenzo Stoakes:
> > > > >
> > > > > > If you wish to utilise a pidfd interface to refer to the current process
> > > > > > (from the point of view of userland - from the kernel point of view - the
> > > > > > thread group leader), it is rather cumbersome, requiring something like:
> > > > > >
> > > > > > int pidfd = pidfd_open(getpid(), 0);
> > > > > >
> > > > > > ...
> > > > > >
> > > > > > close(pidfd);
> > > > > >
> > > > > > Or the equivalent call opening /proc/self. It is more convenient to use a
> > > > > > sentinel value to indicate to an interface that accepts a pidfd that we
> > > > > > simply wish to refer to the current process.
> > > > >
> > > > > The descriptor will refer to the current thread, not process, right?
> > > >
> > > > No it refers to the current process (i.e. thread group leader from kernel
> > > > perspective). Unless you specify PIDFD_THREAD, this is the same if you did the above.
> > > >
> > > > >
> > > > > The distinction matters for pidfd_getfd if a process contains multiple
> > > > > threads with different file descriptor tables, and probably for
> > > > > pidfd_send_signal as well.
> > > >
> > > > You mean if you did a strange set of flags to clone()? Otherwise these are
> > > > shared right?
> > > >
> > > > Again, we are explicitly looking at process not thread from userland
> > > > perspective. A PIDFD_SELF_THREAD might be possible, but this series doesn't try
> > > > to implement that.
> > >
> > > Florian raises a good point. Currently we have:
> > >
> > > (1) int pidfd_tgid = pidfd_open(getpid(), 0);
> > > (2) int pidfd_thread = pidfd_open(getpid(), PIDFD_THREAD);
> > >
> > > and this instructs:
> > >
> > > pidfd_send_signal()
> > > pidfd_getfd()
> > >
> > > to do different things. For pidfd_send_signal() it's whether the
> > > operation has thread-group scope or thread-scope for pidfd_send_signal()
> > > and for pidfd_getfd() it determines the fdtable to use.
> > >
> > > The thing is that if you pass:
> > >
> > > pidfd_getfd(PDIFD_SELF)
> > >
> > > and you have:
> > >
> > > TGID
> > >
> > > T1 {
> > > clone(CLONE_THREAD)
> > > unshare(CLONE_FILES)
> > > }
> > >
> > > T2 {
> > > clone(CLONE_THREAD)
> > > unshare(CLONE_FILES)
> > > }
> > >
> > > You have 3 threads in the same thread-group that all have distinct file
> > > descriptor tables from each other.
> > >
> > > So if T1 did:
> > >
> > > pidfd_getfd(PIDFD_SELF, ...)
> > >
> > > and we mirror the PIDTYPE_TGID behavior then T1 will very likely expect
> > > to get the fd from its file descriptor table. IOW, its reasonable to
> > > expect that T1 is interested in their very own resource, not someone
> > > else's even if it is the thread-group leader.
> > >
> > > But what T1 will get in reality is an fd from TGID's file descriptor
> > > table (and similar for T2).
> > >
> > > Iirc, yes that confusion exists already with /proc/self. But the
> > > question is whether we should add the same confusion to the pidfd api or
> > > whether we make PIDFD_SELF actually mean PIDTYPE_PID aka the actual
> > > calling thread.
> > >
> > > My thinking is that if you have the reasonable suspicion that you're
> > > multi-threaded and that you're interested in the thread-group resource
> > > then you should be using:
> > >
> > > int pidfd = pidfd_open(getpid(), 0)
> > >
> > > and hand that thread-group leader pidfd around since you're interested
> > > in another thread. But if you're really just interested in your own
> > > resource then pidfd_open(getpid(), 0) makes no sense and you would want
> > > PIDFD_SELF.
> > >
> > > Thoughts?
> >
> > I mean from my perspective, my aim is to get current->mm for
> > process_madvise() so both work for me :) however you both raise a very good
> > point here (sorry Florian, perhaps I was a little too dismissive as to your
> > point, you're absolutely right).
> >
> > My intent was for PIDFD_SELF to simply mirror the pidfd_open(getpid(), 0)
> > behaviour, but you and Florian make a strong case that you'd _probably_
> > find this very confusing had you unshared in this fashion.
> >
> > I mean in general this confusion already exists, and is for what
> > PIDFD_THREAD was created, but I suspect ideally if you could go back you
> > might actually do this by default Christian + let the TGL behaviour be the
> > optional thing?
> >
> > For most users this will not be an issue, but for those they'd get the same
> > result whichever they used, but yes actually I think you're both right -
> > PIDFD_SELF should in effect imply PIDFD_THREAD.
>
> Funnily enough we ran into issues with this when running Go code in runc
> that did precisely this -- /proc/self gave you the wrong fd table in
> very specific circumstances that were annoying to debug. For languages
> with green-threading you can't turn off (like Go) these kinds of issues
> pop up surprisingly often.
Yeah, damn, useful insight that such things do happen in the wild.
>
> > We can adjust the pidfd_send_signal() call to infer the correct scope
> > (actually nicely we can do that without any change there, by having
> > __pidfd_get_pid() set f_flags accordingly).
> >
> > So TL;DR: I agree, I will respin with PIDFD_SELF referring to the thread.
> >
> > My question in return here then is - should we introduce PIDFD_SELF_PROCESS
> > also (do advise if you feel this naming isn't quite right) - to provide
> > thread group leader behaviour?
>
> Sorry to bike-shed, but to match /proc/self and /proc/thread-self, maybe
> they should be called PIDFD_SELF (for tgid) and PIDFD_THREAD_SELF (for
> current's tid)? In principle I guess users might use PIDFD_SELF by
> accident but if we mirror the naming with /proc/{,thread-}self that
> might not be that big of an issue?
Lol, you know I wasn't even aware /proc/thread-self existed...
Yeah, that actually makes sense and is consistent, though obviously the
concern is people will reflexively use PIDFD_SELF and end up with
potentially confusing results.
I will obviously be doing a manpage patch for this so we can spell it out
there very clearly and also in the header - so I'd actually lean towards
doing this myself.
Christian, Florian? Thoughts?
Thanks!
>
> Just a thought.
>
> >
> > Thanks!
> >
>
> --
> Aleksa Sarai
> Senior Software Engineer (Containers)
> SUSE Linux GmbH
> <https://www.cyphar.com/>
^ permalink raw reply
* Re: [RFC PATCH 0/3] introduce PIDFD_SELF
From: Aleksa Sarai @ 2024-09-30 14:21 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Christian Brauner, Florian Weimer, Christian Brauner, Shuah Khan,
Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel
In-Reply-To: <cdd24e6d-4300-4afe-b2ef-1b8ee528bccc@lucifer.local>
[-- Attachment #1: Type: text/plain, Size: 5958 bytes --]
On 2024-09-30, Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
> On Mon, Sep 30, 2024 at 02:34:33PM GMT, Christian Brauner wrote:
> > On Mon, Sep 30, 2024 at 11:39:49AM GMT, Lorenzo Stoakes wrote:
> > > On Mon, Sep 30, 2024 at 12:33:18PM GMT, Florian Weimer wrote:
> > > > * Lorenzo Stoakes:
> > > >
> > > > > If you wish to utilise a pidfd interface to refer to the current process
> > > > > (from the point of view of userland - from the kernel point of view - the
> > > > > thread group leader), it is rather cumbersome, requiring something like:
> > > > >
> > > > > int pidfd = pidfd_open(getpid(), 0);
> > > > >
> > > > > ...
> > > > >
> > > > > close(pidfd);
> > > > >
> > > > > Or the equivalent call opening /proc/self. It is more convenient to use a
> > > > > sentinel value to indicate to an interface that accepts a pidfd that we
> > > > > simply wish to refer to the current process.
> > > >
> > > > The descriptor will refer to the current thread, not process, right?
> > >
> > > No it refers to the current process (i.e. thread group leader from kernel
> > > perspective). Unless you specify PIDFD_THREAD, this is the same if you did the above.
> > >
> > > >
> > > > The distinction matters for pidfd_getfd if a process contains multiple
> > > > threads with different file descriptor tables, and probably for
> > > > pidfd_send_signal as well.
> > >
> > > You mean if you did a strange set of flags to clone()? Otherwise these are
> > > shared right?
> > >
> > > Again, we are explicitly looking at process not thread from userland
> > > perspective. A PIDFD_SELF_THREAD might be possible, but this series doesn't try
> > > to implement that.
> >
> > Florian raises a good point. Currently we have:
> >
> > (1) int pidfd_tgid = pidfd_open(getpid(), 0);
> > (2) int pidfd_thread = pidfd_open(getpid(), PIDFD_THREAD);
> >
> > and this instructs:
> >
> > pidfd_send_signal()
> > pidfd_getfd()
> >
> > to do different things. For pidfd_send_signal() it's whether the
> > operation has thread-group scope or thread-scope for pidfd_send_signal()
> > and for pidfd_getfd() it determines the fdtable to use.
> >
> > The thing is that if you pass:
> >
> > pidfd_getfd(PDIFD_SELF)
> >
> > and you have:
> >
> > TGID
> >
> > T1 {
> > clone(CLONE_THREAD)
> > unshare(CLONE_FILES)
> > }
> >
> > T2 {
> > clone(CLONE_THREAD)
> > unshare(CLONE_FILES)
> > }
> >
> > You have 3 threads in the same thread-group that all have distinct file
> > descriptor tables from each other.
> >
> > So if T1 did:
> >
> > pidfd_getfd(PIDFD_SELF, ...)
> >
> > and we mirror the PIDTYPE_TGID behavior then T1 will very likely expect
> > to get the fd from its file descriptor table. IOW, its reasonable to
> > expect that T1 is interested in their very own resource, not someone
> > else's even if it is the thread-group leader.
> >
> > But what T1 will get in reality is an fd from TGID's file descriptor
> > table (and similar for T2).
> >
> > Iirc, yes that confusion exists already with /proc/self. But the
> > question is whether we should add the same confusion to the pidfd api or
> > whether we make PIDFD_SELF actually mean PIDTYPE_PID aka the actual
> > calling thread.
> >
> > My thinking is that if you have the reasonable suspicion that you're
> > multi-threaded and that you're interested in the thread-group resource
> > then you should be using:
> >
> > int pidfd = pidfd_open(getpid(), 0)
> >
> > and hand that thread-group leader pidfd around since you're interested
> > in another thread. But if you're really just interested in your own
> > resource then pidfd_open(getpid(), 0) makes no sense and you would want
> > PIDFD_SELF.
> >
> > Thoughts?
>
> I mean from my perspective, my aim is to get current->mm for
> process_madvise() so both work for me :) however you both raise a very good
> point here (sorry Florian, perhaps I was a little too dismissive as to your
> point, you're absolutely right).
>
> My intent was for PIDFD_SELF to simply mirror the pidfd_open(getpid(), 0)
> behaviour, but you and Florian make a strong case that you'd _probably_
> find this very confusing had you unshared in this fashion.
>
> I mean in general this confusion already exists, and is for what
> PIDFD_THREAD was created, but I suspect ideally if you could go back you
> might actually do this by default Christian + let the TGL behaviour be the
> optional thing?
>
> For most users this will not be an issue, but for those they'd get the same
> result whichever they used, but yes actually I think you're both right -
> PIDFD_SELF should in effect imply PIDFD_THREAD.
Funnily enough we ran into issues with this when running Go code in runc
that did precisely this -- /proc/self gave you the wrong fd table in
very specific circumstances that were annoying to debug. For languages
with green-threading you can't turn off (like Go) these kinds of issues
pop up surprisingly often.
> We can adjust the pidfd_send_signal() call to infer the correct scope
> (actually nicely we can do that without any change there, by having
> __pidfd_get_pid() set f_flags accordingly).
>
> So TL;DR: I agree, I will respin with PIDFD_SELF referring to the thread.
>
> My question in return here then is - should we introduce PIDFD_SELF_PROCESS
> also (do advise if you feel this naming isn't quite right) - to provide
> thread group leader behaviour?
Sorry to bike-shed, but to match /proc/self and /proc/thread-self, maybe
they should be called PIDFD_SELF (for tgid) and PIDFD_THREAD_SELF (for
current's tid)? In principle I guess users might use PIDFD_SELF by
accident but if we mirror the naming with /proc/{,thread-}self that
might not be that big of an issue?
Just a thought.
>
> Thanks!
>
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 0/3] introduce PIDFD_SELF
From: Lorenzo Stoakes @ 2024-09-30 13:10 UTC (permalink / raw)
To: Christian Brauner
Cc: Florian Weimer, Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, pedro.falcato,
linux-kselftest, linux-mm, linux-fsdevel, linux-api, linux-kernel
In-Reply-To: <20240930-verbiegen-zinspolitik-cafb730c3c84@brauner>
On Mon, Sep 30, 2024 at 02:34:33PM GMT, Christian Brauner wrote:
> On Mon, Sep 30, 2024 at 11:39:49AM GMT, Lorenzo Stoakes wrote:
> > On Mon, Sep 30, 2024 at 12:33:18PM GMT, Florian Weimer wrote:
> > > * Lorenzo Stoakes:
> > >
> > > > If you wish to utilise a pidfd interface to refer to the current process
> > > > (from the point of view of userland - from the kernel point of view - the
> > > > thread group leader), it is rather cumbersome, requiring something like:
> > > >
> > > > int pidfd = pidfd_open(getpid(), 0);
> > > >
> > > > ...
> > > >
> > > > close(pidfd);
> > > >
> > > > Or the equivalent call opening /proc/self. It is more convenient to use a
> > > > sentinel value to indicate to an interface that accepts a pidfd that we
> > > > simply wish to refer to the current process.
> > >
> > > The descriptor will refer to the current thread, not process, right?
> >
> > No it refers to the current process (i.e. thread group leader from kernel
> > perspective). Unless you specify PIDFD_THREAD, this is the same if you did the above.
> >
> > >
> > > The distinction matters for pidfd_getfd if a process contains multiple
> > > threads with different file descriptor tables, and probably for
> > > pidfd_send_signal as well.
> >
> > You mean if you did a strange set of flags to clone()? Otherwise these are
> > shared right?
> >
> > Again, we are explicitly looking at process not thread from userland
> > perspective. A PIDFD_SELF_THREAD might be possible, but this series doesn't try
> > to implement that.
>
> Florian raises a good point. Currently we have:
>
> (1) int pidfd_tgid = pidfd_open(getpid(), 0);
> (2) int pidfd_thread = pidfd_open(getpid(), PIDFD_THREAD);
>
> and this instructs:
>
> pidfd_send_signal()
> pidfd_getfd()
>
> to do different things. For pidfd_send_signal() it's whether the
> operation has thread-group scope or thread-scope for pidfd_send_signal()
> and for pidfd_getfd() it determines the fdtable to use.
>
> The thing is that if you pass:
>
> pidfd_getfd(PDIFD_SELF)
>
> and you have:
>
> TGID
>
> T1 {
> clone(CLONE_THREAD)
> unshare(CLONE_FILES)
> }
>
> T2 {
> clone(CLONE_THREAD)
> unshare(CLONE_FILES)
> }
>
> You have 3 threads in the same thread-group that all have distinct file
> descriptor tables from each other.
>
> So if T1 did:
>
> pidfd_getfd(PIDFD_SELF, ...)
>
> and we mirror the PIDTYPE_TGID behavior then T1 will very likely expect
> to get the fd from its file descriptor table. IOW, its reasonable to
> expect that T1 is interested in their very own resource, not someone
> else's even if it is the thread-group leader.
>
> But what T1 will get in reality is an fd from TGID's file descriptor
> table (and similar for T2).
>
> Iirc, yes that confusion exists already with /proc/self. But the
> question is whether we should add the same confusion to the pidfd api or
> whether we make PIDFD_SELF actually mean PIDTYPE_PID aka the actual
> calling thread.
>
> My thinking is that if you have the reasonable suspicion that you're
> multi-threaded and that you're interested in the thread-group resource
> then you should be using:
>
> int pidfd = pidfd_open(getpid(), 0)
>
> and hand that thread-group leader pidfd around since you're interested
> in another thread. But if you're really just interested in your own
> resource then pidfd_open(getpid(), 0) makes no sense and you would want
> PIDFD_SELF.
>
> Thoughts?
I mean from my perspective, my aim is to get current->mm for
process_madvise() so both work for me :) however you both raise a very good
point here (sorry Florian, perhaps I was a little too dismissive as to your
point, you're absolutely right).
My intent was for PIDFD_SELF to simply mirror the pidfd_open(getpid(), 0)
behaviour, but you and Florian make a strong case that you'd _probably_
find this very confusing had you unshared in this fashion.
I mean in general this confusion already exists, and is for what
PIDFD_THREAD was created, but I suspect ideally if you could go back you
might actually do this by default Christian + let the TGL behaviour be the
optional thing?
For most users this will not be an issue, but for those they'd get the same
result whichever they used, but yes actually I think you're both right -
PIDFD_SELF should in effect imply PIDFD_THREAD.
We can adjust the pidfd_send_signal() call to infer the correct scope
(actually nicely we can do that without any change there, by having
__pidfd_get_pid() set f_flags accordingly).
So TL;DR: I agree, I will respin with PIDFD_SELF referring to the thread.
My question in return here then is - should we introduce PIDFD_SELF_PROCESS
also (do advise if you feel this naming isn't quite right) - to provide
thread group leader behaviour?
Thanks!
^ permalink raw reply
* Re: [RFC PATCH 0/3] introduce PIDFD_SELF
From: Christian Brauner @ 2024-09-30 12:34 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Florian Weimer, Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, pedro.falcato,
linux-kselftest, linux-mm, linux-fsdevel, linux-api, linux-kernel
In-Reply-To: <42df57ac-d89c-4111-a04d-290dd2197573@lucifer.local>
On Mon, Sep 30, 2024 at 11:39:49AM GMT, Lorenzo Stoakes wrote:
> On Mon, Sep 30, 2024 at 12:33:18PM GMT, Florian Weimer wrote:
> > * Lorenzo Stoakes:
> >
> > > If you wish to utilise a pidfd interface to refer to the current process
> > > (from the point of view of userland - from the kernel point of view - the
> > > thread group leader), it is rather cumbersome, requiring something like:
> > >
> > > int pidfd = pidfd_open(getpid(), 0);
> > >
> > > ...
> > >
> > > close(pidfd);
> > >
> > > Or the equivalent call opening /proc/self. It is more convenient to use a
> > > sentinel value to indicate to an interface that accepts a pidfd that we
> > > simply wish to refer to the current process.
> >
> > The descriptor will refer to the current thread, not process, right?
>
> No it refers to the current process (i.e. thread group leader from kernel
> perspective). Unless you specify PIDFD_THREAD, this is the same if you did the above.
>
> >
> > The distinction matters for pidfd_getfd if a process contains multiple
> > threads with different file descriptor tables, and probably for
> > pidfd_send_signal as well.
>
> You mean if you did a strange set of flags to clone()? Otherwise these are
> shared right?
>
> Again, we are explicitly looking at process not thread from userland
> perspective. A PIDFD_SELF_THREAD might be possible, but this series doesn't try
> to implement that.
Florian raises a good point. Currently we have:
(1) int pidfd_tgid = pidfd_open(getpid(), 0);
(2) int pidfd_thread = pidfd_open(getpid(), PIDFD_THREAD);
and this instructs:
pidfd_send_signal()
pidfd_getfd()
to do different things. For pidfd_send_signal() it's whether the
operation has thread-group scope or thread-scope for pidfd_send_signal()
and for pidfd_getfd() it determines the fdtable to use.
The thing is that if you pass:
pidfd_getfd(PDIFD_SELF)
and you have:
TGID
T1 {
clone(CLONE_THREAD)
unshare(CLONE_FILES)
}
T2 {
clone(CLONE_THREAD)
unshare(CLONE_FILES)
}
You have 3 threads in the same thread-group that all have distinct file
descriptor tables from each other.
So if T1 did:
pidfd_getfd(PIDFD_SELF, ...)
and we mirror the PIDTYPE_TGID behavior then T1 will very likely expect
to get the fd from its file descriptor table. IOW, its reasonable to
expect that T1 is interested in their very own resource, not someone
else's even if it is the thread-group leader.
But what T1 will get in reality is an fd from TGID's file descriptor
table (and similar for T2).
Iirc, yes that confusion exists already with /proc/self. But the
question is whether we should add the same confusion to the pidfd api or
whether we make PIDFD_SELF actually mean PIDTYPE_PID aka the actual
calling thread.
My thinking is that if you have the reasonable suspicion that you're
multi-threaded and that you're interested in the thread-group resource
then you should be using:
int pidfd = pidfd_open(getpid(), 0)
and hand that thread-group leader pidfd around since you're interested
in another thread. But if you're really just interested in your own
resource then pidfd_open(getpid(), 0) makes no sense and you would want
PIDFD_SELF.
Thoughts?
^ permalink raw reply
* Re: [RFC PATCH 0/3] introduce PIDFD_SELF
From: Lorenzo Stoakes @ 2024-09-30 10:39 UTC (permalink / raw)
To: Florian Weimer
Cc: Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, pedro.falcato,
linux-kselftest, linux-mm, linux-fsdevel, linux-api, linux-kernel
In-Reply-To: <87ttdxl9ch.fsf@oldenburg.str.redhat.com>
On Mon, Sep 30, 2024 at 12:33:18PM GMT, Florian Weimer wrote:
> * Lorenzo Stoakes:
>
> > If you wish to utilise a pidfd interface to refer to the current process
> > (from the point of view of userland - from the kernel point of view - the
> > thread group leader), it is rather cumbersome, requiring something like:
> >
> > int pidfd = pidfd_open(getpid(), 0);
> >
> > ...
> >
> > close(pidfd);
> >
> > Or the equivalent call opening /proc/self. It is more convenient to use a
> > sentinel value to indicate to an interface that accepts a pidfd that we
> > simply wish to refer to the current process.
>
> The descriptor will refer to the current thread, not process, right?
No it refers to the current process (i.e. thread group leader from kernel
perspective). Unless you specify PIDFD_THREAD, this is the same if you did the above.
>
> The distinction matters for pidfd_getfd if a process contains multiple
> threads with different file descriptor tables, and probably for
> pidfd_send_signal as well.
You mean if you did a strange set of flags to clone()? Otherwise these are
shared right?
Again, we are explicitly looking at process not thread from userland
perspective. A PIDFD_SELF_THREAD might be possible, but this series doesn't try
to implement that.
>
> Thanks,
> Florian
>
^ permalink raw reply
* Re: [RFC PATCH 0/3] introduce PIDFD_SELF
From: Florian Weimer @ 2024-09-30 10:33 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, pedro.falcato,
linux-kselftest, linux-mm, linux-fsdevel, linux-api, linux-kernel
In-Reply-To: <cover.1727644404.git.lorenzo.stoakes@oracle.com>
* Lorenzo Stoakes:
> If you wish to utilise a pidfd interface to refer to the current process
> (from the point of view of userland - from the kernel point of view - the
> thread group leader), it is rather cumbersome, requiring something like:
>
> int pidfd = pidfd_open(getpid(), 0);
>
> ...
>
> close(pidfd);
>
> Or the equivalent call opening /proc/self. It is more convenient to use a
> sentinel value to indicate to an interface that accepts a pidfd that we
> simply wish to refer to the current process.
The descriptor will refer to the current thread, not process, right?
The distinction matters for pidfd_getfd if a process contains multiple
threads with different file descriptor tables, and probably for
pidfd_send_signal as well.
Thanks,
Florian
^ permalink raw reply
* [RFC PATCH 3/3] selftests: pidfd: add tests for PIDFD_SELF
From: Lorenzo Stoakes @ 2024-09-30 9:22 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
In-Reply-To: <cover.1727644404.git.lorenzo.stoakes@oracle.com>
Add tests to assert that PIDFD_SELF correctly refers to the current
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).
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
tools/testing/selftests/pidfd/pidfd.h | 5 +++
.../selftests/pidfd/pidfd_getfd_test.c | 38 +++++++++++++++++++
.../selftests/pidfd/pidfd_setns_test.c | 6 +++
tools/testing/selftests/pidfd/pidfd_test.c | 13 +++++++
4 files changed, 62 insertions(+)
diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h
index 88d6830ee004..099ee7178193 100644
--- a/tools/testing/selftests/pidfd/pidfd.h
+++ b/tools/testing/selftests/pidfd/pidfd.h
@@ -50,6 +50,11 @@
#define PIDFD_NONBLOCK O_NONBLOCK
#endif
+/* System header file may not have this available. */
+#ifndef PIDFD_SELF
+#define PIDFD_SELF -200
+#endif
+
/*
* The kernel reserves 300 pids via RESERVED_PIDS in kernel/pid.c
* That means, when it wraps around any pid < 300 will be skipped.
diff --git a/tools/testing/selftests/pidfd/pidfd_getfd_test.c b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
index cd51d547b751..cdf375fd61b2 100644
--- a/tools/testing/selftests/pidfd/pidfd_getfd_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
@@ -15,6 +15,7 @@
#include <sys/prctl.h>
#include <sys/wait.h>
#include <unistd.h>
+#include <sys/mman.h>
#include <sys/socket.h>
#include <linux/kcmp.h>
@@ -264,6 +265,43 @@ TEST_F(child, no_strange_EBADF)
EXPECT_EQ(errno, ESRCH);
}
+TEST(pidfd_self)
+{
+ int memfd = sys_memfd_create("test_self", 0);
+ long page_size = sysconf(_SC_PAGESIZE);
+ int newfd;
+ char *ptr;
+
+ 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, 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);
+}
+
#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..8e1510744aaa 100644
--- a/tools/testing/selftests/pidfd/pidfd_setns_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_setns_test.c
@@ -752,4 +752,10 @@ TEST(setns_einval)
close(fd);
}
+TEST(setns_pidfd_self_disallowed)
+{
+ ASSERT_EQ(setns(PIDFD_SELF, 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..18802b657352 100644
--- a/tools/testing/selftests/pidfd/pidfd_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_test.c
@@ -85,6 +85,19 @@ static int test_pidfd_send_signal_simple_success(void)
test_name);
signal_received = 0;
+
+ /* Now try the same thing only using PIDFD_SELF. */
+ ret = sys_pidfd_send_signal(PIDFD_SELF, SIGUSR1, NULL, 0);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s test: Failed to send PIDFD_SELF signal\n",
+ test_name);
+
+ if (signal_received != 1)
+ ksft_exit_fail_msg("%s test: Failed to receive PIDFD_SELF signal\n",
+ test_name);
+
+ signal_received = 0;
+
ksft_test_result_pass("%s test: Sent signal\n", test_name);
return 0;
}
--
2.46.2
^ permalink raw reply related
* [RFC PATCH 2/3] pidfd: add PIDFD_SELF sentinel to refer to own process
From: Lorenzo Stoakes @ 2024-09-30 9:22 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
In-Reply-To: <cover.1727644404.git.lorenzo.stoakes@oracle.com>
Add a PIDFD_SELF special sentinel value which can be passed as a pidfd to
indicate that the current process is to be targeted (as this is a process
from the userland perspective, this means from the kernel's perspective the
current thread group leader is to be targeted).
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 this case even for the current thread leader, reducing
churn and avoiding any breakage from existing logic which decrements this
reference count.
In the pidfd_send_signal() system call, we can continue to fdput() the
struct fd output by pidfs_to_pid_proc() even if PIDFD_SELF is specified, as
this will be empty and the invocation will be a no-op.
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.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
include/linux/pid.h | 9 +++---
include/uapi/linux/pidfd.h | 3 ++
kernel/exit.c | 3 +-
kernel/nsproxy.c | 1 +
kernel/pid.c | 63 ++++++++++++++++++++++----------------
5 files changed, 47 insertions(+), 32 deletions(-)
diff --git a/include/linux/pid.h b/include/linux/pid.h
index 68b02eab7509..e95dd7b81ae2 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -77,18 +77,19 @@ 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.
+ * @pidfd: The pidfd whose pid we want, the fd of a /proc/<pid> file if
+ * @alloc_proc is also set, or PIDFD_SELF if the pid we require
+ * is the thread group leader.
* @pin_pid: If set, then the reference counter of the returned pid is
* incremented. If not set, then @fd should be provided to pin the
* pidfd.
* @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. If PIDFD_SELF set, this is zero.
* @fd: Output variable, if non-NULL, then the pidfd reference will
* remain elevated and the caller will need to decrement it
- * themselves.
+ * themselves. If PIDFD_SELF set, this is empty.
*
* 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..0bff5276b51d 100644
--- a/include/uapi/linux/pidfd.h
+++ b/include/uapi/linux/pidfd.h
@@ -29,4 +29,7 @@
#define PIDFD_GET_USER_NAMESPACE _IO(PIDFS_IOCTL_MAGIC, 9)
#define PIDFD_GET_UTS_NAMESPACE _IO(PIDFS_IOCTL_MAGIC, 10)
+#define PIDFD_SELF -200 /* Special sentinel value which can be passed as a pidfd
+ * to refer to the current thread group leader. */
+
#endif /* _UAPI_LINUX_PIDFD_H */
diff --git a/kernel/exit.c b/kernel/exit.c
index 619f0014c33b..fc10eb2b180c 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 && upid != PIDFD_SELF)
return -EINVAL;
pid = pidfd_get_pid(upid, &f_flags);
diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
index dc952c3b05af..aee86fcaf670 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 26e2581210c4..9c037441afb8 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -539,23 +539,28 @@ struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
bool allow_proc, unsigned int *flags,
struct fd *fd)
{
- struct fd f;
struct pid *pid;
- struct file *file;
-
- f = fdget(fd);
- file = fd_file(f);
- if (!file)
- return ERR_PTR(-EBADF);
-
- pid = pidfd_pid(file);
- /* If we allow opening a pidfd via /proc/<pid>, do so. */
- if (IS_ERR(pid) && allow_proc)
- pid = tgid_pidfd_to_pid(file);
-
- if (IS_ERR(pid)) {
- fdput(f);
- return pid;
+ struct fd f = {};
+ struct file *file = NULL;
+ unsigned int f_flags = 0;
+
+ if (pidfd == PIDFD_SELF) {
+ pid = *task_pid_ptr(current, PIDTYPE_TGID);
+ } else {
+ f = fdget(pidfd);
+ file = fd_file(f);
+ if (!file)
+ return ERR_PTR(-EBADF);
+
+ pid = pidfd_pid(file);
+ /* If we allow opening a pidfd via /proc/<pid>, do so. */
+ if (IS_ERR(pid) && allow_proc)
+ pid = tgid_pidfd_to_pid(file);
+
+ if (IS_ERR(pid)) {
+ fdput(f);
+ return pid;
+ }
}
if (pin_pid)
@@ -563,18 +568,22 @@ struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
else
WARN_ON_ONCE(!fd); /* Nothing to keep pid/pidfd around? */
- if (flags)
- *flags = file->f_flags;
+ if (file) {
+ f_flags = file->f_flags;
- /*
- * If the user provides an fd output then it will handle decrementing
- * its reference counter.
- */
- if (fd)
- *fd = f;
- else
- /* Otherwise we release it. */
- fdput(f);
+ /*
+ * If the user provides an fd output then it will handle decrementing
+ * its reference counter.
+ */
+ if (fd)
+ *fd = f;
+ else
+ /* Otherwise we release it. */
+ fdput(f);
+ }
+
+ if (flags)
+ *flags = f_flags;
return pid;
}
--
2.46.2
^ permalink raw reply related
* [RFC PATCH 1/3] pidfd: refactor pidfd_get_pid/to_pid() and de-duplicate pid lookup
From: Lorenzo Stoakes @ 2024-09-30 9:22 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
In-Reply-To: <cover.1727644404.git.lorenzo.stoakes@oracle.com>
The means by which a pid is determined from a pidfd is duplicated,
some callers holding a reference to the (pid)fd, and others explicitly
pinning the pid.
Introduce __pidfd_get_pid() which abstracts both approaches and provide
optional output parameters for file->f_flags and the fd (the latter of
which, if provided, prevents the function from decrementing the fd's
reference count).
We add a wrapper function pidfd_get_pid() which performs the same
functionality as the original, only deferring to __pidfd_get_pid() for the
heavy lifting.
Additionally, abstract the ability to open a pidfd by opening a /proc/<pid>
directory (used by the pidfd_send_signal() system call), providing a
pidfd_get_pid_proc() wrapper function to do so.
Doing this allows us to eliminate open-coded pidfd pid lookup and to
consistently handle this in one place.
This lays the groundwork for a subsequent patch which adds a new sentinel
pidfd to explicitly reference the current process (i.e. thread group
leader) without the need for a pidfd.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
include/linux/pid.h | 42 +++++++++++++++++++++++++++++++++-
kernel/pid.c | 55 +++++++++++++++++++++++++++++++--------------
kernel/signal.c | 26 +++++----------------
3 files changed, 84 insertions(+), 39 deletions(-)
diff --git a/include/linux/pid.h b/include/linux/pid.h
index a3aad9b4074c..68b02eab7509 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -2,6 +2,7 @@
#ifndef _LINUX_PID_H
#define _LINUX_PID_H
+#include <linux/file.h>
#include <linux/pid_types.h>
#include <linux/rculist.h>
#include <linux/rcupdate.h>
@@ -72,8 +73,47 @@ extern struct pid init_struct_pid;
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.
+ * @pin_pid: If set, then the reference counter of the returned pid is
+ * incremented. If not set, then @fd should be provided to pin the
+ * pidfd.
+ * @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.
+ * @fd: Output variable, if non-NULL, then the pidfd reference will
+ * remain elevated and the caller will need to decrement it
+ * themselves.
+ *
+ * Returns: If successful, the pid associated with the pidfd, otherwise an
+ * error.
+ */
+struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
+ bool allow_proc, unsigned int *flags,
+ struct fd *fd);
+
+static inline struct pid *pidfd_get_pid(unsigned int pidfd, unsigned int *flags)
+{
+ return __pidfd_get_pid(pidfd, /* pin_pid = */ true,
+ /* allow_proc = */ false,
+ flags, /* fd = */ NULL);
+}
+
+static inline struct pid *pidfd_to_pid_proc(unsigned int pidfd,
+ unsigned int *flags,
+ struct fd *fd)
+{
+ return __pidfd_get_pid(pidfd, /* pin_pid = */ false,
+ /* allow_proc = */ true,
+ flags, fd);
+}
+
struct pid *pidfd_pid(const struct file *file);
-struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags);
struct task_struct *pidfd_get_task(int pidfd, unsigned int *flags);
int pidfd_prepare(struct pid *pid, unsigned int flags, struct file **ret);
void do_notify_pidfd(struct task_struct *task);
diff --git a/kernel/pid.c b/kernel/pid.c
index 2715afb77eab..26e2581210c4 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -36,6 +36,7 @@
#include <linux/pid_namespace.h>
#include <linux/init_task.h>
#include <linux/syscalls.h>
+#include <linux/proc_fs.h>
#include <linux/proc_ns.h>
#include <linux/refcount.h>
#include <linux/anon_inodes.h>
@@ -534,22 +535,47 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
}
EXPORT_SYMBOL_GPL(find_ge_pid);
-struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
+struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
+ bool allow_proc, unsigned int *flags,
+ struct fd *fd)
{
struct fd f;
struct pid *pid;
+ struct file *file;
f = fdget(fd);
- if (!fd_file(f))
+ file = fd_file(f);
+ if (!file)
return ERR_PTR(-EBADF);
- pid = pidfd_pid(fd_file(f));
- if (!IS_ERR(pid)) {
- get_pid(pid);
- *flags = fd_file(f)->f_flags;
+ pid = pidfd_pid(file);
+ /* If we allow opening a pidfd via /proc/<pid>, do so. */
+ if (IS_ERR(pid) && allow_proc)
+ pid = tgid_pidfd_to_pid(file);
+
+ if (IS_ERR(pid)) {
+ fdput(f);
+ return pid;
}
- fdput(f);
+ if (pin_pid)
+ get_pid(pid);
+ else
+ WARN_ON_ONCE(!fd); /* Nothing to keep pid/pidfd around? */
+
+ if (flags)
+ *flags = file->f_flags;
+
+ /*
+ * If the user provides an fd output then it will handle decrementing
+ * its reference counter.
+ */
+ if (fd)
+ *fd = f;
+ else
+ /* Otherwise we release it. */
+ fdput(f);
+
return pid;
}
@@ -747,23 +773,18 @@ SYSCALL_DEFINE3(pidfd_getfd, int, pidfd, int, fd,
unsigned int, flags)
{
struct pid *pid;
- struct fd f;
int ret;
/* flags is currently unused - make sure it's unset */
if (flags)
return -EINVAL;
- f = fdget(pidfd);
- if (!fd_file(f))
- return -EBADF;
-
- pid = pidfd_pid(fd_file(f));
+ pid = pidfd_get_pid(pidfd, NULL);
if (IS_ERR(pid))
- ret = PTR_ERR(pid);
- else
- ret = pidfd_getfd(pid, fd);
+ return PTR_ERR(pid);
- fdput(f);
+ ret = pidfd_getfd(pid, fd);
+
+ put_pid(pid);
return ret;
}
diff --git a/kernel/signal.c b/kernel/signal.c
index 6e57036f947f..abbb1931deba 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3894,17 +3894,6 @@ static int copy_siginfo_from_user_any(kernel_siginfo_t *kinfo,
return copy_siginfo_from_user(kinfo, info);
}
-static struct pid *pidfd_to_pid(const struct file *file)
-{
- struct pid *pid;
-
- pid = pidfd_pid(file);
- if (!IS_ERR(pid))
- return pid;
-
- return tgid_pidfd_to_pid(file);
-}
-
#define PIDFD_SEND_SIGNAL_FLAGS \
(PIDFD_SIGNAL_THREAD | PIDFD_SIGNAL_THREAD_GROUP | \
PIDFD_SIGNAL_PROCESS_GROUP)
@@ -3931,6 +3920,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
struct pid *pid;
kernel_siginfo_t kinfo;
enum pid_type type;
+ unsigned int f_flags;
/* Enforce flags be set to 0 until we add an extension. */
if (flags & ~PIDFD_SEND_SIGNAL_FLAGS)
@@ -3940,16 +3930,10 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
if (hweight32(flags & PIDFD_SEND_SIGNAL_FLAGS) > 1)
return -EINVAL;
- f = fdget(pidfd);
- if (!fd_file(f))
- return -EBADF;
-
/* Is this a pidfd? */
- pid = pidfd_to_pid(fd_file(f));
- if (IS_ERR(pid)) {
- ret = PTR_ERR(pid);
- goto err;
- }
+ pid = pidfd_to_pid_proc(pidfd, &f_flags, &f);
+ if (IS_ERR(pid))
+ return PTR_ERR(pid);
ret = -EINVAL;
if (!access_pidfd_pidns(pid))
@@ -3958,7 +3942,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
switch (flags) {
case 0:
/* Infer scope from the type of pidfd. */
- if (fd_file(f)->f_flags & PIDFD_THREAD)
+ if (f_flags & PIDFD_THREAD)
type = PIDTYPE_PID;
else
type = PIDTYPE_TGID;
--
2.46.2
^ permalink raw reply related
* [RFC PATCH 0/3] introduce PIDFD_SELF
From: Lorenzo Stoakes @ 2024-09-30 9:22 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
If you wish to utilise a pidfd interface to refer to the current process
(from the point of view of userland - from the kernel point of view - the
thread group leader), it is rather cumbersome, requiring something like:
int pidfd = pidfd_open(getpid(), 0);
...
close(pidfd);
Or the equivalent call opening /proc/self. It is more convenient to use a
sentinel value to indicate to an interface that accepts a pidfd that we
simply wish to refer to the current process.
This series introduces such a sentinel, PIDFD_SELF, which can be passed as
the pidfd in this instance rather than having to establish a dummy fd for
this purpose.
The only pidfd interface where this is particularly useful is
process_madvise(), which provides the motivation for this series. However,
as this is a general interface, we ensure that all pidfd interfaces can
handle this correctly.
We ensure that pidfd_send_signal() and pidfd_getfd() work correctly, and
assert as much in selftests. All other interfaces except setns() will work
implicitly with this new interface, however it doesn't make sense to test
waitid(P_PIDFD, ...) as waiting on ourselves is a blocking operation.
In the case of setns() we explicitly disallow use of PIDFD_SELF as it
doesn't make sense to obtain the namespaces of our own process, and it
would require work to implement this functionality there that would be of
no use.
We also do not provide the ability to utilise PIDFD_SELF in ordinary fd
operations such as open() or poll(), as this would require extensive work
and be of no real use.
Lorenzo Stoakes (3):
pidfd: refactor pidfd_get_pid/to_pid() and de-duplicate pid lookup
pidfd: add PIDFD_SELF sentinel to refer to own process
selftests: pidfd: add tests for PIDFD_SELF
include/linux/pid.h | 43 +++++++++++-
include/uapi/linux/pidfd.h | 3 +
kernel/exit.c | 3 +-
kernel/nsproxy.c | 1 +
kernel/pid.c | 70 +++++++++++++------
kernel/signal.c | 26 ++-----
tools/testing/selftests/pidfd/pidfd.h | 5 ++
.../selftests/pidfd/pidfd_getfd_test.c | 38 ++++++++++
.../selftests/pidfd/pidfd_setns_test.c | 6 ++
tools/testing/selftests/pidfd/pidfd_test.c | 13 ++++
10 files changed, 165 insertions(+), 43 deletions(-)
--
2.46.2
^ permalink raw reply
* Re: [PATCH RFT v9 4/8] fork: Add shadow stack support to clone3()
From: Edgecombe, Rick P @ 2024-09-27 15:21 UTC (permalink / raw)
To: broonie@kernel.org, brauner@kernel.org
Cc: dietmar.eggemann@arm.com, shuah@kernel.org, Szabolcs.Nagy@arm.com,
dave.hansen@linux.intel.com, debug@rivosinc.com, mgorman@suse.de,
linux-api@vger.kernel.org, vincent.guittot@linaro.org,
linux-kernel@vger.kernel.org, mingo@redhat.com,
rostedt@goodmis.org, hjl.tools@gmail.com, tglx@linutronix.de,
fweimer@redhat.com, vschneid@redhat.com, catalin.marinas@arm.com,
kees@kernel.org, will@kernel.org, hpa@zytor.com, jannh@google.com,
peterz@infradead.org, yury.khrustalev@arm.com, bp@alien8.de,
wilco.dijkstra@arm.com, linux-kselftest@vger.kernel.org,
bsegall@google.com, juri.lelli@redhat.com, x86@kernel.org
In-Reply-To: <20240927-springen-fortpflanzen-34a303373088@brauner>
On Fri, 2024-09-27 at 10:50 +0200, Christian Brauner wrote:
> The legacy clone system call had required userspace to know in which
> direction the stack was growing and then pass down the stack pointer
> appropriately (e.g., parisc grows upwards).
>
> And in fact, the old clone() system call did take an additional
> stack_size argument on specific architectures. For example, on
> microblaze.
>
> Also, when clone3() was done we still had ia64 in the tree which had a
> separate clone2() system call that also required a stack_size argument.
>
> So userspace ended up with code like this or worse:
>
> #define __STACK_SIZE (8 * 1024 * 1024)
> pid_t sys_clone(int (*fn)(void *), void *arg, int flags, int *pidfd)
> {
> pid_t ret;
> void *stack;
>
> stack = malloc(__STACK_SIZE);
> if (!stack)
> return -ENOMEM;
>
> #ifdef __ia64__
> ret = __clone2(fn, stack, __STACK_SIZE, flags | SIGCHLD, arg, pidfd);
> #elif defined(__parisc__) /* stack grows up */
> ret = clone(fn, stack, flags | SIGCHLD, arg, pidfd);
> #else
> ret = clone(fn, stack + __STACK_SIZE, flags | SIGCHLD, arg, pidfd);
> #endif
> return ret;
> }
>
> So we talked to the glibc folks which preferred the kernel to do all
> this nonsense for them as it has that knowledge.
Thanks for the info!
>
> My preference is to keep the api consistent and require a stack_size for
> shadow stacks as well.
Did you catch that a token can be at a different offsets location on the stack
depending on args passed to map_shadow_stack? So userspace will need something
like the code above, but that adjusts the 'shadow_stack_size' such that the
kernel looks for the token in the right place. It will be even weirder if
someone uses clone3 to switch to a stack that has already been used, and pivoted
off of, such that a token was left in the middle of the stack. In that case
userspace would have to come up with args disconnected from the actual size of
the shadow stack such that the kernel would be cajoled into looking for the
token in the right place.
A shadow stack size is more symmetric on the surface, but I'm not sure it will
be easier for userspace to handle. So I think we should just have a pointer to
the token. But it will be a usable implementation either way.
^ permalink raw reply
* Re: [PATCH RFT v9 4/8] fork: Add shadow stack support to clone3()
From: Christian Brauner @ 2024-09-27 8:50 UTC (permalink / raw)
To: Mark Brown
Cc: Edgecombe, Rick P, dietmar.eggemann@arm.com,
linux-kselftest@vger.kernel.org, shuah@kernel.org,
Szabolcs.Nagy@arm.com, dave.hansen@linux.intel.com,
debug@rivosinc.com, mgorman@suse.de, vincent.guittot@linaro.org,
fweimer@redhat.com, linux-kernel@vger.kernel.org,
mingo@redhat.com, rostedt@goodmis.org, hjl.tools@gmail.com,
tglx@linutronix.de, linux-api@vger.kernel.org,
vschneid@redhat.com, kees@kernel.org, will@kernel.org,
hpa@zytor.com, jannh@google.com, peterz@infradead.org,
yury.khrustalev@arm.com, bp@alien8.de, wilco.dijkstra@arm.com,
catalin.marinas@arm.com, bsegall@google.com,
juri.lelli@redhat.com, x86@kernel.org
In-Reply-To: <158190d9-a4a6-4647-84e8-f4ae036d984b@sirena.org.uk>
On Wed, Aug 21, 2024 at 06:23:18PM GMT, Mark Brown wrote:
> On Wed, Aug 21, 2024 at 03:54:49PM +0000, Edgecombe, Rick P wrote:
> > On Wed, 2024-08-21 at 13:45 +0100, Mark Brown wrote:
>
> > > It's entirely possible it just leaked. My own attempts to dig through
> > > the archives haven't turned up anything on the subjecti either, it seems
> > > to have been there from the get go and just gone in without comment.
> > > Equally it could just be that people felt that this was a more tasteful
> > > way of specifying stacks, or that some future use was envisioned.
>
> > Ok, well I'm suspicious, but won't object over it. The rest seems settled from
> > my side. I may try to attract some other x86 attention to that CMPXCHG helper,
> > but otherwise.
>
> OK, I'll post what I've got (with the current ABI) today, incorporating
> your x86 fixes and the tighter validation and we can see what people
> think. Perhaps Christian remembers what's going on there?
The legacy clone system call had required userspace to know in which
direction the stack was growing and then pass down the stack pointer
appropriately (e.g., parisc grows upwards).
And in fact, the old clone() system call did take an additional
stack_size argument on specific architectures. For example, on
microblaze.
Also, when clone3() was done we still had ia64 in the tree which had a
separate clone2() system call that also required a stack_size argument.
So userspace ended up with code like this or worse:
#define __STACK_SIZE (8 * 1024 * 1024)
pid_t sys_clone(int (*fn)(void *), void *arg, int flags, int *pidfd)
{
pid_t ret;
void *stack;
stack = malloc(__STACK_SIZE);
if (!stack)
return -ENOMEM;
#ifdef __ia64__
ret = __clone2(fn, stack, __STACK_SIZE, flags | SIGCHLD, arg, pidfd);
#elif defined(__parisc__) /* stack grows up */
ret = clone(fn, stack, flags | SIGCHLD, arg, pidfd);
#else
ret = clone(fn, stack + __STACK_SIZE, flags | SIGCHLD, arg, pidfd);
#endif
return ret;
}
So we talked to the glibc folks which preferred the kernel to do all
this nonsense for them as it has that knowledge.
My preference is to keep the api consistent and require a stack_size for
shadow stacks as well.
^ permalink raw reply
* Re: [PATCH v3] mm/madvise: unrestrict process_madvise() for current process
From: Lorenzo Stoakes @ 2024-09-27 8:23 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Andrew Morton, Liam R . Howlett, Suren Baghdasaryan,
Arnd Bergmann, Shakeel Butt, linux-api, linux-mm, linux-kernel,
Minchan Kim, Christian Brauner, pedro.falcato
In-Reply-To: <c96c4cf6-e007-4a19-a830-4a2a073620e7@suse.cz>
On Fri, Sep 27, 2024 at 10:12:33AM GMT, Vlastimil Babka wrote:
> On 9/26/24 17:10, Lorenzo Stoakes wrote:
> > The process_madvise() call was introduced in commit ecb8ac8b1f14
> > ("mm/madvise: introduce process_madvise() syscall: an external memory
> > hinting API") as a means of performing madvise() operations on another
> > process.
> >
> > However, as it provides the means by which to perform multiple madvise()
> > operations in a batch via an iovec, it is useful to utilise the same
> > interface for performing operations on the current process rather than a
> > remote one.
> >
> > Commit 22af8caff7d1 ("mm/madvise: process_madvise() drop capability check
> > if same mm") removed the need for a caller invoking process_madvise() on
> > its own pidfd to possess the CAP_SYS_NICE capability, however this leaves
> > the restrictions on operation in place.
> >
> > Resolve this by only applying the restriction on operations when accessing
> > a remote process.
> >
> > Moving forward we plan to implement a simpler means of specifying this
> > condition other than needing to establish a self pidfd, perhaps in the form
> > of a sentinel pidfd.
> >
> > Also take the opportunity to refactor the system call implementation
> > abstracting the vectorised operation.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
>
> Looks like the destructive modes should work with the vectorized version
> too, and with how it returns a partial success.
>
Right yeah, intent is to allow those modes when referring to the current
process. Partial success logic across ranges mirrors how we generally handle
partial success in destructive operations so should all be good.
> We'll need a man page update though, right?
>
Ack, I intend to send a patch for this separately.
^ permalink raw reply
* Re: [PATCH v3] mm/madvise: unrestrict process_madvise() for current process
From: Lorenzo Stoakes @ 2024-09-27 8:21 UTC (permalink / raw)
To: Christian Brauner
Cc: Shakeel Butt, Andrew Morton, Vlastimil Babka, Liam R . Howlett,
Suren Baghdasaryan, Arnd Bergmann, linux-api, linux-mm,
linux-kernel, Minchan Kim, pedro.falcato
In-Reply-To: <20240927-holzlatten-karierte-2f1e5c0af19d@brauner>
On Fri, Sep 27, 2024 at 10:04:25AM GMT, Christian Brauner wrote:
> On Thu, Sep 26, 2024 at 08:52:32AM GMT, Shakeel Butt wrote:
> > On Thu, Sep 26, 2024 at 04:10:19PM GMT, Lorenzo Stoakes wrote:
> > > The process_madvise() call was introduced in commit ecb8ac8b1f14
> > > ("mm/madvise: introduce process_madvise() syscall: an external memory
> > > hinting API") as a means of performing madvise() operations on another
> > > process.
> > >
> > > However, as it provides the means by which to perform multiple madvise()
> > > operations in a batch via an iovec, it is useful to utilise the same
> > > interface for performing operations on the current process rather than a
> > > remote one.
> > >
> > > Commit 22af8caff7d1 ("mm/madvise: process_madvise() drop capability check
> > > if same mm") removed the need for a caller invoking process_madvise() on
> > > its own pidfd to possess the CAP_SYS_NICE capability, however this leaves
> > > the restrictions on operation in place.
> > >
> > > Resolve this by only applying the restriction on operations when accessing
> > > a remote process.
> > >
> > > Moving forward we plan to implement a simpler means of specifying this
> > > condition other than needing to establish a self pidfd, perhaps in the form
> > > of a sentinel pidfd.
> > >
> > > Also take the opportunity to refactor the system call implementation
> > > abstracting the vectorised operation.
> > >
> > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> >
> > Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
> >
> > > ---
> > > v3:
> > > * Avoid introducing PR_MADV_SELF and defer a non-pidfd version until later.
> >
> > Seems like a good plan to decouple this patch from PR_MADV_SELF vs
> > PIDFD_SELF decision. I am hoping to see the follow up patch as well.
>
> PIDFD_SELF should absolutely not be a per-system call thing. It should
> be generic across all pidfd based system calls similar to AT_FDCWD.
>
> IOW, that should be in:
>
> include/uapi/linux/pidfd.h
>
> #define PIDFD_SELF -200
Yes this is what I was saying elsewhere in the thread :) this is why it's
important to have this as a separate enterprise.
And indeed this is the intent, I will be working on a separate patch series
to this effect. It also gives us the space to implement it in calls which
use pidfd where it makes sense and to extend testing accordingly.
^ permalink raw reply
* Re: [PATCH v3] mm/madvise: unrestrict process_madvise() for current process
From: Vlastimil Babka @ 2024-09-27 8:12 UTC (permalink / raw)
To: Lorenzo Stoakes, Andrew Morton
Cc: Liam R . Howlett, Suren Baghdasaryan, Arnd Bergmann, Shakeel Butt,
linux-api, linux-mm, linux-kernel, Minchan Kim, Christian Brauner,
pedro.falcato
In-Reply-To: <20240926151019.82902-1-lorenzo.stoakes@oracle.com>
On 9/26/24 17:10, Lorenzo Stoakes wrote:
> The process_madvise() call was introduced in commit ecb8ac8b1f14
> ("mm/madvise: introduce process_madvise() syscall: an external memory
> hinting API") as a means of performing madvise() operations on another
> process.
>
> However, as it provides the means by which to perform multiple madvise()
> operations in a batch via an iovec, it is useful to utilise the same
> interface for performing operations on the current process rather than a
> remote one.
>
> Commit 22af8caff7d1 ("mm/madvise: process_madvise() drop capability check
> if same mm") removed the need for a caller invoking process_madvise() on
> its own pidfd to possess the CAP_SYS_NICE capability, however this leaves
> the restrictions on operation in place.
>
> Resolve this by only applying the restriction on operations when accessing
> a remote process.
>
> Moving forward we plan to implement a simpler means of specifying this
> condition other than needing to establish a self pidfd, perhaps in the form
> of a sentinel pidfd.
>
> Also take the opportunity to refactor the system call implementation
> abstracting the vectorised operation.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
Looks like the destructive modes should work with the vectorized version
too, and with how it returns a partial success.
We'll need a man page update though, right?
^ permalink raw reply
* Re: [PATCH v3] mm/madvise: unrestrict process_madvise() for current process
From: Christian Brauner @ 2024-09-27 8:04 UTC (permalink / raw)
To: Shakeel Butt
Cc: Lorenzo Stoakes, Andrew Morton, Vlastimil Babka, Liam R . Howlett,
Suren Baghdasaryan, Arnd Bergmann, linux-api, linux-mm,
linux-kernel, Minchan Kim, pedro.falcato
In-Reply-To: <xv5qg4hfqvqzooounx57hzl4jzmfefitf3qklcdqzz7a4dufxn@v3r47r7p6ono>
On Thu, Sep 26, 2024 at 08:52:32AM GMT, Shakeel Butt wrote:
> On Thu, Sep 26, 2024 at 04:10:19PM GMT, Lorenzo Stoakes wrote:
> > The process_madvise() call was introduced in commit ecb8ac8b1f14
> > ("mm/madvise: introduce process_madvise() syscall: an external memory
> > hinting API") as a means of performing madvise() operations on another
> > process.
> >
> > However, as it provides the means by which to perform multiple madvise()
> > operations in a batch via an iovec, it is useful to utilise the same
> > interface for performing operations on the current process rather than a
> > remote one.
> >
> > Commit 22af8caff7d1 ("mm/madvise: process_madvise() drop capability check
> > if same mm") removed the need for a caller invoking process_madvise() on
> > its own pidfd to possess the CAP_SYS_NICE capability, however this leaves
> > the restrictions on operation in place.
> >
> > Resolve this by only applying the restriction on operations when accessing
> > a remote process.
> >
> > Moving forward we plan to implement a simpler means of specifying this
> > condition other than needing to establish a self pidfd, perhaps in the form
> > of a sentinel pidfd.
> >
> > Also take the opportunity to refactor the system call implementation
> > abstracting the vectorised operation.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
>
> > ---
> > v3:
> > * Avoid introducing PR_MADV_SELF and defer a non-pidfd version until later.
>
> Seems like a good plan to decouple this patch from PR_MADV_SELF vs
> PIDFD_SELF decision. I am hoping to see the follow up patch as well.
PIDFD_SELF should absolutely not be a per-system call thing. It should
be generic across all pidfd based system calls similar to AT_FDCWD.
IOW, that should be in:
include/uapi/linux/pidfd.h
#define PIDFD_SELF -200
^ permalink raw reply
* [PATCH] dup.2: Add ENOMEM to ERRORS
From: Levi Zim via B4 Relay @ 2024-09-27 6:52 UTC (permalink / raw)
To: linux-man; +Cc: Linux API, Levi Zim
From: Levi Zim <rsworktech@outlook.com>
dup2 could return ENOMEM under extreme condition.
For example, when sysctl fs.nr_open=2147483584, and RLIMIT_NOFILE is also
2147483584. The following program fails with ENOMEM:
int main(){
int ret = dup2(0, 2000000000);
if (ret < 0) {
perror("dup2");
return 1;
}
return 0;
}
This ENOMEM comes from an allocation error here:
https://elixir.bootlin.com/linux/v6.1/source/mm/util.c#L596
ENOMEM is already documented for open(2).
Signed-off-by: Levi Zim <rsworktech@outlook.com>
---
While experimenting with fs.nr_open sysctl and RLIMIT_NOFILE,
I noticed that dup(2) could return an undocumented errno ENOMEM.
This patch add ENOMEM to ERRORS in dup.2.
---
man/man2/dup.2 | 3 +++
1 file changed, 3 insertions(+)
diff --git a/man/man2/dup.2 b/man/man2/dup.2
index ee964a827ca062d90419c7d39f457de270d42ca3..719f0ae9e78165733cbe607bbf8a803a5d0943b4 100644
--- a/man/man2/dup.2
+++ b/man/man2/dup.2
@@ -197,6 +197,9 @@ .SH ERRORS
.B RLIMIT_NOFILE
in
.BR getrlimit (2)).
+.TP
+.B ENOMEM
+Insufficient kernel memory was available.
.SH STANDARDS
.TP
.BR dup ()
---
base-commit: 1e2d36deb2de1dbaf7084bb7dc3cb2c170cce852
change-id: 20240927-dup2-727e986e1900
Best regards,
--
Levi Zim <rsworktech@outlook.com>
^ permalink raw reply related
* Re: [PATCH v3] mm/madvise: unrestrict process_madvise() for current process
From: Lorenzo Stoakes @ 2024-09-26 16:36 UTC (permalink / raw)
To: Shakeel Butt
Cc: Andrew Morton, Vlastimil Babka, Liam R . Howlett,
Suren Baghdasaryan, Arnd Bergmann, linux-api, linux-mm,
linux-kernel, Minchan Kim, Christian Brauner, pedro.falcato
In-Reply-To: <xv5qg4hfqvqzooounx57hzl4jzmfefitf3qklcdqzz7a4dufxn@v3r47r7p6ono>
On Thu, Sep 26, 2024 at 08:52:32AM GMT, Shakeel Butt wrote:
> On Thu, Sep 26, 2024 at 04:10:19PM GMT, Lorenzo Stoakes wrote:
> > The process_madvise() call was introduced in commit ecb8ac8b1f14
> > ("mm/madvise: introduce process_madvise() syscall: an external memory
> > hinting API") as a means of performing madvise() operations on another
> > process.
> >
> > However, as it provides the means by which to perform multiple madvise()
> > operations in a batch via an iovec, it is useful to utilise the same
> > interface for performing operations on the current process rather than a
> > remote one.
> >
> > Commit 22af8caff7d1 ("mm/madvise: process_madvise() drop capability check
> > if same mm") removed the need for a caller invoking process_madvise() on
> > its own pidfd to possess the CAP_SYS_NICE capability, however this leaves
> > the restrictions on operation in place.
> >
> > Resolve this by only applying the restriction on operations when accessing
> > a remote process.
> >
> > Moving forward we plan to implement a simpler means of specifying this
> > condition other than needing to establish a self pidfd, perhaps in the form
> > of a sentinel pidfd.
> >
> > Also take the opportunity to refactor the system call implementation
> > abstracting the vectorised operation.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
Thanks!
>
> > ---
> > v3:
> > * Avoid introducing PR_MADV_SELF and defer a non-pidfd version until later.
>
> Seems like a good plan to decouple this patch from PR_MADV_SELF vs
> PIDFD_SELF decision. I am hoping to see the follow up patch as well.
>
> thanks,
> Shakeel
>
Yes agreed, this gets an important part of the change in, and gives us room
to take our time on that side of things.
Plan right now is I will work on a sentinel solution in parallel to other
stuff and see how that goes, if it integrates well can bring it in in a
separate patch series.
Cheers, Lorenzo
^ permalink raw reply
* Re: [PATCH v3] mm/madvise: unrestrict process_madvise() for current process
From: Shakeel Butt @ 2024-09-26 15:52 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Andrew Morton, Vlastimil Babka, Liam R . Howlett,
Suren Baghdasaryan, Arnd Bergmann, linux-api, linux-mm,
linux-kernel, Minchan Kim, Christian Brauner, pedro.falcato
In-Reply-To: <20240926151019.82902-1-lorenzo.stoakes@oracle.com>
On Thu, Sep 26, 2024 at 04:10:19PM GMT, Lorenzo Stoakes wrote:
> The process_madvise() call was introduced in commit ecb8ac8b1f14
> ("mm/madvise: introduce process_madvise() syscall: an external memory
> hinting API") as a means of performing madvise() operations on another
> process.
>
> However, as it provides the means by which to perform multiple madvise()
> operations in a batch via an iovec, it is useful to utilise the same
> interface for performing operations on the current process rather than a
> remote one.
>
> Commit 22af8caff7d1 ("mm/madvise: process_madvise() drop capability check
> if same mm") removed the need for a caller invoking process_madvise() on
> its own pidfd to possess the CAP_SYS_NICE capability, however this leaves
> the restrictions on operation in place.
>
> Resolve this by only applying the restriction on operations when accessing
> a remote process.
>
> Moving forward we plan to implement a simpler means of specifying this
> condition other than needing to establish a self pidfd, perhaps in the form
> of a sentinel pidfd.
>
> Also take the opportunity to refactor the system call implementation
> abstracting the vectorised operation.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
> ---
> v3:
> * Avoid introducing PR_MADV_SELF and defer a non-pidfd version until later.
Seems like a good plan to decouple this patch from PR_MADV_SELF vs
PIDFD_SELF decision. I am hoping to see the follow up patch as well.
thanks,
Shakeel
^ permalink raw reply
* [PATCH v3] mm/madvise: unrestrict process_madvise() for current process
From: Lorenzo Stoakes @ 2024-09-26 15:10 UTC (permalink / raw)
To: Andrew Morton
Cc: Vlastimil Babka, Liam R . Howlett, Suren Baghdasaryan,
Arnd Bergmann, Shakeel Butt, linux-api, linux-mm, linux-kernel,
Minchan Kim, Christian Brauner, pedro.falcato
The process_madvise() call was introduced in commit ecb8ac8b1f14
("mm/madvise: introduce process_madvise() syscall: an external memory
hinting API") as a means of performing madvise() operations on another
process.
However, as it provides the means by which to perform multiple madvise()
operations in a batch via an iovec, it is useful to utilise the same
interface for performing operations on the current process rather than a
remote one.
Commit 22af8caff7d1 ("mm/madvise: process_madvise() drop capability check
if same mm") removed the need for a caller invoking process_madvise() on
its own pidfd to possess the CAP_SYS_NICE capability, however this leaves
the restrictions on operation in place.
Resolve this by only applying the restriction on operations when accessing
a remote process.
Moving forward we plan to implement a simpler means of specifying this
condition other than needing to establish a self pidfd, perhaps in the form
of a sentinel pidfd.
Also take the opportunity to refactor the system call implementation
abstracting the vectorised operation.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
v3:
* Avoid introducing PR_MADV_SELF and defer a non-pidfd version until later.
v2:
* Fix silly mistake referencing unassigned mm variable.
* Add PR_MADV_SELF to architecture-specific mman headers.
https://lore.kernel.org/linux-mm/9cf0e96d-287f-4749-ba85-8414c06aa54c@lucifer.local/
v1:
https://lore.kernel.org/all/cover.1727106751.git.lorenzo.stoakes@oracle.com/
mm/madvise.c | 55 ++++++++++++++++++++++++++++++++++------------------
1 file changed, 36 insertions(+), 19 deletions(-)
diff --git a/mm/madvise.c b/mm/madvise.c
index 50d223ab3894..e871a72a6c32 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1208,7 +1208,8 @@ madvise_behavior_valid(int behavior)
}
}
-static bool process_madvise_behavior_valid(int behavior)
+/* Can we invoke process_madvise() on a remote mm for the specified behavior? */
+static bool process_madvise_remote_valid(int behavior)
{
switch (behavior) {
case MADV_COLD:
@@ -1477,6 +1478,28 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
return do_madvise(current->mm, start, len_in, behavior);
}
+/* Perform an madvise operation over a vector of addresses and lengths. */
+static ssize_t vector_madvise(struct mm_struct *mm, struct iov_iter *iter,
+ int behavior)
+{
+ ssize_t ret = 0;
+ size_t total_len;
+
+ total_len = iov_iter_count(iter);
+
+ while (iov_iter_count(iter)) {
+ ret = do_madvise(mm, (unsigned long)iter_iov_addr(iter),
+ iter_iov_len(iter), behavior);
+ if (ret < 0)
+ break;
+ iov_iter_advance(iter, iter_iov_len(iter));
+ }
+
+ ret = (total_len - iov_iter_count(iter)) ? : ret;
+
+ return ret;
+}
+
SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
size_t, vlen, int, behavior, unsigned int, flags)
{
@@ -1486,7 +1509,6 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
struct iov_iter iter;
struct task_struct *task;
struct mm_struct *mm;
- size_t total_len;
unsigned int f_flags;
if (flags != 0) {
@@ -1504,11 +1526,6 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
goto free_iov;
}
- if (!process_madvise_behavior_valid(behavior)) {
- ret = -EINVAL;
- goto release_task;
- }
-
/* Require PTRACE_MODE_READ to avoid leaking ASLR metadata. */
mm = mm_access(task, PTRACE_MODE_READ_FSCREDS);
if (IS_ERR(mm)) {
@@ -1516,26 +1533,26 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
goto release_task;
}
+ /*
+ * We need only perform this check if we are attempting to manipulate a
+ * remote process's address space.
+ */
+ if (mm != current->mm && !process_madvise_remote_valid(behavior)) {
+ ret = -EINVAL;
+ goto release_mm;
+ }
+
/*
* Require CAP_SYS_NICE for influencing process performance. Note that
- * only non-destructive hints are currently supported.
+ * only non-destructive hints are currently supported for remote
+ * processes.
*/
if (mm != current->mm && !capable(CAP_SYS_NICE)) {
ret = -EPERM;
goto release_mm;
}
- total_len = iov_iter_count(&iter);
-
- while (iov_iter_count(&iter)) {
- ret = do_madvise(mm, (unsigned long)iter_iov_addr(&iter),
- iter_iov_len(&iter), behavior);
- if (ret < 0)
- break;
- iov_iter_advance(&iter, iter_iov_len(&iter));
- }
-
- ret = (total_len - iov_iter_count(&iter)) ? : ret;
+ ret = vector_madvise(mm, &iter, behavior);
release_mm:
mmput(mm);
--
2.46.0
^ permalink raw reply related
* Re: [PATCH v2 0/2] unrestrict process_madvise() for current process
From: Lorenzo Stoakes @ 2024-09-26 9:44 UTC (permalink / raw)
To: Andrew Morton
Cc: Vlastimil Babka, Liam R . Howlett, Suren Baghdasaryan,
Arnd Bergmann, Shakeel Butt, linux-api, linux-mm, linux-kernel,
Minchan Kim, Richard Henderson, Ivan Kokshaysky, Matt Turner,
linux-alpha, Thomas Bogendoerfer, linux-mips,
James E . J . Bottomley, Helge Deller, linux-parisc, Chris Zankel,
Max Filippov, christian
In-Reply-To: <cover.1727176176.git.lorenzo.stoakes@oracle.com>
Hi Andrew, please drop this series, I'm going to take a different approach.
Thanks.
[snip]
^ permalink raw reply
* Re: [PATCH v2 1/2] mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
From: Shakeel Butt @ 2024-09-25 21:37 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Pedro Falcato, Andrew Morton, Vlastimil Babka, Liam R . Howlett,
Suren Baghdasaryan, Arnd Bergmann, linux-api, linux-mm,
linux-kernel, Minchan Kim, Richard Henderson, Ivan Kokshaysky,
Matt Turner, linux-alpha, Thomas Bogendoerfer, linux-mips,
James E . J . Bottomley, Helge Deller, linux-parisc, Chris Zankel,
Max Filippov, christian
In-Reply-To: <6b449c32-0954-4db1-9df5-23b766dc2d9a@lucifer.local>
On Wed, Sep 25, 2024 at 06:04:59PM GMT, Lorenzo Stoakes wrote:
> On Wed, Sep 25, 2024 at 09:19:17AM GMT, Shakeel Butt wrote:
> > I have no idea what makes you think I am blocking the feature that you
> > repond in a weird tone but let me be upfront what I am asking: Let's
> > collectively decide which is the better option (in terms of
> > maintainability and extensibility) and move forward.
>
> I'm not sure what you mean by 'weird tone'... perhaps a miscommunication?
>
> To summarise in my view - a suggestion was made to, rather than provide the
> proposed flag - a pidfd sentinel should be introduced.
>
> Simply introducing a sentinel that represents 'the current process' without
> changing interfaces that accept a pidfd would be broken - so implementing
> this implies that _all_ pidfd interfaces are updated, as well as tests.
>
> I suggest doing so is, of course, entirely out of the scope of this
> change. Therefore if we were to require that here - it would block the
> feature while I go work on that.
>
> I think this is pretty clear right? And I also suggest that doing so is
> likely to take quite some time, and may not even have a positive outcome.
If you have some concrete example on how this may not have a positive
outcome then it will make your case much stronger.
>
> So it's not a case of 'shall we take approach A or approach B?' but rather
> 'should we take approach A or entirely implement a new feature B, then once
> that is done, use it'.
The "entire new feature" is a bit too strong IMHO. (though no pushback
from me).
>
> So as to your 'collectively decide what is the better option' - in my
> previous response I argued that the best approach between 'use an
> unimplemented suggested entirely new feature of pidfd' vs. 'implement a
> flag that would in no way block the prior approach' - a flag works better.
>
> If you can provide specific arguments as to why I'm wrong then by all means
> I'm happy to hear them.
>
> >
> > On Wed, Sep 25, 2024 at 03:48:07PM GMT, Lorenzo Stoakes wrote:
> > > On Wed, Sep 25, 2024 at 07:02:59AM GMT, Shakeel Butt wrote:
> > > > Cced Christian
> > > >
> > > > On Tue, Sep 24, 2024 at 02:12:49PM GMT, Lorenzo Stoakes wrote:
> > > > > On Tue, Sep 24, 2024 at 01:51:11PM GMT, Pedro Falcato wrote:
> > > > > > On Tue, Sep 24, 2024 at 12:16:27PM GMT, Lorenzo Stoakes wrote:
> > > > > > > process_madvise() was conceived as a useful means for performing a vector
> > > > > > > of madvise() operations on a remote process's address space.
> > > > > > >
> > > > > > > However it's useful to be able to do so on the current process also. It is
> > > > > > > currently rather clunky to do this (requiring a pidfd to be opened for the
> > > > > > > current process) and introduces unnecessary overhead in incrementing
> > > > > > > reference counts for the task and mm.
> > > > > > >
> > > > > > > Avoid all of this by providing a PR_MADV_SELF flag, which causes
> > > > > > > process_madvise() to simply ignore the pidfd parameter and instead apply
> > > > > > > the operation to the current process.
> > > > > > >
> > > > > >
> > > > > > How about simply defining a pseudo-fd PIDFD_SELF in the negative int space?
> > > > > > There's precedent for it in the fs space (AT_FDCWD). I think it's more ergonomic
> > > > > > and if you take out the errno space we have around 2^31 - 4096 available sentinel
> > > > > > values.
> > > > > >
> > > > > > e.g:
> > > > > >
> > > > > > /* AT_FDCWD = -10, -1 is dangerous, pick a different value */
> > > > > > #define PIDFD_SELF -11
> > > > > >
> > > > > > int pidfd = target_pid == getpid() ? PIDFD_SELF : pidfd_open(...);
> > > > > > process_madvise(pidfd, ...);
> > > > > >
> > > > > >
> > > > > > What do you think?
> > > > >
> > > > > I like the way you're thinking, but I don't think this is something we can
> > > > > do in the context of this series.
> > > > >
> > > > > I mean, I totally accept using a flag here and ignoring the pidfd field is
> > > > > _ugly_, no question. But I'm trying to find the smallest change that
> > > > > achieves what we want.
> > > >
> > > > I don't think "smallest change" should be the target. We are changing
> > > > user API and we should aim to make it as robust as possible against
> > > > possible misuse or making uninteded assumptions.
> > >
> > > I think introducing a new pidfd sentinel that isn't used anywhere else is
> > > far more liable to mistakes than adding an explicit flag.
> > >
> > > Could you provide examples of possible misuse of this flag or unintended
> > > assumptions it confers (other than the -1 thing addressed below).
> > >
> > > The flag is explicitly 'target this process, ignore pidfd'. We can document
> > > it as such (I will patch manpages too).
> > >
> > > >
> > > > The proposed implementation opened the door for the applications to
> > > > provide dummy pidfd if PR_MADV_SELF is used. You definitely need to
> > > > restrict it to some known value like -1 used by mmap() syscall.
> > >
> > > Why?
> > >
> > > mmap() is special in that you have a 'dual' situation with shmem that is
> > > both file-backed and private and of course you can do MAP_SHARED |
> > > MAP_PRIVATE and have mmap() transparently assign something to you, etc.
> > >
> > > Here we explicitly have a flag whose semantics are 'ignore pidfd, target
> > > self'.
> > >
> > > If you choose to use a brand new flag that explicitly states this and
> > > provide a 'dummy' pidfd which then has nothing done to it - what exactly is
> > > the problem?
> >
> > IMHO having a fixed dummy would allow the kernel more flexibility in
> > future for evolving the API.
>
> OK. I agree with having a fixed dummy value as stated.
>
> >
> > >
> > > I mean if you feel strongly, we can enforce this, but I'm not sure -1
> > > implying a special case for pidfd is a thing either.
> > >
> > > On the other hand it would be _weird_ and broken for the user to provide a
> > > valid pidfd so maybe we should as it is easy to do and the user has clearly
> > > done something wrong.
> > >
> > > So fine, agreed, I'll add that.
> > >
> >
> > No, don't just agree. The response like "-1 is not good for so and so
> > reasons" is totally fine and my request would be add that reasoning in
> > the commit message. My only request is that we have thought through
> > alternatives and document the reasonsing behind the decided approach.
>
> I didn't just agree, as I said, my reasoning is:
>
> On the other hand it would be _weird_ and broken for the user to
> provide a valid pidfd so maybe we should as it is easy to do and
> the user has clearly done something wrong.
>
> If we're in alignment with that then all good!
>
> >
> > > >
> > > > >
> > > > > To add such a sentinel would be a change to the pidfd mechanism as a whole,
> > > > > and we'd be left in the awkward situation that no other user of the pidfd
> > > > > mechanism would be implementing this, but we'd have to expose this as a
> > > > > general sentinel value for all pidfd users.
> > > >
> > > > There might be future users which can take advantage of this. I can even
> > > > imagine pidfd_send_signal() can use PIDFD_SELF as well.
> > >
> > > I'm confused by this comment - I mean absolutely, as I said I like the
> > > idea, but this just proves the point that you'd have to go around and
> > > implement this everywhere that uses a pidfd?
> > >
> > > That is a big undertaking, and not blocked by this change. Nor is
> > > maintaining the flag proposed here egregious.
> >
> > By big undertaking, do you mean other syscalls that take pidfd
> > (pidfd_getfd, pidfd_send_signal & process_mrelease) to handle PIDFD_SELF
> > or something else?
>
> I mean if you add a pidfd sentinel that represents 'the current process' it
> may get passed to any interface that accepts a pidfd, so all of them have
> to handle it _somehow_.
>
> Also you'll want to update tests accordingly and clearly need to get
> community buy-in for that feature.
>
> You may want to just add a bunch of:
>
> if (pidfd == SENTINEL)
> return -EINVAL;
>
> So it's not impossible my instincts are off and we can get away with simply
> doing that.
>
> On the other hand, would that be confusing? Wouldn't we need to update
> documentation, manpages, etc. to say explicitly 'hey this sentinel is just
> not supported'?
>
> Again totally fine with the idea, like it actually, just my instincts are
> it will involve some work. I may be wrong.
>
> >
> > >
> > > Blocking a useful feature because we may in future possibly add a new means
> > > of doing the same thing seems a little silly to me.
> > >
> >
> > Hah!!
>
> See top of mail.
>
> >
> > > > >
> > > > > One nice thing with doing this as a flag is that, later, if somebody is
> > > > > willing to do the larger task of having a special sentinel pidfd value to
> > > > > mean 'the current process', we could use this in process_madvise() and
> > > > > deprecate this flag :)
> > > > >
> > > >
> > > > Once something is added to an API, particularly syscalls, the removal
> > > > is almost impossible.
> > >
> > > And why would it be such a problem to have this flag remain? I said
> > > deprecate not remove. And only in the sense that 'you may as well use the
> > > sentinel'.
> > >
> >
> > My point was to aim for the solution where we can avoid such scenario
> > but it is totally understandable and acceptable that we still have to go
> > through deprecation process in future.
> >
> > > The flag is very clear in its meaning, and confers no special problem in
> > > remaining supported. It is a private flag that overlaps no others.
> > >
> > > I mean it'd in effect being a change to a single line 'if pidfd is sentinel
> > > or flag is used'. If we can't support that going forward, then we should
> > > give up this kernel stuff and frolick in the fields joyously instead...
> > >
> > > Again, if you can tell me why it'd be such a problem then fine we can
> > > address that.
> > >
> > > But blocking a series and demanding a change to an entire other feature
> > > just to support something I'd say requires some pretty specific reasons as
> > > to why you have a problem with the change.
> > >
> > > >
> > > > Anyways, I don't have very strong opinion one way or other but whatever
> > > > we decide, let's make it robust.
> > >
> > > I mean... err... it sounds like you do kinda have pretty strong opinions ;)
> >
> > I am not sure how more explicit I have to be to but I am hoping now it
> > is more clear than before.
>
> I mean perhaps I misinterpreted you as strongly advocating for the sentinel
> and your intent was rather to provide argument on that side also so the
> community can decide as you say - sure.
>
> But with you indifferent as you say as to which way to go, and my having
> provided arguments for the flags (again happy to hear push-back of course)
> - I suggest we go forward with the series as-is, other than a fixpatch I'll
> send for the -1 thing.
>
My only request would be to add all these points in the commit message
i.e. why we took this approach rather than the alternative.
> >
> > Shakeel
>
> Thanks for your review!
^ permalink raw reply
* Re: [PATCH v2 1/2] mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
From: Lorenzo Stoakes @ 2024-09-25 18:31 UTC (permalink / raw)
To: Pedro Falcato
Cc: Shakeel Butt, Andrew Morton, Vlastimil Babka, Liam R . Howlett,
Suren Baghdasaryan, Arnd Bergmann, linux-api, linux-mm,
linux-kernel, Minchan Kim, Richard Henderson, Ivan Kokshaysky,
Matt Turner, linux-alpha, Thomas Bogendoerfer, linux-mips,
James E . J . Bottomley, Helge Deller, linux-parisc, Chris Zankel,
Max Filippov, christian
In-Reply-To: <asbdij3q2iabrnq5wdxmcq3g7ofut2malicushswi3rma6glf5@k6eftopwmwvj>
On Wed, Sep 25, 2024 at 07:14:51PM GMT, Pedro Falcato wrote:
> On Wed, Sep 25, 2024 at 06:04:59PM GMT, Lorenzo Stoakes wrote:
> > On Wed, Sep 25, 2024 at 09:19:17AM GMT, Shakeel Butt wrote:
> > > I have no idea what makes you think I am blocking the feature that you
> > > repond in a weird tone but let me be upfront what I am asking: Let's
> > > collectively decide which is the better option (in terms of
> > > maintainability and extensibility) and move forward.
> >
> > I'm not sure what you mean by 'weird tone'... perhaps a miscommunication?
> >
> > To summarise in my view - a suggestion was made to, rather than provide the
> > proposed flag - a pidfd sentinel should be introduced.
> >
> > Simply introducing a sentinel that represents 'the current process' without
> > changing interfaces that accept a pidfd would be broken - so implementing
> > this implies that _all_ pidfd interfaces are updated, as well as tests.
> >
>
> While I suggested PIDFD_SELF, I never meant that we should change every interface,
> but rather adopt a sound, consistent strategy for pidfd interfaces and stick with
> it for the foreseeable future.
If we add this to a public uapi-facing header and document it as 'refer to
self', we will introduce something that users will receive and be confused
if they are unable to use anywhere else.
This is the fundamental problem with this. It's a fundamental, permanent
uAPI change which either introduces confusion - because it only works for
process_madvise() - or will need work along with test updates etc.
And it is very much out of scope for this series as a result.
I may just back out of doing this and replace this with something simpler
that causes less push-back (and fixes existing broken behaviour in
process_madvise()) that lets me do what I need to do with the guard
pages. I don't think anybody can object to a self-pidfd having unrestricted
access to madvise flags...
I may in parallel just try to implement the pidfd sentinel idea and let it
take that long time and update process_madvise() later, as I can't really
let this block the guard page work.
>
> In this case, we'd adapt process_madvise, then possibly later pidfd_send_signal, etc.
> There are plenty of pidfd interfaces that don't make sense with a PIDFD_SELF. Various
> other interfaces will probably never want to adopt it at all (select _can't_, other
> fs syscalls such as read/write/poll/whatever would require awful handholding from
> various kernel subsystems, while in that sense we would definitely require a proper
> struct file/inode/whatever for each pseudo-fd, which is not exactly what we want).
>
And arguably, you'd have to audit all of them and decide. I mean I think
this kind of sums up my point really right?
Again, I don't object to the idea, I object to the suggestion that you
don't need to do this other work.
> > I suggest doing so is, of course, entirely out of the scope of this
> > change. Therefore if we were to require that here - it would block the
> > feature while I go work on that.
> >
> > I think this is pretty clear right? And I also suggest that doing so is
> > likely to take quite some time, and may not even have a positive outcome.
> >
> > So it's not a case of 'shall we take approach A or approach B?' but rather
> > 'should we take approach A or entirely implement a new feature B, then once
> > that is done, use it'.
> >
> > So as to your 'collectively decide what is the better option' - in my
> > previous response I argued that the best approach between 'use an
> > unimplemented suggested entirely new feature of pidfd' vs. 'implement a
> > flag that would in no way block the prior approach' - a flag works better.
>
> I just don't think it's a new feature, just an established, future-proof way
> of doing things :) Your patch should remain mostly similar apart from switching
> the flag check into an fd check.
It is absolutely a new feature, you're adding an entirely new UAPI-visible
flag that is applicable to all pidfd's, unless we make it
process_madvise()-specific in the flag, and I'm not sure that's going to
get accepted.
It also needs to be separately accepted by the maintainers of the relevant
file header etc.
>
> > >
> > > By big undertaking, do you mean other syscalls that take pidfd
> > > (pidfd_getfd, pidfd_send_signal & process_mrelease) to handle PIDFD_SELF
> > > or something else?
> >
> > I mean if you add a pidfd sentinel that represents 'the current process' it
> > may get passed to any interface that accepts a pidfd, so all of them have
> > to handle it _somehow_.
> >
> > Also you'll want to update tests accordingly and clearly need to get
> > community buy-in for that feature.
> >
> > You may want to just add a bunch of:
> >
> > if (pidfd == SENTINEL)
> > return -EINVAL;
>
> It should already be there in the form of an -EBADF.
:) this is pseudo-code. And I'd want to check all pidfd handled it
correctly. I mean you'd think so right...
>
> >
> > So it's not impossible my instincts are off and we can get away with simply
> > doing that.
> >
> > On the other hand, would that be confusing? Wouldn't we need to update
> > documentation, manpages, etc. to say explicitly 'hey this sentinel is just
> > not supported'?
>
> This is a fair point, but we could also just... not :) which I don't feel is too
> wrong, since the fd works kind of like a flag here.
Yeah, no, I think you would have to. It's not specific to
process_madvise(), it's a general fd flag.
And like I said, if we did introduce this we'd need additional assessment
and review from those guys.
>
> --
> Pedro
^ permalink raw reply
* Re: [PATCH v2 1/2] mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
From: Pedro Falcato @ 2024-09-25 18:14 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Shakeel Butt, Andrew Morton, Vlastimil Babka, Liam R . Howlett,
Suren Baghdasaryan, Arnd Bergmann, linux-api, linux-mm,
linux-kernel, Minchan Kim, Richard Henderson, Ivan Kokshaysky,
Matt Turner, linux-alpha, Thomas Bogendoerfer, linux-mips,
James E . J . Bottomley, Helge Deller, linux-parisc, Chris Zankel,
Max Filippov, christian
In-Reply-To: <6b449c32-0954-4db1-9df5-23b766dc2d9a@lucifer.local>
On Wed, Sep 25, 2024 at 06:04:59PM GMT, Lorenzo Stoakes wrote:
> On Wed, Sep 25, 2024 at 09:19:17AM GMT, Shakeel Butt wrote:
> > I have no idea what makes you think I am blocking the feature that you
> > repond in a weird tone but let me be upfront what I am asking: Let's
> > collectively decide which is the better option (in terms of
> > maintainability and extensibility) and move forward.
>
> I'm not sure what you mean by 'weird tone'... perhaps a miscommunication?
>
> To summarise in my view - a suggestion was made to, rather than provide the
> proposed flag - a pidfd sentinel should be introduced.
>
> Simply introducing a sentinel that represents 'the current process' without
> changing interfaces that accept a pidfd would be broken - so implementing
> this implies that _all_ pidfd interfaces are updated, as well as tests.
>
While I suggested PIDFD_SELF, I never meant that we should change every interface,
but rather adopt a sound, consistent strategy for pidfd interfaces and stick with
it for the foreseeable future.
In this case, we'd adapt process_madvise, then possibly later pidfd_send_signal, etc.
There are plenty of pidfd interfaces that don't make sense with a PIDFD_SELF. Various
other interfaces will probably never want to adopt it at all (select _can't_, other
fs syscalls such as read/write/poll/whatever would require awful handholding from
various kernel subsystems, while in that sense we would definitely require a proper
struct file/inode/whatever for each pseudo-fd, which is not exactly what we want).
> I suggest doing so is, of course, entirely out of the scope of this
> change. Therefore if we were to require that here - it would block the
> feature while I go work on that.
>
> I think this is pretty clear right? And I also suggest that doing so is
> likely to take quite some time, and may not even have a positive outcome.
>
> So it's not a case of 'shall we take approach A or approach B?' but rather
> 'should we take approach A or entirely implement a new feature B, then once
> that is done, use it'.
>
> So as to your 'collectively decide what is the better option' - in my
> previous response I argued that the best approach between 'use an
> unimplemented suggested entirely new feature of pidfd' vs. 'implement a
> flag that would in no way block the prior approach' - a flag works better.
I just don't think it's a new feature, just an established, future-proof way
of doing things :) Your patch should remain mostly similar apart from switching
the flag check into an fd check.
> >
> > By big undertaking, do you mean other syscalls that take pidfd
> > (pidfd_getfd, pidfd_send_signal & process_mrelease) to handle PIDFD_SELF
> > or something else?
>
> I mean if you add a pidfd sentinel that represents 'the current process' it
> may get passed to any interface that accepts a pidfd, so all of them have
> to handle it _somehow_.
>
> Also you'll want to update tests accordingly and clearly need to get
> community buy-in for that feature.
>
> You may want to just add a bunch of:
>
> if (pidfd == SENTINEL)
> return -EINVAL;
It should already be there in the form of an -EBADF.
>
> So it's not impossible my instincts are off and we can get away with simply
> doing that.
>
> On the other hand, would that be confusing? Wouldn't we need to update
> documentation, manpages, etc. to say explicitly 'hey this sentinel is just
> not supported'?
This is a fair point, but we could also just... not :) which I don't feel is too
wrong, since the fd works kind of like a flag here.
--
Pedro
^ permalink raw reply
* Re: [PATCH v2 1/2] mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
From: Lorenzo Stoakes @ 2024-09-25 17:04 UTC (permalink / raw)
To: Shakeel Butt
Cc: Pedro Falcato, Andrew Morton, Vlastimil Babka, Liam R . Howlett,
Suren Baghdasaryan, Arnd Bergmann, linux-api, linux-mm,
linux-kernel, Minchan Kim, Richard Henderson, Ivan Kokshaysky,
Matt Turner, linux-alpha, Thomas Bogendoerfer, linux-mips,
James E . J . Bottomley, Helge Deller, linux-parisc, Chris Zankel,
Max Filippov, christian
In-Reply-To: <wvk5y3m47qmox4by6u3zpxtwartjmoaqaaqswbgui626zkjajq@22wjmqo36hes>
On Wed, Sep 25, 2024 at 09:19:17AM GMT, Shakeel Butt wrote:
> I have no idea what makes you think I am blocking the feature that you
> repond in a weird tone but let me be upfront what I am asking: Let's
> collectively decide which is the better option (in terms of
> maintainability and extensibility) and move forward.
I'm not sure what you mean by 'weird tone'... perhaps a miscommunication?
To summarise in my view - a suggestion was made to, rather than provide the
proposed flag - a pidfd sentinel should be introduced.
Simply introducing a sentinel that represents 'the current process' without
changing interfaces that accept a pidfd would be broken - so implementing
this implies that _all_ pidfd interfaces are updated, as well as tests.
I suggest doing so is, of course, entirely out of the scope of this
change. Therefore if we were to require that here - it would block the
feature while I go work on that.
I think this is pretty clear right? And I also suggest that doing so is
likely to take quite some time, and may not even have a positive outcome.
So it's not a case of 'shall we take approach A or approach B?' but rather
'should we take approach A or entirely implement a new feature B, then once
that is done, use it'.
So as to your 'collectively decide what is the better option' - in my
previous response I argued that the best approach between 'use an
unimplemented suggested entirely new feature of pidfd' vs. 'implement a
flag that would in no way block the prior approach' - a flag works better.
If you can provide specific arguments as to why I'm wrong then by all means
I'm happy to hear them.
>
> On Wed, Sep 25, 2024 at 03:48:07PM GMT, Lorenzo Stoakes wrote:
> > On Wed, Sep 25, 2024 at 07:02:59AM GMT, Shakeel Butt wrote:
> > > Cced Christian
> > >
> > > On Tue, Sep 24, 2024 at 02:12:49PM GMT, Lorenzo Stoakes wrote:
> > > > On Tue, Sep 24, 2024 at 01:51:11PM GMT, Pedro Falcato wrote:
> > > > > On Tue, Sep 24, 2024 at 12:16:27PM GMT, Lorenzo Stoakes wrote:
> > > > > > process_madvise() was conceived as a useful means for performing a vector
> > > > > > of madvise() operations on a remote process's address space.
> > > > > >
> > > > > > However it's useful to be able to do so on the current process also. It is
> > > > > > currently rather clunky to do this (requiring a pidfd to be opened for the
> > > > > > current process) and introduces unnecessary overhead in incrementing
> > > > > > reference counts for the task and mm.
> > > > > >
> > > > > > Avoid all of this by providing a PR_MADV_SELF flag, which causes
> > > > > > process_madvise() to simply ignore the pidfd parameter and instead apply
> > > > > > the operation to the current process.
> > > > > >
> > > > >
> > > > > How about simply defining a pseudo-fd PIDFD_SELF in the negative int space?
> > > > > There's precedent for it in the fs space (AT_FDCWD). I think it's more ergonomic
> > > > > and if you take out the errno space we have around 2^31 - 4096 available sentinel
> > > > > values.
> > > > >
> > > > > e.g:
> > > > >
> > > > > /* AT_FDCWD = -10, -1 is dangerous, pick a different value */
> > > > > #define PIDFD_SELF -11
> > > > >
> > > > > int pidfd = target_pid == getpid() ? PIDFD_SELF : pidfd_open(...);
> > > > > process_madvise(pidfd, ...);
> > > > >
> > > > >
> > > > > What do you think?
> > > >
> > > > I like the way you're thinking, but I don't think this is something we can
> > > > do in the context of this series.
> > > >
> > > > I mean, I totally accept using a flag here and ignoring the pidfd field is
> > > > _ugly_, no question. But I'm trying to find the smallest change that
> > > > achieves what we want.
> > >
> > > I don't think "smallest change" should be the target. We are changing
> > > user API and we should aim to make it as robust as possible against
> > > possible misuse or making uninteded assumptions.
> >
> > I think introducing a new pidfd sentinel that isn't used anywhere else is
> > far more liable to mistakes than adding an explicit flag.
> >
> > Could you provide examples of possible misuse of this flag or unintended
> > assumptions it confers (other than the -1 thing addressed below).
> >
> > The flag is explicitly 'target this process, ignore pidfd'. We can document
> > it as such (I will patch manpages too).
> >
> > >
> > > The proposed implementation opened the door for the applications to
> > > provide dummy pidfd if PR_MADV_SELF is used. You definitely need to
> > > restrict it to some known value like -1 used by mmap() syscall.
> >
> > Why?
> >
> > mmap() is special in that you have a 'dual' situation with shmem that is
> > both file-backed and private and of course you can do MAP_SHARED |
> > MAP_PRIVATE and have mmap() transparently assign something to you, etc.
> >
> > Here we explicitly have a flag whose semantics are 'ignore pidfd, target
> > self'.
> >
> > If you choose to use a brand new flag that explicitly states this and
> > provide a 'dummy' pidfd which then has nothing done to it - what exactly is
> > the problem?
>
> IMHO having a fixed dummy would allow the kernel more flexibility in
> future for evolving the API.
OK. I agree with having a fixed dummy value as stated.
>
> >
> > I mean if you feel strongly, we can enforce this, but I'm not sure -1
> > implying a special case for pidfd is a thing either.
> >
> > On the other hand it would be _weird_ and broken for the user to provide a
> > valid pidfd so maybe we should as it is easy to do and the user has clearly
> > done something wrong.
> >
> > So fine, agreed, I'll add that.
> >
>
> No, don't just agree. The response like "-1 is not good for so and so
> reasons" is totally fine and my request would be add that reasoning in
> the commit message. My only request is that we have thought through
> alternatives and document the reasonsing behind the decided approach.
I didn't just agree, as I said, my reasoning is:
On the other hand it would be _weird_ and broken for the user to
provide a valid pidfd so maybe we should as it is easy to do and
the user has clearly done something wrong.
If we're in alignment with that then all good!
>
> > >
> > > >
> > > > To add such a sentinel would be a change to the pidfd mechanism as a whole,
> > > > and we'd be left in the awkward situation that no other user of the pidfd
> > > > mechanism would be implementing this, but we'd have to expose this as a
> > > > general sentinel value for all pidfd users.
> > >
> > > There might be future users which can take advantage of this. I can even
> > > imagine pidfd_send_signal() can use PIDFD_SELF as well.
> >
> > I'm confused by this comment - I mean absolutely, as I said I like the
> > idea, but this just proves the point that you'd have to go around and
> > implement this everywhere that uses a pidfd?
> >
> > That is a big undertaking, and not blocked by this change. Nor is
> > maintaining the flag proposed here egregious.
>
> By big undertaking, do you mean other syscalls that take pidfd
> (pidfd_getfd, pidfd_send_signal & process_mrelease) to handle PIDFD_SELF
> or something else?
I mean if you add a pidfd sentinel that represents 'the current process' it
may get passed to any interface that accepts a pidfd, so all of them have
to handle it _somehow_.
Also you'll want to update tests accordingly and clearly need to get
community buy-in for that feature.
You may want to just add a bunch of:
if (pidfd == SENTINEL)
return -EINVAL;
So it's not impossible my instincts are off and we can get away with simply
doing that.
On the other hand, would that be confusing? Wouldn't we need to update
documentation, manpages, etc. to say explicitly 'hey this sentinel is just
not supported'?
Again totally fine with the idea, like it actually, just my instincts are
it will involve some work. I may be wrong.
>
> >
> > Blocking a useful feature because we may in future possibly add a new means
> > of doing the same thing seems a little silly to me.
> >
>
> Hah!!
See top of mail.
>
> > > >
> > > > One nice thing with doing this as a flag is that, later, if somebody is
> > > > willing to do the larger task of having a special sentinel pidfd value to
> > > > mean 'the current process', we could use this in process_madvise() and
> > > > deprecate this flag :)
> > > >
> > >
> > > Once something is added to an API, particularly syscalls, the removal
> > > is almost impossible.
> >
> > And why would it be such a problem to have this flag remain? I said
> > deprecate not remove. And only in the sense that 'you may as well use the
> > sentinel'.
> >
>
> My point was to aim for the solution where we can avoid such scenario
> but it is totally understandable and acceptable that we still have to go
> through deprecation process in future.
>
> > The flag is very clear in its meaning, and confers no special problem in
> > remaining supported. It is a private flag that overlaps no others.
> >
> > I mean it'd in effect being a change to a single line 'if pidfd is sentinel
> > or flag is used'. If we can't support that going forward, then we should
> > give up this kernel stuff and frolick in the fields joyously instead...
> >
> > Again, if you can tell me why it'd be such a problem then fine we can
> > address that.
> >
> > But blocking a series and demanding a change to an entire other feature
> > just to support something I'd say requires some pretty specific reasons as
> > to why you have a problem with the change.
> >
> > >
> > > Anyways, I don't have very strong opinion one way or other but whatever
> > > we decide, let's make it robust.
> >
> > I mean... err... it sounds like you do kinda have pretty strong opinions ;)
>
> I am not sure how more explicit I have to be to but I am hoping now it
> is more clear than before.
I mean perhaps I misinterpreted you as strongly advocating for the sentinel
and your intent was rather to provide argument on that side also so the
community can decide as you say - sure.
But with you indifferent as you say as to which way to go, and my having
provided arguments for the flags (again happy to hear push-back of course)
- I suggest we go forward with the series as-is, other than a fixpatch I'll
send for the -1 thing.
>
> Shakeel
Thanks for your review!
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox