From: Shida Zhang zhangshida@kylinos.cn
[ Upstream commit 53280e398471f0bddbb17b798a63d41264651325 ]
Don't call bio->bi_end_io() directly. Use the bio_endio() helper function instead, which handles completion more safely and uniformly.
Suggested-by: Christoph Hellwig hch@infradead.org Reviewed-by: Christoph Hellwig hch@lst.de Signed-off-by: Shida Zhang zhangshida@kylinos.cn Signed-off-by: Jens Axboe axboe@kernel.dk Signed-off-by: Sasha Levin sashal@kernel.org ---
LLM Generated explanations, may be completely bogus:
## Analysis of bcache commit: "fix improper use of bi_end_io"
### 1. COMMIT MESSAGE ANALYSIS
The commit message clearly indicates this is a **bug fix** addressing improper use of the block I/O completion API. Key signals: - Subject explicitly says "fix improper use" - Body explains the correct API (`bio_endio()`) should be used instead of direct `bi_end_io()` calls - Suggested and Reviewed by Christoph Hellwig - one of the most respected Linux kernel developers, particularly in block layer code
### 2. CODE CHANGE ANALYSIS
The commit makes three changes in `drivers/md/bcache/request.c`:
**Change 1** (detached_dev_end_io, line 1107): ```c - bio->bi_end_io(bio); + bio_endio(bio); ``` After restoring the original `bi_end_io`, uses proper API for completion.
**Change 2** (detached_dev_do_request error path, line 1122): ```c - bio->bi_end_io(bio); + bio_endio(bio); ``` Error path when `kzalloc` fails - uses proper API.
**Change 3** (detached_dev_do_request discard handling, line 1139): ```c - bio->bi_end_io(bio); + detached_dev_end_io(bio); ``` **This is the most critical fix.** At this point, `bio->bi_end_io` has already been reassigned to `detached_dev_end_io`. The old code was calling the *saved* original handler directly, completely bypassing the accounting and cleanup in `detached_dev_end_io()`. This could cause: - Missing I/O accounting (`bio_end_io_acct_remapped` never called) - Memory leak (ddip structure never freed) - Incorrect error handling for backing device
### 3. WHY THIS IS A BUG
`bio_endio()` does more than just calling `bi_end_io()`: - Handles bio chaining/splitting properly - Manages completion accounting - Ensures proper memory barriers - Provides unified completion path expected by block layer
Bypassing this function can cause: - Incorrect I/O statistics - Race conditions in bio completion - Potential memory issues - Missing cleanup (especially Change 3)
### 4. CLASSIFICATION
- **Bug fix**: Yes - corrects improper API usage - **Feature addition**: No - **Security**: Not explicitly, but improper completion could lead to memory issues
### 5. SCOPE AND RISK ASSESSMENT
| Factor | Assessment | |--------|------------| | Lines changed | 3 lines | | Files touched | 1 file | | Complexity | Very low - simple API call substitution | | Subsystem | bcache (mature, stable subsystem) | | Risk | Very low - using proper API is strictly safer |
### 6. USER IMPACT
- **Affected users**: bcache users (SSD caching in front of HDDs) - **Severity**: Medium - Change 3 especially could cause I/O accounting issues and memory leaks for discards on devices without discard support - **Visibility**: Subtle issues that may manifest as accounting bugs or resource leaks
### 7. STABILITY INDICATORS
- **Reviewed-by: Christoph Hellwig** - block layer maintainer provides high confidence - **Suggested-by: Christoph Hellwig** - issue identified by expert during code review - `bio_endio()` is a fundamental, long-standing block layer API - no dependency concerns
### 8. DEPENDENCY CHECK
- `bio_endio()` has existed for many years in all stable kernels - bcache has been in mainline since ~3.10 - No dependencies on other commits - Should apply cleanly to any stable tree with bcache
### CONCLUSION
This commit meets all stable kernel criteria:
1. **Obviously correct**: Uses the documented, proper block layer API 2. **Fixes real bugs**: Third change especially fixes missing cleanup/accounting 3. **Small and contained**: Only 3 one-line changes 4. **Reviewed by expert**: Christoph Hellwig's review carries significant weight 5. **No new features**: Pure correctness fix 6. **Low risk**: Switching to proper API cannot introduce new bugs
The fix is surgical, addresses genuine API misuse that causes real issues (particularly the discard path), has top-tier review, and carries essentially no regression risk.
**YES**
drivers/md/bcache/request.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c index af345dc6fde14..82fdea7dea7aa 100644 --- a/drivers/md/bcache/request.c +++ b/drivers/md/bcache/request.c @@ -1104,7 +1104,7 @@ static void detached_dev_end_io(struct bio *bio) }
kfree(ddip); - bio->bi_end_io(bio); + bio_endio(bio); }
static void detached_dev_do_request(struct bcache_device *d, struct bio *bio, @@ -1121,7 +1121,7 @@ static void detached_dev_do_request(struct bcache_device *d, struct bio *bio, ddip = kzalloc(sizeof(struct detached_dev_io_private), GFP_NOIO); if (!ddip) { bio->bi_status = BLK_STS_RESOURCE; - bio->bi_end_io(bio); + bio_endio(bio); return; }
@@ -1136,7 +1136,7 @@ static void detached_dev_do_request(struct bcache_device *d, struct bio *bio,
if ((bio_op(bio) == REQ_OP_DISCARD) && !bdev_max_discard_sectors(dc->bdev)) - bio->bi_end_io(bio); + detached_dev_end_io(bio); else submit_bio_noacct(bio); }