Messages in this thread Patch in this message | | | From | Linus Torvalds <> | Date | Sat, 18 Nov 2023 09:36:14 -0800 | Subject | Re: [GIT PULL] parisc architecture fixes for v6.7-rc1 |
| |
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
| |