timerfd/eventfd context lock doesn't protect against poll_wait
Subsystems (2)
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
@@ -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 cif (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 cschedule ();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
@@ -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) {
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
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.
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