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

[Jeremy Allison]

On Fri, May 08, 2020 at 11:40:30PM +0200, Stefan Metzmacher wrote:
> Am 08.05.20 um 22:51 schrieb Jeremy Allison:
> > On Fri, May 08, 2020 at 01:47:09PM -0700, Jeremy Allison via samba-technical wrote:
> >> On Fri, May 08, 2020 at 09:35:31PM +0200, Stefan Metzmacher via samba-technical wrote:
> >>>
> >>> Thanks very much Jeremy! I didn't noticed that.
> >>>
> >>> I guess the attached patch should be able to fix the recursion.
> >>
> >> Oh Metze that's *really* ugly :-). I thought about
> >> doing that in my code and decided it was in too bad
> >> taste to live :-).
> >>
> >> Is there a cleaner way than putting "busy" and "retry"
> >> variables in the config struct ?
> > 
> > And a "Goto again" as well :-(. Bleegh.
> 
> This version would actually work and looks a bit similar to
> your version.
> 
> Can you life with that version?

Yes I can live with that :-). It at least moves the horror
to the wrapper function where you can at least concentrate
all your attention as to why it's doing what it's doing :-).

RB+ from me if you add a comment header above the function
as well as in the commit so it explains what it's doing.

Feel free to crib my ascii art to explain in the header
comment too :-).

Thanks for the cleanup !

Jeremy.

> From ce467400f4098d0edbee9534a5667c0b92422e8f 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.
> 
> This should make it safe to be called from the completion_fn().
> 
> 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 | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/source3/modules/vfs_io_uring.c b/source3/modules/vfs_io_uring.c
> index ee23449c63c6..df41c74a7953 100644
> --- a/source3/modules/vfs_io_uring.c
> +++ b/source3/modules/vfs_io_uring.c
> @@ -34,6 +34,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;
>  };
> @@ -222,7 +224,7 @@ static int vfs_io_uring_connect(vfs_handle_struct *handle, const char *service,
>  	return 0;
>  }
>  
> -static void vfs_io_uring_queue_run(struct vfs_io_uring_config *config)
> +static void _vfs_io_uring_queue_run(struct vfs_io_uring_config *config)
>  {
>  	struct vfs_io_uring_request *cur = NULL, *next = NULL;
>  	struct io_uring_cqe *cqe = NULL;
> @@ -280,6 +282,22 @@ static void vfs_io_uring_queue_run(struct vfs_io_uring_config *config)
>  	io_uring_cq_advance(&config->uring, nr);
>  }
>  
> +static void vfs_io_uring_queue_run(struct vfs_io_uring_config *config)
> +{
> +	if (config->busy) {
> +		config->need_retry = true;
> +		return;
> +	}
> +	config->busy = true;
> +
> +	do {
> +		config->need_retry = false;
> +		_vfs_io_uring_queue_run(config);
> +	} while (config->need_retry);
> +
> +	config->busy = false;
> +}
> +
>  static void vfs_io_uring_fd_handler(struct tevent_context *ev,
>  				    struct tevent_fd *fde,
>  				    uint16_t flags,
> -- 
> 2.17.1
>