[PATCH] io_uring: ensure open/openat2 name is cleaned on cancelation

[PATCH] io_uring: ensure open/openat2 name is cleaned on cancelation

[Jens Axboe]

io_uring: ensure open/openat2 name is cleaned on cancelation

If we cancel these requests, we'll leak the memory associated with the
filename. Add them to the table of ops that need cleaning, if
REQ_F_NEED_CLEANUP is set.

Cc: stable@vger.kernel.org # v5.6+
Signed-off-by: Jens Axboe <axboe@kernel.dk>

--

diff --git a/fs/io_uring.c b/fs/io_uring.c
index e6004b92e553..0ab16df31288 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -5671,6 +5671,11 @@ static void __io_clean_op(struct io_kiocb *req)
 			io_put_file(req, req->splice.file_in,
 				    (req->splice.flags & SPLICE_F_FD_IN_FIXED));
 			break;
+		case IORING_OP_OPENAT:
+		case IORING_OP_OPENAT2:
+			if (req->open.filename)
+				putname(req->open.filename);
+			break;
 		}
 		req->flags &= ~REQ_F_NEED_CLEANUP;
 	}

-- 
Jens Axboe

Re: [PATCH] io_uring: ensure open/openat2 name is cleaned on cancelation

[Stefano Garzarella]

On Thu, Sep 24, 2020 at 02:59:33PM -0600, Jens Axboe wrote:
> io_uring: ensure open/openat2 name is cleaned on cancelation
> 
> If we cancel these requests, we'll leak the memory associated with the
> filename. Add them to the table of ops that need cleaning, if
> REQ_F_NEED_CLEANUP is set.
> 

IIUC we inadvertently removed 'putname(req->open.filename)' from the cleanup
function in commit e62753e4e292 ("io_uring: call statx directly").

Should we add the Fixes tag?

    Fixes: e62753e4e292 ("io_uring: call statx directly")

I'm not sure since the code is changed a bit.

Anyway the patch LGTM:

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Thanks,
Stefano

> Cc: stable@vger.kernel.org # v5.6+
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
> --
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index e6004b92e553..0ab16df31288 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -5671,6 +5671,11 @@ static void __io_clean_op(struct io_kiocb *req)
>  			io_put_file(req, req->splice.file_in,
>  				    (req->splice.flags & SPLICE_F_FD_IN_FIXED));
>  			break;
> +		case IORING_OP_OPENAT:
> +		case IORING_OP_OPENAT2:
> +			if (req->open.filename)
> +				putname(req->open.filename);
> +			break;
>  		}
>  		req->flags &= ~REQ_F_NEED_CLEANUP;
>  	}
> 
> -- 
> Jens Axboe
> 

Re: [PATCH] io_uring: ensure open/openat2 name is cleaned on cancelation

[Jens Axboe]

On 9/25/20 2:32 AM, Stefano Garzarella wrote:
> On Thu, Sep 24, 2020 at 02:59:33PM -0600, Jens Axboe wrote:
>> io_uring: ensure open/openat2 name is cleaned on cancelation
>>
>> If we cancel these requests, we'll leak the memory associated with the
>> filename. Add them to the table of ops that need cleaning, if
>> REQ_F_NEED_CLEANUP is set.
>>
> 
> IIUC we inadvertently removed 'putname(req->open.filename)' from the cleanup
> function in commit e62753e4e292 ("io_uring: call statx directly").
> 
> Should we add the Fixes tag?
> 
>     Fixes: e62753e4e292 ("io_uring: call statx directly")

You are right, I got a bit tricked by it since that commit removed
the putname(), and then later on we got rid of the (now) empty
openat/openat2 entries.

I'll add the fixes, which means it's 5.8 only.

> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Added, thanks for reviewing!

-- 
Jens Axboe