Messages in this thread Patch in this message | | | Date | Sat, 18 Nov 2023 19:40:07 +0100 | Subject | Re: [GIT PULL] parisc architecture fixes for v6.7-rc1 | From | Helge Deller <> |
| |
On 11/18/23 18:36, Linus Torvalds wrote: > 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 */
Ok. My initial patch was actually doing exatly that, but somehow I finally decided to add it to the switch() instead. Seems this was the wrong decision :-(
> 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.
I agree that returning something else than EINVAL would be better. I used ENODEV in an earlier patch (I didn't liked it either), but according to https://github.com/systemd/systemd/issues/29775#issuecomment-1809563365 EINVAL seems the best solution currently.
Just as a side-note: ENOSYS gives a checkpatch warning: WARNING: ENOSYS means 'invalid syscall nr' and nothing else
Would the patch below be OK? It's basically yours but with EINVAL. (might be whitespace-scrambled!)
Helge
---
From: Helge Deller <deller@gmx.de> Subject: [PATCH] prctl: Disable prctl(PR_SET_MDWE) on parisc
systemd-254 tries to use prctl(PR_SET_MDWE) for it's MemoryDenyWriteExecute functionality, but fails on parisc which still needs executable stacks in certain combinations of gcc/glibc/kernel.
Disable prctl(PR_SET_MDWE) by returning -EINVAL for now on parisc, until userspace has catched up.
Signed-off-by: Helge Deller <deller@gmx.de> Co-developed-by: Linus Torvalds <torvalds@linux-foundation.org> Reported-by: Sam James <sam@gentoo.org> Closes: https://github.com/systemd/systemd/issues/29775 Tested-by: Sam James <sam@gentoo.org> Link: https://lore.kernel.org/all/875y2jro9a.fsf@gentoo.org/ Cc: <stable@vger.kernel.org> # v6.3+
diff --git a/kernel/sys.c b/kernel/sys.c index 420d9cb9cc8e..e219fcfa112d 100644 --- a/kernel/sys.c +++ b/kernel/sys.c @@ -2394,6 +2394,10 @@ static inline int prctl_set_mdwe(unsigned long bits, unsigned long arg3, 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 -EINVAL; + current_bits = get_current_mdwe(); if (current_bits && current_bits != bits) return -EPERM; /* Cannot unset the flags */
| |