Re: [PATCH v4 3/3] md/raid10: bound reused r10bio devs[] walks by used_nr_devs

["Chen Cheng" <chencheng@fnnas.com>]

在 2026/6/21 04:02, yu kuai 写道:
> Hi,
> 
> 在 2026/6/3 11:59, Chen Cheng 写道:
>> From: Chen Cheng <chencheng@fnnas.com>
>>
>> After reshape changes raid_disks, an in-flight r10bio from the old geometry
>> can still be completed or freed later. In that case, using the current
>> geometry to walk r10_bio->devs[] is unsafe. A failure was reproduced with a
>> simple write workload while reshaping a raid10 array from 4 disks to 5 disks.
>> e.g.:
>>
>>     mdadm -C /dev/md777 -l10 -n4 /dev/sda /dev/sdb /dev/sdc /dev/sdd
>>     mkfs.ext4 /dev/md777
>>     mount /dev/md777 /mnt/test
>>     fsstress -d /mnt/test -n 24000 -p 8 -l 24 &
>>     mdadm /dev/md777 --add /dev/sde
>>     mdadm --grow /dev/md777 --raid-devices=5 \
>>       --backup-file=/tmp/md-reshape-backup
>>
>> the sequence above can trigger:
>>
>>     BUG: KASAN: slab-out-of-bounds in free_r10bio+0x1c4/0x260 [raid10]
>>     Read of size 8 at addr ffff00008c2dfac8 by task ksoftirqd/0/15
>>     free_r10bio
>>     raid_end_bio_io
>>     one_write_done
>>     raid10_end_write_request
>>
>> The buggy object was 200 bytes long, which matches an r10bio with space for
>> only four devs[] entries. However, put_all_bios() and find_bio_disk() walk
>> r10_bio->devs[] using the current conf->geo.raid_disks value. Once reshape
>> switches conf->geo.raid_disks from 4 to 5, an old 4-slot r10bio can be
>> completed or freed as if it had 5 slots, and the walk overruns devs[4]. The
> I don't understand, is this still possible after patch 1.



patch 1 and 2 just prepare work.
The true dangerous is in-flight r10bio which use old geometry.
And patch 3 record and use r10bio's own width to free r10bio's bios.



>> same stale-width mismatch can also surface during a 5-disk to 4-disk reshape.
>>
>> Track the number of valid devs[] entries in each reused r10bio with
>> used_nr_devs. Initialize it whenever an r10bio is prepared for regular I/O,
>> discard, or resync/recovery/reshape work, and use it to bound devs[] walks
>> in put_all_bios() and find_bio_disk().
>>
>> Signed-off-by: Chen Cheng <chencheng@fnnas.com>
>> ---
>>    drivers/md/raid10.c | 8 ++++++--
>>    drivers/md/raid10.h | 2 ++
>>    2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>> index 5eca34432e63..f134b93fd593 100644
>> --- a/drivers/md/raid10.c
>> +++ b/drivers/md/raid10.c
>> @@ -273,11 +273,11 @@ static void r10buf_pool_free(void *__r10_bio, void *data)
>>    
>>    static void put_all_bios(struct r10conf *conf, struct r10bio *r10_bio)
>>    {
>>    	int i;
>>    
>> -	for (i = 0; i < conf->geo.raid_disks; i++) {
>> +	for (i = 0; i < r10_bio->used_nr_devs; i++) {
>>    		struct bio **bio = & r10_bio->devs[i].bio;
>>    		if (!BIO_SPECIAL(*bio))
>>    			bio_put(*bio);
>>    		*bio = NULL;
>>    		bio = &r10_bio->devs[i].repl_bio;
>> @@ -370,11 +370,11 @@ static int find_bio_disk(struct r10conf *conf, struct r10bio *r10_bio,
>>    			 struct bio *bio, int *slotp, int *replp)
>>    {
>>    	int slot;
>>    	int repl = 0;
>>    
>> -	for (slot = 0; slot < conf->geo.raid_disks; slot++) {
>> +	for (slot = 0; slot < r10_bio->used_nr_devs; slot++) {
>>    		if (r10_bio->devs[slot].bio == bio)
>>    			break;
>>    		if (r10_bio->devs[slot].repl_bio == bio) {
>>    			repl = 1;
>>    			break;
>> @@ -1561,10 +1561,11 @@ static void __make_request(struct mddev *mddev, struct bio *bio, int sectors)
>>    
>>    	r10_bio->mddev = mddev;
>>    	r10_bio->sector = bio->bi_iter.bi_sector;
>>    	r10_bio->state = 0;
>>    	r10_bio->read_slot = -1;
>> +	r10_bio->used_nr_devs = conf->geo.raid_disks;
>>    	memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) *
>>    			conf->geo.raid_disks);
>>    
>>    	if (bio_data_dir(bio) == READ)
>>    		raid10_read_request(mddev, bio, r10_bio);
>> @@ -1749,10 +1750,11 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
>>    	r10_bio = mempool_alloc(conf->r10bio_pool, GFP_NOIO);
>>    	r10_bio->mddev = mddev;
>>    	r10_bio->state = 0;
>>    	r10_bio->sectors = 0;
>>    	r10_bio->read_slot = -1;
>> +	r10_bio->used_nr_devs = geo->raid_disks;
>>    	memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) * geo->raid_disks);
>>    	wait_blocked_dev(mddev, r10_bio);
>>    
>>    	/*
>>    	 * For far layout it needs more than one r10bio to cover all regions.
>> @@ -3083,10 +3085,12 @@ static struct r10bio *raid10_alloc_init_r10buf(struct r10conf *conf)
>>    	    test_bit(MD_RECOVERY_RESHAPE, &conf->mddev->recovery))
>>    		nalloc = conf->copies; /* resync */
>>    	else
>>    		nalloc = 2; /* recovery */
>>    
>> +	r10bio->used_nr_devs = nalloc;
>> +
>>    	for (i = 0; i < nalloc; i++) {
>>    		bio = r10bio->devs[i].bio;
>>    		rp = bio->bi_private;
>>    		bio_reset(bio, NULL, 0);
>>    		bio->bi_private = rp;
>> diff --git a/drivers/md/raid10.h b/drivers/md/raid10.h
>> index b711626a5db7..4751119f9770 100644
>> --- a/drivers/md/raid10.h
>> +++ b/drivers/md/raid10.h
>> @@ -125,10 +125,12 @@ struct r10bio {
>>    	struct bio		*master_bio;
>>    	/*
>>    	 * if the IO is in READ direction, then this is where we read
>>    	 */
>>    	int			read_slot;
>> +	/* Used to bound devs[] walks when the object is reused. */
>> +	unsigned int		used_nr_devs;
>>    
>>    	struct list_head	retry_list;
>>    	/*
>>    	 * if the IO is in WRITE direction, then multiple bios are used,
>>    	 * one for each copy.
>