Re: please revert kthread from loop.c

[Serge E. Hallyn]

Quoting Andrew Morton (akpm@xxxxxxxx):
> On Tue, 11 Jul 2006 22:26:47 -0500
> "Serge E. Hallyn" <serue@xxxxxxxxxx> 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 <serue@xxxxxxxxxx>

---

 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/