Re: [PATCH] [RFC] arm64: Exposes support for 32bit syscalls

[Mark Rutland]

Hi,

On Tue, Feb 02, 2021 at 08:54:23AM -0800, sonicadvance1@xxxxxxxxx wrote:
> From: Ryan Houdek <Sonicadvance1@xxxxxxxxx>
> 
> 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.