linux-kernel - Re: [RFC + PATCH] signalfd simplification

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: <20070902121943.GA141@tv-sign.ru>
Date:	Sun, 2 Sep 2007 16:19:43 +0400
From:	Oleg Nesterov <oleg@...sign.ru>
To:	Davide Libenzi <davidel@...ilserver.org>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Michael Kerrisk <mtk-manpages@....net>
Subject: Re: [RFC + PATCH] signalfd simplification

On 09/01, Davide Libenzi wrote:
>
> I'm playing at the moment with this patch, that recall Ben's idea  of 
> attaching to the sighand only during read/poll, and calling dequeue_signal()
> only with "current". This simplifies the signalfd logic quite a bit.
> If this patch is applied, a task calling signalfd can read its own private 
> signals, and its own group signals.
>
>  fs/exec.c                 |    3 
>  fs/signalfd.c             |  186 +++++++---------------------------------------
>  include/linux/init_task.h |    2 
>  include/linux/sched.h     |    2 
>  include/linux/signalfd.h  |   29 -------
>  kernel/exit.c             |    9 --
>  kernel/fork.c             |    2 
>  kernel/signal.c           |    8 -
>  8 files changed, 40 insertions(+), 201 deletions(-)

Imho, very very nice. We lose the ability to read the cross-process signals,
but I doubt very much we should regret about that.

I cc'ed Michael, because it makes sense to document a user-visible change.
With this patch, the forked child reads its own signals (not parent's) via
the inherited signalfd (or if it was passed with unix socket).

Small problem: unless I missed something, signalfd_deliver() and sys_signalfd()
should use wake_up_all(), not wake_up() which implies nr_exclusive == 1.

It is possible that we have multiple threads waiting on ->signalfd_wqh with
the the different ->sigmask. In this case, the first woken thread can ignore
the signal, we should wake up all of them.

We can optimize this later, using a "clever" wait_queue_func_t if needed.

> +	spin_lock_irq(&current->sighand->siglock);
> +	if (next_signal(&current->pending, &ctx->sigmask) > 0 ||
> +	    next_signal(&current->signal->shared_pending,
> +			&ctx->sigmask) > 0)

Very minor nit: next_signal() always returns the value >= 0, imho the "> 0"
check looks a bit confusing.

Oleg.

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