Re: [PATCH] md: handle serial pool allocation failures

[yu kuai]

Hi,

在 2026/7/18 18:55, Chen Cheng 写道:
> From: Chen Cheng <chencheng@xxxxxxxxx>
>
> sashiko-bot report a issue:
> mddev_create_serial_pool() silently ignored allocation failures, allowing
> callers to continue with incomplete serialization state.
>
> validation:
>
> fail_page_alloc with a stacktrace filter for rdev_init_serial() to force
> the serial allocation and its kvmalloc fallback to fail.
>
> A/B result:
> - Without this patch, serialize_policy accepts the write despite the
> 	injected allocation failure, leaving serialization state
> 	incomplete.
> - With this patch, the write returns -ENOMEM and serialize_policy remains
> 	unchanged.

I don't see what's the problem here. If mddev_create_serial_pool() failed, then write behind
will be disabled.

>
> Fixes: 3938f5fb82ae ("md: add serialize_policy sysfs node for raid1")
> Link: https://github.com/chencheng-fnnas/reproducer/blob/main/test-serial-pool-oom.sh
>
> sashiko-bot report:
> =====================
> [Severity: Critical]
> This is a pre-existing issue, but further down in backlog_store(), is it
> possible for mddev_create_serial_pool() to fail silently?
>
> drivers/md/md-bitmap.c:backlog_store() {
>      ...
>      } else if (backlog && !mddev->serial_info_pool) {
>          /* serial_info_pool is needed since backlog is not zero */
>          rdev_for_each(rdev, mddev)
>              mddev_create_serial_pool(mddev, rdev);
>      }
>      ...
> }
>
> Since mddev_create_serial_pool() returns void, it hides memory allocation
> failures. If it fails for a disk in this loop, that device is left without
> initialization. Does this silently bypass write-behind serialization for
> that disk, leading to overlapping writes and silent data corruption?
>
> Similarly, if MD_SERIALIZE_POLICY is active, check_and_add_serial() in
> drivers/md/raid1.c will unconditionally dereference rdev->serial:
>
>      int idx = sector_to_idx(r1_bio->sector);
>      struct serial_in_rdev *serial = &rdev->serial[idx];
>      struct serial_info *head_si;
>
>      spin_lock_irqsave(&serial->serial_lock, flags);
>
> Can this cause a NULL pointer dereference for devices that failed
> initialization?

There is a per rdev flag CollisionCheck for protection, I don't think there will
be NULL pointer dereference.

>
> Signed-off-by: Chen Cheng <chencheng@xxxxxxxxx>
> ---
>   drivers/md/md-bitmap.c | 20 +++++++++++++++-----
>   drivers/md/md.c        | 33 ++++++++++++++++++++++-----------
>   drivers/md/md.h        |  2 +-
>   drivers/md/raid1.c     |  9 +++++++--
>   4 files changed, 45 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
> index 6d495cdf3fb2..e0510e3cae3e 100644
> --- a/drivers/md/md-bitmap.c
> +++ b/drivers/md/md-bitmap.c
> @@ -2211,12 +2211,15 @@ static int bitmap_load(struct mddev *mddev)
>   	struct md_rdev *rdev;
>   
>   	if (!bitmap)
>   		goto out;
>   
> -	rdev_for_each(rdev, mddev)
> -		mddev_create_serial_pool(mddev, rdev);
> +	rdev_for_each(rdev, mddev) {
> +		err = mddev_create_serial_pool(mddev, rdev);
> +		if (err)
> +			goto out;
> +	}
>   
>   	if (mddev_is_clustered(mddev))
>   		mddev->cluster_ops->load_bitmaps(mddev, mddev->bitmap_info.nodes);
>   
>   	/* Clear out old bitmap info first:  Either there is none, or we
> @@ -2868,18 +2871,25 @@ backlog_store(struct mddev *mddev, const char *buf, size_t len)
>   		/* serial_info_pool is not needed if backlog is zero */
>   		if (!test_bit(MD_SERIALIZE_POLICY, &mddev->flags))
>   			mddev_destroy_serial_pool(mddev, NULL);
>   	} else if (backlog && !mddev->serial_info_pool) {
>   		/* serial_info_pool is needed since backlog is not zero */
> -		rdev_for_each(rdev, mddev)
> -			mddev_create_serial_pool(mddev, rdev);
> +		rdev_for_each(rdev, mddev) {
> +			rv = mddev_create_serial_pool(mddev, rdev);
> +			if (rv) {
> +				mddev->bitmap_info.max_write_behind = old_mwb;
> +				mddev_destroy_serial_pool(mddev, NULL);
> +				goto out;
> +			}
> +		}
>   	}
>   	if (old_mwb != backlog)
>   		bitmap_update_sb(mddev->bitmap);
>   
> +out:
>   	mddev_unlock_and_resume(mddev);
> -	return len;
> +	return rv ?: len;
>   }
>   
>   static struct md_sysfs_entry bitmap_backlog =
>   __ATTR(backlog, S_IRUGO|S_IWUSR, backlog_show, backlog_store);
>   
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 25e06f088dc1..048ffd28869b 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -228,18 +228,19 @@ static int rdev_need_serial(struct md_rdev *rdev)
>   /*
>    * Init resource for rdev(s), then create serial_info_pool if:
>    * 1. rdev is the first device which return true from rdev_enable_serial.
>    * 2. rdev is NULL, means we want to enable serialization for all rdevs.
>    */
> -void mddev_create_serial_pool(struct mddev *mddev, struct md_rdev *rdev)
> +int mddev_create_serial_pool(struct mddev *mddev, struct md_rdev *rdev)
>   {
>   	int ret = 0;
>   	unsigned int noio_flags;
>   
>   	if (rdev && !rdev_need_serial(rdev) &&
> -	    !test_bit(CollisionCheck, &rdev->flags))
> -		return;
> +	    !test_bit(CollisionCheck, &rdev->flags) &&
> +	    !test_bit(MD_SERIALIZE_POLICY, &mddev->flags))
> +		return 0;
>   
>   	noio_flags = memalloc_noio_save();
>   	if (!rdev)
>   		ret = rdevs_init_serial(mddev);
>   	else
> @@ -248,18 +249,22 @@ void mddev_create_serial_pool(struct mddev *mddev, struct md_rdev *rdev)
>   		goto out;
>   
>   	if (mddev->serial_info_pool == NULL) {
>   		mddev->serial_info_pool =
>   			mempool_create_kmalloc_pool(NR_SERIAL_INFOS,
> -						sizeof(struct serial_info));
> +						    sizeof(struct serial_info));
>   		if (!mddev->serial_info_pool) {
> +			if (rdev)
> +				rdev_uninit_serial(rdev);
>   			rdevs_uninit_serial(mddev);
>   			pr_err("can't alloc memory pool for serialization\n");
> +			ret = -ENOMEM;
>   		}
>   	}
>   out:
>   	memalloc_noio_restore(noio_flags);
> +	return ret;
>   }
>   
>   /*
>    * Free resource from rdev(s), and destroy serial_info_pool under conditions:
>    * 1. rdev is the last device flaged with CollisionCheck.
> @@ -2605,12 +2610,15 @@ static int bind_rdev_to_array(struct md_rdev *rdev, struct mddev *mddev)
>   	strreplace(b, '/', '!');
>   
>   	rdev->mddev = mddev;
>   	pr_debug("md: bind<%s>\n", b);
>   
> -	if (mddev->raid_disks)
> -		mddev_create_serial_pool(mddev, rdev);
> +	if (mddev->raid_disks) {
> +		err = mddev_create_serial_pool(mddev, rdev);
> +		if (err)
> +			return err;
> +	}
>   
>   	if ((err = kobject_add(&rdev->kobj, &mddev->kobj, "dev-%s", b)))
>   		goto fail;
>   
>   	/* failure here is OK */
> @@ -3118,13 +3126,15 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len)
>   				md_new_event();
>   			}
>   		}
>   	} else if (cmd_match(buf, "writemostly")) {
>   		set_bit(WriteMostly, &rdev->flags);
> -		mddev_create_serial_pool(rdev->mddev, rdev);
> -		need_update_sb = true;
> -		err = 0;
> +		err = mddev_create_serial_pool(rdev->mddev, rdev);
> +		if (err)
> +			clear_bit(WriteMostly, &rdev->flags);
> +		else
> +			need_update_sb = true;
>   	} else if (cmd_match(buf, "-writemostly")) {
>   		mddev_destroy_serial_pool(rdev->mddev, rdev);
>   		clear_bit(WriteMostly, &rdev->flags);
>   		need_update_sb = true;
>   		err = 0;
> @@ -5943,12 +5953,13 @@ serialize_policy_store(struct mddev *mddev, const char *buf, size_t len)
>   		err = -EINVAL;
>   		goto unlock;
>   	}
>   
>   	if (value) {
> -		mddev_create_serial_pool(mddev, NULL);
> -		set_bit(MD_SERIALIZE_POLICY, &mddev->flags);
> +		err = mddev_create_serial_pool(mddev, NULL);
> +		if (!err)
> +			set_bit(MD_SERIALIZE_POLICY, &mddev->flags);
>   	} else {
>   		mddev_destroy_serial_pool(mddev, NULL);
>   		clear_bit(MD_SERIALIZE_POLICY, &mddev->flags);
>   	}
>   unlock:
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 76488cd9e81e..e6ea0cc3669a 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -959,11 +959,11 @@ extern void mddev_resume(struct mddev *mddev);
>   extern void md_idle_sync_thread(struct mddev *mddev);
>   extern void md_frozen_sync_thread(struct mddev *mddev);
>   extern void md_unfrozen_sync_thread(struct mddev *mddev);
>   
>   extern void md_update_sb(struct mddev *mddev, int force);
> -extern void mddev_create_serial_pool(struct mddev *mddev, struct md_rdev *rdev);
> +extern int mddev_create_serial_pool(struct mddev *mddev, struct md_rdev *rdev);
>   extern void mddev_destroy_serial_pool(struct mddev *mddev,
>   				      struct md_rdev *rdev);
>   struct md_rdev *md_find_rdev_nr_rcu(struct mddev *mddev, int nr);
>   struct md_rdev *md_find_rdev_rcu(struct mddev *mddev, dev_t dev);
>   
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 5b9368bd9e70..ef3812806f30 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -90,11 +90,11 @@ static int check_and_add_serial(struct md_rdev *rdev, struct r1bio *r1_bio,
>   static void wait_for_serialization(struct md_rdev *rdev, struct r1bio *r1_bio)
>   {
>   	struct mddev *mddev = rdev->mddev;
>   	struct serial_info *si;
>   
> -	if (WARN_ON(!mddev->serial_info_pool))
> +	if (WARN_ON(!mddev->serial_info_pool || !rdev->serial))
>   		return;
>   	si = mempool_alloc(mddev->serial_info_pool, GFP_NOIO);
>   	INIT_LIST_HEAD(&si->waiters);
>   	INIT_LIST_HEAD(&si->list_node);
>   	init_completion(&si->ready);
> @@ -109,11 +109,16 @@ static void remove_serial(struct md_rdev *rdev, sector_t lo, sector_t hi)
>   	struct serial_info *si, *iter_si;
>   	unsigned long flags;
>   	int found = 0;
>   	struct mddev *mddev = rdev->mddev;
>   	int idx = sector_to_idx(lo);
> -	struct serial_in_rdev *serial = &rdev->serial[idx];
> +	struct serial_in_rdev *serial;
> +
> +	if (WARN_ON(!mddev->serial_info_pool || !rdev->serial))
> +		return;
> +
> +	serial = &rdev->serial[idx];
>   
>   	spin_lock_irqsave(&serial->serial_lock, flags);
>   	for (si = raid1_rb_iter_first(&serial->serial_rb, lo, hi);
>   	     si; si = raid1_rb_iter_next(si, lo, hi)) {
>   		if (si->start == lo && si->last == hi) {

-- 
Thanks,
Kuai