* [PATCH] fuse: freezing abort when use wait_event_killable{,_exclusive}().
@ 2016-12-05 3:19 cuilifei
2016-12-06 22:15 ` Rafael J. Wysocki
0 siblings, 1 reply; 6+ messages in thread
From: cuilifei @ 2016-12-05 3:19 UTC (permalink / raw)
To: linux-kernel
Cc: miklos, rjw, pavel, len.brown, linux-fsdevel, viro, piaoyingmin,
liucai, cuilifei
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@xiaomi.com>
---
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 */
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] fuse: freezing abort when use wait_event_killable{,_exclusive}().
2016-12-05 3:19 [PATCH] fuse: freezing abort when use wait_event_killable{,_exclusive}() cuilifei
@ 2016-12-06 22:15 ` Rafael J. Wysocki
2016-12-08 6:31 ` 答复: " 崔立飞
0 siblings, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2016-12-06 22:15 UTC (permalink / raw)
To: cuilifei
Cc: linux-kernel, miklos, pavel, len.brown, linux-fsdevel, viro,
piaoyingmin, liucai, Jiri Kosina
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@xiaomi.com>
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* 答复: [PATCH] fuse: freezing abort when use wait_event_killable{,_exclusive}().
2016-12-06 22:15 ` Rafael J. Wysocki
@ 2016-12-08 6:31 ` 崔立飞
2016-12-14 10:09 ` 崔立飞
2016-12-16 1:34 ` 崔立飞
0 siblings, 2 replies; 6+ messages in thread
From: 崔立飞 @ 2016-12-08 6:31 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-kernel, miklos, pavel, len.brown, linux-fsdevel, viro,
朴英敏, 刘才,
Jiri Kosina, 崔立飞
Hi Rafael,
The fuse we used is without the commit "fuse: don't mess with blocking signals" committed by Al Viro.
So we find the issue SIGBUS. In the page fault, trying to read page in fuse is interrupted,
which will lead to SIGBUS issue.
All Android platforms, include Android N, have the SIGUBS issue. All the SIGBUS issues are produced
in the freezing process.
In our Android platform, the root-cause of SIGBUS is :
suspend procedure will set the signal pending flag(TIF_SIGPENDING) and then wake up the task
which is TASK_INTERRUPTIBLE status . The request_wait_answer, which calls wait_event_interruptible,
is interrupted and the fuse request is still in pending status, which leads to SIGBUS sent to caller.
This issue can be reproduced by adding delays in funciton handle_read,
which is in the Android sdcard daemon process.
So we merge the commit "fuse: don't mess with blocking signals", which just use wait_event_killable{,_exclusive}().
It fix the SIGBUS issue. But the freezing will fail, because the freezing procedure cannot wake up the task which
is in TASK_KILLABLE(TASK_WAKEKILL | TASK_UNINTERRUPTIBLE) status. And the suspend procedure will abort
until the time is out.
In this patch, we try to fix the issue mentioned above.
> ---
> 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, \ //set the TASK_INTERRUPTIBLE
> + condition); \ //can be waked up by suspend procedure
> + else \
> + __ret = wait_event_interruptible(wq, \
> + condition); \
> + if (!__ret || fatal_signal_pending(current)) \ //check condition or there is fatal signal
> + break; \ //if true, wait finish
> + } while (try_to_freeze()); \ //if freezing is in effect, try to freeze the task
> + __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.
BR,
________________________________________
发件人: Rafael J. Wysocki <rjw@rjwysocki.net>
发送时间: 2016年12月7日 6:15
收件人: 崔立飞
抄送: linux-kernel@vger.kernel.org; miklos@szeredi.hu; pavel@ucw.cz; len.brown@intel.com; linux-fsdevel@vger.kernel.org; viro@zeniv.linux.org.uk; 朴英敏; 刘才; Jiri Kosina
主题: Re: [PATCH] fuse: freezing abort when use wait_event_killable{,_exclusive}().
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@xiaomi.com>
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* 答复: [PATCH] fuse: freezing abort when use wait_event_killable{,_exclusive}().
2016-12-08 6:31 ` 答复: " 崔立飞
@ 2016-12-14 10:09 ` 崔立飞
2016-12-15 22:40 ` Rafael J. Wysocki
2016-12-16 1:34 ` 崔立飞
1 sibling, 1 reply; 6+ messages in thread
From: 崔立飞 @ 2016-12-14 10:09 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-kernel, miklos, pavel, len.brown, linux-fsdevel, viro,
朴英敏, 刘才,
Jiri Kosina
Rafael,
Any questions about the patch, please let me know.
Thanks!
BR,
Cui Li Fei
________________________________________
发件人: 崔立飞
发送时间: 2016年12月8日 14:31
收件人: Rafael J. Wysocki
抄送: linux-kernel@vger.kernel.org; miklos@szeredi.hu; pavel@ucw.cz; len.brown@intel.com; linux-fsdevel@vger.kernel.org; viro@zeniv.linux.org.uk; 朴英敏; 刘才; Jiri Kosina; 崔立飞
主题: 答复: [PATCH] fuse: freezing abort when use wait_event_killable{,_exclusive}().
Hi Rafael,
The fuse we used is without the commit "fuse: don't mess with blocking signals" committed by Al Viro.
So we find the issue SIGBUS. In the page fault, trying to read page in fuse is interrupted,
which will lead to SIGBUS issue.
All Android platforms, include Android N, have the SIGUBS issue. All the SIGBUS issues are produced
in the freezing process.
In our Android platform, the root-cause of SIGBUS is :
suspend procedure will set the signal pending flag(TIF_SIGPENDING) and then wake up the task
which is TASK_INTERRUPTIBLE status . The request_wait_answer, which calls wait_event_interruptible,
is interrupted and the fuse request is still in pending status, which leads to SIGBUS sent to caller.
This issue can be reproduced by adding delays in funciton handle_read,
which is in the Android sdcard daemon process.
So we merge the commit "fuse: don't mess with blocking signals", which just use wait_event_killable{,_exclusive}().
It fix the SIGBUS issue. But the freezing will fail, because the freezing procedure cannot wake up the task which
is in TASK_KILLABLE(TASK_WAKEKILL | TASK_UNINTERRUPTIBLE) status. And the suspend procedure will abort
until the time is out.
In this patch, we try to fix the issue mentioned above.
> ---
> 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, \ //set the TASK_INTERRUPTIBLE
> + condition); \ //can be waked up by suspend procedure
> + else \
> + __ret = wait_event_interruptible(wq, \
> + condition); \
> + if (!__ret || fatal_signal_pending(current)) \ //check condition or there is fatal signal
> + break; \ //if true, wait finish
> + } while (try_to_freeze()); \ //if freezing is in effect, try to freeze the task
> + __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.
BR,
________________________________________
发件人: Rafael J. Wysocki <rjw@rjwysocki.net>
发送时间: 2016年12月7日 6:15
收件人: 崔立飞
抄送: linux-kernel@vger.kernel.org; miklos@szeredi.hu; pavel@ucw.cz; len.brown@intel.com; linux-fsdevel@vger.kernel.org; viro@zeniv.linux.org.uk; 朴英敏; 刘才; Jiri Kosina
主题: Re: [PATCH] fuse: freezing abort when use wait_event_killable{,_exclusive}().
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@xiaomi.com>
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: 答复: [PATCH] fuse: freezing abort when use wait_event_killable{,_exclusive}().
2016-12-14 10:09 ` 崔立飞
@ 2016-12-15 22:40 ` Rafael J. Wysocki
0 siblings, 0 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2016-12-15 22:40 UTC (permalink / raw)
To: 崔立飞
Cc: linux-kernel, miklos, pavel, len.brown, linux-fsdevel, viro,
朴英敏, 刘才,
Jiri Kosina
On Wednesday, December 14, 2016 10:09:25 AM 崔立飞 wrote:
> Rafael,
>
> Any questions about the patch, please let me know.
To be honest, I don't particularly like it, but I wonder what FUSE maintainers
think about it.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 6+ messages in thread
* 答复: [PATCH] fuse: freezing abort when use wait_event_killable{,_exclusive}().
2016-12-08 6:31 ` 答复: " 崔立飞
2016-12-14 10:09 ` 崔立飞
@ 2016-12-16 1:34 ` 崔立飞
1 sibling, 0 replies; 6+ messages in thread
From: 崔立飞 @ 2016-12-16 1:34 UTC (permalink / raw)
To: miklos
Cc: linux-kernel, pavel, len.brown, linux-fsdevel, viro,
朴英敏, 刘才,
Jiri Kosina, rjw
Hi Miklos,
Please help to review the patch.
Thanks.
________________________________________
发件人: 崔立飞
发送时间: 2016年12月8日 14:31
收件人: Rafael J. Wysocki
抄送: linux-kernel@vger.kernel.org; miklos@szeredi.hu; pavel@ucw.cz; len.brown@intel.com; linux-fsdevel@vger.kernel.org; viro@zeniv.linux.org.uk; 朴英敏; 刘才; Jiri Kosina; 崔立飞
主题: 答复: [PATCH] fuse: freezing abort when use wait_event_killable{,_exclusive}().
Hi Rafael,
The fuse we used is without the commit "fuse: don't mess with blocking signals" committed by Al Viro.
So we find the issue SIGBUS. In the page fault, trying to read page in fuse is interrupted,
which will lead to SIGBUS issue.
All Android platforms, include Android N, have the SIGUBS issue. All the SIGBUS issues are produced
in the freezing process.
In our Android platform, the root-cause of SIGBUS is :
suspend procedure will set the signal pending flag(TIF_SIGPENDING) and then wake up the task
which is TASK_INTERRUPTIBLE status . The request_wait_answer, which calls wait_event_interruptible,
is interrupted and the fuse request is still in pending status, which leads to SIGBUS sent to caller.
This issue can be reproduced by adding delays in funciton handle_read,
which is in the Android sdcard daemon process.
So we merge the commit "fuse: don't mess with blocking signals", which just use wait_event_killable{,_exclusive}().
It fix the SIGBUS issue. But the freezing will fail, because the freezing procedure cannot wake up the task which
is in TASK_KILLABLE(TASK_WAKEKILL | TASK_UNINTERRUPTIBLE) status. And the suspend procedure will abort
until the time is out.
In this patch, we try to fix the issue mentioned above.
> ---
> 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, \ //set the TASK_INTERRUPTIBLE
> + condition); \ //can be waked up by suspend procedure
> + else \
> + __ret = wait_event_interruptible(wq, \
> + condition); \
> + if (!__ret || fatal_signal_pending(current)) \ //check condition or there is fatal signal
> + break; \ //if true, wait finish
> + } while (try_to_freeze()); \ //if freezing is in effect, try to freeze the task
> + __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.
BR,
________________________________________
发件人: Rafael J. Wysocki <rjw@rjwysocki.net>
发送时间: 2016年12月7日 6:15
收件人: 崔立飞
抄送: linux-kernel@vger.kernel.org; miklos@szeredi.hu; pavel@ucw.cz; len.brown@intel.com; linux-fsdevel@vger.kernel.org; viro@zeniv.linux.org.uk; 朴英敏; 刘才; Jiri Kosina
主题: Re: [PATCH] fuse: freezing abort when use wait_event_killable{,_exclusive}().
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@xiaomi.com>
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-12-16 1:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-05 3:19 [PATCH] fuse: freezing abort when use wait_event_killable{,_exclusive}() cuilifei
2016-12-06 22:15 ` Rafael J. Wysocki
2016-12-08 6:31 ` 答复: " 崔立飞
2016-12-14 10:09 ` 崔立飞
2016-12-15 22:40 ` Rafael J. Wysocki
2016-12-16 1:34 ` 崔立飞
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.