Quoting Andrew Morton ([email protected]):
> On Tue, 11 Jul 2006 22:26:47 -0500
> "Serge E. Hallyn" <[email protected]> wrote:
>
> > > If so, this should plug it. The same race is not possible against the
> > > loop_set_fd() wakeup because the thread isn't running at that stage, yes?
> >
> > Right, it's not yet running at loop_set_fd(). However what about
> > kthread_stop() called from loop_clr_fd()? Unfortunately fixing
> > that seems hairy. Need to think about it...
>
> Yes, there does seem to be a little race there.
>
> I think it would be sufficient to do
>
>
> diff -puN drivers/block/loop.c~a drivers/block/loop.c
> --- a/drivers/block/loop.c~a
> +++ a/drivers/block/loop.c
> @@ -602,7 +602,8 @@ static int loop_thread(void *data)
> }
> __set_current_state(TASK_INTERRUPTIBLE);
> spin_unlock_irq(&lo->lo_lock);
> - schedule();
> + if (lo->state != Lo_rundown)
> + schedule();
> }
>
> return 0;
> @@ -888,12 +889,11 @@ static int loop_clr_fd(struct loop_devic
> if (filp == NULL)
> return -EINVAL;
>
> + kthread_stop(lo->lo_thread);
> spin_lock_irq(&lo->lo_lock);
> lo->lo_state = Lo_rundown;
> spin_unlock_irq(&lo->lo_lock);
>
> - kthread_stop(lo->lo_thread);
> -
> lo->lo_backing_file = NULL;
>
> loop_release_xfer(lo);
> _
>
> where the tweak to loop_clr_fd() is just there to prevent loop_thread()
> from going into a very brief busyloop.
Why does this fix the problem? Can't the wake_up_process() in
kthread_stop() still happen right before loop_thread's schedule()?
This also means that after loop_thread() has decided to stop,
make_request() has a chance to make a few more requests. It
will see lo->lo_state as bound, assume all is well, but when it goes to
wake_up_thread(), the thread will have been put_task_struct()d.
If I'm not entirely wrong above, how about the following alternate fix?
Unfortunately I guess it doesn't stop the brief busyloop...
> I'm not sure why it's all so tricky in there, really. Loop is doing a
> pretty conventional stop, wakeup, stick-things-on-lists operation and we do
> that all over the kernel using pretty well-understood idioms. But for some
> reason, loop is all difficult about it. I wonder why. hm.
Perhaps I should give completions another go.
thanks,
-serge
Subject: [PATCH 3/3] kthread: fix loop.c race at thread stop
The wake_up_process() from kthread_stop() could happen
between loop_thread's __set_current_state(TASK_INTERRUPTIBLE)
and schedule(). But we can't put kthread_stop() under the
spin_lock like we did the wake_up_process() in make_request().
So turn the thread stopping into a two-phase process. Do
a wake_up_process() under spin_lock after setting the
lo_state to Lo_rundown, after which the loop_thread no
long sleeps.
Signed-off-by: Serge Hallyn <[email protected]>
---
drivers/block/loop.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
e972f09b6ca27a7ac3421ab49bde6dba33fca62c
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index f944536..df38e05 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -600,9 +600,13 @@ static int loop_thread(void *data)
spin_unlock_irq(&lo->lo_lock);
break;
}
- __set_current_state(TASK_INTERRUPTIBLE);
+ if (lo->lo_state != Lo_rundown)
+ __set_current_state(TASK_INTERRUPTIBLE);
spin_unlock_irq(&lo->lo_lock);
- schedule();
+ if (lo->lo_state != Lo_rundown)
+ schedule();
+ else
+ __set_current_state(TASK_UNINTERRUPTIBLE);
}
return 0;
@@ -896,6 +900,7 @@ static int loop_clr_fd(struct loop_devic
spin_lock_irq(&lo->lo_lock);
lo->lo_state = Lo_rundown;
+ wake_up_process(lo->lo_thread);
spin_unlock_irq(&lo->lo_lock);
kthread_stop(lo->lo_thread);
--
1.1.6
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
