public inbox for linux-btrfs@vger.kernel.org
 help / color / mirror / Atom feed
From: Sasha Levin <sashal@kernel.org>
To: patches@lists.linux.dev, stable@vger.kernel.org
Cc: Filipe Manana <fdmanana@suse.com>,
	David Sterba <dsterba@suse.com>, Sasha Levin <sashal@kernel.org>,
	clm@fb.com, linux-btrfs@vger.kernel.org
Subject: [PATCH AUTOSEL 6.18-6.1] btrfs: fix reservation leak in some error paths when inserting inline extent
Date: Mon, 12 Jan 2026 09:58:22 -0500	[thread overview]
Message-ID: <20260112145840.724774-21-sashal@kernel.org> (raw)
In-Reply-To: <20260112145840.724774-1-sashal@kernel.org>

From: Filipe Manana <fdmanana@suse.com>

[ Upstream commit c1c050f92d8f6aac4e17f7f2230160794fceef0c ]

If we fail to allocate a path or join a transaction, we return from
__cow_file_range_inline() without freeing the reserved qgroup data,
resulting in a leak. Fix this by ensuring we call btrfs_qgroup_free_data()
in such cases.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

## Commit Analysis: btrfs: fix reservation leak in some error paths when
inserting inline extent

### 1. COMMIT MESSAGE ANALYSIS

The subject clearly indicates a **bug fix** for a "reservation leak" in
error paths. The body explains:
- When `btrfs_alloc_path()` fails or `btrfs_join_transaction()` fails,
  the function returned early
- The reserved qgroup data was never freed in these error paths
- This results in a resource leak

**Tags**: Reviewed-by and Signed-off-by from David Sterba (btrfs
maintainer), authored by Filipe Manana (experienced btrfs developer).
Excellent review coverage.

### 2. CODE CHANGE ANALYSIS

**The Bug (Before):**
```c
path = btrfs_alloc_path();
if (!path)
    return -ENOMEM;  // Early return - leaks qgroup reservation

trans = btrfs_join_transaction(root);
if (IS_ERR(trans)) {
    btrfs_free_path(path);
    return PTR_ERR(trans);  // Early return - leaks qgroup reservation
}
```

**The Fix (After):**
```c
path = btrfs_alloc_path();
if (!path) {
    ret = -ENOMEM;
    goto out;  // Goes to cleanup that calls btrfs_qgroup_free_data()
}

trans = btrfs_join_transaction(root);
if (IS_ERR(trans)) {
    ret = PTR_ERR(trans);
    trans = NULL;
    goto out;  // Goes to cleanup that calls btrfs_qgroup_free_data()
}
```

Additional changes:
- Initialize `trans = NULL` at declaration
- Add NULL check before `btrfs_end_transaction(trans)` in cleanup

**Root cause**: Early returns bypassed the `out` label where
`btrfs_qgroup_free_data()` is called for error cases (`ret <= 0`).

**Why the fix works**: Redirects error paths to use `goto out`, ensuring
all exits pass through existing cleanup logic.

### 3. CLASSIFICATION

- **Type**: Bug fix (resource leak)
- **Category**: Error handling fix in filesystem code
- **Not a feature addition**: Pure cleanup path fix

### 4. SCOPE AND RISK ASSESSMENT

- **Size**: ~20 lines changed in a single file (`fs/btrfs/inode.c`)
- **Scope**: Single function `__cow_file_range_inline()`
- **Complexity**: Very low - simple control flow change (early return →
  goto)
- **Risk**: Very low
  - Uses existing cleanup logic already tested for other error paths
  - The NULL check on `trans` is a proper safeguard
  - No algorithm changes, no new logic

### 5. USER IMPACT

- **Affected users**: btrfs users with qgroups enabled
- **Trigger conditions**: ENOMEM or transaction join failure during
  inline extent insertion
- **Severity**: Resource leak that accumulates over time, can cause
  incorrect qgroup accounting
- **Frequency**: Error conditions are rare, but when they occur, the
  leak is guaranteed

### 6. STABILITY INDICATORS

- Authored by Filipe Manana (prolific btrfs contributor)
- Reviewed by David Sterba (btrfs maintainer)
- Signed-off by David Sterba

### 7. DEPENDENCY CHECK

- Self-contained fix
- No dependencies on other commits
- The affected function and `btrfs_qgroup_free_data()` exist in stable
  trees
- The code structure is stable and not recently added

## Conclusion

This commit is an **ideal stable candidate**:

1. **Obviously correct**: Simple redirection of error paths to existing
   cleanup code
2. **Fixes a real bug**: Qgroup data reservation leak that can
   accumulate over time
3. **Small and contained**: ~20 lines, one file, one function
4. **No new features**: Pure bug fix
5. **Well-reviewed**: By btrfs maintainers
6. **Low risk**: Uses existing cleanup logic, minimal code change
7. **Important for users**: Btrfs with qgroups is a common configuration

The fix is surgical, low-risk, and addresses a genuine resource leak in
the btrfs filesystem. It meets all stable kernel criteria.

**YES**

 fs/btrfs/inode.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 9e8be59ea3deb..0d61e0ee2f86f 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -614,19 +614,22 @@ static noinline int __cow_file_range_inline(struct btrfs_inode *inode,
 	struct btrfs_drop_extents_args drop_args = { 0 };
 	struct btrfs_root *root = inode->root;
 	struct btrfs_fs_info *fs_info = root->fs_info;
-	struct btrfs_trans_handle *trans;
+	struct btrfs_trans_handle *trans = NULL;
 	u64 data_len = (compressed_size ?: size);
 	int ret;
 	struct btrfs_path *path;
 
 	path = btrfs_alloc_path();
-	if (!path)
-		return -ENOMEM;
+	if (!path) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	trans = btrfs_join_transaction(root);
 	if (IS_ERR(trans)) {
-		btrfs_free_path(path);
-		return PTR_ERR(trans);
+		ret = PTR_ERR(trans);
+		trans = NULL;
+		goto out;
 	}
 	trans->block_rsv = &inode->block_rsv;
 
@@ -677,7 +680,8 @@ static noinline int __cow_file_range_inline(struct btrfs_inode *inode,
 	if (ret <= 0)
 		btrfs_qgroup_free_data(inode, NULL, 0, fs_info->sectorsize, NULL);
 	btrfs_free_path(path);
-	btrfs_end_transaction(trans);
+	if (trans)
+		btrfs_end_transaction(trans);
 	return ret;
 }
 
-- 
2.51.0


      parent reply	other threads:[~2026-01-12 14:59 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20260112145840.724774-1-sashal@kernel.org>
2026-01-12 14:58 ` [PATCH AUTOSEL 6.18] btrfs: do not free data reservation in fallback from inline due to -ENOSPC Sasha Levin
2026-01-12 14:58 ` [PATCH AUTOSEL 6.18-5.10] btrfs: fix deadlock in wait_current_trans() due to ignored transaction type Sasha Levin
2026-01-19 11:46   ` Motiejus Jakštys
2026-01-20 11:03     ` Greg KH
2026-01-12 14:58 ` Sasha Levin [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260112145840.724774-21-sashal@kernel.org \
    --to=sashal@kernel.org \
    --cc=clm@fb.com \
    --cc=dsterba@suse.com \
    --cc=fdmanana@suse.com \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=patches@lists.linux.dev \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox