Data Corruption bug with Samba's vfs_iouring and Linux 5.6.7/5.7rc3

[Stefan Metzmacher]

Am 08.05.20 um 20:53 schrieb Jeremy Allison via samba-technical:
> On Fri, May 08, 2020 at 09:35:40PM +0530, Anoop C S wrote:
>>
>> Jeremy's patch(for handling short reads) still works with updated
>> kernel(v5.6.10).
>>
>>> Can you please also test the patch I posted here:
>>> https://bugzilla.samba.org/show_bug.cgi?id=14361#c21
>>
>> Unfortunately copy failed to start with this patch set. I don't know if
>> its because of updated kernel(v5.6.8 -> v5.6.10) but smbd terminated
>> with SIGABRT right at the beginning of copy. I have attached backtrace
>> and log file. Please have a look.
> 
> I see the problem. It's the reason I initially introduced
> an immediate event, and then refactored my code to call
> _vfs_io_uring_queue_run() in a loop if it returned 'need_retry'.
> 
> Metze's current code is recursive, and it's running out
> of stack.
> 
> It uses:
> 
> vfs_io_uring_pread_send()
> 	->vfs_io_uring_pread_submit()  <-----------------------------------
> 		->vfs_io_uring_request_submit()                           |
> 			->vfs_io_uring_queue_run()                        |
>                                                                           |
> But inside vfs_io_uring_queue_run() once a read                           |
> completes it calls:                                                       |
>                                                                           |
> vfs_io_uring_queue_run()                                                  |
> 	->vfs_io_uring_finish_req()                                       |
> 		->cur->completion_fn()                                    |
>                                                                           |
> cur->completion_fn() for pread is set to vfs_io_uring_pread_completion()  |
>                                                                           |
> vfs_io_uring_pread_completion()                                           |
> 	->vfs_io_uring_pread_submit() -------------------------------------
> 
> (the ascii art will probably only work in fixed-font text mailers
> but you should get the idea).
> 
> I don't think this pattern will work. You have to exit
> from vfs_io_uring_queue_run() all the way out so that
> you've called io_uring_cq_advance() to move the completion
> queue forward before you can submit another request to
> finish the short IO.

Thanks very much Jeremy! I didn't noticed that.

I guess the attached patch should be able to fix the recursion.

metze

-------------- next part --------------
From a1819f8b03ca43c860bdff7b2bdde79674fe7cc4 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Fri, 8 May 2020 21:29:53 +0200
Subject: [PATCH] vfs_io_uring: avoid stack recursion of
 vfs_io_uring_queue_run()

Instead we remember if recursion was triggered and jump to
the start of the function again from the end.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=14361

Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
 source3/modules/vfs_io_uring.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/source3/modules/vfs_io_uring.c b/source3/modules/vfs_io_uring.c
index a7308ff9edd3..1340d3a5ca9e 100644
--- a/source3/modules/vfs_io_uring.c
+++ b/source3/modules/vfs_io_uring.c
@@ -35,6 +35,8 @@ struct vfs_io_uring_request;
 struct vfs_io_uring_config {
 	struct io_uring uring;
 	struct tevent_fd *fde;
+	bool busy;
+	bool need_retry;
 	struct vfs_io_uring_request *queue;
 	struct vfs_io_uring_request *pending;
 	bool nowait_pread;
@@ -253,13 +255,22 @@ static void vfs_io_uring_queue_run(struct vfs_io_uring_config *config)
 	struct timespec end_time;
 	int ret;
 
-	PROFILE_TIMESTAMP(&start_time);
-
 	if (config->uring.ring_fd == -1) {
 		vfs_io_uring_config_destroy(config, -ESTALE, __location__);
 		return;
 	}
 
+	if (config->busy) {
+		config->need_retry = true;
+		return;
+	}
+	config->busy = true;
+
+start_again:
+	config->need_retry = false;
+
+	PROFILE_TIMESTAMP(&start_time);
+
 	for (cur = config->queue; cur != NULL; cur = next) {
 		struct io_uring_sqe *sqe = NULL;
 		void *state = _tevent_req_data(cur->req);
@@ -299,6 +310,12 @@ static void vfs_io_uring_queue_run(struct vfs_io_uring_config *config)
 	}
 
 	io_uring_cq_advance(&config->uring, nr);
+
+	if (config->need_retry) {
+		goto start_again;
+	}
+
+	config->busy = false;
 }
 
 static void vfs_io_uring_request_submit(struct vfs_io_uring_request *cur)
-- 
2.17.1

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20200508/cca7f079/signature.sig>