linux-kernel - Re: [PATCH 4/6] signal: sigprocmask() should do retarget_shared

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BANLkTin6t7fSuyxw4Nipj1DGy8w9XE3+ug@mail.gmail.com>
Date:	Tue, 12 Apr 2011 07:32:20 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Tejun Heo <tj@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Nikita V. Youshchenko" <nyoushchenko@...sta.com>,
	Matt Fleming <matt@...sole-pimps.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/6] signal: sigprocmask() should do retarget_shared_pending()

On Mon, Apr 11, 2011 at 10:21 AM, Oleg Nesterov <oleg@...hat.com> wrote:
>
> I am not sure this is bug, but at least this looks strange imho. T1 should
> not sleep forever, there is a signal which should wake it up.

Hmm. I worry about the overhead of this, and I'm not 100% convinced we need it.

But my _biggest_ objection to the series is a purely technical one:

> --- sigprocmask/include/linux/signal.h~4_sigprocmask_retarget   2011-04-06 21:33:50.000000000 +0200
> +++ sigprocmask/include/linux/signal.h  2011-04-11 18:16:51.000000000 +0200
> @@ -2131,6 +2131,11 @@ int sigprocmask(int how, sigset_t *set,
>        }
>
>        spin_lock_irq(&tsk->sighand->siglock);
> +       if (signal_pending(tsk) && !thread_group_empty(tsk)) {
> +               sigset_t not_newblocked;
> +               signorsets(&not_newblocked, &current->blocked, &newset);
> +               retarget_shared_pending(tsk, &not_newblocked);
> +       }
>        tsk->blocked = newset;
>        recalc_sigpending();
>        spin_unlock_irq(&tsk->sighand->siglock);

I absolutely detest how you made "sigprocmask()" the main interface to
do this all, and then add new callers.

It's a horrid interface with that crazy "how" argument, and comes out
of the user-space system call interface. If we make kernel users do
this, especially critical ones like the signal handling code, please
just extract out just the actual "set new signal mask" part.

So please just introduce a "sig_set_blocked()" or something, without
the crazy "switch (how)" crud, and make sigprocmask() and everybody
else use _that_ instead.

That would make me much happier about the patch series, I suspect.

                     Linus
--
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/