* 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
* Re: [PATCH v2 1/2] mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
From: Shakeel Butt @ 2024-09-25 16:19 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: <7f40a8f6-c2f1-45f2-b9ff-88e169a33906@lucifer.local>
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.
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.
>
> 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.
> >
> > >
> > > 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?
>
> 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!!
> > >
> > > 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.
Shakeel
^ permalink raw reply
* Re: [PATCH v2 1/2] mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
From: Lorenzo Stoakes @ 2024-09-25 14:48 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: <xilfrvlstq4fqr46jlrzvq2vlr22nizdrwlcdizp774nlt6pfj@jukzlcwc7bed>
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?
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.
>
> >
> > 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.
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.
> >
> > 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'.
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 ;)
But anyway - as to robustness, again, could you please provide examples of
possible misuse of this flag or unintended assumptions it confers (other
than the -1 thing addressed above)? I would be happy to address them.
If not then let's move forward with this useful feature?
^ permalink raw reply
* Re: [PATCH v2 1/2] mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
From: Shakeel Butt @ 2024-09-25 14:02 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: <4740dfc7-71da-4eb4-b071-35116288571f@lucifer.local>
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.
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.
>
> 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.
>
> 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.
Anyways, I don't have very strong opinion one way or other but whatever
we decide, let's make it robust.
^ permalink raw reply
* Re: [PATCH v2 1/2] mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
From: Lorenzo Stoakes @ 2024-09-24 13:12 UTC (permalink / raw)
To: Pedro Falcato
Cc: Andrew Morton, 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
In-Reply-To: <u64scsk52b3ek4b7fh72tdylkf3qh537txcqhvozmaasrlug3r@eqsmstvs324c>
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.
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.
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 :)
>
> --
> Pedro
^ permalink raw reply
* Re: [PATCH v2 1/2] mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
From: Pedro Falcato @ 2024-09-24 12:51 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Andrew Morton, 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
In-Reply-To: <1ecf2692b3bcdd693ad61d510ce0437abb43a1bd.1727176176.git.lorenzo.stoakes@oracle.com>
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?
--
Pedro
^ permalink raw reply
* [PATCH v2 1/2] mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
From: Lorenzo Stoakes @ 2024-09-24 11:16 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
In-Reply-To: <cover.1727176176.git.lorenzo.stoakes@oracle.com>
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.
Since we are operating on our own process, no restrictions need be applied
on behaviors we can perform, so do not limit these in that case.
Also extend the case of a user specifying the current process via pidfd to
not be restricted on behaviors which can be performed.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
arch/alpha/include/uapi/asm/mman.h | 2 +
arch/mips/include/uapi/asm/mman.h | 2 +
arch/parisc/include/uapi/asm/mman.h | 2 +
arch/xtensa/include/uapi/asm/mman.h | 2 +
include/uapi/asm-generic/mman-common.h | 2 +
mm/madvise.c | 66 ++++++++++++++++++--------
6 files changed, 56 insertions(+), 20 deletions(-)
diff --git a/arch/alpha/include/uapi/asm/mman.h b/arch/alpha/include/uapi/asm/mman.h
index 763929e814e9..0148e6de35ab 100644
--- a/arch/alpha/include/uapi/asm/mman.h
+++ b/arch/alpha/include/uapi/asm/mman.h
@@ -86,4 +86,6 @@
#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\
PKEY_DISABLE_WRITE)
+#define PR_MADV_SELF (1<<0) /* process_madvise() flag - apply to self */
+
#endif /* __ALPHA_MMAN_H__ */
diff --git a/arch/mips/include/uapi/asm/mman.h b/arch/mips/include/uapi/asm/mman.h
index 9c48d9a21aa0..acb4c3bc92b2 100644
--- a/arch/mips/include/uapi/asm/mman.h
+++ b/arch/mips/include/uapi/asm/mman.h
@@ -113,4 +113,6 @@
#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\
PKEY_DISABLE_WRITE)
+#define PR_MADV_SELF (1<<0) /* process_madvise() flag - apply to self */
+
#endif /* _ASM_MMAN_H */
diff --git a/arch/parisc/include/uapi/asm/mman.h b/arch/parisc/include/uapi/asm/mman.h
index 68c44f99bc93..0f839b2cad13 100644
--- a/arch/parisc/include/uapi/asm/mman.h
+++ b/arch/parisc/include/uapi/asm/mman.h
@@ -83,4 +83,6 @@
#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\
PKEY_DISABLE_WRITE)
+#define PR_MADV_SELF (1<<0) /* process_madvise() flag - apply to self */
+
#endif /* __PARISC_MMAN_H__ */
diff --git a/arch/xtensa/include/uapi/asm/mman.h b/arch/xtensa/include/uapi/asm/mman.h
index 1ff0c858544f..37dd27d09251 100644
--- a/arch/xtensa/include/uapi/asm/mman.h
+++ b/arch/xtensa/include/uapi/asm/mman.h
@@ -121,4 +121,6 @@
#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\
PKEY_DISABLE_WRITE)
+#define PR_MADV_SELF (1<<0) /* process_madvise() flag - apply to self */
+
#endif /* _XTENSA_MMAN_H */
diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
index 6ce1f1ceb432..8f59f23dee09 100644
--- a/include/uapi/asm-generic/mman-common.h
+++ b/include/uapi/asm-generic/mman-common.h
@@ -87,4 +87,6 @@
#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\
PKEY_DISABLE_WRITE)
+#define PR_MADV_SELF (1<<0) /* process_madvise() flag - apply to self */
+
#endif /* __ASM_GENERIC_MMAN_COMMON_H */
diff --git a/mm/madvise.c b/mm/madvise.c
index ff139e57cca2..49d12f98b677 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,10 +1509,9 @@ 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) {
+ if (flags & ~PR_MADV_SELF) {
ret = -EINVAL;
goto out;
}
@@ -1498,17 +1520,21 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
if (ret < 0)
goto out;
+ /*
+ * Perform an madvise operation on the current process. No restrictions
+ * need be applied, nor do we need to pin the task or mm_struct.
+ */
+ if (flags & PR_MADV_SELF) {
+ ret = vector_madvise(current->mm, &iter, behavior);
+ goto free_iov;
+ }
+
task = pidfd_get_task(pidfd, &f_flags);
if (IS_ERR(task)) {
ret = PTR_ERR(task);
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_OR_NULL(mm)) {
@@ -1516,26 +1542,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
* [PATCH v2 2/2] selftests/mm: add test for process_madvise PR_MADV_SELF flag use
From: Lorenzo Stoakes @ 2024-09-24 11:16 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
In-Reply-To: <cover.1727176176.git.lorenzo.stoakes@oracle.com>
Add a new process_madvise() selftest, and add a test for the newly
introduced PR_MADV_SELF flag.
Assert that we can perform a vector operation of an operation that would
not be permitted on a remote mm (MADV_DONTNEED in this instance) on ones in
our own mm and that the operation is correctly peformed.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
tools/testing/selftests/mm/.gitignore | 1 +
tools/testing/selftests/mm/Makefile | 1 +
tools/testing/selftests/mm/process_madvise.c | 115 +++++++++++++++++++
3 files changed, 117 insertions(+)
create mode 100644 tools/testing/selftests/mm/process_madvise.c
diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore
index da030b43e43b..a875376601b7 100644
--- a/tools/testing/selftests/mm/.gitignore
+++ b/tools/testing/selftests/mm/.gitignore
@@ -51,3 +51,4 @@ hugetlb_madv_vs_map
mseal_test
seal_elf
droppable
+process_madvise
diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
index 02e1204971b0..7503ec177cd2 100644
--- a/tools/testing/selftests/mm/Makefile
+++ b/tools/testing/selftests/mm/Makefile
@@ -79,6 +79,7 @@ TEST_GEN_FILES += hugetlb_fault_after_madv
TEST_GEN_FILES += hugetlb_madv_vs_map
TEST_GEN_FILES += hugetlb_dio
TEST_GEN_FILES += droppable
+TEST_GEN_FILES += process_madvise
ifneq ($(ARCH),arm64)
TEST_GEN_FILES += soft-dirty
diff --git a/tools/testing/selftests/mm/process_madvise.c b/tools/testing/selftests/mm/process_madvise.c
new file mode 100644
index 000000000000..7a29b77d837d
--- /dev/null
+++ b/tools/testing/selftests/mm/process_madvise.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#define _GNU_SOURCE
+#include "../kselftest_harness.h"
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <sys/uio.h>
+
+/* May not be available in host system yet. */
+#ifndef PR_MADV_SELF
+#define PR_MADV_SELF (1<<0)
+#endif
+
+FIXTURE(process_madvise)
+{
+ unsigned long page_size;
+};
+
+FIXTURE_SETUP(process_madvise)
+{
+ self->page_size = (unsigned long)sysconf(_SC_PAGESIZE);
+};
+
+FIXTURE_TEARDOWN(process_madvise)
+{
+}
+
+static void populate_range(char *ptr, size_t len)
+{
+ memset(ptr, 'x', len);
+}
+
+static bool is_range_zeroed(char *ptr, size_t len)
+{
+ size_t i;
+
+ for (i = 0; i < len; i++) {
+ if (ptr[i] != '\0')
+ return false;
+ }
+
+ return true;
+}
+
+TEST_F(process_madvise, pr_madv_self)
+{
+ const unsigned long page_size = self->page_size;
+ struct iovec vec[3];
+ char *ptr_region, *ptr, *ptr2, *ptr3;
+
+ /* Establish a region in which to place VMAs. */
+ ptr_region = mmap(NULL, 100 * page_size, PROT_NONE,
+ MAP_PRIVATE | MAP_ANON, -1, 0);
+ ASSERT_NE(ptr_region, MAP_FAILED);
+
+ /* Place a 5 page mapping offset by one page into the region. */
+ ptr = mmap(&ptr_region[page_size], 5 * page_size,
+ PROT_READ | PROT_WRITE,
+ MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+ populate_range(ptr, 5 * page_size);
+ vec[0].iov_base = ptr;
+ vec[0].iov_len = 5 * page_size;
+ /* Free the PROT_NONE region before this region. */
+ ASSERT_EQ(munmap(ptr_region, page_size), 0);
+
+ /* Place a 10 page mapping in the middle of the region. */
+ ptr2 = mmap(&ptr_region[50 * page_size], 10 * page_size,
+ PROT_READ | PROT_WRITE,
+ MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr2, MAP_FAILED);
+ populate_range(ptr2, 10 * page_size);
+ vec[1].iov_base = ptr2;
+ vec[1].iov_len = 10 * page_size;
+ /* Free the PROT_NONE region before this region. */
+ ASSERT_EQ(munmap(&ptr_region[6 * page_size], 44 * page_size), 0);
+
+ /* Place a 3 page mapping at the end of the region, offset by 1. */
+ ptr3 = mmap(&ptr_region[96 * page_size], 3 * page_size,
+ PROT_READ | PROT_WRITE,
+ MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr3, MAP_FAILED);
+ populate_range(ptr3, 3 * page_size);
+ vec[2].iov_base = ptr3;
+ vec[2].iov_len = 3 * page_size;
+ /* Free the PROT_NONE region before this region. */
+ ASSERT_EQ(munmap(&ptr_region[60 * page_size], 36 * page_size), 0);
+ /* Free the PROT_NONE region after this region. */
+ ASSERT_EQ(munmap(&ptr_region[99 * page_size], page_size), 0);
+
+ /*
+ * OK now we should have three distinct regions of memory. Zap
+ * them with MADV_DONTNEED. This should clear the populated ranges and
+ * we can then assert on them being zeroed.
+ *
+ * The function returns the number of bytes advised, so assert this is
+ * equal to the total size of the three regions.
+ */
+ ASSERT_EQ(process_madvise(0, vec, 3, MADV_DONTNEED, PR_MADV_SELF),
+ (5 + 10 + 3) * page_size);
+
+ /* Make sure these ranges are now zeroed. */
+ ASSERT_TRUE(is_range_zeroed(ptr, 5 * page_size));
+ ASSERT_TRUE(is_range_zeroed(ptr2, 10 * page_size));
+ ASSERT_TRUE(is_range_zeroed(ptr2, 3 * page_size));
+
+ /* Cleanup. */
+ ASSERT_EQ(munmap(ptr, 5 * page_size), 0);
+ ASSERT_EQ(munmap(ptr2, 10 * page_size), 0);
+ ASSERT_EQ(munmap(ptr3, 3 * page_size), 0);
+}
+
+TEST_HARNESS_MAIN
--
2.46.0
^ permalink raw reply related
* [PATCH v2 0/2] unrestrict process_madvise() for current process
From: Lorenzo Stoakes @ 2024-09-24 11:16 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
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.
Using this interface targeting the current process is cumbersome - a pidfd
needs to be setup for the current pid, and we are limited to only a subset
of madvise() operations, a limitation sensible for manipulating remote
processes but not meaningful when manipulating the current 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 and the cumbersome need for a 'self
pidfd'.
This patch series eliminates both limitations:
1. The restriction on permitted operations is removed when operating
on the current process.
2. A new flag is introduced - PR_MADV_SELF - which eliminates the need for
a pidfd - if this flag is set, the pidfd argument is ignored and the
operation is simply applied to the current process.
Therefore a user can simply invoke:
process_madvise(0, iovec, n, MADV_..., PR_MADV_SELF);
And perform any madvise() operation they like on the n ranges specified by
the iovec parameter.
This series also introduces a series of self-tests for this feature
asserting that the flag functions as expected.
v2:
* Fix silly mistake referencing unassigned mm variable.
* Add PR_MADV_SELF to architecture-specific mman headers.
v1:
https://lore.kernel.org/all/cover.1727106751.git.lorenzo.stoakes@oracle.com/
Lorenzo Stoakes (2):
mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
selftests/mm: add test for process_madvise PR_MADV_SELF flag use
arch/alpha/include/uapi/asm/mman.h | 2 +
arch/mips/include/uapi/asm/mman.h | 2 +
arch/parisc/include/uapi/asm/mman.h | 2 +
arch/xtensa/include/uapi/asm/mman.h | 2 +
include/uapi/asm-generic/mman-common.h | 2 +
mm/madvise.c | 66 +++++++----
tools/testing/selftests/mm/.gitignore | 1 +
tools/testing/selftests/mm/Makefile | 1 +
tools/testing/selftests/mm/process_madvise.c | 115 +++++++++++++++++++
9 files changed, 173 insertions(+), 20 deletions(-)
create mode 100644 tools/testing/selftests/mm/process_madvise.c
--
2.46.0
^ permalink raw reply
* Re: [PATCH 1/2] mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
From: Lorenzo Stoakes @ 2024-09-24 8:47 UTC (permalink / raw)
To: kernel test robot
Cc: Andrew Morton, llvm, oe-kbuild-all, Linux Memory Management List,
Vlastimil Babka, Liam R . Howlett, Suren Baghdasaryan,
Arnd Bergmann, Shakeel Butt, linux-api, linux-kernel, Minchan Kim
In-Reply-To: <202409241034.6ilzMh4w-lkp@intel.com>
On Tue, Sep 24, 2024 at 11:15:17AM GMT, kernel test robot wrote:
> Hi Lorenzo,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on akpm-mm/mm-everything]
> [also build test ERROR on soc/for-next linus/master v6.11 next-20240923]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Lorenzo-Stoakes/mm-madvise-introduce-PR_MADV_SELF-flag-to-process_madvise/20240924-000845
> base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
> patch link: https://lore.kernel.org/r/077be0d59cb1047870a84c87c62e7b027af1c75d.1727106751.git.lorenzo.stoakes%40oracle.com
> patch subject: [PATCH 1/2] mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
> config: mips-ip32_defconfig (https://download.01.org/0day-ci/archive/20240924/202409241034.6ilzMh4w-lkp@intel.com/config)
> compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 8663a75fa2f31299ab8d1d90288d9df92aadee88)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240924/202409241034.6ilzMh4w-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202409241034.6ilzMh4w-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
> In file included from mm/madvise.c:9:
> In file included from include/linux/mman.h:5:
> In file included from include/linux/mm.h:2198:
> include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
> 518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
> | ~~~~~~~~~~~ ^ ~~~
> In file included from mm/madvise.c:21:
> include/linux/mm_inline.h:47:41: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
> 47 | __mod_lruvec_state(lruvec, NR_LRU_BASE + lru, nr_pages);
> | ~~~~~~~~~~~ ^ ~~~
> include/linux/mm_inline.h:49:22: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
> 49 | NR_ZONE_LRU_BASE + lru, nr_pages);
> | ~~~~~~~~~~~~~~~~ ^ ~~~
> >> mm/madvise.c:1514:15: error: use of undeclared identifier 'PR_MADV_SELF'
> 1514 | if (flags & ~PR_MADV_SELF) {
> | ^
> mm/madvise.c:1527:14: error: use of undeclared identifier 'PR_MADV_SELF'
> 1527 | if (flags & PR_MADV_SELF) {
> | ^
> 3 warnings and 2 errors generated.
OK looks like mman-common.h is insufficient for some arches, will fix up and send out a v2.
>
>
> vim +/PR_MADV_SELF +1514 mm/madvise.c
>
> 1502
> 1503 SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
> 1504 size_t, vlen, int, behavior, unsigned int, flags)
> 1505 {
> 1506 ssize_t ret;
> 1507 struct iovec iovstack[UIO_FASTIOV];
> 1508 struct iovec *iov = iovstack;
> 1509 struct iov_iter iter;
> 1510 struct task_struct *task;
> 1511 struct mm_struct *mm;
> 1512 unsigned int f_flags;
> 1513
> > 1514 if (flags & ~PR_MADV_SELF) {
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH 1/2] mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
From: Lorenzo Stoakes @ 2024-09-24 7:49 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Shakeel Butt, Andrew Morton, Vlastimil Babka, Liam R. Howlett,
Suren Baghdasaryan, linux-api, linux-mm, linux-kernel,
Minchan Kim
In-Reply-To: <337e4359-37e0-4ed7-894d-6c88b3498a42@app.fastmail.com>
On Mon, Sep 23, 2024 at 09:49:43PM GMT, Arnd Bergmann wrote:
> On Mon, Sep 23, 2024, at 19:34, Lorenzo Stoakes wrote:
> > On Mon, Sep 23, 2024 at 11:56:06AM GMT, Shakeel Butt wrote:
> >
> > + /* Require PTRACE_MODE_READ to avoid leaking ASLR metadata. */
> > + mm = mm_access(task, PTRACE_MODE_READ_FSCREDS);
> > + if (IS_ERR_OR_NULL(mm)) {
> > + ret = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH;
> > + goto release_task;
> > + }
>
> Any chance we can fix mm_access() to not be able to return
> a NULL pointer and an error pointer? IS_ERR_OR_NULL() is
> usually an indication of a confusing API, and this is
> clearly one of them, given that only one of the
> callers actually wants the NULL value instead of -ESRCH.
>
> Arnd
Agreed, this should be fixed. I think it'd be a bit out of the scope of
this series so will send something separately for this.
^ permalink raw reply
* Re: [PATCH 1/2] mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
From: kernel test robot @ 2024-09-24 3:15 UTC (permalink / raw)
To: Lorenzo Stoakes, Andrew Morton
Cc: llvm, oe-kbuild-all, Linux Memory Management List,
Vlastimil Babka, Liam R . Howlett, Suren Baghdasaryan,
Arnd Bergmann, Shakeel Butt, linux-api, linux-kernel, Minchan Kim
In-Reply-To: <077be0d59cb1047870a84c87c62e7b027af1c75d.1727106751.git.lorenzo.stoakes@oracle.com>
Hi Lorenzo,
kernel test robot noticed the following build errors:
[auto build test ERROR on akpm-mm/mm-everything]
[also build test ERROR on soc/for-next linus/master v6.11 next-20240923]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Lorenzo-Stoakes/mm-madvise-introduce-PR_MADV_SELF-flag-to-process_madvise/20240924-000845
base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link: https://lore.kernel.org/r/077be0d59cb1047870a84c87c62e7b027af1c75d.1727106751.git.lorenzo.stoakes%40oracle.com
patch subject: [PATCH 1/2] mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
config: mips-ip32_defconfig (https://download.01.org/0day-ci/archive/20240924/202409241034.6ilzMh4w-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 8663a75fa2f31299ab8d1d90288d9df92aadee88)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240924/202409241034.6ilzMh4w-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409241034.6ilzMh4w-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from mm/madvise.c:9:
In file included from include/linux/mman.h:5:
In file included from include/linux/mm.h:2198:
include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
In file included from mm/madvise.c:21:
include/linux/mm_inline.h:47:41: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
47 | __mod_lruvec_state(lruvec, NR_LRU_BASE + lru, nr_pages);
| ~~~~~~~~~~~ ^ ~~~
include/linux/mm_inline.h:49:22: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
49 | NR_ZONE_LRU_BASE + lru, nr_pages);
| ~~~~~~~~~~~~~~~~ ^ ~~~
>> mm/madvise.c:1514:15: error: use of undeclared identifier 'PR_MADV_SELF'
1514 | if (flags & ~PR_MADV_SELF) {
| ^
mm/madvise.c:1527:14: error: use of undeclared identifier 'PR_MADV_SELF'
1527 | if (flags & PR_MADV_SELF) {
| ^
3 warnings and 2 errors generated.
vim +/PR_MADV_SELF +1514 mm/madvise.c
1502
1503 SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
1504 size_t, vlen, int, behavior, unsigned int, flags)
1505 {
1506 ssize_t ret;
1507 struct iovec iovstack[UIO_FASTIOV];
1508 struct iovec *iov = iovstack;
1509 struct iov_iter iter;
1510 struct task_struct *task;
1511 struct mm_struct *mm;
1512 unsigned int f_flags;
1513
> 1514 if (flags & ~PR_MADV_SELF) {
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH 1/2] mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
From: Arnd Bergmann @ 2024-09-23 21:49 UTC (permalink / raw)
To: Lorenzo Stoakes, Shakeel Butt
Cc: Andrew Morton, Vlastimil Babka, Liam R. Howlett,
Suren Baghdasaryan, linux-api, linux-mm, linux-kernel,
Minchan Kim
In-Reply-To: <c44d373d-d72b-4e62-a613-a746a2c290e7@lucifer.local>
On Mon, Sep 23, 2024, at 19:34, Lorenzo Stoakes wrote:
> On Mon, Sep 23, 2024 at 11:56:06AM GMT, Shakeel Butt wrote:
>
> + /* Require PTRACE_MODE_READ to avoid leaking ASLR metadata. */
> + mm = mm_access(task, PTRACE_MODE_READ_FSCREDS);
> + if (IS_ERR_OR_NULL(mm)) {
> + ret = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH;
> + goto release_task;
> + }
Any chance we can fix mm_access() to not be able to return
a NULL pointer and an error pointer? IS_ERR_OR_NULL() is
usually an indication of a confusing API, and this is
clearly one of them, given that only one of the
callers actually wants the NULL value instead of -ESRCH.
Arnd
^ permalink raw reply
* Re: [PATCH 1/2] mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
From: kernel test robot @ 2024-09-23 21:30 UTC (permalink / raw)
To: Lorenzo Stoakes, Andrew Morton
Cc: oe-kbuild-all, Linux Memory Management List, Vlastimil Babka,
Liam R . Howlett, Suren Baghdasaryan, Arnd Bergmann, Shakeel Butt,
linux-api, linux-kernel, Minchan Kim
In-Reply-To: <077be0d59cb1047870a84c87c62e7b027af1c75d.1727106751.git.lorenzo.stoakes@oracle.com>
Hi Lorenzo,
kernel test robot noticed the following build errors:
[auto build test ERROR on akpm-mm/mm-everything]
[also build test ERROR on arnd-asm-generic/master soc/for-next linus/master v6.11 next-20240923]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Lorenzo-Stoakes/mm-madvise-introduce-PR_MADV_SELF-flag-to-process_madvise/20240924-000845
base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link: https://lore.kernel.org/r/077be0d59cb1047870a84c87c62e7b027af1c75d.1727106751.git.lorenzo.stoakes%40oracle.com
patch subject: [PATCH 1/2] mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
config: parisc-allnoconfig (https://download.01.org/0day-ci/archive/20240924/202409240556.LgM8vOIF-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240924/202409240556.LgM8vOIF-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409240556.LgM8vOIF-lkp@intel.com/
All errors (new ones prefixed by >>):
mm/madvise.c: In function '__do_sys_process_madvise':
>> mm/madvise.c:1514:22: error: 'PR_MADV_SELF' undeclared (first use in this function)
1514 | if (flags & ~PR_MADV_SELF) {
| ^~~~~~~~~~~~
mm/madvise.c:1514:22: note: each undeclared identifier is reported only once for each function it appears in
vim +/PR_MADV_SELF +1514 mm/madvise.c
1502
1503 SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
1504 size_t, vlen, int, behavior, unsigned int, flags)
1505 {
1506 ssize_t ret;
1507 struct iovec iovstack[UIO_FASTIOV];
1508 struct iovec *iov = iovstack;
1509 struct iov_iter iter;
1510 struct task_struct *task;
1511 struct mm_struct *mm;
1512 unsigned int f_flags;
1513
> 1514 if (flags & ~PR_MADV_SELF) {
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH 1/2] mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
From: kernel test robot @ 2024-09-23 21:20 UTC (permalink / raw)
To: Lorenzo Stoakes, Andrew Morton
Cc: llvm, oe-kbuild-all, Linux Memory Management List,
Vlastimil Babka, Liam R . Howlett, Suren Baghdasaryan,
Arnd Bergmann, Shakeel Butt, linux-api, linux-kernel, Minchan Kim
In-Reply-To: <077be0d59cb1047870a84c87c62e7b027af1c75d.1727106751.git.lorenzo.stoakes@oracle.com>
Hi Lorenzo,
kernel test robot noticed the following build warnings:
[auto build test WARNING on akpm-mm/mm-everything]
[also build test WARNING on arnd-asm-generic/master soc/for-next linus/master v6.11 next-20240923]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Lorenzo-Stoakes/mm-madvise-introduce-PR_MADV_SELF-flag-to-process_madvise/20240924-000845
base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link: https://lore.kernel.org/r/077be0d59cb1047870a84c87c62e7b027af1c75d.1727106751.git.lorenzo.stoakes%40oracle.com
patch subject: [PATCH 1/2] mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
config: arm-aspeed_g4_defconfig (https://download.01.org/0day-ci/archive/20240924/202409240527.pAgR35QJ-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 8663a75fa2f31299ab8d1d90288d9df92aadee88)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240924/202409240527.pAgR35QJ-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409240527.pAgR35QJ-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from mm/madvise.c:9:
In file included from include/linux/mman.h:5:
In file included from include/linux/mm.h:2198:
include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
In file included from mm/madvise.c:21:
include/linux/mm_inline.h:47:41: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
47 | __mod_lruvec_state(lruvec, NR_LRU_BASE + lru, nr_pages);
| ~~~~~~~~~~~ ^ ~~~
include/linux/mm_inline.h:49:22: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
49 | NR_ZONE_LRU_BASE + lru, nr_pages);
| ~~~~~~~~~~~~~~~~ ^ ~~~
>> mm/madvise.c:1542:6: warning: variable 'mm' is uninitialized when used here [-Wuninitialized]
1542 | if (mm != current->mm && !process_madvise_remote_valid(behavior)) {
| ^~
mm/madvise.c:1511:22: note: initialize the variable 'mm' to silence this warning
1511 | struct mm_struct *mm;
| ^
| = NULL
4 warnings generated.
vim +/mm +1542 mm/madvise.c
1502
1503 SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
1504 size_t, vlen, int, behavior, unsigned int, flags)
1505 {
1506 ssize_t ret;
1507 struct iovec iovstack[UIO_FASTIOV];
1508 struct iovec *iov = iovstack;
1509 struct iov_iter iter;
1510 struct task_struct *task;
1511 struct mm_struct *mm;
1512 unsigned int f_flags;
1513
1514 if (flags & ~PR_MADV_SELF) {
1515 ret = -EINVAL;
1516 goto out;
1517 }
1518
1519 ret = import_iovec(ITER_DEST, vec, vlen, ARRAY_SIZE(iovstack), &iov, &iter);
1520 if (ret < 0)
1521 goto out;
1522
1523 /*
1524 * Perform an madvise operation on the current process. No restrictions
1525 * need be applied, nor do we need to pin the task or mm_struct.
1526 */
1527 if (flags & PR_MADV_SELF) {
1528 ret = vector_madvise(current->mm, &iter, behavior);
1529 goto free_iov;
1530 }
1531
1532 task = pidfd_get_task(pidfd, &f_flags);
1533 if (IS_ERR(task)) {
1534 ret = PTR_ERR(task);
1535 goto free_iov;
1536 }
1537
1538 /*
1539 * We need only perform this check if we are attempting to manipulate a
1540 * remote process's address space.
1541 */
> 1542 if (mm != current->mm && !process_madvise_remote_valid(behavior)) {
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH 1/2] mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
From: Lorenzo Stoakes @ 2024-09-23 19:34 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
In-Reply-To: <njjxbroy5nvn2gxmvsvk7m23rrsoyih24nhmbmf7lpd5yzwwk7@ijudgtbiwyq6>
On Mon, Sep 23, 2024 at 11:56:06AM GMT, Shakeel Butt wrote:
> On Mon, Sep 23, 2024 at 05:03:56PM GMT, Lorenzo Stoakes wrote:
> [...]
> > SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
> > size_t, vlen, int, behavior, unsigned int, flags)
> > {
> > @@ -1486,10 +1509,9 @@ 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) {
> > + if (flags & ~PR_MADV_SELF) {
> > ret = -EINVAL;
> > goto out;
> > }
> > @@ -1498,13 +1520,26 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
> > if (ret < 0)
> > goto out;
> >
> > + /*
> > + * Perform an madvise operation on the current process. No restrictions
> > + * need be applied, nor do we need to pin the task or mm_struct.
> > + */
> > + if (flags & PR_MADV_SELF) {
> > + ret = vector_madvise(current->mm, &iter, behavior);
> > + goto free_iov;
> > + }
> > +
> > task = pidfd_get_task(pidfd, &f_flags);
> > if (IS_ERR(task)) {
> > ret = PTR_ERR(task);
> > goto free_iov;
> > }
> >
> > - if (!process_madvise_behavior_valid(behavior)) {
> > + /*
> > + * 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)) {
>
> Move the above check after mm is initialized i.e. mm = mm_access().
>
> Shakeel
Ugh, sorry silly one there! Reflexively put that check in the original position.
Enclose a quick fix-patch for it, will fix on any respin also.
----8<----
From dc09e0edf1cf71a89cc4cfc3ec73fdae3c2ab86c Mon Sep 17 00:00:00 2001
From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Date: Mon, 23 Sep 2024 20:33:07 +0100
Subject: [PATCH] mm/madvise: retrieve mm before checking
---
mm/madvise.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/mm/madvise.c b/mm/madvise.c
index 549b36d1463c..49d12f98b677 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1535,20 +1535,20 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
goto free_iov;
}
+ /* Require PTRACE_MODE_READ to avoid leaking ASLR metadata. */
+ mm = mm_access(task, PTRACE_MODE_READ_FSCREDS);
+ if (IS_ERR_OR_NULL(mm)) {
+ ret = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH;
+ 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_task;
- }
-
- /* Require PTRACE_MODE_READ to avoid leaking ASLR metadata. */
- mm = mm_access(task, PTRACE_MODE_READ_FSCREDS);
- if (IS_ERR_OR_NULL(mm)) {
- ret = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH;
- goto release_task;
+ goto release_mm;
}
/*
--
2.46.0
^ permalink raw reply related
* Re: [PATCH 1/2] mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
From: Shakeel Butt @ 2024-09-23 18:56 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
In-Reply-To: <077be0d59cb1047870a84c87c62e7b027af1c75d.1727106751.git.lorenzo.stoakes@oracle.com>
On Mon, Sep 23, 2024 at 05:03:56PM GMT, Lorenzo Stoakes wrote:
[...]
> SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
> size_t, vlen, int, behavior, unsigned int, flags)
> {
> @@ -1486,10 +1509,9 @@ 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) {
> + if (flags & ~PR_MADV_SELF) {
> ret = -EINVAL;
> goto out;
> }
> @@ -1498,13 +1520,26 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
> if (ret < 0)
> goto out;
>
> + /*
> + * Perform an madvise operation on the current process. No restrictions
> + * need be applied, nor do we need to pin the task or mm_struct.
> + */
> + if (flags & PR_MADV_SELF) {
> + ret = vector_madvise(current->mm, &iter, behavior);
> + goto free_iov;
> + }
> +
> task = pidfd_get_task(pidfd, &f_flags);
> if (IS_ERR(task)) {
> ret = PTR_ERR(task);
> goto free_iov;
> }
>
> - if (!process_madvise_behavior_valid(behavior)) {
> + /*
> + * 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)) {
Move the above check after mm is initialized i.e. mm = mm_access().
Shakeel
^ permalink raw reply
* [PATCH 1/2] mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
From: Lorenzo Stoakes @ 2024-09-23 16:03 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
In-Reply-To: <cover.1727106751.git.lorenzo.stoakes@oracle.com>
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.
Since we are operating on our own process, no restrictions need be applied
on behaviors we can perform, so do not limit these in that case.
Also extend the case of a user specifying the current process via pidfd to
not be restricted on behaviors which can be performed.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
include/uapi/asm-generic/mman-common.h | 2 +
mm/madvise.c | 58 +++++++++++++++++++-------
2 files changed, 44 insertions(+), 16 deletions(-)
diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
index 6ce1f1ceb432..8f59f23dee09 100644
--- a/include/uapi/asm-generic/mman-common.h
+++ b/include/uapi/asm-generic/mman-common.h
@@ -87,4 +87,6 @@
#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\
PKEY_DISABLE_WRITE)
+#define PR_MADV_SELF (1<<0) /* process_madvise() flag - apply to self */
+
#endif /* __ASM_GENERIC_MMAN_COMMON_H */
diff --git a/mm/madvise.c b/mm/madvise.c
index ff139e57cca2..549b36d1463c 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,10 +1509,9 @@ 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) {
+ if (flags & ~PR_MADV_SELF) {
ret = -EINVAL;
goto out;
}
@@ -1498,13 +1520,26 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
if (ret < 0)
goto out;
+ /*
+ * Perform an madvise operation on the current process. No restrictions
+ * need be applied, nor do we need to pin the task or mm_struct.
+ */
+ if (flags & PR_MADV_SELF) {
+ ret = vector_madvise(current->mm, &iter, behavior);
+ goto free_iov;
+ }
+
task = pidfd_get_task(pidfd, &f_flags);
if (IS_ERR(task)) {
ret = PTR_ERR(task);
goto free_iov;
}
- if (!process_madvise_behavior_valid(behavior)) {
+ /*
+ * 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_task;
}
@@ -1518,24 +1553,15 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
/*
* 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
* [PATCH 2/2] selftests/mm: add test for process_madvise PR_MADV_SELF flag use
From: Lorenzo Stoakes @ 2024-09-23 16:03 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
In-Reply-To: <cover.1727106751.git.lorenzo.stoakes@oracle.com>
Add a new process_madvise() selftest, and add a test for the newly
introduced PR_MADV_SELF flag.
Assert that we can perform a vector operation of an operation that would
not be permitted on a remote mm (MADV_DONTNEED in this instance) on ones in
our own mm and that the operation is correctly peformed.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
tools/testing/selftests/mm/.gitignore | 1 +
tools/testing/selftests/mm/Makefile | 1 +
tools/testing/selftests/mm/process_madvise.c | 115 +++++++++++++++++++
3 files changed, 117 insertions(+)
create mode 100644 tools/testing/selftests/mm/process_madvise.c
diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore
index da030b43e43b..a875376601b7 100644
--- a/tools/testing/selftests/mm/.gitignore
+++ b/tools/testing/selftests/mm/.gitignore
@@ -51,3 +51,4 @@ hugetlb_madv_vs_map
mseal_test
seal_elf
droppable
+process_madvise
diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
index 02e1204971b0..7503ec177cd2 100644
--- a/tools/testing/selftests/mm/Makefile
+++ b/tools/testing/selftests/mm/Makefile
@@ -79,6 +79,7 @@ TEST_GEN_FILES += hugetlb_fault_after_madv
TEST_GEN_FILES += hugetlb_madv_vs_map
TEST_GEN_FILES += hugetlb_dio
TEST_GEN_FILES += droppable
+TEST_GEN_FILES += process_madvise
ifneq ($(ARCH),arm64)
TEST_GEN_FILES += soft-dirty
diff --git a/tools/testing/selftests/mm/process_madvise.c b/tools/testing/selftests/mm/process_madvise.c
new file mode 100644
index 000000000000..7a29b77d837d
--- /dev/null
+++ b/tools/testing/selftests/mm/process_madvise.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#define _GNU_SOURCE
+#include "../kselftest_harness.h"
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <sys/uio.h>
+
+/* May not be available in host system yet. */
+#ifndef PR_MADV_SELF
+#define PR_MADV_SELF (1<<0)
+#endif
+
+FIXTURE(process_madvise)
+{
+ unsigned long page_size;
+};
+
+FIXTURE_SETUP(process_madvise)
+{
+ self->page_size = (unsigned long)sysconf(_SC_PAGESIZE);
+};
+
+FIXTURE_TEARDOWN(process_madvise)
+{
+}
+
+static void populate_range(char *ptr, size_t len)
+{
+ memset(ptr, 'x', len);
+}
+
+static bool is_range_zeroed(char *ptr, size_t len)
+{
+ size_t i;
+
+ for (i = 0; i < len; i++) {
+ if (ptr[i] != '\0')
+ return false;
+ }
+
+ return true;
+}
+
+TEST_F(process_madvise, pr_madv_self)
+{
+ const unsigned long page_size = self->page_size;
+ struct iovec vec[3];
+ char *ptr_region, *ptr, *ptr2, *ptr3;
+
+ /* Establish a region in which to place VMAs. */
+ ptr_region = mmap(NULL, 100 * page_size, PROT_NONE,
+ MAP_PRIVATE | MAP_ANON, -1, 0);
+ ASSERT_NE(ptr_region, MAP_FAILED);
+
+ /* Place a 5 page mapping offset by one page into the region. */
+ ptr = mmap(&ptr_region[page_size], 5 * page_size,
+ PROT_READ | PROT_WRITE,
+ MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+ populate_range(ptr, 5 * page_size);
+ vec[0].iov_base = ptr;
+ vec[0].iov_len = 5 * page_size;
+ /* Free the PROT_NONE region before this region. */
+ ASSERT_EQ(munmap(ptr_region, page_size), 0);
+
+ /* Place a 10 page mapping in the middle of the region. */
+ ptr2 = mmap(&ptr_region[50 * page_size], 10 * page_size,
+ PROT_READ | PROT_WRITE,
+ MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr2, MAP_FAILED);
+ populate_range(ptr2, 10 * page_size);
+ vec[1].iov_base = ptr2;
+ vec[1].iov_len = 10 * page_size;
+ /* Free the PROT_NONE region before this region. */
+ ASSERT_EQ(munmap(&ptr_region[6 * page_size], 44 * page_size), 0);
+
+ /* Place a 3 page mapping at the end of the region, offset by 1. */
+ ptr3 = mmap(&ptr_region[96 * page_size], 3 * page_size,
+ PROT_READ | PROT_WRITE,
+ MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr3, MAP_FAILED);
+ populate_range(ptr3, 3 * page_size);
+ vec[2].iov_base = ptr3;
+ vec[2].iov_len = 3 * page_size;
+ /* Free the PROT_NONE region before this region. */
+ ASSERT_EQ(munmap(&ptr_region[60 * page_size], 36 * page_size), 0);
+ /* Free the PROT_NONE region after this region. */
+ ASSERT_EQ(munmap(&ptr_region[99 * page_size], page_size), 0);
+
+ /*
+ * OK now we should have three distinct regions of memory. Zap
+ * them with MADV_DONTNEED. This should clear the populated ranges and
+ * we can then assert on them being zeroed.
+ *
+ * The function returns the number of bytes advised, so assert this is
+ * equal to the total size of the three regions.
+ */
+ ASSERT_EQ(process_madvise(0, vec, 3, MADV_DONTNEED, PR_MADV_SELF),
+ (5 + 10 + 3) * page_size);
+
+ /* Make sure these ranges are now zeroed. */
+ ASSERT_TRUE(is_range_zeroed(ptr, 5 * page_size));
+ ASSERT_TRUE(is_range_zeroed(ptr2, 10 * page_size));
+ ASSERT_TRUE(is_range_zeroed(ptr2, 3 * page_size));
+
+ /* Cleanup. */
+ ASSERT_EQ(munmap(ptr, 5 * page_size), 0);
+ ASSERT_EQ(munmap(ptr2, 10 * page_size), 0);
+ ASSERT_EQ(munmap(ptr3, 3 * page_size), 0);
+}
+
+TEST_HARNESS_MAIN
--
2.46.0
^ permalink raw reply related
* [PATCH 0/2] unrestrict process_madvise() for current process
From: Lorenzo Stoakes @ 2024-09-23 16:03 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
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.
Using this interface targeting the current process is cumbersome - a pidfd
needs to be setup for the current pid, and we are limited to only a subset
of madvise() operations, a limitation sensible for manipulating remote
processes but not meaningful when manipulating the current 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 and the cumbersome need for a 'self
pidfd'.
This patch series eliminates both limitations:
1. The restriction on permitted operations is removed when operating
on the current process.
2. A new flag is introduced - PR_MADV_SELF - which eliminates the need for
a pidfd - if this flag is set, the pidfd argument is ignored and the
operation is simply applied to the current process.
Therefore a user can simply invoke:
process_madvise(0, iovec, n, MADV_..., PR_MADV_SELF);
And perform any madvise() operation they like on the n ranges specified by
the iovec parameter.
This series also introduces a series of self-tests for this feature
asserting that the flag functions as expected.
Lorenzo Stoakes (2):
mm/madvise: introduce PR_MADV_SELF flag to process_madvise()
selftests/mm: add test for process_madvise PR_MADV_SELF flag use
include/uapi/asm-generic/mman-common.h | 2 +
mm/madvise.c | 58 +++++++---
tools/testing/selftests/mm/.gitignore | 1 +
tools/testing/selftests/mm/Makefile | 1 +
tools/testing/selftests/mm/process_madvise.c | 115 +++++++++++++++++++
5 files changed, 161 insertions(+), 16 deletions(-)
create mode 100644 tools/testing/selftests/mm/process_madvise.c
--
2.46.0
^ permalink raw reply
* Re: [PATCH v11 1/8] Landlock: Add abstract UNIX socket restriction
From: Mickaël Salaün @ 2024-09-13 13:32 UTC (permalink / raw)
To: Tahera Fahimi, linux-api
Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module,
linux-kernel, bjorn3_gh, jannh, netdev
In-Reply-To: <5f7ad85243b78427242275b93481cfc7c127764b.1725494372.git.fahimitahera@gmail.com>
On Wed, Sep 04, 2024 at 06:13:55PM -0600, Tahera Fahimi wrote:
> This patch introduces a new "scoped" attribute to the
> landlock_ruleset_attr that can specify
> "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to scope abstract UNIX sockets
> from connecting to a process outside of the same Landlock domain. It
> implements two hooks, unix_stream_connect and unix_may_send to enforce
> this restriction.
>
> Closes: https://github.com/landlock-lsm/linux/issues/7
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
>
> ---
> v11:
> - For a connected abstract datagram socket, the hook_unix_may_send
> allows the socket to send a data. (it is treated as a connected stream
> socket)
> - Minor comment revision.
> v10:
> - Minor code improvement based on reviews on v9.
> v9:
> - Editting inline comments.
> - Major refactoring in domain_is_scoped() and is_abstract_socket
> v8:
> - Code refactoring (improve code readability, renaming variable, etc.)
> based on reviews by Mickaël Salaün on version 7.
> - Adding warn_on_once to check (impossible) inconsistencies.
> - Adding inline comments.
> - Adding check_unix_address_format to check if the scoping socket is an
> abstract UNIX sockets.
> v7:
> - Using socket's file credentials for both connected(STREAM) and
> non-connected(DGRAM) sockets.
> - Adding "domain_sock_scope" instead of the domain scoping mechanism
> used in ptrace ensures that if a server's domain is accessible from
> the client's domain (where the client is more privileged than the
> server), the client can connect to the server in all edge cases.
> - Removing debug codes.
> v6:
> - Removing curr_ruleset from landlock_hierarchy, and switching back to
> use the same domain scoping as ptrace.
> - code clean up.
> v5:
> - Renaming "LANDLOCK_*_ACCESS_SCOPE" to "LANDLOCK_*_SCOPE"
> - Adding curr_ruleset to hierarachy_ruleset structure to have access
> from landlock_hierarchy to its respective landlock_ruleset.
> - Using curr_ruleset to check if a domain is scoped while walking in the
> hierarchy of domains.
> - Modifying inline comments.
> v4:
> - Rebased on Günther's Patch:
> https://lore.kernel.org/all/20240610082115.1693267-1-gnoack@google.com/
> so there is no need for "LANDLOCK_SHIFT_ACCESS_SCOPE", then it is
> removed.
> - Adding get_scope_accesses function to check all scoped access masks in
> a ruleset.
> - Using socket's file credentials instead of credentials stored in
> peer_cred for datagram sockets. (see discussion in [1])
> - Modifying inline comments.
> V3:
> - Improving commit description.
> - Introducing "scoped" attribute to landlock_ruleset_attr for IPC
> scoping purpose, and adding related functions.
> - Changing structure of ruleset based on "scoped".
> - Removing rcu lock and using unix_sk lock instead.
> - Introducing scoping for datagram sockets in unix_may_send.
> V2:
> - Removing wrapper functions
>
> [1]https://lore.kernel.org/all/20240610.Aifee5ingugh@digikod.net/
> ---
> include/uapi/linux/landlock.h | 28 ++++
> security/landlock/limits.h | 3 +
> security/landlock/ruleset.c | 7 +-
> security/landlock/ruleset.h | 24 +++-
> security/landlock/syscalls.c | 17 ++-
> security/landlock/task.c | 136 +++++++++++++++++++
> tools/testing/selftests/landlock/base_test.c | 2 +-
> 7 files changed, 208 insertions(+), 9 deletions(-)
>
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 2c8dbc74b955..dfd48d722834 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -44,6 +44,12 @@ struct landlock_ruleset_attr {
> * flags`_).
> */
> __u64 handled_access_net;
> + /**
> + * @scoped: Bitmask of scopes (cf. `Scope flags`_)
> + * restricting a Landlock domain from accessing outside
> + * resources(e.g. IPCs).
> + */
> + __u64 scoped;
> };
>
> /*
> @@ -274,4 +280,26 @@ struct landlock_net_port_attr {
> #define LANDLOCK_ACCESS_NET_BIND_TCP (1ULL << 0)
> #define LANDLOCK_ACCESS_NET_CONNECT_TCP (1ULL << 1)
> /* clang-format on */
> +
> +/**
> + * DOC: scope
> + *
> + * Scope flags
> + * ~~~~~~~~~~~
> + *
> + * These flags enable to restrict a sandboxed process from a set of IPC
> + * actions. Setting a flag for a ruleset will isolate the Landlock domain
> + * to forbid connections to resources outside the domain.
> + *
> + * IPCs with scoped actions:
> + *
> + * - %LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET: Restrict a sandboxed process
> + * from connecting to an abstract unix socket created by a process
> + * outside the related Landlock domain (e.g. a parent domain or a
> + * non-sandboxed process).
> + */
> +/* clang-format off */
> +#define LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET (1ULL << 0)
Thinking more about it, it makes more sense to rename it to
LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET (s/SCOPED/SCOPE/) because it
express a scope (not a "scoped") and it allign with the current
LANDLOCK_ACCESS_* and other internal variables such as
LANDLOCK_LAST_SCOPE...
However, it still makes sense to keep the "scoped" ruleset's field,
which is pretty similar to the "handled_*" semantic: it describes what
will be *scoped* by the ruleset.
> +/* clang-format on*/
> +
> #endif /* _UAPI_LINUX_LANDLOCK_H */
> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> index 4eb643077a2a..eb01d0fb2165 100644
> --- a/security/landlock/limits.h
> +++ b/security/landlock/limits.h
> @@ -26,6 +26,9 @@
> #define LANDLOCK_MASK_ACCESS_NET ((LANDLOCK_LAST_ACCESS_NET << 1) - 1)
> #define LANDLOCK_NUM_ACCESS_NET __const_hweight64(LANDLOCK_MASK_ACCESS_NET)
>
> +#define LANDLOCK_LAST_SCOPE LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET
> +#define LANDLOCK_MASK_SCOPE ((LANDLOCK_LAST_SCOPE << 1) - 1)
> +#define LANDLOCK_NUM_SCOPE __const_hweight64(LANDLOCK_MASK_SCOPE)
> /* clang-format on */
^ permalink raw reply
* Re: [PATCH v11 1/8] Landlock: Add abstract UNIX socket restriction
From: Mickaël Salaün @ 2024-09-13 10:46 UTC (permalink / raw)
To: Tahera Fahimi, linux-api
Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module,
linux-kernel, bjorn3_gh, jannh, netdev, Alejandro Colomar
In-Reply-To: <5f7ad85243b78427242275b93481cfc7c127764b.1725494372.git.fahimitahera@gmail.com>
On Wed, Sep 04, 2024 at 06:13:55PM -0600, Tahera Fahimi wrote:
> This patch introduces a new "scoped" attribute to the
> landlock_ruleset_attr that can specify
> "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to scope abstract UNIX sockets
> from connecting to a process outside of the same Landlock domain. It
> implements two hooks, unix_stream_connect and unix_may_send to enforce
> this restriction.
>
> Closes: https://github.com/landlock-lsm/linux/issues/7
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> diff --git a/security/landlock/task.c b/security/landlock/task.c
> index 849f5123610b..b9390445d242 100644
> --- a/security/landlock/task.c
> +++ b/security/landlock/task.c
> +static int hook_unix_stream_connect(struct sock *const sock,
> + struct sock *const other,
> + struct sock *const newsk)
> +{
> + const struct landlock_ruleset *const dom =
> + landlock_get_current_domain();
> +
> + /* quick return for non-sandboxed processes */
> + if (!dom)
> + return 0;
> +
> + if (is_abstract_socket(other) && sock_is_scoped(other, dom))
> + return -EPERM;
I was wondering if it would make more sense to return -EACCES here.
EACCES is usually related to file permission, but send(2)/sendto(2)
don't return EPERM according to man pages. Well, according to the
kernel code they can return EPERM so I think we are good with EPERM.
It looks like other LSMs always use EACCES though...
> +
> + return 0;
> +}
> +
> +static int hook_unix_may_send(struct socket *const sock,
> + struct socket *const other)
> +{
> + const struct landlock_ruleset *const dom =
> + landlock_get_current_domain();
> +
> + if (!dom)
> + return 0;
> +
> + if (is_abstract_socket(other->sk)) {
> + /*
> + * Checks if this datagram socket was already allowed to
> + * be connected to other.
> + */
> + if (unix_peer(sock->sk) == other->sk)
> + return 0;
> +
> + if (sock_is_scoped(other->sk, dom))
> + return -EPERM;
ditto
^ permalink raw reply
* Re: [linus:master] [mseal] 8be7258aad: stress-ng.pagemove.page_remaps_per_sec -4.4% regression
From: Christophe Leroy @ 2024-09-13 5:47 UTC (permalink / raw)
To: Michael Ellerman, Linus Torvalds, Nicholas Piggin
Cc: Jeff Xu, Pedro Falcato, kernel test robot, Jeff Xu, oe-lkp, lkp,
linux-kernel, Andrew Morton, Kees Cook, Liam R. Howlett,
Dave Hansen, Greg Kroah-Hartman, Guenter Roeck, Jann Horn,
Jonathan Corbet, Jorge Lucangeli Obes, Matthew Wilcox,
Muhammad Usama Anjum, Stephen Röttger, Suren Baghdasaryan,
Amer Al Shanawany, Javier Carrasco, Shuah Khan, linux-api,
linux-mm, ying.huang, feng.tang, fengwei.yin
In-Reply-To: <87r0b2if4t.fsf@mail.lhotse>
Le 06/08/2024 à 04:01, Michael Ellerman a écrit :
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>> On Mon, 5 Aug 2024 at 16:25, Nicholas Piggin <npiggin@gmail.com> wrote:
>>>
>>> Can userspace on other archs not unmap their vdsos?
>>
>> I think they can, and nobody cares. The "context.vdso" value stays at
>> some stale value, and anybody who tries to use it will just fail.
>>
>> So what makes powerpc special is not "you can unmap the vdso", but
>> "powerpc cares".
>>
>> I just don't quite know _why_ powerpc cares.
>
> AFAIK for CRIU the problem is signal delivery:
>
> arch/powerpc/kernel/signal_64.c:
>
> int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
> struct task_struct *tsk)
> {
> ...
> /* Set up to return from userspace. */
> if (tsk->mm->context.vdso) {
> regs_set_return_ip(regs, VDSO64_SYMBOL(tsk->mm->context.vdso, sigtramp_rt64));
>
>
> ie. if the VDSO is moved but mm->context.vdso is not updated, signal
> delivery will crash in userspace.
>
> x86-64 always uses SA_RESTORER, and arm64 & s390 can use SA_RESTORER, so
> I think CRIU uses that to avoid problems with signal delivery when the
> VDSO is moved.
>
> riscv doesn't support SA_RESTORER but I guess CRIU doesn't support riscv
> yet so it's not become a problem.
>
> There was a patch to support SA_RESTORER on powerpc, but I balked at
> merging it because I couldn't find anyone on the glibc side to say
> whether they wanted it or not. I guess I should have just merged it.
The patch is at
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/afe50d1db63a10fde9547ea08fe1fa68b0638aba.1624618157.git.christophe.leroy@csgroup.eu/
It still applies cleanly.
Christophe
>
> There was an attempt to unify all the vdso stuff and handle the
> VDSO mremap case in generic code:
>
> https://lore.kernel.org/lkml/20210611180242.711399-17-dima@arista.com/
>
> But I think that series got a bit big and complicated and Dmitry had to
> move on to other things.
>
> cheers
^ permalink raw reply
* Re: [PATCH v23 1/4] mm: add MAP_DROPPABLE for designating always lazily freeable mappings
From: Miaohe Lin @ 2024-09-12 3:11 UTC (permalink / raw)
To: Jason A. Donenfeld, linux-kernel, patches, tglx
Cc: linux-crypto, linux-api, x86, Linus Torvalds, Greg Kroah-Hartman,
Adhemerval Zanella Netto, Carlos O'Donell, Florian Weimer,
Arnd Bergmann, Jann Horn, Christian Brauner, David Hildenbrand,
linux-mm, David Hildenbrand
In-Reply-To: <20240712014009.281406-2-Jason@zx2c4.com>
On 2024/7/12 9:40, Jason A. Donenfeld wrote:
> The vDSO getrandom() implementation works with a buffer allocated with a
> new system call that has certain requirements:
>
> - It shouldn't be written to core dumps.
> * Easy: VM_DONTDUMP.
> - It should be zeroed on fork.
> * Easy: VM_WIPEONFORK.
>
> - It shouldn't be written to swap.
> * Uh-oh: mlock is rlimited.
> * Uh-oh: mlock isn't inherited by forks.
>
> - It shouldn't reserve actual memory, but it also shouldn't crash when
> page faulting in memory if none is available
> * Uh-oh: VM_NORESERVE means segfaults.
>
> It turns out that the vDSO getrandom() function has three really nice
> characteristics that we can exploit to solve this problem:
>
> 1) Due to being wiped during fork(), the vDSO code is already robust to
> having the contents of the pages it reads zeroed out midway through
> the function's execution.
>
> 2) In the absolute worst case of whatever contingency we're coding for,
> we have the option to fallback to the getrandom() syscall, and
> everything is fine.
>
> 3) The buffers the function uses are only ever useful for a maximum of
> 60 seconds -- a sort of cache, rather than a long term allocation.
>
> These characteristics mean that we can introduce VM_DROPPABLE, which
> has the following semantics:
>
> a) It never is written out to swap.
> b) Under memory pressure, mm can just drop the pages (so that they're
> zero when read back again).
> c) It is inherited by fork.
> d) It doesn't count against the mlock budget, since nothing is locked.
> e) If there's not enough memory to service a page fault, it's not fatal,
> and no signal is sent.
>
> This way, allocations used by vDSO getrandom() can use:
>
> VM_DROPPABLE | VM_DONTDUMP | VM_WIPEONFORK | VM_NORESERVE
>
> And there will be no problem with OOMing, crashing on overcommitment,
> using memory when not in use, not wiping on fork(), coredumps, or
> writing out to swap.
>
> In order to let vDSO getrandom() use this, expose these via mmap(2) as
> MAP_DROPPABLE.
>
> Note that this involves removing the MADV_FREE special case from
> sort_folio(), which according to Yu Zhao is unnecessary and will simply
> result in an extra call to shrink_folio_list() in the worst case. The
> chunk removed reenables the swapbacked flag, which we don't want for
> VM_DROPPABLE, and we can't conditionalize it here because there isn't a
> vma reference available.
>
> Finally, the provided self test ensures that this is working as desired.
>
> Cc: linux-mm@kvack.org
> Acked-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
...
> diff --git a/mm/memory.c b/mm/memory.c
> index d10e616d7389..18fe893ce96d 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -5690,6 +5690,10 @@ vm_fault_t handle_mm_fault(struct vm_area_struct *vma, unsigned long address,
>
> lru_gen_exit_fault();
>
> + /* If the mapping is droppable, then errors due to OOM aren't fatal. */
> + if (vma->vm_flags & VM_DROPPABLE)
> + ret &= ~VM_FAULT_OOM;
> +
I'm sorry for jumping in here. I am confused about the code in handle_mm_fault(). Since VM_FAULT_OOM is simply
dropped, page fault will be re-triggered soon? If so, when oom is disabled or fails to move forward, page fault
will re-trigger again and again as no memory is available? I might be miss something.
Thanks.
.
^ permalink raw reply
* [PATCH RFC v2 10/10] selftests: mount_setattr: add CHECK_FIELDS selftest
From: Aleksa Sarai @ 2024-09-05 14:56 UTC (permalink / raw)
To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
Valentin Schneider, Alexander Viro, Christian Brauner, Jan Kara,
Arnd Bergmann, Shuah Khan
Cc: Kees Cook, Florian Weimer, Arnd Bergmann, Mark Rutland,
linux-kernel, linux-api, linux-fsdevel, linux-arch,
linux-kselftest, Aleksa Sarai
In-Reply-To: <20240906-extensible-structs-check_fields-v2-0-0f46d2de9bad@cyphar.com>
While we're at it -- to make testing for error codes with ASSERT_*
easier, make the sys_* wrappers return -errno in case of an error.
For some reason, the <sys/sysinfo.h> include doesn't correct import
<asm/posix_types.h>, leading to compilation errors -- so just import
<asm/posix_types.h> manually.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
tools/include/uapi/asm-generic/posix_types.h | 101 +++++++++++++++++++++
tools/testing/selftests/mount_setattr/Makefile | 2 +-
.../selftests/mount_setattr/mount_setattr_test.c | 53 ++++++++++-
3 files changed, 153 insertions(+), 3 deletions(-)
diff --git a/tools/include/uapi/asm-generic/posix_types.h b/tools/include/uapi/asm-generic/posix_types.h
new file mode 100644
index 000000000000..b5f7594eee7a
--- /dev/null
+++ b/tools/include/uapi/asm-generic/posix_types.h
@@ -0,0 +1,101 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef __ASM_GENERIC_POSIX_TYPES_H
+#define __ASM_GENERIC_POSIX_TYPES_H
+
+#include <asm/bitsperlong.h>
+/*
+ * This file is generally used by user-level software, so you need to
+ * be a little careful about namespace pollution etc.
+ *
+ * First the types that are often defined in different ways across
+ * architectures, so that you can override them.
+ */
+
+#ifndef __kernel_long_t
+typedef long __kernel_long_t;
+typedef unsigned long __kernel_ulong_t;
+#endif
+
+#ifndef __kernel_ino_t
+typedef __kernel_ulong_t __kernel_ino_t;
+#endif
+
+#ifndef __kernel_mode_t
+typedef unsigned int __kernel_mode_t;
+#endif
+
+#ifndef __kernel_pid_t
+typedef int __kernel_pid_t;
+#endif
+
+#ifndef __kernel_ipc_pid_t
+typedef int __kernel_ipc_pid_t;
+#endif
+
+#ifndef __kernel_uid_t
+typedef unsigned int __kernel_uid_t;
+typedef unsigned int __kernel_gid_t;
+#endif
+
+#ifndef __kernel_suseconds_t
+typedef __kernel_long_t __kernel_suseconds_t;
+#endif
+
+#ifndef __kernel_daddr_t
+typedef int __kernel_daddr_t;
+#endif
+
+#ifndef __kernel_uid32_t
+typedef unsigned int __kernel_uid32_t;
+typedef unsigned int __kernel_gid32_t;
+#endif
+
+#ifndef __kernel_old_uid_t
+typedef __kernel_uid_t __kernel_old_uid_t;
+typedef __kernel_gid_t __kernel_old_gid_t;
+#endif
+
+#ifndef __kernel_old_dev_t
+typedef unsigned int __kernel_old_dev_t;
+#endif
+
+/*
+ * Most 32 bit architectures use "unsigned int" size_t,
+ * and all 64 bit architectures use "unsigned long" size_t.
+ */
+#ifndef __kernel_size_t
+#if __BITS_PER_LONG != 64
+typedef unsigned int __kernel_size_t;
+typedef int __kernel_ssize_t;
+typedef int __kernel_ptrdiff_t;
+#else
+typedef __kernel_ulong_t __kernel_size_t;
+typedef __kernel_long_t __kernel_ssize_t;
+typedef __kernel_long_t __kernel_ptrdiff_t;
+#endif
+#endif
+
+#ifndef __kernel_fsid_t
+typedef struct {
+ int val[2];
+} __kernel_fsid_t;
+#endif
+
+/*
+ * anything below here should be completely generic
+ */
+typedef __kernel_long_t __kernel_off_t;
+typedef long long __kernel_loff_t;
+typedef __kernel_long_t __kernel_old_time_t;
+#ifndef __KERNEL__
+typedef __kernel_long_t __kernel_time_t;
+#endif
+typedef long long __kernel_time64_t;
+typedef __kernel_long_t __kernel_clock_t;
+typedef int __kernel_timer_t;
+typedef int __kernel_clockid_t;
+typedef char * __kernel_caddr_t;
+typedef unsigned short __kernel_uid16_t;
+typedef unsigned short __kernel_gid16_t;
+
+#endif /* __ASM_GENERIC_POSIX_TYPES_H */
diff --git a/tools/testing/selftests/mount_setattr/Makefile b/tools/testing/selftests/mount_setattr/Makefile
index 0c0d7b1234c1..3f260506fe60 100644
--- a/tools/testing/selftests/mount_setattr/Makefile
+++ b/tools/testing/selftests/mount_setattr/Makefile
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
# Makefile for mount selftests.
-CFLAGS = -g $(KHDR_INCLUDES) -Wall -O2 -pthread
+CFLAGS = -g $(KHDR_INCLUDES) $(TOOLS_INCLUDES) -Wall -O2 -pthread
TEST_GEN_PROGS := mount_setattr_test
diff --git a/tools/testing/selftests/mount_setattr/mount_setattr_test.c b/tools/testing/selftests/mount_setattr/mount_setattr_test.c
index c6a8c732b802..36b8286e5f43 100644
--- a/tools/testing/selftests/mount_setattr/mount_setattr_test.c
+++ b/tools/testing/selftests/mount_setattr/mount_setattr_test.c
@@ -11,6 +11,7 @@
#include <sys/wait.h>
#include <sys/vfs.h>
#include <sys/statvfs.h>
+#include <asm/posix_types.h> /* get __kernel_* typedefs */
#include <sys/sysinfo.h>
#include <stdlib.h>
#include <unistd.h>
@@ -137,7 +138,8 @@
static inline int sys_mount_setattr(int dfd, const char *path, unsigned int flags,
struct mount_attr *attr, size_t size)
{
- return syscall(__NR_mount_setattr, dfd, path, flags, attr, size);
+ int ret = syscall(__NR_mount_setattr, dfd, path, flags, attr, size);
+ return ret >= 0 ? ret : -errno;
}
#ifndef OPEN_TREE_CLONE
@@ -152,9 +154,24 @@ static inline int sys_mount_setattr(int dfd, const char *path, unsigned int flag
#define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
#endif
+#define FSMOUNT_VALID_FLAGS \
+ (MOUNT_ATTR_RDONLY | MOUNT_ATTR_NOSUID | MOUNT_ATTR_NODEV | \
+ MOUNT_ATTR_NOEXEC | MOUNT_ATTR__ATIME | MOUNT_ATTR_NODIRATIME | \
+ MOUNT_ATTR_NOSYMFOLLOW)
+
+#define MOUNT_SETATTR_VALID_FLAGS (FSMOUNT_VALID_FLAGS | MOUNT_ATTR_IDMAP)
+
+#define MOUNT_SETATTR_PROPAGATION_FLAGS \
+ (MS_UNBINDABLE | MS_PRIVATE | MS_SLAVE | MS_SHARED)
+
+#ifndef CHECK_FIELDS
+#define CHECK_FIELDS (1ULL << 63)
+#endif
+
static inline int sys_open_tree(int dfd, const char *filename, unsigned int flags)
{
- return syscall(__NR_open_tree, dfd, filename, flags);
+ int ret = syscall(__NR_open_tree, dfd, filename, flags);
+ return ret >= 0 ? ret : -errno;
}
static ssize_t write_nointr(int fd, const void *buf, size_t count)
@@ -1497,4 +1514,36 @@ TEST_F(mount_setattr, mount_attr_nosymfollow)
ASSERT_EQ(close(fd), 0);
}
+TEST_F(mount_setattr, check_fields)
+{
+ struct mount_attr_ext {
+ struct mount_attr inner;
+ uint64_t extra1;
+ uint64_t extra2;
+ uint64_t extra3;
+ } attr;
+
+ /* Set the structure to dummy values. */
+ memset(&attr, 0xAB, sizeof(attr));
+
+ if (!mount_setattr_supported())
+ SKIP(return, "mount_setattr syscall not supported");
+
+ /* Make sure that invalid sizes are detected even with CHECK_FIELDS. */
+ EXPECT_EQ(sys_mount_setattr(-1, "", 0, (struct mount_attr *) &attr, CHECK_FIELDS), -EINVAL);
+ EXPECT_EQ(sys_mount_setattr(-1, "", 0, (struct mount_attr *) &attr, CHECK_FIELDS | 0xF000), -E2BIG);
+
+ /* Actually get the CHECK_FIELDS results. */
+ ASSERT_EQ(sys_mount_setattr(-1, "", 0, (struct mount_attr *) &attr, CHECK_FIELDS | sizeof(attr)), -EEXTSYS_NOOP);
+
+ EXPECT_EQ(attr.inner.attr_set, MOUNT_SETATTR_VALID_FLAGS);
+ EXPECT_EQ(attr.inner.attr_clr, MOUNT_SETATTR_VALID_FLAGS);
+ EXPECT_EQ(attr.inner.propagation, MOUNT_SETATTR_PROPAGATION_FLAGS);
+ EXPECT_EQ(attr.inner.userns_fd, 0xFFFFFFFFFFFFFFFF);
+ /* Trailing fields outside ksize must be zeroed. */
+ EXPECT_EQ(attr.extra1, 0);
+ EXPECT_EQ(attr.extra2, 0);
+ EXPECT_EQ(attr.extra3, 0);
+}
+
TEST_HARNESS_MAIN
--
2.46.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox