Skip to contentSkip to conversation

timerfd/eventfd context lock doesn't protect against poll_wait

PATCH In review Davi Arnaut 4 participants 4 messages May 18, 2007

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

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

Hi,

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

fs/eventfd.c +7 -7
index 480e2b3..9c672be 100644
@@ -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);
 
fs/timerfd.c +3 -3
index e329e37..e48eae7 100644
@@ -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) {
#1 lore raw
Quoted 4 lines · Davi Arnaut

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

#2 lore raw
Quoted 4 lines · On Thu, 17 May 2007 23:20:05 -0700 (PDT) Davide Libenzi…
Quoted 2 lines · Davi Arnaut

Hi,

Quoted 8 lines
Quoted 2 lines

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.

#3 lore raw
In reply to Davide Libenzi
Quoted 5 lines · Davide Libenzi

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