Re: [PATCH] fuse: freezing abort when use wait_event_killable{,_exclusive}().

[Rafael J. Wysocki]

On Monday, December 05, 2016 11:19:45 AM cuilifei wrote:
> Freezing process can abort when a client is waiting uninterruptibly
> for a response. Add new macro wait_fatal_freezable to try to fix it.
> 
> Signed-off-by: cuilifei <cuilifei@xxxxxxxxxx>

I'm not a big fan of this to be honest.

Do we really want to suspend the system (or freeze tasks for other reasons)
if that happens?

> ---
>  fs/fuse/dev.c           | 30 ++++++++++++++++++++++++++----
>  include/linux/freezer.h | 26 ++++++++++++++++++++++++++
>  2 files changed, 52 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 70ea57c..e33a081 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -19,6 +19,7 @@
>  #include <linux/pipe_fs_i.h>
>  #include <linux/swap.h>
>  #include <linux/splice.h>
> +#include <linux/freezer.h>
>  
>  MODULE_ALIAS_MISCDEV(FUSE_MINOR);
>  MODULE_ALIAS("devname:fuse");
> @@ -99,6 +100,19 @@ void fuse_request_free(struct fuse_req *req)
>  	kmem_cache_free(fuse_req_cachep, req);
>  }
>  
> +static void block_sigs(sigset_t *oldset)
> +{
> +	sigset_t mask;
> +
> +	siginitsetinv(&mask, sigmask(SIGKILL));
> +	sigprocmask(SIG_BLOCK, &mask, oldset);
> +}
> +
> +static void restore_sigs(sigset_t *oldset)
> +{
> +	sigprocmask(SIG_SETMASK, oldset, NULL);
> +}
> +
>  void __fuse_get_request(struct fuse_req *req)
>  {
>  	atomic_inc(&req->count);
> @@ -134,13 +148,18 @@ static struct fuse_req *__fuse_get_req(struct fuse_conn *fc, unsigned npages,
>  				       bool for_background)
>  {
>  	struct fuse_req *req;
> +	sigset_t oldset;
> +	int intr;
>  	int err;
>  	atomic_inc(&fc->num_waiting);
>  
>  	if (fuse_block_alloc(fc, for_background)) {
>  		err = -EINTR;
> -		if (wait_event_killable_exclusive(fc->blocked_waitq,
> -				!fuse_block_alloc(fc, for_background)))
> +		block_sigs(&oldset);
> +		intr = wait_fatal_freezable(fc->blocked_waitq,
> +				!fuse_block_alloc(fc, for_background), true);
> +		restore_sigs(&oldset);
> +		if (intr)
>  			goto out;
>  	}
>  	/* Matches smp_wmb() in fuse_set_initialized() */
> @@ -427,9 +446,12 @@ static void request_wait_answer(struct fuse_conn *fc, struct fuse_req *req)
>  	}
>  
>  	if (!test_bit(FR_FORCE, &req->flags)) {
> +		sigset_t oldset;
>  		/* Only fatal signals may interrupt this */
> -		err = wait_event_killable(req->waitq,
> -					test_bit(FR_FINISHED, &req->flags));
> +		block_sigs(&oldset);
> +		err = wait_fatal_freezable(req->waitq,
> +				test_bit(FR_FINISHED, &req->flags), false);
> +		restore_sigs(&oldset);
>  		if (!err)
>  			return;
>  
> diff --git a/include/linux/freezer.h b/include/linux/freezer.h
> index dd03e83..2504cd0 100644
> --- a/include/linux/freezer.h
> +++ b/include/linux/freezer.h
> @@ -256,6 +256,22 @@ static inline int freezable_schedule_hrtimeout_range(ktime_t *expires,
>  	__retval;							\
>  })
>  
> +#define wait_fatal_freezable(wq, condition, exclusive)			\
> +({									\
> +	int __ret = 0;							\
> +	do {								\
> +		if (exclusive)						\
> +			__ret = wait_event_interruptible_exclusive(wq,	\
> +							condition);	\
> +		else							\
> +			__ret = wait_event_interruptible(wq,		\
> +							condition);	\
> +		if (!__ret || fatal_signal_pending(current))		\
> +			break;						\
> +	} while (try_to_freeze());					\
> +	__ret;								\
> +})
> +
>  #else /* !CONFIG_FREEZER */
>  static inline bool frozen(struct task_struct *p) { return false; }
>  static inline bool freezing(struct task_struct *p) { return false; }
> @@ -296,6 +312,16 @@ static inline void set_freezable(void) {}
>  #define wait_event_freezekillable_unsafe(wq, condition)			\
>  		wait_event_killable(wq, condition)
>  
> +#define wait_fatal_freezable(wq, condition, exclusive)			\
> +({									\
> +	int __ret = 0;							\
> +	if (exclusive)							\
> +		__ret = wait_event_killable_exclusive(wq, condition);	\
> +	else								\
> +		__ret = wait_event_killable(wq,	condition);		\
> +	__ret;								\
> +})
> +
>  #endif /* !CONFIG_FREEZER */
>  
>  #endif	/* FREEZER_H_INCLUDED */
> 

Thanks,
Rafael