Re: [patch 2/5] signalfd v2 - signalfd core ...

From: Oleg Nesterov
Date: Thu Mar 08 2007 - 15:44:07 EST


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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/