linux-kernel - Re: [patch 2/5] signalfd v2

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-next>] [day] [month] [year] [list]

Message-ID: <20070308204353.GA6733@tv-sign.ru>
Date:	Thu, 8 Mar 2007 23:43:53 +0300
From:	Oleg Nesterov <oleg@...sign.ru>
To:	Davide Libenzi <davidel@...ilserver.org>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Avi Kivity <avi@...o.co.il>, linux-kernel@...r.kernel.org
Subject: Re: [patch 2/5] signalfd v2 - signalfd core ...

Davide Libenzi wrote:
>
> +int signalfd_deliver(struct sighand_struct *sighand, int sig, struct siginfo *info)
> +{
> +	int nsig = 0;
> +	struct list_head *pos;
> +	struct signalfd_ctx *ctx;
> +	struct signalfd_sq *sq;
> +
> +	list_for_each(pos, &sighand->sfdlist) {
> +		ctx = list_entry(pos, struct signalfd_ctx, lnk);
> +		/*
> +		 * We use a negative signal value as a way to broadcast that the
> +		 * sighand has been orphaned, so that we can notify all the
> +		 * listeners about this.
> +		 */
> +		if (sig < 0)
> +			__wake_up_locked(&ctx->wqh, TASK_UNINTERRUPTIBLE | TASK_INTERRUPTIBLE);
> +		else if (sigismember(&ctx->sigmask, sig) &&
> +			 (sig >= SIGRTMIN || !sigismember(&ctx->pending, sig))) {
> +			sigaddset(&ctx->pending, sig);

I don't understand the "(sig >= SIGRTMIN || !sigismember(&ctx->pending, sig))"
check. This mimics the LEGACY_QUEUE() check, but seems strange. The signal may
be pending in ctx->pending just because it was not signalfd_fetchsig()ed, yes?

Please also see below.

> +asmlinkage long sys_signalfd(int ufd, sigset_t __user *user_mask, size_t sizemask)
> +{
>
> [...snip...]
>
> +	} else {
> +		error = -EBADF;
> +		file = fget(ufd);
> +		if (!file)
> +			goto err_exit;
> +		ctx = file->private_data;
> +		error = -EINVAL;
> +		if (file->f_op != &signalfd_fops) {
> +			fput(file);
> +			goto err_exit;
> +		}
> +		spin_lock_irq(&ctx->sighand->siglock);
> +		ctx->sigmask = sigmask;
> +		spin_unlock_irq(&ctx->sighand->siglock);
> +		wake_up(&ctx->wqh);

Can't this race with sys_signalfd_dequeue() which use lockless __add_wait_queue()?
Looks like we should do __wake_up_locked() under ctx->sighand->siglock.

> --- linux-2.6.20.ep2.orig/kernel/signal.c	2007-03-07 15:55:43.000000000 -0800
> +++ linux-2.6.20.ep2/kernel/signal.c	2007-03-07 15:59:01.000000000 -0800
>
> [...snip...]
>
> @@ -780,6 +785,11 @@
>  	BUG_ON(!irqs_disabled());
>  	assert_spin_locked(&t->sighand->siglock);
>  
> +	/*
> +	 * Deliver the signal to listening signalfds ...
> +	 */
> +	signalfd_notify(t->sighand, sig, info);
> +
>  	/* Short-circuit ignored signals.  */
>  	if (sig_ignored(t, sig))
>  		goto out;
> @@ -968,6 +978,11 @@
>  	assert_spin_locked(&p->sighand->siglock);
>  	handle_stop_signal(sig, p);
>  
> +	/*
> +	 * Deliver the signal to listening signalfds ...
> +	 */
> +	signalfd_notify(p->sighand, sig, info);
> +
>  	/* Short-circuit ignored signals.  */
>  	if (sig_ignored(p, sig))
>  		return ret;

It is strange that we are doing signalfd_notify() even if the signal is ignored.
Isn't it better to shift signalfd_notify() into send_signal() ? This way we do
not need the special check in signalfd_deliver() above.

Also, this patch doesn't take send_sigqueue/send_group_sigqueue into account.

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/