Messages in this thread | | | Date | Tue, 2 Feb 2021 19:23:31 +0000 | From | Mark Rutland <> | Subject | Re: [PATCH] [RFC] arm64: Exposes support for 32bit syscalls |
| |
Hi,
On Tue, Feb 02, 2021 at 08:54:23AM -0800, sonicadvance1@gmail.com wrote: > From: Ryan Houdek <Sonicadvance1@gmail.com> > > This is a continuation of https://lkml.org/lkml/2021/1/6/47 > This patch is currently based against a 5.10 kernel but rebasing against > latest HEAD is trivial > > Specifically Amanieu pointed out a couple of problem spaces that would > show up around memory management and various other bits. > > This convinced me that the previous path of only having an ioctl32 > syscall is only a bandage on a much larger problem. > > This takes a patch from the Tango support tree and modifies it a bit to > not rely on a Tango specific quirk. > > Original patch: > https://github.com/Amanieu/linux/commit/b4783002afb027ae702da8f56e43e45c7332d226
Please reconsider the presentation of this commit message, because as it stands it's practically unreviewable, and for the vast majority of people CC'd this is noise.
You need to write this such that someone can read this from start to finish and understand each step without jumping back-and-forth, without the reader having to read external links. Introduce the big picture first (what problem are you trying to solve? Who does this matter to?), then context (There's a constraint ..., someone previously tried this but there was a specific problem ...), then give an overview and justification for the code, with any caveats that reviewers may need to take into account.
As-is, this commit message doesn't follow that flow, and that places a burden on reviewers to expend significant effort to reverse-engineer and extract the information they need.
You'll find it's much easier to make progress if you optimize for the reviewers. Please look at the general way commits in the Linux source tree are written, and try to follow that approach.
> This patch changes the KSTK_EIP and KSTK_ESP helpers to only fall to > Tango specific behaviour once a "Tango" syscall has been invoked.
At this point the reader has no idea what a "Tango" syscall is, nor why this should affect KSTK_EIP or KSTK_ESP.
What is a "Tango" syscall?
Who is this useful for?
Why does this matter to the upstream kernel?
> I'm working on a backwards compatibility project that is unrelated to > Tango, but it would be nice to have a solution that works for both of > us. Since we are both working on projects that run 32bit applications > inside of a 64bit process for compatibility purposes.
Only here do reviewers discover this something to do with running 32-bit applications somehow, when really that should be in the first couple of sentences.
Generally, I have significant misgivings about exposing a new syscall interface (or exposing an existing interface to different callers). It's a maintainability nightmare, and since it's liable to violate implicit assumptions made in syscall implementations there's huge scope for error including bugs and exploitable behaviour.
So without significant justification, my view is to NAK this sort of change.
Thanks, Mark.
| |