signalfd: introduce signalfd_cleanup()

PATCH In review Oleg Nesterov 3 participants 17 messages Feb 22, 2012 – Mar 1, 2012 1 message not in the archive

Subsystems (8)

VFS KVM MM core Scheduler +4 more subsystems

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Oleg Nesterov

author

2012-02-22 17:40 UTC #1 lore raw

PATCH 1/4 signalfd: introduce signalfd_cleanup()

Preparation. __cleanup_sighand() obviously must not free sighand if ->signalfd_wqh is in use. And the new helper which checks this wait_queue_head_t before kmem_cache_free(). Currently it does nothing except it helps to catch the bug early.

Reported-by Maxime Bizon <[email protected]>

Cc <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/signalfd.c            |    7 +++++++
 include/linux/signalfd.h |    5 ++++-
 kernel/fork.c            |    5 ++++-
 3 files changed, 15 insertions(+), 2 deletions(-)

fs/signalfd.c +7 -0

index 492465b..35d19ae 100644
@@ -30,6 +30,13 @@
 #include <linux/signalfd.h>
 #include <linux/syscalls.h>
 
+void signalfd_cleanup(struct sighand_struct *sighand)
+{
+	wait_queue_head_t *wqh = &sighand->signalfd_wqh;
+
+	BUG_ON(waitqueue_active(wqh));
+}
+
 struct signalfd_ctx {
 	sigset_t sigmask;
 };

include/linux/signalfd.h +4 -1

index 3ff4961..247399b 100644
@@ -61,13 +61,16 @@ static inline void signalfd_notify(struct task_struct *tsk, int sig)
 		wake_up(&tsk->sighand->signalfd_wqh);
 }
 
+extern void signalfd_cleanup(struct sighand_struct *sighand);
+
 #else /* CONFIG_SIGNALFD */
 
 static inline void signalfd_notify(struct task_struct *tsk, int sig) { }
 
+static inline void signalfd_cleanup(struct sighand_struct *sighand) { }
+
 #endif /* CONFIG_SIGNALFD */
 
 #endif /* __KERNEL__ */
 
 #endif /* _LINUX_SIGNALFD_H */
-

kernel/fork.c +4 -1

index b77fd55..e2cd3e2 100644
@@ -66,6 +66,7 @@
 #include <linux/user-return-notifier.h>
 #include <linux/oom.h>
 #include <linux/khugepaged.h>
+#include <linux/signalfd.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>

@@ -935,8 +936,10 @@ static int copy_sighand(unsigned long clone_flags, struct task_struct *tsk)
 
 void __cleanup_sighand(struct sighand_struct *sighand)
 {
-	if (atomic_dec_and_test(&sighand->count))
+	if (atomic_dec_and_test(&sighand->count)) {
+		signalfd_cleanup(sighand);
 		kmem_cache_free(sighand_cachep, sighand);
+	}
 }

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2012-02-22 17:41 UTC #2 lore raw

PATCH 2/4 epoll: introduce POLLFREE for ep_poll_callback()

In reply to a message not in the archive

Note: this patch is intentionally incomplete to simplify the review. It ignores ep_unregister_pollwait() which plays with the same wqh. See the next changes.

epoll assumes that the EPOLL_CTL_ADD'ed file controls everything f_op->poll() needs. In particular it assumes that the wait queue can't go away until eventpoll_release(). This is not true in case of signalfd, the task which does EPOLL_CTL_ADD uses its ->sighand which is not connected to the file.

This patch adds the special event, POLLFREE, currently only for epoll. It expects that init_poll_funcptr()'ed hook should do the necessary cleanup. Perhaps it should be defined as EPOLLFREE in eventpoll.

ep_poll_callback(POLLFREE) simply does list_del_init(task_list). This make this poll entry inconsistent, but we don't care. If you share epoll fd which contains our sigfd with another process you should blame yourself. signalfd is "really special". I simply do not know how we can define the "right" semantics if it used with epoll.

The main problem is, epoll calls signalfd_poll() once to establish the connection with the wait queue, after that signalfd_poll(NULL) returns the different/inconsistent results depending on who does EPOLL_CTL_MOD/signalfd_read/etc. IOW: apart from sigmask, signalfd has nothing to do with the file, it works with the current thread.

In short: this patch is the hack which tries to fix the symptoms. It also assumes that nobody can take tasklist_lock under epoll locks, this seems to be true.

Note: we do not have wake_up_all_poll() but wake_up_poll() should be fine, poll/epoll doesn't use WQ_FLAG_EXCLUSIVE.

Reported-by Maxime Bizon <[email protected]>

Cc <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/eventpoll.c             |    4 ++++
 fs/signalfd.c              |    5 +++++
 include/asm-generic/poll.h |    2 ++
 3 files changed, 11 insertions(+), 0 deletions(-)

fs/eventpoll.c +4 -0

index aabdfc3..442bedb 100644
@@ -844,6 +844,10 @@ static int ep_poll_callback(wait_queue_t *wait, unsigned mode, int sync, void *k
 
 	spin_lock_irqsave(&ep->lock, flags);
 
+	/* the caller holds eppoll_entry->whead->lock */
+	if ((unsigned long)key & POLLFREE)
+		list_del_init(&wait->task_list);
+
 	/*
 	 * If the event mask does not contain any poll(2) event, we consider the
 	 * descriptor to be disabled. This condition is likely the effect of the

fs/signalfd.c +5 -0

index 35d19ae..838ba21 100644
@@ -34,6 +34,11 @@ void signalfd_cleanup(struct sighand_struct *sighand)
 {
 	wait_queue_head_t *wqh = &sighand->signalfd_wqh;
 
+	if (likely(!waitqueue_active(wqh)))
+		return;
+
+	/* ask wait_queue_t->func() to remove_wait_queue() */
+	wake_up_poll(wqh, POLLHUP | POLLFREE);
 	BUG_ON(waitqueue_active(wqh));
 }

include/asm-generic/poll.h +2 -0

index 44bce83..9ce7f44 100644
@@ -28,6 +28,8 @@
 #define POLLRDHUP       0x2000
 #endif
 
+#define POLLFREE	0x4000	/* currently only for epoll */
+
 struct pollfd {
 	int fd;
 	short events;

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2012-02-22 17:41 UTC #3 lore raw

PATCH 3/4 signalfd: signalfd_cleanup() can race with remove_wait_queue()

In reply to a message not in the archive

signalfd_cleanup() checks waitqueue_active() lockless, this can race with ep_unregister_pollwait(). We can see list_empty() == T before remove_wait_queue() completes and list_empty_careful() can't help. Add spin_unlock_wait() to serialize.

Reported-by Maxime Bizon <[email protected]>

Cc <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/signalfd.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

fs/signalfd.c +4 -0

index 838ba21..6e51887 100644
@@ -34,6 +34,10 @@ void signalfd_cleanup(struct sighand_struct *sighand)
 {
 	wait_queue_head_t *wqh = &sighand->signalfd_wqh;
 
+	/* make sure we can't race with remove_wait_queue() in progress */
+	spin_unlock_wait(&wqh->lock);
+	smp_rmb();
+
 	if (likely(!waitqueue_active(wqh)))
 		return;

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2012-02-22 17:42 UTC #4 lore raw

PATCH 4/4 epoll: ep_unregister_pollwait() can use the freed pwq->whead

In reply to a message not in the archive

signalfd_cleanup() ensures that ->signalfd_wqh is not used, but this is not enough. eppoll_entry->whead still points to the memory we are going to free, ep_unregister_pollwait()->remove_wait_queue() is obviously unsafe.

Change ep_poll_callback(POLLFREE) to set eppoll_entry->whead = NULL, change ep_unregister_pollwait() to check pwq->whead != NULL before remove_wait_queue(). We add the new ep_remove_wait_queue() helper for this.

However this needs more locking. ep_remove_wait_queue() should take ep->lock first to avoid the race and pin pwq->whead, then it needs pwq->whead->lock for __remove_wait_queue().

This can obviously AB-BA deadlock with wake_up()->ep_poll_callback(), so ep_remove_wait_queue() does the nasty lock + trylock-or-retry dance.

Of course, this also assumes that it is safe to take ep->lock in ep_unregister_pollwait() paths, afaics this is true.

Reported-by Maxime Bizon <[email protected]>

Cc <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/eventpoll.c |   43 +++++++++++++++++++++++++++++++++++++++----
 1 files changed, 39 insertions(+), 4 deletions(-)

fs/eventpoll.c +39 -4

index 442bedb..ac8bd15 100644
@@ -320,6 +320,11 @@ static inline int ep_is_linked(struct list_head *p)
 	return !list_empty(p);
 }
 
+static inline struct eppoll_entry *ep_pwq_from_wait(wait_queue_t *p)
+{
+	return container_of(p, struct eppoll_entry, wait);
+}
+
 /* Get the "struct epitem" from a wait queue pointer */
 static inline struct epitem *ep_item_from_wait(wait_queue_t *p)
 {

@@ -467,6 +472,33 @@ static void ep_poll_safewake(wait_queue_head_t *wq)
 	put_cpu();
 }
 
+static void ep_remove_wait_queue(struct eventpoll *ep, struct eppoll_entry *pwq)
+{
+	for (;;) {
+		unsigned long flags;	/* probably unneeded */
+
+		spin_lock_irqsave(&ep->lock, flags);
+		/* can be cleared by ep_poll_callback(POLLFREE) */
+		if (!pwq->whead)
+			goto unlock;
+
+		/* _trylock to avoid the deadlock, retry if it fails */
+		if (!spin_trylock(&pwq->whead->lock))
+			goto unlock;
+
+		__remove_wait_queue(pwq->whead, &pwq->wait);
+		spin_unlock(&pwq->whead->lock);
+		pwq->whead = NULL;
+ unlock:
+		spin_unlock_irqrestore(&ep->lock, flags);
+
+		if (!pwq->whead)
+			break;
+
+		cpu_relax();
+	}
+}
+
 /*
  * This function unregisters poll callbacks from the associated file
  * descriptor.  Must be called with "mtx" held (or "epmutex" if called from

@@ -481,7 +513,7 @@ static void ep_unregister_pollwait(struct eventpoll *ep, struct epitem *epi)
 		pwq = list_first_entry(lsthead, struct eppoll_entry, llink);
 
 		list_del(&pwq->llink);
-		remove_wait_queue(pwq->whead, &pwq->wait);
+		ep_remove_wait_queue(ep, pwq);
 		kmem_cache_free(pwq_cache, pwq);
 	}
 }

@@ -844,9 +876,12 @@ static int ep_poll_callback(wait_queue_t *wait, unsigned mode, int sync, void *k
 
 	spin_lock_irqsave(&ep->lock, flags);
 
-	/* the caller holds eppoll_entry->whead->lock */
-	if ((unsigned long)key & POLLFREE)
-		list_del_init(&wait->task_list);
+	if ((unsigned long)key & POLLFREE) {
+		struct eppoll_entry *pwq = ep_pwq_from_wait(wait);
+		/* the caller holds pwq->whead->lock */
+		__remove_wait_queue(pwq->whead, wait);
+		pwq->whead = NULL;
+	}
 
 	/*
 	 * If the event mask does not contain any poll(2) event, we consider the

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2012-02-23 15:51 UTC #5 lore raw

Re: [PATCH 4/4] epoll: ep_unregister_pollwait() can use the freed pwq->whead

Quoted 7 lines · Oleg Nesterov

However this needs more locking. ep_remove_wait_queue() should take ep->lock first to avoid the race and pin pwq->whead, then it needs pwq->whead->lock for __remove_wait_queue().
This can obviously AB-BA deadlock with wake_up()->ep_poll_callback(), so ep_remove_wait_queue() does the nasty lock + trylock-or-retry dance.

Or we can rely on the fact that sighand_cachep is SLAB_DESTROY_BY_RCU, and assume that ->whead is always rcu-protected if it can go away.

In this case we don't need 3/4 (although it makes sense to add the fat comment), and 4/4 can be simplified, see below.

ep_pwq_from_wait() is not really needed, it has a single caller. Just I tried to follow the coding style.

I'd prefer the explicit locking, but I don't really mind.

Oleg.

fs/eventpoll.c +21 -2

@@ -320,6 +320,11 @@ static inline int ep_is_linked(struct list_head *p)
 	return !list_empty(p);
 }
 
+static inline struct eppoll_entry *ep_pwq_from_wait(wait_queue_t *p)
+{
+	return container_of(p, struct eppoll_entry, wait);
+}
+
 /* Get the "struct epitem" from a wait queue pointer */
 static inline struct epitem *ep_item_from_wait(wait_queue_t *p)
 {

@@ -467,6 +472,17 @@ static void ep_poll_safewake(wait_queue_head_t *wq)
 	put_cpu();
 }
 
+static void ep_remove_wait_queue(struct eppoll_entry *pwq)
+{
+	wait_queue_head_t *whead;
+
+	rcu_read_lock();
+	whead = rcu_dereference(pwq->whead);
+	if (whead)
+		remove_wait_queue(whead, &pwq->wait);
+	rcu_read_unlock();
+}
+
 /*
  * This function unregisters poll callbacks from the associated file
  * descriptor.  Must be called with "mtx" held (or "epmutex" if called from

@@ -481,7 +497,7 @@ static void ep_unregister_pollwait(struct eventpoll *ep, struct epitem *epi)
 		pwq = list_first_entry(lsthead, struct eppoll_entry, llink);
 
 		list_del(&pwq->llink);
-		remove_wait_queue(pwq->whead, &pwq->wait);
+		ep_remove_wait_queue(pwq);
 		kmem_cache_free(pwq_cache, pwq);
 	}
 }

@@ -845,8 +861,11 @@ static int ep_poll_callback(wait_queue_t *wait, unsigned mode, int sync, void *k
 	spin_lock_irqsave(&ep->lock, flags);
 
 	/* the caller holds eppoll_entry->whead->lock */
-	if ((unsigned long)key & POLLFREE)
+	if ((unsigned long)key & POLLFREE) {
+		/* can't use __remove_wait_queue(), we need list_del_init() */
 		list_del_init(&wait->task_list);
+		ep_pwq_from_wait(wait)->whead = NULL;
+	}
 
 	/*
 	 * If the event mask does not contain any poll(2) event, we consider the

Linus Torvalds

2012-02-23 22:17 UTC #6 lore raw

Re: [PATCH 4/4] epoll: ep_unregister_pollwait() can use the freed pwq->whead

Quoted 6 lines · Oleg Nesterov

Or we can rely on the fact that sighand_cachep is SLAB_DESTROY_BY_RCU, and assume that ->whead is always rcu-protected if it can go away.
In this case we don't need 3/4 (although it makes sense to add the fat comment), and 4/4 can be simplified, see below.

Ok.

Can you also get rid of 1/4, because quite frankly, adding that BUG_ON() is just annoying. Either the thing gets fixed or not, but at no point is it ok to say "ok, I'm going to fix it, but before I do I'll just make it much worse".

Hmm?
Linus

Oleg Nesterov

author

2012-02-24 19:13 UTC #7 lore raw

v2 cover signalfd/epoll fixes

Quoted 9 lines · Linus Torvalds

Quoted 6 lines · Oleg Nesterov
Or we can rely on the fact that sighand_cachep is SLAB_DESTROY_BY_RCU, and assume that ->whead is always rcu-protected if it can go away.
In this case we don't need 3/4 (although it makes sense to add the fat comment), and 4/4 can be simplified, see below.

Quoted 6 lines

Ok.
Can you also get rid of 1/4, because quite frankly, adding that BUG_ON() is just annoying. Either the thing gets fixed or not, but at no point is it ok to say "ok, I'm going to fix it, but before I do I'll just make it much worse".

OK. Please see v2.

Other changes:

- some comments

- now that we rely on rcu, ep_poll_callback() can do remove_wait_queue() outsife of ep->lock

Davide, I see the new email, but it is too late for me to reply today. Anyway, I think it makes sense to make the "simple" fix before anything else. IOW, I'd suggest these changes for now in any case (unless, of course, you see some problems).

Oleg.

Oleg Nesterov

author

2012-02-24 19:14 UTC #8 lore raw

PATCH v2 1/2 epoll: introduce POLLFREE to flush ->signalfd_wqh before kfree()

This patch is intentionally incomplete to simplify the review. It ignores ep_unregister_pollwait() which plays with the same wqh. See the next change.

__cleanup_sighand() is changed to do wake_up_poll(POLLFREE) if ->signalfd_wqh is not empty, we add the new signalfd_cleanup() helper.

In short: this patch is the hack which tries to fix the symptoms. It also assumes that nobody can take tasklist_lock under epoll locks, this seems to be true.

Note:

- we do not have wake_up_all_poll() but wake_up_poll() is fine, poll/epoll doesn't use WQ_FLAG_EXCLUSIVE.

- signalfd_cleanup() uses POLLHUP along with POLLFREE, we need a couple of simple changes in eventpoll.c to make sure it can't be "lost".

Reported-by Maxime Bizon <[email protected]>

Cc <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/eventpoll.c             |    4 ++++
 fs/signalfd.c              |   11 +++++++++++
 include/asm-generic/poll.h |    2 ++
 include/linux/signalfd.h   |    5 ++++-
 kernel/fork.c              |    5 ++++-
 5 files changed, 25 insertions(+), 2 deletions(-)

fs/eventpoll.c +4 -0

index aabdfc3..34bbfc6 100644
@@ -842,6 +842,10 @@ static int ep_poll_callback(wait_queue_t *wait, unsigned mode, int sync, void *k
 	struct epitem *epi = ep_item_from_wait(wait);
 	struct eventpoll *ep = epi->ep;
 
+	/* the caller holds eppoll_entry->whead->lock */
+	if ((unsigned long)key & POLLFREE)
+		list_del_init(&wait->task_list);
+
 	spin_lock_irqsave(&ep->lock, flags);
 
 	/*

fs/signalfd.c +11 -0

index 492465b..79c1eea 100644
@@ -30,6 +30,17 @@
 #include <linux/signalfd.h>
 #include <linux/syscalls.h>
 
+void signalfd_cleanup(struct sighand_struct *sighand)
+{
+	wait_queue_head_t *wqh = &sighand->signalfd_wqh;
+
+	if (likely(!waitqueue_active(wqh)))
+		return;
+
+	/* wait_queue_t->func(POLLFREE) should do remove_wait_queue() */
+	wake_up_poll(wqh, POLLHUP | POLLFREE);
+}
+
 struct signalfd_ctx {
 	sigset_t sigmask;
 };

include/asm-generic/poll.h +2 -0

index 44bce83..9ce7f44 100644
@@ -28,6 +28,8 @@
 #define POLLRDHUP       0x2000
 #endif
 
+#define POLLFREE	0x4000	/* currently only for epoll */
+
 struct pollfd {
 	int fd;
 	short events;

include/linux/signalfd.h +4 -1

index 3ff4961..247399b 100644
@@ -61,13 +61,16 @@ static inline void signalfd_notify(struct task_struct *tsk, int sig)
 		wake_up(&tsk->sighand->signalfd_wqh);
 }
 
+extern void signalfd_cleanup(struct sighand_struct *sighand);
+
 #else /* CONFIG_SIGNALFD */
 
 static inline void signalfd_notify(struct task_struct *tsk, int sig) { }
 
+static inline void signalfd_cleanup(struct sighand_struct *sighand) { }
+
 #endif /* CONFIG_SIGNALFD */
 
 #endif /* __KERNEL__ */
 
 #endif /* _LINUX_SIGNALFD_H */
-

kernel/fork.c +4 -1

index b77fd55..e2cd3e2 100644
@@ -66,6 +66,7 @@
 #include <linux/user-return-notifier.h>
 #include <linux/oom.h>
 #include <linux/khugepaged.h>
+#include <linux/signalfd.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>

@@ -935,8 +936,10 @@ static int copy_sighand(unsigned long clone_flags, struct task_struct *tsk)
 
 void __cleanup_sighand(struct sighand_struct *sighand)
 {
-	if (atomic_dec_and_test(&sighand->count))
+	if (atomic_dec_and_test(&sighand->count)) {
+		signalfd_cleanup(sighand);
 		kmem_cache_free(sighand_cachep, sighand);
+	}
 }

Signature · 1 line

1.5.5.1

Andy Lutomirski

2012-02-29 19:57 UTC #9 lore raw

Re: [PATCH v2 1/2] epoll: introduce POLLFREE to flush ->signalfd_wqh before kfree()

Quoted 10 lines · Oleg Nesterov

This patch is intentionally incomplete to simplify the review. It ignores ep_unregister_pollwait() which plays with the same wqh. See the next change.
epoll assumes that the EPOLL_CTL_ADD'ed file controls everything f_op->poll() needs. In particular it assumes that the wait queue can't go away until eventpoll_release(). This is not true in case of signalfd, the task which does EPOLL_CTL_ADD uses its ->sighand which is not connected to the file.

Quoted 6 lines

This patch adds the special event, POLLFREE, currently only for epoll. It expects that init_poll_funcptr()'ed hook should do the necessary cleanup. Perhaps it should be defined as EPOLLFREE in eventpoll.
[lots of kernel-internal technical stuff]

I have a bunch of userspace code that uses signalfd via epoll. Does this affect the ABI? Will epoll_wait ever set POLLFREE? Does EPOLL_CTL_MOD accept POLLFREE?

IOW, from a userspace point of view, wtf does this do? I don't want to end up with another POLLRDHUP-like* special case in my code.

--Andy

* IMO the right fix would have been to make EPOLLET fire POLLIN again when the read point advances to EOF but before EOF is actually seen when read() returns zero. Then POLLRDHUP would be unnecessary and user code could do its thing in blissful ignorance. I hope POLLFREE isn't like that.

Oleg Nesterov

author

2012-02-29 20:13 UTC #10 lore raw

Re: [PATCH v2 1/2] epoll: introduce POLLFREE to flush ->signalfd_wqh before kfree()

Quoted 9 lines · Andy Lutomirski

Quoted 4 lines · Oleg Nesterov
This patch adds the special event, POLLFREE, currently only for epoll. It expects that init_poll_funcptr()'ed hook should do the necessary cleanup. Perhaps it should be defined as EPOLLFREE in eventpoll.
I have a bunch of userspace code that uses signalfd via epoll. Does this affect the ABI?

I hope not ;)

Quoted 1 line

Will epoll_wait ever set POLLFREE?

No.

Quoted 2 lines

Does
EPOLL_CTL_MOD accept POLLFREE?

Yes, it doesn't check the bits. EPOLL_CTL_MOD accepts any bit.

Quoted 1 line

IOW, from a userspace point of view, wtf does this do?

I hope this is transparent to the user-space. If the application does EPOLL_CTL_MOD(POLLFREE) by mistake then ep_poll_callback(POLLFREE) can trigger the unnecessary wakeup and move the file to ep->rdllist but this is harmless. The subsequent ->poll() can't return POLLFREE.

Oleg.

Andrew Lutomirski

2012-02-29 20:16 UTC #11 lore raw

Re: [PATCH v2 1/2] epoll: introduce POLLFREE to flush ->signalfd_wqh before kfree()

Quoted 8 lines · Oleg Nesterov

Quoted 7 lines · Andy Lutomirski
Quoted 4 lines · Oleg Nesterov
This patch adds the special event, POLLFREE, currently only for epoll. It expects that init_poll_funcptr()'ed hook should do the necessary cleanup. Perhaps it should be defined as EPOLLFREE in eventpoll.

Quoted 5 lines

Quoted 2 lines
I have a bunch of userspace code that uses signalfd via epoll. Does this affect the ABI?
I hope not ;)

Excellent!

To avoid further confusion, would it make sense to update the comment in poll.h to indicate that POLLFREE is only for epoll and is internal to the kernel?

--Andy

Oleg Nesterov

author

2012-03-01 19:33 UTC #12 lore raw

Re: [PATCH v2 1/2] epoll: introduce POLLFREE to flush ->signalfd_wqh before kfree()

Quoted 13 lines · Andrew Lutomirski

Quoted 11 lines · Oleg Nesterov
Quoted 9 lines · Andy Lutomirski
Quoted 4 lines · Oleg Nesterov
This patch adds the special event, POLLFREE, currently only for epoll. It expects that init_poll_funcptr()'ed hook should do the necessary cleanup. Perhaps it should be defined as EPOLLFREE in eventpoll.
I have a bunch of userspace code that uses signalfd via epoll. Does this affect the ABI?

Quoted 8 lines

Quoted 2 lines
I hope not ;)
Excellent!
To avoid further confusion, would it make sense to update the comment in poll.h to indicate that POLLFREE is only for epoll and is internal to the kernel?

Agreed. Or perhaps we can simply use POLLREMOVE instead...

But note that nobody (including me) likes this fix, just I was asked to make something simple/backportable. Probably it will be reverted later.

If not, then perhaps it makes sense to do a couple of simple changes on top to cleanup the usage of POLLFREE and to ensure that POLLHUP is actually delivered (the latter will be user-visible).

Oleg.

Oleg Nesterov

author

2012-02-24 19:14 UTC #13 lore raw

PATCH v2 2/2 epoll: ep_unregister_pollwait() can use the freed pwq->whead

In reply to Oleg Nesterov

Change ep_poll_callback(POLLFREE) to set eppoll_entry->whead = NULL, change ep_unregister_pollwait() to check pwq->whead != NULL under rcu_read_lock() before remove_wait_queue(). We add the new helper, ep_remove_wait_queue(), for this.

This works because sighand_cachep is SLAB_DESTROY_BY_RCU and because ->signalfd_wqh is initialized in sighand_ctor(), not in copy_sighand. ep_unregister_pollwait()->remove_wait_queue() can play with already freed and potentially reused ->sighand, but this is fine. This memory must have the valid ->signalfd_wqh until rcu_read_unlock().

Reported-by Maxime Bizon <[email protected]>

Cc <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/eventpoll.c |   30 +++++++++++++++++++++++++++---
 fs/signalfd.c  |    6 +++++-
 2 files changed, 32 insertions(+), 4 deletions(-)

fs/eventpoll.c +27 -3

index 34bbfc6..ea54cde 100644
@@ -320,6 +320,11 @@ static inline int ep_is_linked(struct list_head *p)
 	return !list_empty(p);
 }
 
+static inline struct eppoll_entry *ep_pwq_from_wait(wait_queue_t *p)
+{
+	return container_of(p, struct eppoll_entry, wait);
+}
+
 /* Get the "struct epitem" from a wait queue pointer */
 static inline struct epitem *ep_item_from_wait(wait_queue_t *p)
 {

@@ -467,6 +472,18 @@ static void ep_poll_safewake(wait_queue_head_t *wq)
 	put_cpu();
 }
 
+static void ep_remove_wait_queue(struct eppoll_entry *pwq)
+{
+	wait_queue_head_t *whead;
+
+	rcu_read_lock();
+	/* If it is cleared by POLLFREE, it should be rcu-safe */
+	whead = rcu_dereference(pwq->whead);
+	if (whead)
+		remove_wait_queue(whead, &pwq->wait);
+	rcu_read_unlock();
+}
+
 /*
  * This function unregisters poll callbacks from the associated file
  * descriptor.  Must be called with "mtx" held (or "epmutex" if called from

@@ -481,7 +498,7 @@ static void ep_unregister_pollwait(struct eventpoll *ep, struct epitem *epi)
 		pwq = list_first_entry(lsthead, struct eppoll_entry, llink);
 
 		list_del(&pwq->llink);
-		remove_wait_queue(pwq->whead, &pwq->wait);
+		ep_remove_wait_queue(pwq);
 		kmem_cache_free(pwq_cache, pwq);
 	}
 }

@@ -842,9 +859,16 @@ static int ep_poll_callback(wait_queue_t *wait, unsigned mode, int sync, void *k
 	struct epitem *epi = ep_item_from_wait(wait);
 	struct eventpoll *ep = epi->ep;
 
-	/* the caller holds eppoll_entry->whead->lock */
-	if ((unsigned long)key & POLLFREE)
+	if ((unsigned long)key & POLLFREE) {
+		ep_pwq_from_wait(wait)->whead = NULL;
+		/*
+		 * whead = NULL above can race with ep_remove_wait_queue()
+		 * which can do another remove_wait_queue() after us, so we
+		 * can't use __remove_wait_queue(). whead->lock is held by
+		 * the caller.
+		 */
 		list_del_init(&wait->task_list);
+	}
 
 	spin_lock_irqsave(&ep->lock, flags);

fs/signalfd.c +5 -1

index 79c1eea..7ae2a57 100644
@@ -33,7 +33,11 @@
 void signalfd_cleanup(struct sighand_struct *sighand)
 {
 	wait_queue_head_t *wqh = &sighand->signalfd_wqh;
-
+	/*
+	 * The lockless check can race with remove_wait_queue() in progress,
+	 * but in this case its caller should run under rcu_read_lock() and
+	 * sighand_cachep is SLAB_DESTROY_BY_RCU, we can safely return.
+	 */
 	if (likely(!waitqueue_active(wqh)))
 		return;

Signature · 1 line

1.5.5.1

Linus Torvalds

2012-02-24 20:23 UTC #14 lore raw

Re: [PATCH v2 0/2] signalfd/epoll fixes

In reply to Oleg Nesterov

Quoted 2 lines · Oleg Nesterov

OK. Please see v2.

Ok, I applied these.

And then, dammit, I unapplied them again.

And then I applied them again.

I'm *really* conflicted, because I have this really strong feeling that it's just papering over a symptom, and we damn well shouldn't be doing that. I really think that what we really should do is allow "poll()" to have a "poll_remove" callback (so each "add_poll_wait()" will have a callback when it gets remove).

Then we could make the poll() functions actually do allocations and crap - or at least add refcounts - and the "poll_remove()" ones would undo them.

And then we could rip out all this, and make signalfd just do

  static void poll_remove(struct file *file, struct wait_queue *wq)
  {
    struct sighand *sighand = container_of(wq, struct sighand, signalfd_wqh);
    __cleanup_sighand(sighand);
  }

and add that "poll_remove" to its file handler operations. And in "poll()", it would just do

   atomic_inc(&sighand->count);

as it does the poll_wait() thing. Sure, we need to have some way to test "did we really add it", and only increment the count if so (so poll_wait() would need to return a value), but this seems to be the *real* fix. Because the real problem is that we cannot currently refcount the poll users.

Ok, so it's just a strong feeling, and I *did* end up applying these two patches after all, but I really wonder how hard it would be to just add a single new callback function and be able to refcount that sighand structure itself.

Linus

Linus Torvalds

2012-02-24 23:15 UTC #15 lore raw

Re: [PATCH v2 0/2] signalfd/epoll fixes

Quoted 10 lines · Linus Torvalds

I'm *really* conflicted, because I have this really strong feeling that it's just papering over a symptom, and we damn well shouldn't be doing that. I really think that what we really should do is allow "poll()" to have a "poll_remove" callback (so each "add_poll_wait()" will have a callback when it gets remove).
Then we could make the poll() functions actually do allocations and crap - or at least add refcounts - and the "poll_remove()" ones would undo them.

Ok, I have *NOT* tested this, and by now it's certainly not 3.3 material any more, but here's a patch to kind of lay the groundwork and show what I mean.

UNTESTED! UNTESTED! CAVEAT! Probably horribly broken.

Linus

 drivers/vhost/vhost.c |    5 +++--
 fs/eventpoll.c        |    9 ++++++++-
 fs/select.c           |   12 ++++++++----
 fs/signalfd.c         |   12 +++++++++++-
 include/linux/fs.h    |    1 +
 include/linux/poll.h  |    7 ++++---
 kernel/cgroup.c       |    3 ++-
 net/9p/trans_fd.c     |    5 +++--
 virt/kvm/eventfd.c    |    3 ++-
 9 files changed, 42 insertions(+), 15 deletions(-)

drivers/vhost/vhost.c +3 -2

index c14c42b95ab8..5880f3d81333 100644
@@ -42,14 +42,15 @@ static unsigned vhost_zcopy_mask __read_mostly;
 #define vhost_used_event(vq) ((u16 __user *)&vq->avail->ring[vq->num])
 #define vhost_avail_event(vq) ((u16 __user *)&vq->used->ring[vq->num])
 
-static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
-			    poll_table *pt)
+static int vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
+			   poll_table *pt)
 {
 	struct vhost_poll *poll;
 
 	poll = container_of(pt, struct vhost_poll, table);
 	poll->wqh = wqh;
 	add_wait_queue(wqh, &poll->wait);
+	return 1;
 }
 
 static int vhost_poll_wakeup(wait_queue_t *wait, unsigned mode, int sync,

fs/eventpoll.c +8 -1

index aabdfc38cf24..010b9eb9de74 100644
@@ -474,13 +474,18 @@ static void ep_poll_safewake(wait_queue_head_t *wq)
  */
 static void ep_unregister_pollwait(struct eventpoll *ep, struct epitem *epi)
 {
+	struct file *file = epi->ffd.file;
 	struct list_head *lsthead = &epi->pwqlist;
 	struct eppoll_entry *pwq;
+	void (*poll_rm) (struct file *, wait_queue_head_t *);
 
+	poll_rm = file->f_op->poll_rm;
 	while (!list_empty(lsthead)) {
 		pwq = list_first_entry(lsthead, struct eppoll_entry, llink);
 
 		list_del(&pwq->llink);
+		if (poll_rm)
+			poll_rm(file, pwq->whead);
 		remove_wait_queue(pwq->whead, &pwq->wait);
 		kmem_cache_free(pwq_cache, pwq);
 	}

@@ -903,7 +908,7 @@ out_unlock:
  * This is the callback that is used to add our wait queue to the
  * target file wakeup lists.
  */
-static void ep_ptable_queue_proc(struct file *file, wait_queue_head_t *whead,
+static int ep_ptable_queue_proc(struct file *file, wait_queue_head_t *whead,
 				 poll_table *pt)
 {
 	struct epitem *epi = ep_item_from_epqueue(pt);

@@ -916,9 +921,11 @@ static void ep_ptable_queue_proc(struct file *file, wait_queue_head_t *whead,
 		add_wait_queue(whead, &pwq->wait);
 		list_add_tail(&pwq->llink, &epi->pwqlist);
 		epi->nwait++;
+		return 1;
 	} else {
 		/* We have to signal that an error occurred */
 		epi->nwait = -1;
+		return 0;
 	}
 }

fs/select.c +8 -4

index e782258d0de3..a4e369dda85c 100644
@@ -110,7 +110,7 @@ struct poll_table_page {
  * as all select/poll functions have to call it to add an entry to the
  * poll table.
  */
-static void __pollwait(struct file *filp, wait_queue_head_t *wait_address,
+static int __pollwait(struct file *filp, wait_queue_head_t *wait_address,
 		       poll_table *p);
 
 void poll_initwait(struct poll_wqueues *pwq)

@@ -126,8 +126,11 @@ EXPORT_SYMBOL(poll_initwait);
 
 static void free_poll_entry(struct poll_table_entry *entry)
 {
+	struct file *file = entry->filp;
+	if (file->f_op->poll_rm)
+		file->f_op->poll_rm(file, entry->wait_address);
 	remove_wait_queue(entry->wait_address, &entry->wait);
-	fput(entry->filp);
+	fput(file);
 }
 
 void poll_freewait(struct poll_wqueues *pwq)

@@ -213,13 +216,13 @@ static int pollwake(wait_queue_t *wait, unsigned mode, int sync, void *key)
 }
 
 /* Add a new entry */
-static void __pollwait(struct file *filp, wait_queue_head_t *wait_address,
+static int __pollwait(struct file *filp, wait_queue_head_t *wait_address,
 				poll_table *p)
 {
 	struct poll_wqueues *pwq = container_of(p, struct poll_wqueues, pt);
 	struct poll_table_entry *entry = poll_get_entry(pwq);
 	if (!entry)
-		return;
+		return 0;
 	get_file(filp);
 	entry->filp = filp;
 	entry->wait_address = wait_address;

@@ -227,6 +230,7 @@ static void __pollwait(struct file *filp, wait_queue_head_t *wait_address,
 	init_waitqueue_func_entry(&entry->wait, pollwake);
 	entry->wait.private = pwq;
 	add_wait_queue(wait_address, &entry->wait);
+	return 1;
 }
 
 int poll_schedule_timeout(struct poll_wqueues *pwq, int state,

fs/signalfd.c +11 -1

index 492465b451dd..89afafafbf2e 100644
@@ -40,12 +40,21 @@ static int signalfd_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
+static void signalfd_poll_rm(struct file *file, wait_queue_head_t *p)
+{
+	struct sighand_struct *sighand;
+
+	sighand = container_of(p, struct sighand_struct, signalfd_wqh);
+	__cleanup_sighand(sighand);
+}
+
 static unsigned int signalfd_poll(struct file *file, poll_table *wait)
 {
 	struct signalfd_ctx *ctx = file->private_data;
 	unsigned int events = 0;
 
-	poll_wait(file, &current->sighand->signalfd_wqh, wait);
+	if (poll_wait(file, &current->sighand->signalfd_wqh, wait))
+		atomic_inc(&current->sighand->count);
 
 	spin_lock_irq(&current->sighand->siglock);
 	if (next_signal(&current->pending, &ctx->sigmask) ||

@@ -215,6 +224,7 @@ static ssize_t signalfd_read(struct file *file, char __user *buf, size_t count,
 static const struct file_operations signalfd_fops = {
 	.release	= signalfd_release,
 	.poll		= signalfd_poll,
+	.poll_rm	= signalfd_poll_rm,
 	.read		= signalfd_read,
 	.llseek		= noop_llseek,
 };

include/linux/fs.h +1 -0

index 69cd5bb640f5..a96e193dfa6b 100644
@@ -1604,6 +1604,7 @@ struct file_operations {
 	ssize_t (*aio_write) (struct kiocb *, const struct iovec *, unsigned long, loff_t);
 	int (*readdir) (struct file *, void *, filldir_t);
 	unsigned int (*poll) (struct file *, struct poll_table_struct *);
+	void (*poll_rm) (struct file *, wait_queue_head_t * wait_address);
 	long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
 	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
 	int (*mmap) (struct file *, struct vm_area_struct *);

include/linux/poll.h +4 -3

index cf40010ce0cd..9b73a2557add 100644
@@ -30,17 +30,18 @@ struct poll_table_struct;
 /* 
  * structures and helpers for f_op->poll implementations
  */
-typedef void (*poll_queue_proc)(struct file *, wait_queue_head_t *, struct poll_table_struct *);
+typedef int (*poll_queue_proc)(struct file *, wait_queue_head_t *, struct poll_table_struct *);
 
 typedef struct poll_table_struct {
 	poll_queue_proc qproc;
 	unsigned long key;
 } poll_table;
 
-static inline void poll_wait(struct file * filp, wait_queue_head_t * wait_address, poll_table *p)
+static inline int poll_wait(struct file * filp, wait_queue_head_t * wait_address, poll_table *p)
 {
 	if (p && wait_address)
-		p->qproc(filp, wait_address, p);
+		return p->qproc(filp, wait_address, p);
+	return 0;
 }
 
 static inline void init_poll_funcptr(poll_table *pt, poll_queue_proc qproc)

kernel/cgroup.c +2 -1

index a5d3b5325f77..3029d66d8b51 100644
@@ -3520,7 +3520,7 @@ static int cgroup_event_wake(wait_queue_t *wait, unsigned mode,
 	return 0;
 }
 
-static void cgroup_event_ptable_queue_proc(struct file *file,
+static int cgroup_event_ptable_queue_proc(struct file *file,
 		wait_queue_head_t *wqh, poll_table *pt)
 {
 	struct cgroup_event *event = container_of(pt,

@@ -3528,6 +3528,7 @@ static void cgroup_event_ptable_queue_proc(struct file *file,
 
 	event->wqh = wqh;
 	add_wait_queue(wqh, &event->wait);
+	return 1;
 }
 
 /*

net/9p/trans_fd.c +3 -2

index fccae26fa674..d87b30b6e8e9 100644
@@ -538,7 +538,7 @@ static int p9_pollwake(wait_queue_t *wait, unsigned mode, int sync, void *key)
  * called by files poll operation to add v9fs-poll task to files wait queue
  */
 
-static void
+static int
 p9_pollwait(struct file *filp, wait_queue_head_t *wait_address, poll_table *p)
 {
 	struct p9_conn *m = container_of(p, struct p9_conn, pt);

@@ -554,13 +554,14 @@ p9_pollwait(struct file *filp, wait_queue_head_t *wait_address, poll_table *p)
 
 	if (!pwait) {
 		p9_debug(P9_DEBUG_ERROR, "not enough wait_address slots\n");
-		return;
+		return 0;
 	}
 
 	pwait->conn = m;
 	pwait->wait_addr = wait_address;
 	init_waitqueue_func_entry(&pwait->wait, p9_pollwake);
 	add_wait_queue(wait_address, &pwait->wait);
+	return 1;
 }
 
 /**

virt/kvm/eventfd.c +2 -1

index f59c1e8de7a2..f5e59036a937 100644
@@ -168,12 +168,13 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
 	return 0;
 }
 
-static void
+static int
 irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,
 			poll_table *pt)
 {
 	struct _irqfd *irqfd = container_of(pt, struct _irqfd, pt);
 	add_wait_queue(wqh, &irqfd->wait);
+	return 1;
 }
 
 /* Must be called under irqfds.lock */

Oleg Nesterov

author

2012-02-25 16:15 UTC #16 lore raw

Re: [PATCH v2 0/2] signalfd/epoll fixes

Quoted 7 lines · Linus Torvalds

Quoted 4 lines · Linus Torvalds
I'm *really* conflicted, because I have this really strong feeling that it's just papering over a symptom, and we damn well shouldn't be doing that.

Heh. This is even documented in the changelog.

Quoted 3 lines

I really think that what we really should do is allow
Quoted 2 lines
"poll()" to have a "poll_remove" callback (so each "add_poll_wait()" will have a callback when it gets remove).

Yes. I also thought about this. In fact I think this probably makes sense even ignoring epoll problems. Although I was thinking about file_operations::poll_v2(file, poll_table, poll_table_entry, mode) which could could replace the old ->poll() eventually. But perhaps the explicit poll_rm() is better.

However, speaking about epoll/sigfd, imho this hides the problem too.

Quoted 6 lines

fs/signalfd.c +6 -0

+static void signalfd_poll_rm(struct file *file, wait_queue_head_t *p)
+{
+	struct sighand_struct *sighand;
+
+	sighand = container_of(p, struct sighand_struct, signalfd_wqh);
+	__cleanup_sighand(sighand);

fs/signalfd.c static int signalfd_release(struct inode *inode, struct file *file) · line 57

Quoted 10 lines

fs/signalfd.c +4 -1

+}
+
 static unsigned int signalfd_poll(struct file *file, poll_table *wait)
 {
 	struct signalfd_ctx *ctx = file->private_data;
 	unsigned int events = 0;
 
-	poll_wait(file, &current->sighand->signalfd_wqh, wait);
+	if (poll_wait(file, &current->sighand->signalfd_wqh, wait))
+		atomic_inc(&current->sighand->count);

Yes, this fixes use-after-free. But suppose the the application does:

	sigfd = signalfd(...);
	epoll_ctl(epollfd, EPOLL_CTL_ADD, sigfd);
	execve();

de_thread() unshares ->sighand because of atomic_inc() above and epollfd no longer works after exec, and the application can't know this. (yes, currently we have the same problem with CLONE_SIGHAND'ed apps, but still).

We can turn ->signalfd_wqh into the pointer to the refcountable memory, or add another counter, but this is not nice.

And in any case. If the task exits, to me it looks simply pointless to keep its ->sighand until __fput(). This only makes poll_rm() or whatever safe, it can't report a signal. OK, OK, I am not arguing, POLLFREE is equally ugly or even worse.

I _think_ that the right fix should move wait_queue_head_t from sighand_struct to signalfd_ctx (file->private_data) somehow...

Oleg.

Linus Torvalds

2012-02-25 19:00 UTC #17 lore raw

Re: [PATCH v2 0/2] signalfd/epoll fixes

Quoted 6 lines · Oleg Nesterov

Yes, this fixes use-after-free. But suppose the the application does:
sigfd = signalfd(...);
epoll_ctl(epollfd, EPOLL_CTL_ADD, sigfd);
execve();

Well, that can't work anyway. We're adding to the wrong sighand.

Sure, we can force a wakeup for it at execve(), but that's not the old behavior either, so why would we care about the above case? It's not a sane thing to do, and it has never worked before either, so why bother? My patch should make it "work" as well as it ever did.

Quite frankly, if you wanted to make signalfd work sanely with eventpoll, you'd need to change the semantics entirely, and just say "signalfd works in the context it was creared in, no other". Those aren't the semantics we have, though, and there's no point in trying to make up some *new* semantics for "if you execve() we'll still follow it"

Quoted 4 lines

And in any case. If the task exits, to me it looks simply pointless to keep its ->sighand until __fput(). This only makes poll_rm() or whatever safe, it can't report a signal. OK, OK, I am not arguing, POLLFREE is equally ugly or even worse.

That's my point. POLLFREE really does look worse, and doesn't really give any saner behaviors.

Quoted 2 lines

I _think_ that the right fix should move wait_queue_head_t from sighand_struct to signalfd_ctx (file->private_data) somehow...

I looked at it - and it requires the same "poll_rm()" callback, and then instead you have to make allocations in poll() (for the list) and de-allocate in "poll_rm()".

I do agree that in many ways it would be the "right" thing to do, but it does require the same infrastructure, and the advantage is dubious for this case. The main advantage is that we can extend the signalfd + epoll interaction in new directions (like the "across execve" or other cases), but I'd argue that we don't *want* to do that.

So your POLLFREE patches are merged, and I think they work. The reason I like the poll_rm() thing is that I think it's conceptually the right solution, and while our "poll_wait()" interface has really worked very well, the fact that you cannot do anything stateful in it (because there is never any way to undo anything except for the "remove from wait queue" action) is very rigid and inflexible.

Of course, aside from signalfd, that inflexibility has never really mattered. So..

Linus