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>, Qu Wenruo <wqu@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.6] btrfs: abort transaction on item count overflow in __push_leaf_left()
Date: Mon,  8 Dec 2025 19:14:58 -0500	[thread overview]
Message-ID: <20251209001610.611575-6-sashal@kernel.org> (raw)
In-Reply-To: <20251209001610.611575-1-sashal@kernel.org>

From: Filipe Manana <fdmanana@suse.com>

[ Upstream commit 5d8222a50ad37c98455da08b33ce49fe6b726c72 ]

If we try to push an item count from the right leaf that is greater than
the number of items in the leaf, we just emit a warning. This should
never happen but if it does we get an underflow in the new number of
items in the right leaf and chaos follows from it. So replace the warning
with proper error handling, by aborting the transaction and returning
-EUCLEAN, and proper logging by using btrfs_crit() instead of WARN(),
which gives us proper formatting and information about the filesystem.

Reviewed-by: Qu Wenruo <wqu@suse.com>
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: abort transaction on item count overflow in
__push_leaf_left()

### 1. COMMIT MESSAGE ANALYSIS

**Key points:**
- Fixes an item count overflow condition where `push_items >
  right_nritems`
- Currently only emits a WARN() but continues execution
- If triggered, causes "an underflow in the new number of items in the
  right leaf and chaos follows"
- Replaces warning with proper error handling (abort transaction, return
  -EUCLEAN)

**Tags:**
- No `Cc: stable@vger.kernel.org` tag
- No `Fixes:` tag
- Has two `Reviewed-by:` tags (Qu Wenruo and David Sterba - btrfs
  maintainer)

### 2. CODE CHANGE ANALYSIS

**Before (problematic):**
```c
if (push_items > right_nritems)
    WARN(1, KERN_CRIT "push items %d nr %u\n", push_items,
right_nritems);
// Continues execution despite the error!
```

**After (fixed):**
```c
if (unlikely(push_items > right_nritems)) {
    ret = -EUCLEAN;
    btrfs_abort_transaction(trans, ret);
    btrfs_crit(fs_info, "push items (%d) > right leaf items (%u)",
               push_items, right_nritems);
    goto out;
}
```

**Technical mechanism of the bug:**
- `__push_leaf_left()` pushes items from right leaf to left leaf in
  btrfs B-tree
- If `push_items > right_nritems`, later code does `right_nritems -=
  push_items`
- Since `right_nritems` is `u32`, this causes an **integer underflow**
- The underflowed value is then set via `btrfs_set_header_nritems(right,
  right_nritems)`
- This corrupts the B-tree structure, leading to filesystem corruption

### 3. CLASSIFICATION

- **Type:** Bug fix - upgrading inadequate error handling to proper
  abort
- **Not adding features:** Purely defensive error handling improvement
- **Severity:** Prevents potential filesystem/data corruption
- **Pattern:** Converts warn-and-continue to abort-and-return on
  corruption detection

### 4. SCOPE AND RISK ASSESSMENT

| Factor | Assessment |
|--------|------------|
| Lines changed | ~10 lines |
| Files touched | 1 (fs/btrfs/ctree.c) |
| Complexity | Very low |
| Subsystem maturity | btrfs is mature and widely used |
| Regression risk | **Near zero** - if condition never triggers,
behavior unchanged; if it does, abort is safer than corruption |

The change uses existing infrastructure (`btrfs_abort_transaction`,
`goto out` path) that's already present and tested.

### 5. USER IMPACT

- **Affected users:** All btrfs users
- **Code location:** Core B-tree code - fundamental to all btrfs
  operations
- **If bug triggers:** Filesystem corruption, potentially unmountable
  filesystem
- **Frequency:** Described as "should never happen" but defensive check
  was already there

### 6. STABILITY INDICATORS

- Two reviewers from btrfs team
- Signed off by btrfs maintainer (David Sterba)
- Conservative, defensive fix with well-understood behavior

### 7. DEPENDENCY CHECK

- Self-contained fix with no dependencies
- Uses existing error handling patterns present in all stable trees
- This code has existed in btrfs for many years

### DECISION RATIONALE

**For backporting:**
1. **Prevents data corruption** - the most critical category of bugs for
   filesystems
2. **Minimal risk** - the fix cannot cause regression in normal
   operation
3. **Small and surgical** - ~10 lines, single file, obvious correctness
4. **Well-reviewed** - two experienced btrfs developers signed off
5. **Core subsystem** - btrfs is used in production by enterprise users
   relying on stable trees
6. **Integer underflow** - these are exactly the type of bugs stable
   trees want fixed

**Against backporting:**
1. No explicit `Cc: stable` tag
2. No `Fixes:` tag
3. Condition described as "should never happen"

**Assessment:** While the maintainer didn't explicitly request stable
backport, the fix meets all stable criteria:
- Obviously correct (straightforward error handling)
- Fixes a real bug (underflow leading to corruption)
- Small and contained (10 lines, 1 file)
- No new features

The severity of the potential consequence (filesystem corruption)
combined with the trivial risk of the fix makes this an appropriate
stable candidate. Filesystems are exactly where defensive hardening
matters most for stable users.

**YES**

 fs/btrfs/ctree.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 561658aca018b..3acb3027584d7 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -3398,9 +3398,13 @@ static noinline int __push_leaf_left(struct btrfs_trans_handle *trans,
 	btrfs_set_header_nritems(left, old_left_nritems + push_items);
 
 	/* fixup right node */
-	if (push_items > right_nritems)
-		WARN(1, KERN_CRIT "push items %d nr %u\n", push_items,
-		       right_nritems);
+	if (unlikely(push_items > right_nritems)) {
+		ret = -EUCLEAN;
+		btrfs_abort_transaction(trans, ret);
+		btrfs_crit(fs_info, "push items (%d) > right leaf items (%u)",
+			   push_items, right_nritems);
+		goto out;
+	}
 
 	if (push_items < right_nritems) {
 		push_space = btrfs_item_offset(right, push_items - 1) -
-- 
2.51.0


       reply	other threads:[~2025-12-09  0:16 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20251209001610.611575-1-sashal@kernel.org>
2025-12-09  0:14 ` Sasha Levin [this message]
2025-12-09  0:15 ` [PATCH AUTOSEL 6.18-5.10] btrfs: scrub: always update btrfs_scrub_progress::last_physical Sasha Levin
2025-12-09  0:15 ` [PATCH AUTOSEL 6.18-6.17] btrfs: use kvcalloc for btrfs_bio::csum allocation Sasha Levin

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=20251209001610.611575-6-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 \
    --cc=wqu@suse.com \
    /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