All of lore.kernel.org
 help / color / mirror / Atom feed
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Richard Kuo <rkuo@codeaurora.org>
Cc: linux-kernel@vger.kernel.org, linux-hexagon@vger.kernel.org,
	Linus Torvalds <torvalds@linux-foundation.org>
Subject: hexagon: signal handling bugs
Date: Sat, 11 Feb 2012 23:27:40 +0000	[thread overview]
Message-ID: <20120211232740.GM23916@ZenIV.linux.org.uk> (raw)
In-Reply-To: <20110817163521.165013232@codeaurora.org>

	1) unless I'm seriously misreading hexagon vm_entry.S, once we'd
called do_notify_resume, we do *not* recheck if there's more work to
be done.  In particular, if we'd got more signals pending - e.g. from
SIGSEGV we'd got while trying to set a sigframe up.  That's not a good
thing for a lot of reasons.  Note that once that is fixed, you'll need
to make sure that restart logics is applied only *once* - not for subsequent
sigframes.

	2) sigreturn should *NOT* be restartable at all, or you'll get
a lot of hard-to-reproduce fun.  AFAICS, the right thing to do would
be to set ->syscall_nr to something negative, not __NR_rt_sigreturn in there
(and return regs->r00 instead of 0, to avoid the check in do_trap0).
Reason: suppose you have e.g. a loop that runs through different values in
r00.  No syscalls in it.  A signal comes and gets treated on the way
out of kernel after the next e.g. timer interrupt.  We save the values
of registers into a sigframe and go into the handler.  Another signal
comes; the next time we reach the kernel is when we get to rt_sigreturn().
There we
	* have registers restored from on-stack sigframe
	* have ->syscall_nr set to __NR_rt_sigreturn
	* hit handle_signal() on the way out
	* check ->r00; note that this is the value we have just restored
from sigframe.  If it happens to be -ERESTARTNOHAND, silently replace it
with -EINTR.  And eventually return to clueless userland.
Result: if timer interrupt happens when the userland loop goes through
-ERESTARTNOHAND, the value of r00 is clobbered upon return from signal handler.
Fun debugging that kind of bugs: priceless...

	3) on sigreturn we need to set current->restart_block.fn; hexagon
doesn't do that.

	4) altstack can overflow; you don't check that.  See
commit 83bd01024b1fdfc41d9b758e5669e80fca72df66
Author: Roland McGrath <roland@redhat.com>
Date:   Wed Jan 30 13:30:06 2008 +0100
for an analog.

	5) try_to_freeze() has no business being called in do_signal();
let get_signal_to_deliver() handle that.

I don't have any access to hardware *or* cross-toolchain, so all I can do
here is RTFS and review.  For #2..#5 I would not be afraid to post untested
patches (they are essentially trivial), but #1 is well beyond my arrogance
threshold - messing in assembler glue on an architecture I don't know *and*
can't test...  Sorry.

  reply	other threads:[~2012-02-11 23:27 UTC|newest]

Thread overview: 70+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-08-17 16:34 [patch 00/36] Hexagon: Add support for Qualcomm Hexagon architecture Richard Kuo
2011-08-17 16:34 ` [patch 01/36] Hexagon: Add generic headers Richard Kuo
2011-08-17 19:19   ` Arnd Bergmann
2011-08-17 16:34 ` [patch 02/36] Hexagon: Core arch-specific header files Richard Kuo
2011-08-17 19:19   ` Arnd Bergmann
2011-08-17 16:35 ` [patch 03/36] Hexagon: Add bitops support Richard Kuo
2011-08-17 19:19   ` Arnd Bergmann
2011-08-26 20:34   ` Pavel Machek
2011-08-30 21:14     ` ARM assembly syntax (was Re: [patch 03/36] Hexagon: Add bitops support) Linas Vepstas (Code Aurora)
2011-09-02 16:53       ` Pavel Machek
2011-08-30 21:59     ` [patch 03/36] Hexagon: Add bitops support Måns Rullgård
2011-08-17 16:35 ` [patch 04/36] Hexagon: Add atomic ops support Richard Kuo
2011-08-17 19:20   ` Arnd Bergmann
2011-08-17 16:35 ` [patch 05/36] Hexagon: Add syscalls Richard Kuo
2011-08-17 19:29   ` Arnd Bergmann
2011-08-17 20:12   ` Jonas Bonn
2011-08-17 20:12     ` Jonas Bonn
2011-08-17 16:35 ` [patch 06/36] Hexagon: Add processor and system headers Richard Kuo
2011-08-17 19:31   ` Arnd Bergmann
2011-08-17 16:35 ` [patch 07/36] Hexagon: Add threadinfo Richard Kuo
2011-08-17 19:37   ` Arnd Bergmann
2011-08-17 16:35 ` [patch 08/36] Hexagon: Add delay functions Richard Kuo
2011-08-17 19:38   ` Arnd Bergmann
2011-08-17 16:35 ` [patch 09/36] Hexagon: Add checksum functions Richard Kuo
2011-08-17 19:40   ` Arnd Bergmann
2011-08-17 16:35 ` [patch 10/36] Hexagon: Add memcpy and memset accelerated functions Richard Kuo
2011-08-17 16:35 ` [patch 11/36] Hexagon: Add hypervisor interface Richard Kuo
2011-08-17 16:35 ` [patch 12/36] Hexagon: Export ksyms defined in assembly files Richard Kuo
2011-08-17 16:35 ` [patch 13/36] Hexagon: Support dynamic module loading Richard Kuo
2011-08-17 19:41   ` Arnd Bergmann
2011-08-17 16:35 ` [patch 14/36] Hexagon: Add signal functions Richard Kuo
2012-02-11 23:27   ` Al Viro [this message]
2012-02-15 17:45     ` hexagon: signal handling bugs Richard Kuo
2012-02-15 18:18     ` Linas Vepstas
2011-08-17 16:35 ` [patch 15/36] Hexagon: Add init_task and process functions Richard Kuo
2011-08-17 19:45   ` Arnd Bergmann
2011-08-17 16:35 ` [patch 16/36] Hexagon: Add startup code Richard Kuo
2011-08-17 16:35 ` [patch 17/36] Hexagon: Add interrupts Richard Kuo
2011-08-17 16:35 ` [patch 18/36] Hexagon: Add time and timer functions Richard Kuo
2011-08-17 16:35 ` [patch 19/36] Hexagon: Add ptrace support Richard Kuo
2011-08-17 19:47   ` Arnd Bergmann
2011-08-17 16:35 ` [patch 20/36] Hexagon: Provide basic debugging and system trap support Richard Kuo
2011-08-17 16:35 ` [patch 21/36] Hexagon: Add SMP support Richard Kuo
2011-08-17 16:35 ` [patch 22/36] Hexagon: Add locking types and functions Richard Kuo
2011-08-17 16:35 ` [patch 23/36] Hexagon: Add user access functions Richard Kuo
2011-08-17 16:35 ` [patch 24/36] Hexagon: Provide basic implementation and/or stubs for I/O routines Richard Kuo
2011-08-17 19:55   ` Arnd Bergmann
2011-08-17 16:35 ` [patch 25/36] Hexagon: Implement basic cache-flush support Richard Kuo
2011-08-17 16:35 ` [patch 26/36] Hexagon: Implement basic TLB management routines for Hexagon Richard Kuo
2011-08-17 16:35 ` [patch 27/36] Hexagon: Provide DMA implementation Richard Kuo
2011-08-17 20:01   ` Arnd Bergmann
2011-08-17 16:35 ` [patch 28/36] Hexagon: Add ioremap support Richard Kuo
2011-08-17 16:35 ` [patch 29/36] Hexagon: Add page table header files & etc Richard Kuo
2011-08-17 16:35 ` [patch 30/36] Hexagon: Add page-fault support Richard Kuo
2011-08-17 16:35 ` [patch 31/36] Hexagon: kgdb support files Richard Kuo
2011-08-17 16:35 ` [patch 32/36] Hexagon: Comet platform support Richard Kuo
2011-08-17 20:07   ` Arnd Bergmann
2011-08-17 23:45     ` Linas Vepstas
2011-08-17 20:08   ` David Brown
2011-08-17 16:35 ` [patch 33/36] Hexagon: Platform-generic support Richard Kuo
2011-08-17 20:20   ` Arnd Bergmann
2011-08-17 16:35 ` [patch 34/36] Hexagon: Add configuration and makefiles for the Hexagon architecture Richard Kuo
2011-08-17 20:27   ` Arnd Bergmann
2011-08-17 16:35 ` [patch 35/36] Hexagon: Add basic stacktrace functionality for " Richard Kuo
2011-08-17 16:35 ` [patch 36/36] Hexagon: Add self to MAINTAINERS Richard Kuo
2011-08-17 19:00 ` [patch 00/36] Hexagon: Add support for Qualcomm Hexagon architecture Zan Lynx
2011-08-17 19:42   ` Richard Kuo
2011-08-17 20:34 ` Arnd Bergmann
2011-08-18  0:31   ` Richard Kuo
2011-08-31  5:48 ` Avi Kivity

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=20120211232740.GM23916@ZenIV.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=linux-hexagon@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rkuo@codeaurora.org \
    --cc=torvalds@linux-foundation.org \
    /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.