Re: [RFC][PATCH -mm 3/3] freezer: Fix problem with kthread_stop

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Monday, 23 April 2007 14:35, Gautham R Shenoy wrote:
> On Sun, Apr 22, 2007 at 09:40:59PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Fix the problem with kthread_stop() that causes the freezer to fail if a
> > freezable thread is attempting to stop a frozen one and that may cause the
> > freezer to fail if the thread being stopped is freezable and
> > try_to_freeze_tasks() is running concurrently with kthread_stop().
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  kernel/kthread.c |    9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > Index: linux-2.6.21-rc6-mm1/kernel/kthread.c
> > ===================================================================
> > --- linux-2.6.21-rc6-mm1.orig/kernel/kthread.c	2007-04-09 15:23:48.000000000 +0200
> > +++ linux-2.6.21-rc6-mm1/kernel/kthread.c	2007-04-22 19:05:29.000000000 +0200
> > @@ -13,6 +13,7 @@
> >  #include <linux/file.h>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> > +#include <linux/freezer.h>
> >  #include <asm/semaphore.h>
> > 
> >  /*
> > @@ -232,6 +233,14 @@ int kthread_stop(struct task_struct *k)
> > 
> >  	/* Now set kthread_should_stop() to true, and wake it up. */
> >  	kthread_stop_info.k = k;
> > +	if (!freezer_should_exempt(current)) {
> > +		/* We are freezable, so we must make sure that the thread being
> > +		 * stopped is not frozen and will not be frozen until it dies
> > +		 */
> > +		freezer_exempt(k);
> > +		if (frozen(k))
> > +			clear_frozen_flag(k);
> > +	}
> 
> I'm trying hard to convince myself that this will work. May be I am
> missing something here, but I find a potential race window (very small though) 
> when k is entering the refrigerator.
> 
> Here's how.
> 
> kthread_stop(k)					k->refrigerator()
> ---------------------------------------------------------------------
> 						task_lock(k);
> 						1) check_if_exempted();
> 						/* not exempted. So 
> 						 * we will freeze
> 						 * ourself.
> 						 */
> 2) freezer_exempt(k);
> 
> 3) if(frozen(k))
> /* No, we're not yet frozen. So we
>  * don't clear_frozen_flag(k) here
>  */
> 						4) frozen_process(k);
> 						   task_unlock(k);
> 						
> 						5) for(;;) {
> 						 set_current_state(UNINTERRUPTIBLE);
> 						  if(!frozen_process(k))
> 						  /* k is frozen. We
> 						   * don't break :( 
> 						   */
> 						 
> 						  	schedule();
> 						}
> 						
> >  	wake_up_process(k);
> >  	put_task_struct(k);
> > 
> 
> Thus the freezer can still fail, no?
> IMO, we need the to take the task_lock for k here. Something like
> 
> > +	if (!freezer_should_exempt(current)) {
> 		task_lock(k);
> > +		/* We are freezable, so we must make sure that the thread being
> > +		 * stopped is not frozen and will not be frozen until it dies
> > +		 */
> > +		freezer_exempt(k);
> > +		if (frozen(k))
> > +			clear_frozen_flag(k);
> 		task_unlock(k);
> > +	}

Yes, that's correct.  We need to take task_lock() to avoid the race with
refrigerator().

I'll fix the patch.

BTW, I think I should rediff the entire series against -mm with your patch from
http://lkml.org/lkml/2007/4/23/98 applied.

Greetings,
Rafael