Re: [RFC] TIF_NOTIFY_RESUME, arch/*/*/*signal*.c and all such

[Al Viro <viro@ZenIV.linux.org.uk>]

On Fri, Apr 27, 2012 at 07:24:44PM +0200, Oleg Nesterov wrote:
> > static inline sigset_t *sigmask_to_save(void)
> > {
> > 	struct sigset *res = &current->blocked;
> > 	if (unlikely(test_restore_sigmask()))
> > 		res = current->saved_sigmask;
> > 	return res;
> > }
> 
> Perhaps... but test_*_restore_sigmask() depends on TIF_ or TS_

... and is in thread_info.h; you might want to pull it again ;-)
Right now the signal.git#master is at 349b4565ad9e9b891245590319567c0a042046d9

> > Umm...  Probably, and as far as I can see all callers are only reached if
> > we have SIGPENDING, but that requires at least documenting what's going on.
> 
> WARN_ON(!test_bit(TIF_SIGPENDING)) looks like the perfect documentation ;)

OK, I'm convinced.  Done.

BTW, I've a better name than set_current_blocked_carefully(); the thing
is, only 3 callers out of 63 are _not_ immediately preceded by excluding
SIGKILL/SIGSTOP out of the set.  So I've copied the existing variant
to __set_current_blocked() and folded that sigdelsetmask(...) into the
set_current_blocked().

> The only comment I have,
> 38671a3e831ed7327affb24d715f98bb99c80e56 m68k: add TIF_NOTIFY_RESUME and handle it
> forgets to unexport do_signal().

Meh...  The thing is, there are _two_ of them.  signal_mm.c and signal_no.c
badly need merging, with common stuff moved to signal.c.  I really hate
to see static function that isn't called anywhere in file it's defined in,
leaving one to trace what happens to include that file.  Running into that
in USB code is bloody annoying and I'd rather not add to that pile.  It's
certainly a valid C, but it's hell on casual reader...

	BTW, I'm somewhat tempted to do the following: *ALL* calls of
tracehook_signal_handler() are now immediately preceded by block_signals().
Moreover, tracehook_signal_handler(...., 0) is currently a no-op, so
it could be painlessly added after the remaining block_signals() instances.
How about *folding* block_signals() (along with clear_restore_sigmask())
into tracehook_signal_handler()?  I don't know if anyone has conflicting
plans for that sucker; Roland?

	Even more ambitious variant: take the "setting sigframe up has
failed, send ourselves SIGSEGV" in there as well (adding an extra argument
to tell which one it is).  Hell knows; I know at least one place where
such failure does _not_ lead to SIGSEGV, but there we do do_exit(SIGILL)
right in setup_..._frame() and do_exit() never returns.  There might be
other such suckers...
 
> The last thing. Matt, could you please look at
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/signal.git ? It seems to me
> you already sent some of these changes (use set_current_blocked/block_sigmask).
> Perhaps there are alreay in -mm or linux-next?