[PATCH] md: handle serial pool allocation failures

[PATCH] md: handle serial pool allocation failures

[Chen Cheng]

From: Chen Cheng <chencheng@fnnas.com>

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.

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?

Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
 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) {
-- 
2.54.0