[Devel,2/2] fuse: handle only fatal signals while waiting request answer

Submitted by Stanislav Kinsburskiy on Oct. 13, 2016, 10:03 a.m.

Details

Message ID 20161013100345.27083.34082.stgit@localhost.localdomain
State New
Series "fuse: fix signals handling while processing request"
Headers show

Commit Message

Stanislav Kinsburskiy Oct. 13, 2016, 10:03 a.m.
This patch is backport of 7d3a07fcb8a0d5c06718de14fb91fdf1ef20a0e2 commit.
Although this commit loks more like cleanup, it's not.
It fixes the issue with signals, which can lead to abort of page read in fuse,
called from page fault, immediatly leading to SIGBUS sent to the caller.
The issue is in block_sigs implementation: it doesn't drop pending signal
flag, which interrupts wait_event_interruptible in request_wait_answer, while
it's supposed to be interrupted by SIGKILL only.
IOW, any signal, arrived to the process, which does page fault handling on
fuse file, _before_ request_wait_answer is called, will lead to request
interruption, producing SIGBUS error in page fault handler (filemap_fault).

https://jira.sw.ru/browse/PSBM-53581

Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
---
 fs/fuse/dev.c |   42 ++++++++++++++++--------------------------
 1 file changed, 16 insertions(+), 26 deletions(-)

Patch hide | download patch | download mbox

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index a0d0b1a..0b6d000 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -99,19 +99,6 @@  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);
@@ -144,15 +131,9 @@  static struct fuse_req *__fuse_get_req(struct fuse_conn *fc, unsigned npages,
 	atomic_inc(&fc->num_waiting);
 
 	if (fuse_block_alloc(fc, for_background)) {
-		sigset_t oldset;
-		int intr;
-
-		block_sigs(&oldset);
-		intr = wait_event_interruptible_exclusive(fc->blocked_waitq,
-				!fuse_block_alloc(fc, for_background));
-		restore_sigs(&oldset);
 		err = -EINTR;
-		if (intr)
+		if (wait_event_killable_exclusive(fc->blocked_waitq,
+				!fuse_block_alloc(fc, for_background)))
 			goto out;
 	}
 
@@ -412,6 +393,19 @@  __acquires(fc->lock)
 	spin_lock(&fc->lock);
 }
 
+static void wait_answer_killable(struct fuse_conn *fc,
+				 struct fuse_req *req)
+__releases(fc->lock)
+__acquires(fc->lock)
+{
+	if (fatal_signal_pending(current))
+		return;
+
+	spin_unlock(&fc->lock);
+	wait_event_killable(req->waitq, req->state == FUSE_REQ_FINISHED);
+	spin_lock(&fc->lock);
+}
+
 static void queue_interrupt(struct fuse_conn *fc, struct fuse_req *req)
 {
 	list_add_tail(&req->intr_entry, &fc->interrupts);
@@ -438,12 +432,8 @@  __acquires(fc->lock)
 	}
 
 	if (!req->force) {
-		sigset_t oldset;
-
 		/* Only fatal signals may interrupt this */
-		block_sigs(&oldset);
-		wait_answer_interruptible(fc, req);
-		restore_sigs(&oldset);
+		wait_answer_killable(fc, req);
 
 		if (req->aborted)
 			goto aborted;

Comments

Stanislav Kinsburskiy Oct. 17, 2016, 12:01 p.m.
Kostya, please, modify patch comment with the following substitution:


s/
it doesn't drop pending signal flag/
it doesn't drop pending signal flag, if processes is traced/
g

Thanks.

13.10.2016 12:03, Stanislav Kinsburskiy пишет:
> This patch is backport of 7d3a07fcb8a0d5c06718de14fb91fdf1ef20a0e2 commit.
> Although this commit loks more like cleanup, it's not.
> It fixes the issue with signals, which can lead to abort of page read in fuse,
> called from page fault, immediatly leading to SIGBUS sent to the caller.
> The issue is in block_sigs implementation: it doesn't drop pending signal
> flag, which interrupts wait_event_interruptible in request_wait_answer, while
> it's supposed to be interrupted by SIGKILL only.
> IOW, any signal, arrived to the process, which does page fault handling on
> fuse file, _before_ request_wait_answer is called, will lead to request
> interruption, producing SIGBUS error in page fault handler (filemap_fault).
>
> https://jira.sw.ru/browse/PSBM-53581
>
> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
> ---
>   fs/fuse/dev.c |   42 ++++++++++++++++--------------------------
>   1 file changed, 16 insertions(+), 26 deletions(-)
>
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index a0d0b1a..0b6d000 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -99,19 +99,6 @@ 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);
> @@ -144,15 +131,9 @@ static struct fuse_req *__fuse_get_req(struct fuse_conn *fc, unsigned npages,
>   	atomic_inc(&fc->num_waiting);
>   
>   	if (fuse_block_alloc(fc, for_background)) {
> -		sigset_t oldset;
> -		int intr;
> -
> -		block_sigs(&oldset);
> -		intr = wait_event_interruptible_exclusive(fc->blocked_waitq,
> -				!fuse_block_alloc(fc, for_background));
> -		restore_sigs(&oldset);
>   		err = -EINTR;
> -		if (intr)
> +		if (wait_event_killable_exclusive(fc->blocked_waitq,
> +				!fuse_block_alloc(fc, for_background)))
>   			goto out;
>   	}
>   
> @@ -412,6 +393,19 @@ __acquires(fc->lock)
>   	spin_lock(&fc->lock);
>   }
>   
> +static void wait_answer_killable(struct fuse_conn *fc,
> +				 struct fuse_req *req)
> +__releases(fc->lock)
> +__acquires(fc->lock)
> +{
> +	if (fatal_signal_pending(current))
> +		return;
> +
> +	spin_unlock(&fc->lock);
> +	wait_event_killable(req->waitq, req->state == FUSE_REQ_FINISHED);
> +	spin_lock(&fc->lock);
> +}
> +
>   static void queue_interrupt(struct fuse_conn *fc, struct fuse_req *req)
>   {
>   	list_add_tail(&req->intr_entry, &fc->interrupts);
> @@ -438,12 +432,8 @@ __acquires(fc->lock)
>   	}
>   
>   	if (!req->force) {
> -		sigset_t oldset;
> -
>   		/* Only fatal signals may interrupt this */
> -		block_sigs(&oldset);
> -		wait_answer_interruptible(fc, req);
> -		restore_sigs(&oldset);
> +		wait_answer_killable(fc, req);
>   
>   		if (req->aborted)
>   			goto aborted;
>
> _______________________________________________
> Devel mailing list
> Devel@openvz.org
> https://lists.openvz.org/mailman/listinfo/devel
Konstantin Khorenko Oct. 17, 2016, 3:01 p.m.
On 10/17/2016 03:01 PM, Stanislav Kinsburskiy wrote:
> Kostya, please, modify patch comment with the following substitution:
>
>
> s/
> it doesn't drop pending signal flag/
> it doesn't drop pending signal flag, if processes is traced/
> g

done, thank you.

>
> Thanks.
>
> 13.10.2016 12:03, Stanislav Kinsburskiy пишет:
>> This patch is backport of 7d3a07fcb8a0d5c06718de14fb91fdf1ef20a0e2 commit.
>> Although this commit loks more like cleanup, it's not.
>> It fixes the issue with signals, which can lead to abort of page read in fuse,
>> called from page fault, immediatly leading to SIGBUS sent to the caller.
>> The issue is in block_sigs implementation: it doesn't drop pending signal
>> flag, which interrupts wait_event_interruptible in request_wait_answer, while
>> it's supposed to be interrupted by SIGKILL only.
>> IOW, any signal, arrived to the process, which does page fault handling on
>> fuse file, _before_ request_wait_answer is called, will lead to request
>> interruption, producing SIGBUS error in page fault handler (filemap_fault).
>>
>> https://jira.sw.ru/browse/PSBM-53581
>>
>> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
>> ---
>>   fs/fuse/dev.c |   42 ++++++++++++++++--------------------------
>>   1 file changed, 16 insertions(+), 26 deletions(-)
>>
>> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
>> index a0d0b1a..0b6d000 100644
>> --- a/fs/fuse/dev.c
>> +++ b/fs/fuse/dev.c
>> @@ -99,19 +99,6 @@ 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);
>> @@ -144,15 +131,9 @@ static struct fuse_req *__fuse_get_req(struct fuse_conn *fc, unsigned npages,
>>   	atomic_inc(&fc->num_waiting);
>>
>>   	if (fuse_block_alloc(fc, for_background)) {
>> -		sigset_t oldset;
>> -		int intr;
>> -
>> -		block_sigs(&oldset);
>> -		intr = wait_event_interruptible_exclusive(fc->blocked_waitq,
>> -				!fuse_block_alloc(fc, for_background));
>> -		restore_sigs(&oldset);
>>   		err = -EINTR;
>> -		if (intr)
>> +		if (wait_event_killable_exclusive(fc->blocked_waitq,
>> +				!fuse_block_alloc(fc, for_background)))
>>   			goto out;
>>   	}
>>
>> @@ -412,6 +393,19 @@ __acquires(fc->lock)
>>   	spin_lock(&fc->lock);
>>   }
>>
>> +static void wait_answer_killable(struct fuse_conn *fc,
>> +				 struct fuse_req *req)
>> +__releases(fc->lock)
>> +__acquires(fc->lock)
>> +{
>> +	if (fatal_signal_pending(current))
>> +		return;
>> +
>> +	spin_unlock(&fc->lock);
>> +	wait_event_killable(req->waitq, req->state == FUSE_REQ_FINISHED);
>> +	spin_lock(&fc->lock);
>> +}
>> +
>>   static void queue_interrupt(struct fuse_conn *fc, struct fuse_req *req)
>>   {
>>   	list_add_tail(&req->intr_entry, &fc->interrupts);
>> @@ -438,12 +432,8 @@ __acquires(fc->lock)
>>   	}
>>
>>   	if (!req->force) {
>> -		sigset_t oldset;
>> -
>>   		/* Only fatal signals may interrupt this */
>> -		block_sigs(&oldset);
>> -		wait_answer_interruptible(fc, req);
>> -		restore_sigs(&oldset);
>> +		wait_answer_killable(fc, req);
>>
>>   		if (req->aborted)
>>   			goto aborted;
>>
>> _______________________________________________
>> Devel mailing list
>> Devel@openvz.org
>> https://lists.openvz.org/mailman/listinfo/devel
>
> _______________________________________________
> Devel mailing list
> Devel@openvz.org
> https://lists.openvz.org/mailman/listinfo/devel
>