[RFC,NOT,FOR,INCLUSION] Concerns about logic of raid1.c::fix_sync_read_error()

Commit Message

Alex Lyakas July 9, 2024, 9:46 a.m. UTC

fix_sync_read_error() is called when a SYNC read (resync, repair or check) failes to read from any devices.
This funciton retries the reads starting from "read_disk" until it suceeds, otherwise it aborts.
If it succeeds to read from a particular device, it attempts to write the same data backwards until it returns to "read_disk".
Then it retries the reads backwards, starting from the device that it succeeded to read from, till "read_disk".
On any failure of these reads/writes, assuming bad blocks feature is disabled, md_error() is called on a particular rdev,
which will kick it out the array, and set MD_RECOVERY_INTR, which will eventually abort the resync/check/repair.

The issue that I see, is that this function is oriented on the "read_disk" to not fail.
I have marked in the patch some suspicious lines.
This function reads and writes always from/into pages of the "read_disk" bio, and eventually it marks the "read_disk"
bio as "success".

Consider the following scenario:
- We have RAID1 going through "repair" with drives A, B; all were selected for reading in raid1_sync_request().
- A was marked as "read_disk".
- Reads from both drives failed, so fix_sync_read_error() is called.
- fix_sync_read_error() retries the reads starting from A, but A still fails the read, whereas B succeeds.
- Now the code writes the same data back to A, which let's assume fails.
- md_error() is called, A is marked as Faulty, MD_RECOVERY_INTR is set
- r1_bio->bios[A]->bi_end_io = NULL;
- Now the code skips reading data from A, because bi_end_io is set to NULL.
- bio->bi_status = 0; // this marks the A's bios as success, but bi_end_io is NULL on it
- fix_sync_read_error() returns success
- process_checks() is called
- process_checks() executes the loop to find "primary", but no primary can be selected, because:
  - A has bi_end_io set to NULL
  - B has bad bi_status
- As a result, "primary" will be set to out of bounds, and r1_bio->bios[primary] will access some invalid memory location.

Can you please review the scenario above, and please let me know whether I am missing something?
---
 drivers/md/raid1.c | 5 +++++
 1 file changed, 5 insertions(+)

Patch

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 7b8a71c..2eb1d0b 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -2119,10 +2119,11 @@  static int fix_sync_read_error(struct r1bio *r1_bio)
 	 */
 	struct mddev *mddev = r1_bio->mddev;
 	struct r1conf *conf = mddev->private;
 	struct bio *bio = r1_bio->bios[r1_bio->read_disk];
 	struct page **pages = get_resync_pages(bio)->pages;
+	===> these pages belong to the bio on "read_disk"
 	sector_t sect = r1_bio->sector;
 	int sectors = r1_bio->sectors;
 	int idx = 0;
 	struct md_rdev *rdev;
 
@@ -2153,10 +2154,11 @@  static int fix_sync_read_error(struct r1bio *r1_bio)
 				 * active, and resync is currently active
 				 */
 				rdev = conf->mirrors[d].rdev;
 				if (sync_page_io(rdev, sect, s<<9,
 						 pages[idx],
+						 ===> we are reading into pages of "read_disk"
 						 REQ_OP_READ, false)) {
 					success = 1;
 					break;
 				}
 			}
@@ -2206,10 +2208,11 @@  static int fix_sync_read_error(struct r1bio *r1_bio)
 			if (r1_bio->bios[d]->bi_end_io != end_sync_read)
 				continue;
 			rdev = conf->mirrors[d].rdev;
 			if (r1_sync_page_io(rdev, sect, s,
 					    pages[idx],
+					    ===> we are writing pages from "read_disk"
 					    REQ_OP_WRITE) == 0) {
 				r1_bio->bios[d]->bi_end_io = NULL;
 				rdev_dec_pending(rdev, mddev);
 			}
 		}
@@ -2221,19 +2224,21 @@  static int fix_sync_read_error(struct r1bio *r1_bio)
 			if (r1_bio->bios[d]->bi_end_io != end_sync_read)
 				continue;
 			rdev = conf->mirrors[d].rdev;
 			if (r1_sync_page_io(rdev, sect, s,
 					    pages[idx],
+					    ===> we are reading into pages of "read_disk"
 					    REQ_OP_READ) != 0)
 				atomic_add(s, &rdev->corrected_errors);
 		}
 		sectors -= s;
 		sect += s;
 		idx ++;
 	}
 	set_bit(R1BIO_Uptodate, &r1_bio->state);
 	bio->bi_status = 0;
+	==> This bio belongs to "read_disk", but the appropriate rdev might have failed to "fix" the error
 	return 1;
 }
 
 static void process_checks(struct r1bio *r1_bio)
 {