linux-kernel - Re: [PATCH v2 1/2] mm/madvise: introduce PR_MADV_SELF flag to process

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <asbdij3q2iabrnq5wdxmcq3g7ofut2malicushswi3rma6glf5@k6eftopwmwvj>
Date: Wed, 25 Sep 2024 19:14:51 +0100
From: Pedro Falcato <pedro.falcato@...il.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.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 06:04:59PM GMT, Lorenzo Stoakes wrote:
> On Wed, Sep 25, 2024 at 09:19:17AM GMT, Shakeel Butt wrote:
> > I have no idea what makes you think I am blocking the feature that you
> > repond in a weird tone but let me be upfront what I am asking: Let's
> > collectively decide which is the better option (in terms of
> > maintainability and extensibility) and move forward.
> 
> I'm not sure what you mean by 'weird tone'... perhaps a miscommunication?
> 
> To summarise in my view - a suggestion was made to, rather than provide the
> proposed flag - a pidfd sentinel should be introduced.
> 
> Simply introducing a sentinel that represents 'the current process' without
> changing interfaces that accept a pidfd would be broken - so implementing
> this implies that _all_ pidfd interfaces are updated, as well as tests.
>

While I suggested PIDFD_SELF, I never meant that we should change every interface,
but rather adopt a sound, consistent strategy for pidfd interfaces and stick with
it for the foreseeable future.

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

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

I just don't think it's a new feature, just an established, future-proof way
of doing things :) Your patch should remain mostly similar apart from switching
the flag check into an fd check.

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

It should already be there in the form of an -EBADF.

> 
> So it's not impossible my instincts are off and we can get away with simply
> doing that.
> 
> On the other hand, would that be confusing? Wouldn't we need to update
> documentation, manpages, etc. to say explicitly 'hey this sentinel is just
> not supported'?

This is a fair point, but we could also just... not :) which I don't feel is too
wrong, since the fd works kind of like a flag here.

-- 
Pedro