Message-ID: <9bad20fe-147c-409a-b3d4-36292266ad36@lucifer.local>
Date: Wed, 25 Sep 2024 19:31:36 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Pedro Falcato <pedro.falcato@...il.com>
Cc: Shakeel Butt <shakeel.butt@...ux.dev>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>,
        Suren Baghdasaryan <surenb@...gle.com>, Arnd Bergmann <arnd@...db.de>,
        linux-api@...r.kernel.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, Minchan Kim <minchan@...nel.org>,
        Richard Henderson <richard.henderson@...aro.org>,
        Ivan Kokshaysky <ink@...assic.park.msu.ru>,
        Matt Turner <mattst88@...il.com>, linux-alpha@...r.kernel.org,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        linux-mips@...r.kernel.org,
        "James E . J . Bottomley" <James.Bottomley@...senpartnership.com>,
        Helge Deller <deller@....de>, linux-parisc@...r.kernel.org,
        Chris Zankel <chris@...kel.net>, Max Filippov <jcmvbkbc@...il.com>,
        christian@...uner.io
Subject: Re: [PATCH v2 1/2] mm/madvise: introduce PR_MADV_SELF flag to
 process_madvise()

On Wed, Sep 25, 2024 at 07:14:51PM GMT, Pedro Falcato wrote:
> On Wed, Sep 25, 2024 at 06:04:59PM GMT, Lorenzo Stoakes wrote:
> > On Wed, Sep 25, 2024 at 09:19:17AM GMT, Shakeel Butt wrote:
> > > I have no idea what makes you think I am blocking the feature that you
> > > repond in a weird tone but let me be upfront what I am asking: Let's
> > > collectively decide which is the better option (in terms of
> > > maintainability and extensibility) and move forward.
> >
> > I'm not sure what you mean by 'weird tone'... perhaps a miscommunication?
> >
> > To summarise in my view - a suggestion was made to, rather than provide the
> > proposed flag - a pidfd sentinel should be introduced.
> >
> > Simply introducing a sentinel that represents 'the current process' without
> > changing interfaces that accept a pidfd would be broken - so implementing
> > this implies that _all_ pidfd interfaces are updated, as well as tests.
> >
>
> While I suggested PIDFD_SELF, I never meant that we should change every interface,
> but rather adopt a sound, consistent strategy for pidfd interfaces and stick with
> it for the foreseeable future.

If we add this to a public uapi-facing header and document it as 'refer to
self', we will introduce something that users will receive and be confused
if they are unable to use anywhere else.

This is the fundamental problem with this. It's a fundamental, permanent
uAPI change which either introduces confusion - because it only works for
process_madvise() - or will need work along with test updates etc.

And it is very much out of scope for this series as a result.

I may just back out of doing this and replace this with something simpler
that causes less push-back (and fixes existing broken behaviour in
process_madvise()) that lets me do what I need to do with the guard
pages. I don't think anybody can object to a self-pidfd having unrestricted
access to madvise flags...

I may in parallel just try to implement the pidfd sentinel idea and let it
take that long time and update process_madvise() later, as I can't really
let this block the guard page work.

>
> In this case, we'd adapt process_madvise, then possibly later pidfd_send_signal, etc.
> There are plenty of pidfd interfaces that don't make sense with a PIDFD_SELF. Various
> other interfaces will probably never want to adopt it at all (select _can't_, other
> fs syscalls such as read/write/poll/whatever would require awful handholding from
> various kernel subsystems, while in that sense we would definitely require a proper
> struct file/inode/whatever for each pseudo-fd, which is not exactly what we want).
>

And arguably, you'd have to audit all of them and decide. I mean I think
this kind of sums up my point really right?

Again, I don't object to the idea, I object to the suggestion that you
don't need to do this other work.

> > I suggest doing so is, of course, entirely out of the scope of this
> > change. Therefore if we were to require that here - it would block the
> > feature while I go work on that.
> >
> > I think this is pretty clear right? And I also suggest that doing so is
> > likely to take quite some time, and may not even have a positive outcome.
> >
> > So it's not a case of 'shall we take approach A or approach B?' but rather
> > 'should we take approach A or entirely implement a new feature B, then once
> > that is done, use it'.
> >
> > So as to your 'collectively decide what is the better option' - in my
> > previous response I argued that the best approach between 'use an
> > unimplemented suggested entirely new feature of pidfd' vs. 'implement a
> > flag that would in no way block the prior approach' - a flag works better.
>
> I just don't think it's a new feature, just an established, future-proof way
> of doing things :) Your patch should remain mostly similar apart from switching
> the flag check into an fd check.

It is absolutely a new feature, you're adding an entirely new UAPI-visible
flag that is applicable to all pidfd's, unless we make it
process_madvise()-specific in the flag, and I'm not sure that's going to
get accepted.

It also needs to be separately accepted by the maintainers of the relevant
file header etc.

>
> > >
> > > By big undertaking, do you mean other syscalls that take pidfd
> > > (pidfd_getfd, pidfd_send_signal & process_mrelease) to handle PIDFD_SELF
> > > or something else?
> >
> > I mean if you add a pidfd sentinel that represents 'the current process' it
> > may get passed to any interface that accepts a pidfd, so all of them have
> > to handle it _somehow_.
> >
> > Also you'll want to update tests accordingly and clearly need to get
> > community buy-in for that feature.
> >
> > You may want to just add a bunch of:
> >
> > if (pidfd == SENTINEL)
> > 	return -EINVAL;
>
> It should already be there in the form of an -EBADF.

:) this is pseudo-code. And I'd want to check all pidfd handled it
correctly. I mean you'd think so right...

>
> >
> > So it's not impossible my instincts are off and we can get away with simply
> > doing that.
> >
> > On the other hand, would that be confusing? Wouldn't we need to update
> > documentation, manpages, etc. to say explicitly 'hey this sentinel is just
> > not supported'?
>
> This is a fair point, but we could also just... not :) which I don't feel is too
> wrong, since the fd works kind of like a flag here.

Yeah, no, I think you would have to. It's not specific to
process_madvise(), it's a general fd flag.

And like I said, if we did introduce this we'd need additional assessment
and review from those guys.

>
> --
> Pedro

Powered by blists - more mailing lists