[PATCH] timerfd/eventfd context lock doesn't protect against poll_wait

[PATCH] timerfd/eventfd context lock doesn't protect against poll_wait

[Davi Arnaut]

Hi,

poll_wait() callback may modify the waitqueue without holding the
context private lock.

Signed-off-by: Davi E. M. Arnaut <davi@haxent.com.br>

diff --git a/fs/eventfd.c b/fs/eventfd.c
index 480e2b3..9c672be 100644
--- a/fs/eventfd.c
+++ b/fs/eventfd.c
@@ -50,7 +50,7 @@ int eventfd_signal(struct file *file, int n)
 		n = (int) (ULLONG_MAX - ctx->count);
 	ctx->count += n;
 	if (waitqueue_active(&ctx->wqh))
-		wake_up_locked(&ctx->wqh);
+		wake_up(&ctx->wqh);
 	spin_unlock_irqrestore(&ctx->lock, flags);
 
 	return n;
@@ -98,7 +98,7 @@ static ssize_t eventfd_read(struct file *file, char __user *buf, size_t count,
 	if (ucnt > 0)
 		res = sizeof(ucnt);
 	else if (!(file->f_flags & O_NONBLOCK)) {
-		__add_wait_queue(&ctx->wqh, &wait);
+		add_wait_queue(&ctx->wqh, &wait);
 		for (res = 0;;) {
 			set_current_state(TASK_INTERRUPTIBLE);
 			if (ctx->count > 0) {
@@ -114,13 +114,13 @@ static ssize_t eventfd_read(struct file *file, char __user *buf, size_t count,
 			schedule();
 			spin_lock_irq(&ctx->lock);
 		}
-		__remove_wait_queue(&ctx->wqh, &wait);
+		remove_wait_queue(&ctx->wqh, &wait);
 		__set_current_state(TASK_RUNNING);
 	}
 	if (res > 0) {
 		ctx->count = 0;
 		if (waitqueue_active(&ctx->wqh))
-			wake_up_locked(&ctx->wqh);
+			wake_up(&ctx->wqh);
 	}
 	spin_unlock_irq(&ctx->lock);
 	if (res > 0 && put_user(ucnt, (__u64 __user *) buf))
@@ -148,7 +148,7 @@ static ssize_t eventfd_write(struct file *file, const char __user *buf, size_t c
 	if (ULLONG_MAX - ctx->count > ucnt)
 		res = sizeof(ucnt);
 	else if (!(file->f_flags & O_NONBLOCK)) {
-		__add_wait_queue(&ctx->wqh, &wait);
+		add_wait_queue(&ctx->wqh, &wait);
 		for (res = 0;;) {
 			set_current_state(TASK_INTERRUPTIBLE);
 			if (ULLONG_MAX - ctx->count > ucnt) {
@@ -163,13 +163,13 @@ static ssize_t eventfd_write(struct file *file, const char __user *buf, size_t c
 			schedule();
 			spin_lock_irq(&ctx->lock);
 		}
-		__remove_wait_queue(&ctx->wqh, &wait);
+		remove_wait_queue(&ctx->wqh, &wait);
 		__set_current_state(TASK_RUNNING);
 	}
 	if (res > 0) {
 		ctx->count += ucnt;
 		if (waitqueue_active(&ctx->wqh))
-			wake_up_locked(&ctx->wqh);
+			wake_up(&ctx->wqh);
 	}
 	spin_unlock_irq(&ctx->lock);
 
diff --git a/fs/timerfd.c b/fs/timerfd.c
index e329e37..e48eae7 100644
--- a/fs/timerfd.c
+++ b/fs/timerfd.c
@@ -41,8 +41,8 @@ static enum hrtimer_restart timerfd_tmrproc(struct hrtimer *htmr)
 
 	spin_lock_irqsave(&ctx->lock, flags);
 	ctx->expired = 1;
-	wake_up_locked(&ctx->wqh);
 	spin_unlock_irqrestore(&ctx->lock, flags);
+	wake_up(&ctx->wqh);
 
 	return HRTIMER_NORESTART;
 }
@@ -104,7 +104,7 @@ static ssize_t timerfd_read(struct file *file, char __user *buf, size_t count,
 	spin_lock_irq(&ctx->lock);
 	res = -EAGAIN;
 	if (!ctx->expired && !(file->f_flags & O_NONBLOCK)) {
-		__add_wait_queue(&ctx->wqh, &wait);
+		add_wait_queue(&ctx->wqh, &wait);
 		for (res = 0;;) {
 			set_current_state(TASK_INTERRUPTIBLE);
 			if (ctx->expired) {
@@ -119,7 +119,7 @@ static ssize_t timerfd_read(struct file *file, char __user *buf, size_t count,
 			schedule();
 			spin_lock_irq(&ctx->lock);
 		}
-		__remove_wait_queue(&ctx->wqh, &wait);
+		remove_wait_queue(&ctx->wqh, &wait);
 		__set_current_state(TASK_RUNNING);
 	}
 	if (ctx->expired) {

Re: [PATCH] timerfd/eventfd context lock doesn't protect against poll_wait

[Davide Libenzi]

On Fri, 18 May 2007, Davi Arnaut wrote:

> Hi,
> 
> poll_wait() callback may modify the waitqueue without holding the
> context private lock.

Thx Davi, patch is correct. Nice catch. But at this point instead of 
ending up getting two locks, we may look into using Andrew suggestion of 
reusing the waitqueue lock. Is it universally considered a "legal" 
operation?


- Davide

Re: [PATCH] timerfd/eventfd context lock doesn't protect against poll_wait

[Andrew Morton]

On Thu, 17 May 2007 23:20:05 -0700 (PDT) Davide Libenzi <davidel@xmailserver.org> wrote:

> On Fri, 18 May 2007, Davi Arnaut wrote:
> 
> > Hi,
> > 
> > poll_wait() callback may modify the waitqueue without holding the
> > context private lock.
> 
> Thx Davi, patch is correct. Nice catch. But at this point instead of 
> ending up getting two locks, we may look into using Andrew suggestion of 
> reusing the waitqueue lock. Is it universally considered a "legal" 
> operation?
> 

I think it's a reasonable thing to do in core kernel code.  It'd be more
worrisome if it was done way down in some rarely-visited device driver.

btw, the code at present doesn't explicitly take care of WQ_FLAG_EXCLUSIVE.
I guess as you want wake-all behaviour that's OK, but it might be worth a
mention somewhrre.

hm, fs/signalfd.c is "Copyright (C) 2003 Linus Torvalds".  He was before
his time, that lad.

Re: [PATCH] timerfd/eventfd context lock doesn't protect against poll_wait

[Linus Torvalds]

On Thu, 17 May 2007, Davide Libenzi wrote:
> 
> Thx Davi, patch is correct. Nice catch. But at this point instead of 
> ending up getting two locks, we may look into using Andrew suggestion of 
> reusing the waitqueue lock. Is it universally considered a "legal" 
> operation?

Yes. Perhaps not for drivers etc, but stuff that is deeply involved in 
waitqueue handling anyway, sure.

		Linus