Re: [PATCH pm] freezer: fix wait_event_freezable/__thaw_task races

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

On Tuesday, November 01, 2011, Tejun Heo wrote:
> From: Oleg Nesterov <oleg@redhat.com>
> 
> wait_event_freezable() and friends stop the waiting if try_to_freeze()
> fails. This is not right, we can race with __thaw_task() and in this
> case
> 
> 	- wait_event_freezable() returns the wrong ERESTARTSYS
> 
> 	- wait_event_freezable_timeout() can return the positive
> 	  value while condition == F
> 
> Change the code to always check __retval/condition before return.
> 
> Note: with or without this patch the timeout logic looks strange,
> probably we should recalc timeout if try_to_freeze() returns T.
> 
> tj: Updated to apply to wait_event_freezekillable() too.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Acked-by: Tejun Heo <tj@kernel.org>

Applied to linux-pm/linux-next.

Thanks,
Rafael


> ---
> 
> So, this should be the last of lost patches.  It's on top of
> 
> linus/master
> +[1] freezer: fix various bugs and simplify implementation, take#2
> +[2] usb_storage: don't use set_freezable_with_signal()" + [3]
> +[3] freezer: kill unused set_freezable_with_signal()
> 
> and available in the following git branch.
> 
>  git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git pm-fix-wait_freezable
> 
> Thank you.
> 
> [1] http://thread.gmane.org/gmane.linux.kernel/1209247
> [2] http://thread.gmane.org/gmane.linux.kernel/1209416
> [3] http://thread.gmane.org/gmane.linux.kernel/1209416/focus=1209417
> 
>  include/linux/freezer.h |   27 ++++++++++++++-------------
>  1 files changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/freezer.h b/include/linux/freezer.h
> index 5f32321..4a1d933 100644
> --- a/include/linux/freezer.h
> +++ b/include/linux/freezer.h
> @@ -113,42 +113,43 @@ static inline int freezer_should_skip(struct task_struct *p)
>  #define wait_event_freezekillable(wq, condition)			\
>  ({									\
>  	int __retval;							\
> -	do {								\
> +	for (;;) {							\
>  		__retval = wait_event_killable(wq,			\
>  				(condition) || freezing(current));	\
> -		if (__retval && !freezing(current))			\
> +		if (__retval || (condition))				\
>  			break;						\
> -		else if (!(condition))					\
> -			__retval = -ERESTARTSYS;			\
> -	} while (try_to_freeze());					\
> +		try_to_freeze();					\
> +	}								\
>  	__retval;							\
>  })
>  
>  #define wait_event_freezable(wq, condition)				\
>  ({									\
>  	int __retval;							\
> -	do {								\
> +	for (;;) {							\
>  		__retval = wait_event_interruptible(wq, 		\
>  				(condition) || freezing(current));	\
> -		if (__retval && !freezing(current))			\
> +		if (__retval || (condition))				\
>  			break;						\
> -		else if (!(condition))					\
> -			__retval = -ERESTARTSYS;			\
> -	} while (try_to_freeze());					\
> +		try_to_freeze();					\
> +	}								\
>  	__retval;							\
>  })
>  
> -
>  #define wait_event_freezable_timeout(wq, condition, timeout)		\
>  ({									\
>  	long __retval = timeout;					\
> -	do {								\
> +	for (;;) {							\
>  		__retval = wait_event_interruptible_timeout(wq,		\
>  				(condition) || freezing(current),	\
>  				__retval); 				\
> -	} while (try_to_freeze());					\
> +		if (__retval <= 0 || (condition))			\
> +			break;						\
> +		try_to_freeze();					\
> +	}								\
>  	__retval;							\
>  })
> +
>  #else /* !CONFIG_FREEZER */
>  static inline bool frozen(struct task_struct *p) { return false; }
>  static inline bool freezing(struct task_struct *p) { return false; }
>