Message-Id: <200903170923.56430.mega@retes.hu>
Date:	Tue, 17 Mar 2009 09:23:56 +0100
From:	Gábor Melis <mega@...es.hu>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Davide Libenzi <davidel@...ilserver.org>,
	Ingo Molnar <mingo@...e.hu>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Roland McGrath <roland@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Chris Friesen <cfriesen@...tel.com>,
	linux-kernel@...r.kernel.org
Subject: Re: Q: SEGSEGV && uc_mcontext->ip (Was: Signal delivery order)

On Martes 17 Marzo 2009, Oleg Nesterov wrote:
> (see http://marc.info/?t=123704955800002)
>
> First of all, perhaps I missed somethings and this is solvable
> without kernel changes, but I can't see how.
>
> To simplify, suppose that the application wants to log the faulting
> instruction before exit, so it installs the handler for SIGSEGV
>
> 	void sigsegv_handler(int sig, siginfo_t *info, struct ucontext
> *context) {
> 		fprintf(stderr, "bug at %p\n", context->uc_mcontext->ip);
> 		exit(1);
> 	}
>
> But this doesn't work. It is possible that, before the task dequeues
> SIGSEGV, another private signal, say, SIGHUP (note that SIGHUP <
> SIGSEGV) is sent to this app.
>
> In this case the task dequeues both signals, SIGHUP and then SIGSEGV
> before return to user-space. This is correct, but now uc_mcontext->ip
> points to sighup_handler, not to the faulted instruction.
>
>
> Can/should we change force_sig_info_fault() to help?
>
> We can add siginfo_t->_sigfault.ip to solve this problem. This
> shouldn't enlarge the size of siginfo_t. With this change
> sigsegv_handler() always knows the address of the instruction which
> caused the fault.
>
>
> But this doesn't allow to change uc_mcontext->ip to, say, skip the
> offending instruction or call another function which does a fixup.
> Actually, ->si_ip helps, we can do
>
> 	void sigsegv_handler(int sig, siginfo_t *info, struct ucontext
> *context) {
> 		if (info->si_ip != context->uc_mcontext->ip)
> 			/*
> 			 * The offending instruction will be repeated, and
> 			 * we will have the "same" SIGSEGV again.
> 			 */
> 			return;
>
> 		context->uc_mcontext->ip = fixup;
> 		...
> 	}
>
> But this doesn't look very nice. So, perhaps we can do another
> change?
>
> 	--- arch/x86/mm/fault.c
> 	+++ arch/x86/mm/fault.c
> 	@@ -177,6 +177,13 @@ static void force_sig_info_fault(int si_
> 	 {
> 		siginfo_t info;
>
> 	+	current->saved_sigmask = current->blocked;
> 	+	spin_lock_irq(&current->sighand->siglock);
> 	+	siginitsetinv(&current->blocked, sigmask(si_signo) |
> 	+			sigmask(SIGKILL) | sigmask(SIGSTOP));
> 	+	spin_unlock_irq(&current->sighand->siglock);
> 	+	set_restore_sigmask();
> 	+
> 		info.si_signo = si_signo;
> 		info.si_errno = 0;
> 		info.si_code = si_code;
>
> But this is a user-visible change, all signals will be blocked until
> sigsegv_handler() returns. But with this change sigsegv_handler()
> always has the "correct" rt_sigframe.
>
>
> Comments?
>
> And once again, I have a nasty feeling I missed something and we
> don't need to change the kernel.
>
> Oleg.

As an application developer what I'd like to have is this: synchronously 
generated signals are delivered before asynchronously generated ones. 
That is, if a number of signals are generated but not yet delivered 
then the synchronously generated ones are delivered first. I guess, in 
the kernel this would mean that the private/non-private distinction is 
not enough.

I think Chris is right in that standard allows the current behaviour. I 
would have quoted it already if I had thought otherwise ... He's also 
right about quality of implementation. The standard doesn't say much 
about synchronously and asynchronously generated signals and it even 
goes further:

"The order in which multiple, simultaneously pending signals outside the 
range SIGRTMIN to SIGRTMAX are delivered to or accepted by a process is 
unspecified."

Bleh. It also says that the context argument represents the context at 
the time of delivery. For a handler for sigsegv, sigtrap (to name just 
the most likely suspects) the context -  in order for it to be useful 
at all - must be from the time of generation. There is an obvious 
contradiction here and the only way I see to resolve this is to deliver 
syncrhronously generated signals while the two contexts are the same 
which is exactly what allowing other signals to 'overtake' violates. Of 
course, if sigsegv is blocked then this is impossible to do, but that's 
fine.

For the application I'm working on this whole issue is kind of moot 
since kernels with the current behaviour will be around for ages to 
come and I have the workaround of not pthread_kill()ing with a signal 
whose signum is lower than the signum of any of the syncrhonously 
generated signals. In practice, it only means that I'll use SIGUSR2 
instead of SIGUSR1 because that's greater than SIGSEGV and pay 
attention not to pthread_kill() with SIGINT. The only thing that 
worries me is this remark from Oleg 
(http://marc.info/?l=linux-kernel&m=123711058421913&w=2):

"But please note that it is still possible to hit is_signal_blocked()
even with test_with_kill(), but the probability is very low."

I still can't see how that's possible, but if it is, then it defeats the 
workaround and I have even bigger problems than I thought. Not only me, 
I guess, most applications with a sigtrap, sigsegv handler that use the 
context will be broken by a C-c.

Gabor
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists