signalfd simplification

This simplifies signalfd code, by avoiding it to remain attached to the
sighand during its lifetime.

In this way, the signalfd remain attached to the sighand only during
poll(2) (and select and epoll) and read(2).  This also allows to remove
all the custom "tsk == current" checks in kernel/signal.c, since
dequeue_signal() will only be called by "current".

I think this is also what Ben was suggesting time ago.

The external effect of this, is that a thread can extract only its own
private signals and the group ones.  I think this is an acceptable
behaviour, in that those are the signals the thread would be able to
fetch w/out signalfd.

Signed-off-by: Davide Libenzi <davidel@xmailserver.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/fs/exec.c b/fs/exec.c
index c21a8cc0627709db512597ff2af82ab7c094e67d..073b0b8c6d055a3fd947f16fbdbe5552a6bc34c8 100644 (file)
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -50,7 +50,6 @@
 #include <linux/tsacct_kern.h>
 #include <linux/cn_proc.h>
 #include <linux/audit.h>
 #include <linux/tsacct_kern.h>
 #include <linux/cn_proc.h>
 #include <linux/audit.h>

-#include <linux/signalfd.h>

 
 #include <asm/uaccess.h>
 #include <asm/mmu_context.h>
 
 #include <asm/uaccess.h>
 #include <asm/mmu_context.h>


@@ -784,7 +783,6 @@ static int de_thread(struct task_struct *tsk)
         * and we can just re-use it all.
         */
        if (atomic_read(&oldsighand->count) <= 1) {
         * and we can just re-use it all.
         */
        if (atomic_read(&oldsighand->count) <= 1) {

-               signalfd_detach(tsk);

                exit_itimers(sig);
                return 0;
        }
                exit_itimers(sig);
                return 0;
        }


@@ -923,7 +921,6 @@ static int de_thread(struct task_struct *tsk)
        sig->flags = 0;
 
 no_thread_group:
        sig->flags = 0;
 
 no_thread_group:

-       signalfd_detach(tsk);

        exit_itimers(sig);
        if (leader)
                release_task(leader);
        exit_itimers(sig);
        if (leader)
                release_task(leader);

diff --git a/fs/signalfd.c b/fs/signalfd.c
index a8e293d3003432ff493eff7069f32305fa35006a..aefb0be07942d735de747ad8697834368f8a907a 100644 (file)
--- a/fs/signalfd.c
+++ b/fs/signalfd.c
@@ -11,8 +11,10 @@
  *      Now using anonymous inode source.
  *      Thanks to Oleg Nesterov for useful code review and suggestions.
  *      More comments and suggestions from Arnd Bergmann.
  *      Now using anonymous inode source.
  *      Thanks to Oleg Nesterov for useful code review and suggestions.
  *      More comments and suggestions from Arnd Bergmann.

- * Sat May 19, 2007: Davi E. M. Arnaut <davi@haxent.com.br>
+ *  Sat May 19, 2007: Davi E. M. Arnaut <davi@haxent.com.br>

  *      Retrieve multiple signals with one read() call
  *      Retrieve multiple signals with one read() call

+ *  Sun Jul 15, 2007: Davide Libenzi <davidel@xmailserver.org>
+ *      Attach to the sighand only during read() and poll().

  */
 
 #include <linux/file.h>
  */
 
 #include <linux/file.h>


@@ -27,102 +29,12 @@
 #include <linux/signalfd.h>
 
 struct signalfd_ctx {
 #include <linux/signalfd.h>
 
 struct signalfd_ctx {

-       struct list_head lnk;
-       wait_queue_head_t wqh;

        sigset_t sigmask;
        sigset_t sigmask;

-       struct task_struct *tsk;

 };
 
 };
 

-struct signalfd_lockctx {
-       struct task_struct *tsk;
-       unsigned long flags;
-};
-
-/*
- * Tries to acquire the sighand lock. We do not increment the sighand
- * use count, and we do not even pin the task struct, so we need to
- * do it inside an RCU read lock, and we must be prepared for the
- * ctx->tsk going to NULL (in signalfd_deliver()), and for the sighand
- * being detached. We return 0 if the sighand has been detached, or
- * 1 if we were able to pin the sighand lock.
- */
-static int signalfd_lock(struct signalfd_ctx *ctx, struct signalfd_lockctx *lk)
-{
-       struct sighand_struct *sighand = NULL;
-
-       rcu_read_lock();
-       lk->tsk = rcu_dereference(ctx->tsk);
-       if (likely(lk->tsk != NULL))
-               sighand = lock_task_sighand(lk->tsk, &lk->flags);
-       rcu_read_unlock();
-
-       if (!sighand)
-               return 0;
-
-       if (!ctx->tsk) {
-               unlock_task_sighand(lk->tsk, &lk->flags);
-               return 0;
-       }
-
-       if (lk->tsk->tgid == current->tgid)
-               lk->tsk = current;
-
-       return 1;
-}
-
-static void signalfd_unlock(struct signalfd_lockctx *lk)
-{
-       unlock_task_sighand(lk->tsk, &lk->flags);
-}
-
-/*
- * This must be called with the sighand lock held.
- */
-void signalfd_deliver(struct task_struct *tsk, int sig)
-{
-       struct sighand_struct *sighand = tsk->sighand;
-       struct signalfd_ctx *ctx, *tmp;
-
-       BUG_ON(!sig);
-       list_for_each_entry_safe(ctx, tmp, &sighand->signalfd_list, 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. Remember the ctx->sigmask is inverted,
-                * so if the user is interested in a signal, that corresponding
-                * bit will be zero.
-                */
-               if (sig < 0) {
-                       if (ctx->tsk == tsk) {
-                               ctx->tsk = NULL;
-                               list_del_init(&ctx->lnk);
-                               wake_up(&ctx->wqh);
-                       }
-               } else {
-                       if (!sigismember(&ctx->sigmask, sig))
-                               wake_up(&ctx->wqh);
-               }
-       }
-}
-
-static void signalfd_cleanup(struct signalfd_ctx *ctx)
-{
-       struct signalfd_lockctx lk;
-
-       /*
-        * This is tricky. If the sighand is gone, we do not need to remove
-        * context from the list, the list itself won't be there anymore.
-        */
-       if (signalfd_lock(ctx, &lk)) {
-               list_del(&ctx->lnk);
-               signalfd_unlock(&lk);
-       }
-       kfree(ctx);
-}
-

 static int signalfd_release(struct inode *inode, struct file *file)
 {
 static int signalfd_release(struct inode *inode, struct file *file)
 {

-       signalfd_cleanup(file->private_data);
+       kfree(file->private_data);

        return 0;
 }
 
        return 0;
 }
 


@@ -130,23 +42,15 @@ static unsigned int signalfd_poll(struct file *file, poll_table *wait)
 {
        struct signalfd_ctx *ctx = file->private_data;
        unsigned int events = 0;
 {
        struct signalfd_ctx *ctx = file->private_data;
        unsigned int events = 0;

-       struct signalfd_lockctx lk;

 
 

-       poll_wait(file, &ctx->wqh, wait);
+       poll_wait(file, &current->sighand->signalfd_wqh, wait);

 
 

-       /*
-        * Let the caller get a POLLIN in this case, ala socket recv() when
-        * the peer disconnects.
-        */
-       if (signalfd_lock(ctx, &lk)) {
-               if ((lk.tsk == current &&
-                    next_signal(&lk.tsk->pending, &ctx->sigmask) > 0) ||
-                   next_signal(&lk.tsk->signal->shared_pending,
-                               &ctx->sigmask) > 0)
-                       events |= POLLIN;
-               signalfd_unlock(&lk);
-       } else
+       spin_lock_irq(&current->sighand->siglock);
+       if (next_signal(&current->pending, &ctx->sigmask) ||
+           next_signal(&current->signal->shared_pending,
+                       &ctx->sigmask))

                events |= POLLIN;
                events |= POLLIN;

+       spin_unlock_irq(&current->sighand->siglock);

 
        return events;
 }
 
        return events;
 }


@@ -219,59 +123,46 @@ static ssize_t signalfd_dequeue(struct signalfd_ctx *ctx, siginfo_t *info,
                                int nonblock)
 {
        ssize_t ret;
                                int nonblock)
 {
        ssize_t ret;

-       struct signalfd_lockctx lk;

        DECLARE_WAITQUEUE(wait, current);
 
        DECLARE_WAITQUEUE(wait, current);
 

-       if (!signalfd_lock(ctx, &lk))
-               return 0;
-
-       ret = dequeue_signal(lk.tsk, &ctx->sigmask, info);
+       spin_lock_irq(&current->sighand->siglock);
+       ret = dequeue_signal(current, &ctx->sigmask, info);

        switch (ret) {
        case 0:
                if (!nonblock)
                        break;
                ret = -EAGAIN;
        default:
        switch (ret) {
        case 0:
                if (!nonblock)
                        break;
                ret = -EAGAIN;
        default:

-               signalfd_unlock(&lk);
+               spin_unlock_irq(&current->sighand->siglock);

                return ret;
        }
 
                return ret;
        }
 

-       add_wait_queue(&ctx->wqh, &wait);
+       add_wait_queue(&current->sighand->signalfd_wqh, &wait);

        for (;;) {
                set_current_state(TASK_INTERRUPTIBLE);
        for (;;) {
                set_current_state(TASK_INTERRUPTIBLE);

-               ret = dequeue_signal(lk.tsk, &ctx->sigmask, info);
-               signalfd_unlock(&lk);
+               ret = dequeue_signal(current, &ctx->sigmask, info);

                if (ret != 0)
                        break;
                if (signal_pending(current)) {
                        ret = -ERESTARTSYS;
                        break;
                }
                if (ret != 0)
                        break;
                if (signal_pending(current)) {
                        ret = -ERESTARTSYS;
                        break;
                }

+               spin_unlock_irq(&current->sighand->siglock);

                schedule();
                schedule();

-               ret = signalfd_lock(ctx, &lk);
-               if (unlikely(!ret)) {
-                       /*
-                        * Let the caller read zero byte, ala socket
-                        * recv() when the peer disconnect. This test
-                        * must be done before doing a dequeue_signal(),
-                        * because if the sighand has been orphaned,
-                        * the dequeue_signal() call is going to crash
-                        * because ->sighand will be long gone.
-                        */
-                        break;
-               }
+               spin_lock_irq(&current->sighand->siglock);

        }
        }

+       spin_unlock_irq(&current->sighand->siglock);

 
 

-       remove_wait_queue(&ctx->wqh, &wait);
+       remove_wait_queue(&current->sighand->signalfd_wqh, &wait);

        __set_current_state(TASK_RUNNING);
 
        return ret;
 }
 
 /*
        __set_current_state(TASK_RUNNING);
 
        return ret;
 }
 
 /*

- * Returns either the size of a "struct signalfd_siginfo", or zero if the
- * sighand we are attached to, has been orphaned. The "count" parameter
- * must be at least the size of a "struct signalfd_siginfo".
+ * Returns a multiple of the size of a "struct signalfd_siginfo", or a negative
+ * error code. The "count" parameter must be at least the size of a
+ * "struct signalfd_siginfo".

  */
 static ssize_t signalfd_read(struct file *file, char __user *buf, size_t count,
                             loff_t *ppos)
  */
 static ssize_t signalfd_read(struct file *file, char __user *buf, size_t count,
                             loff_t *ppos)


@@ -287,7 +178,6 @@ static ssize_t signalfd_read(struct file *file, char __user *buf, size_t count,
                return -EINVAL;
 
        siginfo = (struct signalfd_siginfo __user *) buf;
                return -EINVAL;
 
        siginfo = (struct signalfd_siginfo __user *) buf;

-

        do {
                ret = signalfd_dequeue(ctx, &info, nonblock);
                if (unlikely(ret <= 0))
        do {
                ret = signalfd_dequeue(ctx, &info, nonblock);
                if (unlikely(ret <= 0))


@@ -300,7 +190,7 @@ static ssize_t signalfd_read(struct file *file, char __user *buf, size_t count,
                nonblock = 1;
        } while (--count);
 
                nonblock = 1;
        } while (--count);
 

-       return total ? total : ret;
+       return total ? total: ret;

 }
 
 static const struct file_operations signalfd_fops = {
 }
 
 static const struct file_operations signalfd_fops = {


@@ -309,20 +199,13 @@ static const struct file_operations signalfd_fops = {
        .read           = signalfd_read,
 };
 
        .read           = signalfd_read,
 };
 

-/*
- * Create a file descriptor that is associated with our signal
- * state. We can pass it around to others if we want to, but
- * it will always be _our_ signal state.
- */

 asmlinkage long sys_signalfd(int ufd, sigset_t __user *user_mask, size_t sizemask)
 {
        int error;
        sigset_t sigmask;
        struct signalfd_ctx *ctx;
 asmlinkage long sys_signalfd(int ufd, sigset_t __user *user_mask, size_t sizemask)
 {
        int error;
        sigset_t sigmask;
        struct signalfd_ctx *ctx;

-       struct sighand_struct *sighand;

        struct file *file;
        struct inode *inode;
        struct file *file;
        struct inode *inode;

-       struct signalfd_lockctx lk;

 
        if (sizemask != sizeof(sigset_t) ||
            copy_from_user(&sigmask, user_mask, sizeof(sigmask)))
 
        if (sizemask != sizeof(sigset_t) ||
            copy_from_user(&sigmask, user_mask, sizeof(sigmask)))


@@ -335,17 +218,7 @@ asmlinkage long sys_signalfd(int ufd, sigset_t __user *user_mask, size_t sizemas
                if (!ctx)
                        return -ENOMEM;
 
                if (!ctx)
                        return -ENOMEM;
 

-               init_waitqueue_head(&ctx->wqh);

                ctx->sigmask = sigmask;
                ctx->sigmask = sigmask;

-               ctx->tsk = current->group_leader;
-
-               sighand = current->sighand;
-               /*
-                * Add this fd to the list of signal listeners.
-                */
-               spin_lock_irq(&sighand->siglock);
-               list_add_tail(&ctx->lnk, &sighand->signalfd_list);
-               spin_unlock_irq(&sighand->siglock);

 
                /*
                 * When we call this, the initialization must be complete, since
 
                /*
                 * When we call this, the initialization must be complete, since


@@ -364,23 +237,18 @@ asmlinkage long sys_signalfd(int ufd, sigset_t __user *user_mask, size_t sizemas
                        fput(file);
                        return -EINVAL;
                }
                        fput(file);
                        return -EINVAL;
                }

-               /*
-                * We need to be prepared of the fact that the sighand this fd
-                * is attached to, has been detched. In that case signalfd_lock()
-                * will return 0, and we'll just skip setting the new mask.
-                */
-               if (signalfd_lock(ctx, &lk)) {
-                       ctx->sigmask = sigmask;
-                       signalfd_unlock(&lk);
-               }
-               wake_up(&ctx->wqh);
+               spin_lock_irq(&current->sighand->siglock);
+               ctx->sigmask = sigmask;
+               spin_unlock_irq(&current->sighand->siglock);
+
+               wake_up(&current->sighand->signalfd_wqh);

                fput(file);
        }
 
        return ufd;
 
 err_fdalloc:
                fput(file);
        }
 
        return ufd;
 
 err_fdalloc:

-       signalfd_cleanup(ctx);
+       kfree(ctx);

        return error;
 }
 
        return error;
 }
 

diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index cab741c2d6033ad937b761852def3fbbf60b763b..f8abfa349ef924344a11b1fd9739e68bd4d72b24 100644 (file)
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -86,7 +86,7 @@ extern struct nsproxy init_nsproxy;
        .count          = ATOMIC_INIT(1),                               \
        .action         = { { { .sa_handler = NULL, } }, },             \
        .siglock        = __SPIN_LOCK_UNLOCKED(sighand.siglock),        \
        .count          = ATOMIC_INIT(1),                               \
        .action         = { { { .sa_handler = NULL, } }, },             \
        .siglock        = __SPIN_LOCK_UNLOCKED(sighand.siglock),        \

-       .signalfd_list  = LIST_HEAD_INIT(sighand.signalfd_list),        \
+       .signalfd_wqh   = __WAIT_QUEUE_HEAD_INITIALIZER(sighand.signalfd_wqh),  \

 }
 
 extern struct group_info init_groups;
 }
 
 extern struct group_info init_groups;

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 3de79016f2a669dbbb5bd16395d3458032be2b86..a01ac6dd5f5e1c07d9a54a8f879512db2eac3905 100644 (file)
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -438,7 +438,7 @@ struct sighand_struct {
        atomic_t                count;
        struct k_sigaction      action[_NSIG];
        spinlock_t              siglock;
        atomic_t                count;
        struct k_sigaction      action[_NSIG];
        spinlock_t              siglock;

-       struct list_head        signalfd_list;
+       wait_queue_head_t       signalfd_wqh;

 };
 
 struct pacct_struct {
 };
 
 struct pacct_struct {

diff --git a/include/linux/signalfd.h b/include/linux/signalfd.h
index 5104294956906c038e4359ab4e37c35632e2e478..4c9ff0910ae01d39f691a23c9acf5d1db48b06d2 100644 (file)
--- a/include/linux/signalfd.h
+++ b/include/linux/signalfd.h
@@ -45,49 +45,17 @@ struct signalfd_siginfo {
 #ifdef CONFIG_SIGNALFD
 
 /*
 #ifdef CONFIG_SIGNALFD
 
 /*

- * Deliver the signal to listening signalfd. This must be called
- * with the sighand lock held. Same are the following that end up
- * calling signalfd_deliver().
- */
-void signalfd_deliver(struct task_struct *tsk, int sig);
-
-/*
- * No need to fall inside signalfd_deliver() if no signal listeners
- * are available.
+ * Deliver the signal to listening signalfd.

  */
 static inline void signalfd_notify(struct task_struct *tsk, int sig)
 {
  */
 static inline void signalfd_notify(struct task_struct *tsk, int sig)
 {

-       if (unlikely(!list_empty(&tsk->sighand->signalfd_list)))
-               signalfd_deliver(tsk, sig);
-}
-
-/*
- * The signal -1 is used to notify the signalfd that the sighand
- * is on its way to be detached.
- */
-static inline void signalfd_detach_locked(struct task_struct *tsk)
-{
-       if (unlikely(!list_empty(&tsk->sighand->signalfd_list)))
-               signalfd_deliver(tsk, -1);
-}
-
-static inline void signalfd_detach(struct task_struct *tsk)
-{
-       struct sighand_struct *sighand = tsk->sighand;
-
-       if (unlikely(!list_empty(&sighand->signalfd_list))) {
-               spin_lock_irq(&sighand->siglock);
-               signalfd_deliver(tsk, -1);
-               spin_unlock_irq(&sighand->siglock);
-       }
+       if (unlikely(waitqueue_active(&tsk->sighand->signalfd_wqh)))
+               wake_up(&tsk->sighand->signalfd_wqh);

 }
 
 #else /* CONFIG_SIGNALFD */
 
 }
 
 #else /* CONFIG_SIGNALFD */
 

-#define signalfd_deliver(t, s) do { } while (0)
-#define signalfd_notify(t, s) do { } while (0)
-#define signalfd_detach_locked(t) do { } while (0)
-#define signalfd_detach(t) do { } while (0)
+static inline void signalfd_notify(struct task_struct *tsk, int sig) { }

 
 #endif /* CONFIG_SIGNALFD */
 
 
 #endif /* CONFIG_SIGNALFD */
 

diff --git a/kernel/exit.c b/kernel/exit.c
index 06b24b3aa370c69577012023b12573d4c7624d19..993369ee94d1ebb8d4c7675a821e90abf8a1737e 100644 (file)
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -24,7 +24,6 @@
 #include <linux/pid_namespace.h>
 #include <linux/ptrace.h>
 #include <linux/profile.h>
 #include <linux/pid_namespace.h>
 #include <linux/ptrace.h>
 #include <linux/profile.h>

-#include <linux/signalfd.h>

 #include <linux/mount.h>
 #include <linux/proc_fs.h>
 #include <linux/kthread.h>
 #include <linux/mount.h>
 #include <linux/proc_fs.h>
 #include <linux/kthread.h>


@@ -86,14 +85,6 @@ static void __exit_signal(struct task_struct *tsk)
        sighand = rcu_dereference(tsk->sighand);
        spin_lock(&sighand->siglock);
 
        sighand = rcu_dereference(tsk->sighand);
        spin_lock(&sighand->siglock);
 

-       /*
-        * Notify that this sighand has been detached. This must
-        * be called with the tsk->sighand lock held. Also, this
-        * access tsk->sighand internally, so it must be called
-        * before tsk->sighand is reset.
-        */
-       signalfd_detach_locked(tsk);
-

        posix_cpu_timers_exit(tsk);
        if (atomic_dec_and_test(&sig->count))
                posix_cpu_timers_exit_group(tsk);
        posix_cpu_timers_exit(tsk);
        if (atomic_dec_and_test(&sig->count))
                posix_cpu_timers_exit_group(tsk);

diff --git a/kernel/fork.c b/kernel/fork.c
index 7332e236d3676153dc9f40c1a78848e6e2810632..33f12f48684a0053a82d72506c1d7ec1e7ca6a15 100644 (file)
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1438,7 +1438,7 @@ static void sighand_ctor(void *data, struct kmem_cache *cachep,
        struct sighand_struct *sighand = data;
 
        spin_lock_init(&sighand->siglock);
        struct sighand_struct *sighand = data;
 
        spin_lock_init(&sighand->siglock);

-       INIT_LIST_HEAD(&sighand->signalfd_list);
+       init_waitqueue_head(&sighand->signalfd_wqh);

 }
 
 void __init proc_caches_init(void)
 }
 
 void __init proc_caches_init(void)

diff --git a/kernel/signal.c b/kernel/signal.c
index 3169bed0b4d0e44166b37c48ed663d516a9ccba5..9fb91a32eddaddfd47d09cb0aa66586a5635a39e 100644 (file)
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -378,8 +378,7 @@ int dequeue_signal(struct task_struct *tsk, sigset_t *mask, siginfo_t *info)
        /* We only dequeue private signals from ourselves, we don't let
         * signalfd steal them
         */
        /* We only dequeue private signals from ourselves, we don't let
         * signalfd steal them
         */

-       if (likely(tsk == current))
-               signr = __dequeue_signal(&tsk->pending, mask, info);
+       signr = __dequeue_signal(&tsk->pending, mask, info);

        if (!signr) {
                signr = __dequeue_signal(&tsk->signal->shared_pending,
                                         mask, info);
        if (!signr) {
                signr = __dequeue_signal(&tsk->signal->shared_pending,
                                         mask, info);


@@ -407,8 +406,7 @@ int dequeue_signal(struct task_struct *tsk, sigset_t *mask, siginfo_t *info)
                        }
                }
        }
                        }
                }
        }

-       if (likely(tsk == current))
-               recalc_sigpending();
+       recalc_sigpending();

        if (signr && unlikely(sig_kernel_stop(signr))) {
                /*
                 * Set a marker that we have dequeued a stop signal.  Our
        if (signr && unlikely(sig_kernel_stop(signr))) {
                /*
                 * Set a marker that we have dequeued a stop signal.  Our


@@ -425,7 +423,7 @@ int dequeue_signal(struct task_struct *tsk, sigset_t *mask, siginfo_t *info)
                if (!(tsk->signal->flags & SIGNAL_GROUP_EXIT))
                        tsk->signal->flags |= SIGNAL_STOP_DEQUEUED;
        }
                if (!(tsk->signal->flags & SIGNAL_GROUP_EXIT))
                        tsk->signal->flags |= SIGNAL_STOP_DEQUEUED;
        }

-       if (signr && likely(tsk == current) &&
+       if (signr &&

             ((info->si_code & __SI_MASK) == __SI_TIMER) &&
             info->si_sys_private){
                /*
             ((info->si_code & __SI_MASK) == __SI_TIMER) &&
             info->si_sys_private){
                /*