All of lore.kernel.org
 help / color / mirror / Atom feed
From: YiFei Zhu <zhuyifei1999@gmail.com>
To: Johannes Berg <johannes@sipsolutions.net>,
	Colin Ian King <colin.king@canonical.com>
Cc: linux-um@lists.infradead.org, Jeff Dike <jdike@addtoit.com>,
	Richard Weinberger <richard@nod.at>,
	Anton Ivanov <anton.ivanov@cambridgegreys.com>,
	Benjamin Berg <benjamin@sipsolutions.net>
Subject: Re: [PATCH v2] um: Fix stack pointer alignment
Date: Mon, 19 Apr 2021 15:36:11 -0500	[thread overview]
Message-ID: <CABqSeASZLeK8dq-75dcM51zmkHUteuJc07bS8ixKVhEAAeuPDw@mail.gmail.com> (raw)
In-Reply-To: <546324c4e77ebe65f27b0f06fea3ace28dd75fe6.camel@sipsolutions.net>

On Mon, Apr 19, 2021 at 2:41 PM Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2021-04-19 at 10:32 -0500, YiFei Zhu wrote:
> > On a side note, musl is unaffected by this issue because it forces
> > 16 byte alignment via `and $-16,%rsi` in its clone wrapper.
> > Similarly, glibc i386 is also uneffected because it has

I realize I forgot to fix the typo here. Can you amend it or shall I send a v3?

> > `andl $0xfffffff0, %ecx`.
>
> I wonder if this isn't really a glibc bug?
>
> After all, the man page states no alignment restrictions, except when
> documenting the error codes:
>
> EINVAL
> stack is not aligned to a suitable boundary for  this  architecture.
> For example, on aarch64, stack must be a multiple of 16.

This could be considered a glibc bug that it doesn't force alignment,
yeah, considering musl does it for both x86_32 and x86_64, and glibc
does it for only x86_32 and not x86_64. However, I'm unaware that
anywhere saying something like "it's libc's duty to align the stack
pointer to clone()"

Speaking of aarch64, it looks like that message might be out of date.
I was trying to find where this is being enforced, and could not
quickly find the code, so did a quick search on this and see commit
e6d9a5254333 ("arm64: do not enforce strict 16 byte alignment to stack
pointer"), and also two related discussions [1][2]. It seems that the
error entry to the man page was added around the same time as the
check got removed, and the check was there only because it would have
caused SIGBUS when the clone returns. Although in x86, non-16-byte
aligned push / pop would not SIGBUS, unlike aarch64...

[1] https://patchwork.kernel.org/project/linux-arm-kernel/patch/1462985814-16146-1-git-send-email-colin.king@canonical.com/
[2] https://lore.kernel.org/linux-man/571E731A.6050809@canonical.com/

> > To reproduce this bug, enable CONFIG_UML_RTC. uml_rtc will call
> > add_sigio_fd which will then cause write_sigio_thread to go
> > into segfault loop.
>
> It must also depend on the glibc version, because I've definitely been
> testing UML_RTC on 64-bit, on Fedora 32 at the time.
>

Hmm. Interesting. I can't seem to find anything suggesting Fedora has
a patch that would align the stack within clone() [3][4]. I also got a
Fedora 32 docker image and could not see the aligning from disassembly
of clone, and the gcc version installed by yum is 10.2.1-9.fc32, which
is supposed to be affected by this issue... weird. I would expect this
to fail outright. I'm considering compiling uml inside this container
to see what is going on.

[3] https://github.com/bminor/glibc/commits/master/sysdeps/unix/sysv/linux/x86_64/clone.S
[4] https://src.fedoraproject.org/rpms/glibc/tree/rawhide

YiFei Zhu

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um


  reply	other threads:[~2021-04-19 20:36 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-19 15:32 [PATCH v2] um: Fix stack pointer alignment YiFei Zhu
2021-04-19 19:41 ` Johannes Berg
2021-04-19 20:36   ` YiFei Zhu [this message]
2021-04-20  5:47     ` YiFei Zhu
2021-04-20  6:20       ` YiFei Zhu
2021-04-20  6:51       ` Johannes Berg
2021-04-20  6:50     ` Johannes Berg

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=CABqSeASZLeK8dq-75dcM51zmkHUteuJc07bS8ixKVhEAAeuPDw@mail.gmail.com \
    --to=zhuyifei1999@gmail.com \
    --cc=anton.ivanov@cambridgegreys.com \
    --cc=benjamin@sipsolutions.net \
    --cc=colin.king@canonical.com \
    --cc=jdike@addtoit.com \
    --cc=johannes@sipsolutions.net \
    --cc=linux-um@lists.infradead.org \
    --cc=richard@nod.at \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.