[PATCH] btrfs: free pages on error on 'btrfs_uring_read_extent'

[PATCH] btrfs: free pages on error on 'btrfs_uring_read_extent'

[Miquel Sabaté Solà]

In this function the 'pages' object is never freed in the hopes that is
picked up by btrfs_uring_read_finished() whenever that executes in the
future. But that's just the happy path. Along the way previous
allocations might have gone wrong, or we might not get -EIOCBQUEUED from
btrfs_encoded_read_regular_fill_pages(). In all these cases, we go to a
cleanup section that frees all memory allocated by this function without
assuming any deferred execution, and this also needs to happen for the
'pages' allocation.

Signed-off-by: Miquel Sabaté Solà <mssola@mssola.com>
---
 fs/btrfs/ioctl.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 38d93dae71ca..b3e8a8d9b19d 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4651,7 +4651,7 @@ static int btrfs_uring_read_extent(struct kiocb *iocb, struct iov_iter *iter,
 {
 	struct btrfs_inode *inode = BTRFS_I(file_inode(iocb->ki_filp));
 	struct extent_io_tree *io_tree = &inode->io_tree;
-	struct page **pages;
+	struct page **pages = NULL;
 	struct btrfs_uring_priv *priv = NULL;
 	unsigned long nr_pages;
 	int ret;
@@ -4709,6 +4709,11 @@ static int btrfs_uring_read_extent(struct kiocb *iocb, struct iov_iter *iter,
 	btrfs_unlock_extent(io_tree, start, lockend, &cached_state);
 	btrfs_inode_unlock(inode, BTRFS_ILOCK_SHARED);
 	kfree(priv);
+	for (int i = 0; i < nr_pages; i++) {
+		if (pages[i])
+			__free_page(pages[i]);
+	}
+	kfree(pages);
 	return ret;
 }
 
-- 
2.53.0

Re: [PATCH] btrfs: free pages on error on 'btrfs_uring_read_extent'

[Filipe Manana]

On Mon, Feb 16, 2026 at 9:13 PM Miquel Sabaté Solà <mssola@mssola.com> wrote:
>

As for the subject, should be instead:

btrfs: free pages on error in btrfs_read_uring_extent()

Note we don't usually surround function names with quotes and we
usually add the () after their name.

> In this function the 'pages' object is never freed in the hopes that is

that is -> that it is

> picked up by btrfs_uring_read_finished() whenever that executes in the
> future. But that's just the happy path. Along the way previous
> allocations might have gone wrong, or we might not get -EIOCBQUEUED from
> btrfs_encoded_read_regular_fill_pages(). In all these cases, we go to a
> cleanup section that frees all memory allocated by this function without
> assuming any deferred execution, and this also needs to happen for the
> 'pages' allocation.
>
> Signed-off-by: Miquel Sabaté Solà <mssola@mssola.com>

Not contrary to what you had just suggested for a cleanup patch here:
https://lore.kernel.org/linux-btrfs/87tsvfu11i.fsf@/

This is the sort of change that should have a Fixes tag, because it
fixes a bug, something that affects users, therefore useful and
important to have backported to stable releases.

So adding a:

Fixes: 34310c442e17 ("btrfs: add io_uring command for encoded reads
(ENCODED_READ ioctl)")

You don't need to do any of these changes, I've done that changes
myself and added it to the github for-next branch, thanks.

Reviewed-by: Filipe Manana <fdmanana@suse.com>


> ---
>  fs/btrfs/ioctl.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 38d93dae71ca..b3e8a8d9b19d 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4651,7 +4651,7 @@ static int btrfs_uring_read_extent(struct kiocb *iocb, struct iov_iter *iter,
>  {
>         struct btrfs_inode *inode = BTRFS_I(file_inode(iocb->ki_filp));
>         struct extent_io_tree *io_tree = &inode->io_tree;
> -       struct page **pages;
> +       struct page **pages = NULL;
>         struct btrfs_uring_priv *priv = NULL;
>         unsigned long nr_pages;
>         int ret;
> @@ -4709,6 +4709,11 @@ static int btrfs_uring_read_extent(struct kiocb *iocb, struct iov_iter *iter,
>         btrfs_unlock_extent(io_tree, start, lockend, &cached_state);
>         btrfs_inode_unlock(inode, BTRFS_ILOCK_SHARED);
>         kfree(priv);
> +       for (int i = 0; i < nr_pages; i++) {
> +               if (pages[i])
> +                       __free_page(pages[i]);
> +       }
> +       kfree(pages);
>         return ret;
>  }
>
> --
> 2.53.0
>
>

Re: [PATCH] btrfs: free pages on error on 'btrfs_uring_read_extent'

[Miquel Sabaté Solà]

[-- Attachment #1: Type: text/plain, Size: 2880 bytes --]

Filipe Manana @ 2026-02-17 11:10 GMT:

> On Mon, Feb 16, 2026 at 9:13 PM Miquel Sabaté Solà <mssola@mssola.com> wrote:
>>
>
> As for the subject, should be instead:
>
> btrfs: free pages on error in btrfs_read_uring_extent()
>
> Note we don't usually surround function names with quotes and we
> usually add the () after their name.
>
>> In this function the 'pages' object is never freed in the hopes that is
>
> that is -> that it is
>
>> picked up by btrfs_uring_read_finished() whenever that executes in the
>> future. But that's just the happy path. Along the way previous
>> allocations might have gone wrong, or we might not get -EIOCBQUEUED from
>> btrfs_encoded_read_regular_fill_pages(). In all these cases, we go to a
>> cleanup section that frees all memory allocated by this function without
>> assuming any deferred execution, and this also needs to happen for the
>> 'pages' allocation.
>>
>> Signed-off-by: Miquel Sabaté Solà <mssola@mssola.com>
>
> Not contrary to what you had just suggested for a cleanup patch here:
> https://lore.kernel.org/linux-btrfs/87tsvfu11i.fsf@/
>
> This is the sort of change that should have a Fixes tag, because it
> fixes a bug, something that affects users, therefore useful and
> important to have backported to stable releases.
>
> So adding a:
>
> Fixes: 34310c442e17 ("btrfs: add io_uring command for encoded reads
> (ENCODED_READ ioctl)")
>
> You don't need to do any of these changes, I've done that changes
> myself and added it to the github for-next branch, thanks.
>
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
>
>

You are totally right, completely missed that one.

Thanks!
Miquel

>> ---
>>  fs/btrfs/ioctl.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 38d93dae71ca..b3e8a8d9b19d 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -4651,7 +4651,7 @@ static int btrfs_uring_read_extent(struct kiocb *iocb, struct iov_iter *iter,
>>  {
>>         struct btrfs_inode *inode = BTRFS_I(file_inode(iocb->ki_filp));
>>         struct extent_io_tree *io_tree = &inode->io_tree;
>> -       struct page **pages;
>> +       struct page **pages = NULL;
>>         struct btrfs_uring_priv *priv = NULL;
>>         unsigned long nr_pages;
>>         int ret;
>> @@ -4709,6 +4709,11 @@ static int btrfs_uring_read_extent(struct kiocb *iocb, struct iov_iter *iter,
>>         btrfs_unlock_extent(io_tree, start, lockend, &cached_state);
>>         btrfs_inode_unlock(inode, BTRFS_ILOCK_SHARED);
>>         kfree(priv);
>> +       for (int i = 0; i < nr_pages; i++) {
>> +               if (pages[i])
>> +                       __free_page(pages[i]);
>> +       }
>> +       kfree(pages);
>>         return ret;
>>  }
>>
>> --
>> 2.53.0
>>
>>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 897 bytes --]