From: Linus Torvalds <torvalds@linux-foundation.org>
To: Helge Deller <deller@gmx.de>
Cc: linux-kernel@vger.kernel.org, linux-parisc@vger.kernel.org,
James Bottomley <James.Bottomley@hansenpartnership.com>,
John David Anglin <dave.anglin@bell.net>,
Kees Cook <keescook@chromium.org>
Subject: Re: [GIT PULL] parisc architecture fixes for v6.7-rc1
Date: Sat, 18 Nov 2023 09:36:14 -0800 [thread overview]
Message-ID: <CAHk-=wgtxsw8hrLNcGz9aJtP17NR3=NSvRdYGfKB52VorhkWzQ@mail.gmail.com> (raw)
In-Reply-To: <ZVjC9P0h5mw3ZbnD@p100>
On Sat, 18 Nov 2023 at 05:58, Helge Deller <deller@gmx.de> wrote:
>
> On parisc we still sometimes need writeable stacks, e.g. if programs aren't
> compiled with gcc-14. To avoid issues with the upcoming systemd-254 we
> therefore have to disable prctl(PR_SET_MDWE) for now (for parisc only).
Ugh.
I pulled this, but I *really* cannot live with how ugly that is.
Seriously, that code is just unacceptable. Doing something like
+ if (IS_ENABLED(CONFIG_PARISC))
+ error = -EINVAL;
+ else
+ error = prctl_set_mdwe(arg2, arg3, arg4, arg5);
in generic code with no comment is just truly crazy. If you have to go
and do a "git blame -C" just to understand why the code exists, the
code is a problem.
But it goes beyond that. The code is just *ugly*, and it's done
entirely in the wrong place.
Things like "mdwe is special on parisc" should *NOT* be done in the
generic "prctl()" function. This issue is not specific to prctl() -
it's very much specific to mdwe.
So I think it would have been both much more legible, and *much* more
appropriate, to do it in prctl_set_mdwe() itself, where it makes more
sense, and where it matches all the *other* mdwe-specific checks the
code does wrt arguments and existing state.
And honestly, why wouldn't 'get_mdwe' work? So the *other* hunk in
that patch (which isn't even mentioned in the commit message) that
returns -EINVAL for get_mdwe makes no sense at all, and shouldn't have
existed.
End result: I think the code should have been something like this
(whitespace-damaged) thing:
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2394,6 +2394,10 @@ static inline int prctl_set_mdwe(unsigned
long bits,
if (bits & PR_MDWE_NO_INHERIT && !(bits & PR_MDWE_REFUSE_EXEC_GAIN))
return -EINVAL;
+ /* PARISC cannot allow mdwe as it needs writable stacks */
+ if (IS_ENABLED(CONFIG_PARISC))
+ return -ENOSYS;
+
current_bits = get_current_mdwe();
if (current_bits && current_bits != bits)
return -EPERM; /* Cannot unset the flags */
where I also picked another error code, because it's not that the
prctl value or the arguments are invalid, I think the error should
show that there's something else going on.
No, I don't think -ENOSYS is necessarily the best possible error
value, but I think it at least conceptually matches the "this prctl
doesn't exist on PARISC". Maybe
Maybe ENOSYS should be avoided (prctl() obvious does exist), but I do
think this should be a different error than the EINVAL that the
generic checks do.
End result: I really hated this change so much that I ended up
unpulling after doing the pull. This needs to be done right, or not at
all.
Linus
next prev parent reply other threads:[~2023-11-18 17:36 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-18 13:58 [GIT PULL] parisc architecture fixes for v6.7-rc1 Helge Deller
2023-11-18 17:36 ` Linus Torvalds [this message]
2023-11-18 18:40 ` Helge Deller
2023-11-18 18:42 ` Linus Torvalds
2023-11-18 18:47 ` Helge Deller
-- strict thread matches above, loose matches on Subject: below --
2023-10-31 10:27 Helge Deller
2023-11-01 20:50 ` pr-tracker-bot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAHk-=wgtxsw8hrLNcGz9aJtP17NR3=NSvRdYGfKB52VorhkWzQ@mail.gmail.com' \
--to=torvalds@linux-foundation.org \
--cc=James.Bottomley@hansenpartnership.com \
--cc=dave.anglin@bell.net \
--cc=deller@gmx.de \
--cc=keescook@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-parisc@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).