fs/fuse kio: fix stack overrun in request_end()

Submitted by Kirill Tkhai on May 31, 2019, 9:16 a.m.

Details

Message ID 85136999-b28b-ecbf-be7e-76b55f1eac40@virtuozzo.com
State New
Series "fs/fuse kio: fix stack overrun in request_end()"
Headers show

Commit Message

Kirill Tkhai May 31, 2019, 9:16 a.m.
On 31.05.2019 11:39, Pavel Butsykin wrote:
> Unexpectedly, request_end() function turned out to be recursive, which leads to
> stack overrun:
> [76293.902783]  [<ffffffffc0faa4ff>] request_end+0x334/0x3cf [fuse]
> [76293.902789]  [<ffffffffc0fdc9d3>] pcs_fuse_submit+0x49d/0x4f5 [fuse_kio_pcs]
> [76293.902795]  [<ffffffffc0fbd775>] ? fuse_flush_writepages+0x8f/0x8f [fuse]
> [76293.902800]  [<ffffffffc0fdd745>] kpcs_req_send+0x210/0x3bb [fuse_kio_pcs]
> [76293.902805]  [<ffffffffc0faa17b>] flush_bg_queue+0x1ba/0x20a [fuse]
> [76293.902810]  [<ffffffffc0faa4ff>] request_end+0x334/0x3cf [fuse]
> [76293.902816]  [<ffffffffc0fdc9d3>] pcs_fuse_submit+0x49d/0x4f5 [fuse_kio_pcs]
> [76293.902818]  [<ffffffff8c57cdd0>] ? debug_check_no_locks_freed+0x2a0/0x2a0
> [76293.902824]  [<ffffffffc0fbd775>] ? fuse_flush_writepages+0x8f/0x8f [fuse]
> [76293.902829]  [<ffffffffc0fdd745>] kpcs_req_send+0x210/0x3bb [fuse_kio_pcs]
> [76293.902834]  [<ffffffffc0faa17b>] flush_bg_queue+0x1ba/0x20a [fuse]
> [76293.902839]  [<ffffffffc0faa4ff>] request_end+0x334/0x3cf [fuse]
> 
> To fix this, let's call request_end() inside kpcs_req_send() synchronously for
> background requests.

Oh. I'm not sure this complexity is worth its payload.

The problem is we call request_end() from flush_bg_queue().
Can't we simply prohibit to call flush_bg_queue() from request_end()?

Something like this:

Patch hide | download patch | download mbox

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index b8a524f04298..16143d05111c 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -434,7 +434,7 @@  static void flush_bg_queue(struct fuse_conn *fc, struct fuse_iqueue *fiq)
  * the 'end' callback is called if given, else the reference to the
  * request is released
  */
-void request_end(struct fuse_conn *fc, struct fuse_req *req)
+void __request_end(struct fuse_conn *fc, struct fuse_req *req, bool flush_bg)
 {
 	struct fuse_iqueue *fiq = req->fiq;
 	bool bg;
@@ -481,7 +481,8 @@  void request_end(struct fuse_conn *fc, struct fuse_req *req)
 		}
 		fc->num_background--;
 		fc->active_background--;
-		flush_bg_queue(fc, fiq);
+		if (flush_bg)
+			flush_bg_queue(fc, fiq);
 		spin_unlock(&fc->bg_lock);
 	}
 	if (req->end) {
@@ -493,7 +494,7 @@  void request_end(struct fuse_conn *fc, struct fuse_req *req)
 		wake_up(&req->waitq);
 	fuse_put_request(fc, req);
 }
-EXPORT_SYMBOL_GPL(request_end);
+EXPORT_SYMBOL_GPL(__request_end);
 
 static void queue_interrupt(struct fuse_iqueue *fiq, struct fuse_req *req)
 {
diff --git a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
index 4dad29418e0d..37387ee20523 100644
--- a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
+++ b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
@@ -1052,7 +1052,7 @@  static void pcs_fuse_submit(struct pcs_fuse_cluster *pfc, struct fuse_req *req,
 error:
 	DTRACE("do fuse_request_end req:%p op:%d err:%d\n", req, req->in.h.opcode, req->out.h.error);
 
-	request_end(pfc->fc, req);
+	__request_end(pfc->fc, req, false);
 	return;
 
 submit:
@@ -1249,7 +1249,7 @@  static int kpcs_req_classify(struct fuse_conn* fc, struct fuse_req *req,
 		req->out.h.error = ret;
 		if (lk)
 			spin_unlock(&fc->bg_lock);
-		request_end(fc, req);
+		__request_end(fc, req, false);
 		if (lk)
 			spin_lock(&fc->bg_lock);
 		return ret;

Comments

Pavel Butsykin May 31, 2019, 9:28 a.m.
On 31.05.2019 12:16, Kirill Tkhai wrote:
> On 31.05.2019 11:39, Pavel Butsykin wrote:
>> Unexpectedly, request_end() function turned out to be recursive, which leads to
>> stack overrun:
>> [76293.902783]  [<ffffffffc0faa4ff>] request_end+0x334/0x3cf [fuse]
>> [76293.902789]  [<ffffffffc0fdc9d3>] pcs_fuse_submit+0x49d/0x4f5 [fuse_kio_pcs]
>> [76293.902795]  [<ffffffffc0fbd775>] ? fuse_flush_writepages+0x8f/0x8f [fuse]
>> [76293.902800]  [<ffffffffc0fdd745>] kpcs_req_send+0x210/0x3bb [fuse_kio_pcs]
>> [76293.902805]  [<ffffffffc0faa17b>] flush_bg_queue+0x1ba/0x20a [fuse]
>> [76293.902810]  [<ffffffffc0faa4ff>] request_end+0x334/0x3cf [fuse]
>> [76293.902816]  [<ffffffffc0fdc9d3>] pcs_fuse_submit+0x49d/0x4f5 [fuse_kio_pcs]
>> [76293.902818]  [<ffffffff8c57cdd0>] ? debug_check_no_locks_freed+0x2a0/0x2a0
>> [76293.902824]  [<ffffffffc0fbd775>] ? fuse_flush_writepages+0x8f/0x8f [fuse]
>> [76293.902829]  [<ffffffffc0fdd745>] kpcs_req_send+0x210/0x3bb [fuse_kio_pcs]
>> [76293.902834]  [<ffffffffc0faa17b>] flush_bg_queue+0x1ba/0x20a [fuse]
>> [76293.902839]  [<ffffffffc0faa4ff>] request_end+0x334/0x3cf [fuse]
>>
>> To fix this, let's call request_end() inside kpcs_req_send() synchronously for
>> background requests.
> 
> Oh. I'm not sure this complexity is worth its payload.

I just wanted to avoid modification of fuse code. But probably the price is too
expensive.

> The problem is we call request_end() from flush_bg_queue().
> Can't we simply prohibit to call flush_bg_queue() from request_end()?
> 
> Something like this:
> 
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index b8a524f04298..16143d05111c 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -434,7 +434,7 @@ static void flush_bg_queue(struct fuse_conn *fc, struct fuse_iqueue *fiq)
>    * the 'end' callback is called if given, else the reference to the
>    * request is released
>    */
> -void request_end(struct fuse_conn *fc, struct fuse_req *req)
> +void __request_end(struct fuse_conn *fc, struct fuse_req *req, bool flush_bg)
>   {
>   	struct fuse_iqueue *fiq = req->fiq;
>   	bool bg;
> @@ -481,7 +481,8 @@ void request_end(struct fuse_conn *fc, struct fuse_req *req)
>   		}
>   		fc->num_background--;
>   		fc->active_background--;
> -		flush_bg_queue(fc, fiq);
> +		if (flush_bg)
> +			flush_bg_queue(fc, fiq);
>   		spin_unlock(&fc->bg_lock);
>   	}
>   	if (req->end) {
> @@ -493,7 +494,7 @@ void request_end(struct fuse_conn *fc, struct fuse_req *req)
>   		wake_up(&req->waitq);
>   	fuse_put_request(fc, req);
>   }
> -EXPORT_SYMBOL_GPL(request_end);
> +EXPORT_SYMBOL_GPL(__request_end);
>   
>   static void queue_interrupt(struct fuse_iqueue *fiq, struct fuse_req *req)
>   {
> diff --git a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
> index 4dad29418e0d..37387ee20523 100644
> --- a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
> +++ b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
> @@ -1052,7 +1052,7 @@ static void pcs_fuse_submit(struct pcs_fuse_cluster *pfc, struct fuse_req *req,
>   error:
>   	DTRACE("do fuse_request_end req:%p op:%d err:%d\n", req, req->in.h.opcode, req->out.h.error);
>   
> -	request_end(pfc->fc, req);
> +	__request_end(pfc->fc, req, false);
>   	return;
>   
>   submit:
> @@ -1249,7 +1249,7 @@ static int kpcs_req_classify(struct fuse_conn* fc, struct fuse_req *req,
>   		req->out.h.error = ret;
>   		if (lk)
>   			spin_unlock(&fc->bg_lock);
> -		request_end(fc, req);
> +		__request_end(fc, req, false);
>   		if (lk)
>   			spin_lock(&fc->bg_lock);
>   		return ret;
>
Kirill Tkhai May 31, 2019, 9:30 a.m.
On 31.05.2019 12:28, Pavel Butsykin wrote:
> 
> 
> On 31.05.2019 12:16, Kirill Tkhai wrote:
>> On 31.05.2019 11:39, Pavel Butsykin wrote:
>>> Unexpectedly, request_end() function turned out to be recursive, which leads to
>>> stack overrun:
>>> [76293.902783]  [<ffffffffc0faa4ff>] request_end+0x334/0x3cf [fuse]
>>> [76293.902789]  [<ffffffffc0fdc9d3>] pcs_fuse_submit+0x49d/0x4f5 [fuse_kio_pcs]
>>> [76293.902795]  [<ffffffffc0fbd775>] ? fuse_flush_writepages+0x8f/0x8f [fuse]
>>> [76293.902800]  [<ffffffffc0fdd745>] kpcs_req_send+0x210/0x3bb [fuse_kio_pcs]
>>> [76293.902805]  [<ffffffffc0faa17b>] flush_bg_queue+0x1ba/0x20a [fuse]
>>> [76293.902810]  [<ffffffffc0faa4ff>] request_end+0x334/0x3cf [fuse]
>>> [76293.902816]  [<ffffffffc0fdc9d3>] pcs_fuse_submit+0x49d/0x4f5 [fuse_kio_pcs]
>>> [76293.902818]  [<ffffffff8c57cdd0>] ? debug_check_no_locks_freed+0x2a0/0x2a0
>>> [76293.902824]  [<ffffffffc0fbd775>] ? fuse_flush_writepages+0x8f/0x8f [fuse]
>>> [76293.902829]  [<ffffffffc0fdd745>] kpcs_req_send+0x210/0x3bb [fuse_kio_pcs]
>>> [76293.902834]  [<ffffffffc0faa17b>] flush_bg_queue+0x1ba/0x20a [fuse]
>>> [76293.902839]  [<ffffffffc0faa4ff>] request_end+0x334/0x3cf [fuse]
>>>
>>> To fix this, let's call request_end() inside kpcs_req_send() synchronously for
>>> background requests.
>>
>> Oh. I'm not sure this complexity is worth its payload.
> 
> I just wanted to avoid modification of fuse code. But probably the price is too
> expensive.

The problem is this gives also synchronization complexity like we need to think
whether we should care about flushing pcs_dentry_info::work on inode destroy.

Kirill