Re: [PATCH] mm: move the check of READ_IMPLIES_EXEC out of do_mmap()

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Wed, Sep 25, 2024 at 05:09:47PM GMT, Shu Han wrote:
> > You have sent this non-RFC intentionally conflicting with [0] to provide
> > 'alternatives' that is not what a [PATCH] submission is.
> >
> > In any case, speculative changes like this should ABSOLUTELY be sent RFC,
> > and sending things that are merge conflicts as ordinary patches is actually
> > bordering on being a little rude!
> >
> > I'm sure it's unintentional :) but for the sake of us being able to work
> > with these properly you should just send one as RFC and ask whether it'd be
> > appropriate to send an alternative, and just allude to it in the one you do
> > send.
> >
> > [0]:https://lore.kernel.org/all/20240925081628.408-1-ebpqwerty472123@gmail.com/
>
> I am very sorry that I sent the wrong subject which should add "RFC",
> due to lack of experience.

No need to be sorry! :) Sorry if I sound harsh here - it's more a case of
trying to be as clear as I can be as that is the best approach for everyone
I think.

This code is sensitive, so we have to super careful!

>
> > It's a bit weird to send 'alternatives' - you should by now have a good
> > sense of which ought to work, if not perhaps more research is required on
> > your part?
>
> I think both solutions can work, and the preliminary discussion is on
> the mail list for [1]
> (as this issue is related to security before it was fixed, the
> discussion is on security@kernel.org).
> The choice depends only on taste.
>
> Link: https://lore.kernel.org/all/20240919080905.4506-2-paul@paul-moore.com/ [1]

I would disagree it's down to taste, I noted on the move the check to
do_mmap() series a number of issues and concerns, to me that seems
unworkable in it's current form, the locking thing is fatal for instance.

What you link to there seems to be neither approach (I didn't read your
second series though as that needs an RFC tag)? I mean I think perhaps what
you are doing there is the best _first step_ - simply add the checks in
each of the callsites that you feel are missing them.

This is the least controversial way and then allows maintainers of the
callers to assess whether they intended for that.

If then you end up wtih _all_ callers doing this check, we can take another
look at possibly bringing it into do_mmap() but we would absolutely have to
ensure it was done correctly, however.

I do feel we need to better document these functions, so I will add
comments. I see you did so as part of your other series, but think maybe we
need to expand this and possibly rename both and add some asserts... it's
on the todo list!

So moving forward I suggest:

1. (If you haven't already) Submit a series that adds patches to add checks
   at call sites that don't already check.

2. If these are accepted at _all_ callsites, revisit the do_mmap() change,
   properly accounting for locks (I can help with this).

Thanks for your contribution!