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

From: åçé
Date: Thu Dec 15 2016 - 20:36:05 EST

Next message: Li, Liang Z: "RE: [Qemu-devel] [PATCH kernel v5 0/5] Extend virtio-balloon for fast (de)inflating & fast live migration"
Previous message: Xishi Qiu: "Re: [RFC PATCH] Memory hotplug support for arm64 platform"
In reply to: Rafael J. Wysocki: "Re: çå: [PATCH] fuse: freezing abort when use wait_event_killable{,_exclusive}()."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Miklos,

Please help to review the patch.
Thanks.

________________________________________
åää: åçé
åéæé: 2016å12æ8æ 14:31
æää: Rafael J. Wysocki
æé: linux-kernel@xxxxxxxxxxxxxxx; miklos@xxxxxxxxxx; pavel@xxxxxx; len.brown@xxxxxxxxx; linux-fsdevel@xxxxxxxxxxxxxxx; viro@xxxxxxxxxxxxxxxxxx; æèæ; åæ; 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@xxxxxxxxxxxxx>
åéæé: 2016å12æ7æ 6:15
æää: åçé
æé: linux-kernel@xxxxxxxxxxxxxxx; miklos@xxxxxxxxxx; pavel@xxxxxx; len.brown@xxxxxxxxx; linux-fsdevel@xxxxxxxxxxxxxxx; viro@xxxxxxxxxxxxxxxxxx; æèæ; åæ; 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@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

Next message: Li, Liang Z: "RE: [Qemu-devel] [PATCH kernel v5 0/5] Extend virtio-balloon for fast (de)inflating & fast live migration"
Previous message: Xishi Qiu: "Re: [RFC PATCH] Memory hotplug support for arm64 platform"
In reply to: Rafael J. Wysocki: "Re: çå: [PATCH] fuse: freezing abort when use wait_event_killable{,_exclusive}()."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]