[PATCH v2 0/5] md: bitmap grow fixes

[PATCH v2 0/5] md: bitmap grow fixes

[Su Yue]

Hi, This v2 series is to fixes bugs exposed by mdadm/clustermd-autotest.
The bugs are not cluster md only but also bitmap related.

The series based on v7.0-rc7 passes tests with mdadm v4.6 without regression.  

v2:
    Add a dummy bitmap operations per Kuai's suggestion.

To Yu Kuai:
A NULL group can't used for internal_bitmap_group since
the entries from internal_bitmap_attrs should be under bitmap.
So instead of sysfs_create_groups(), sysfs_create_group() and sysfs_merge_group()
are still needed /sigh.

Su Yue (5):
  md/md-bitmap: call md_bitmap_create,destroy in location_store
  md/md-bitmap: add an extra sysfs argument to md_bitmap_create and
    destroy
  md/md-bitmap: add dummy bitmap ops for none to fix wrong bitmap offset
  md: skip ID_BITMAP_NONE when show available bitmap types
  md/md-bitmap: remove member group from bitmap_operations

 drivers/md/md-bitmap.c   | 121 ++++++++++++++++++++++++++++++++++++---
 drivers/md/md-bitmap.h   |   3 +-
 drivers/md/md-llbitmap.c |  13 ++++-
 drivers/md/md.c          |  55 +++++++++---------
 drivers/md/md.h          |   2 +
 5 files changed, 155 insertions(+), 39 deletions(-)

-- 
2.53.0

[PATCH v2 1/5] md/md-bitmap: call md_bitmap_create,destroy in location_store

[Su Yue]

If bitmap/location is present, mdadm will call update_array_info()
while growing bitmap from none to internal via location_store().
md_bitmap_create() is needed to set mddev->bitmap_ops otherwise
mddev->bitmap_ops->get_stats() in update_array_info() will trigger
kernel NULL pointer dereference.

Fixes: fb8cc3b0d9db ("md/md-bitmap: delay registration of bitmap_ops until creating bitmap")
Signed-off-by: Su Yue <glass.su@suse.com>
---
 drivers/md/md-bitmap.c | 11 ++++++++---
 drivers/md/md.c        |  4 ++--
 drivers/md/md.h        |  2 ++
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
index 83378c033c72..2f24aae05552 100644
--- a/drivers/md/md-bitmap.c
+++ b/drivers/md/md-bitmap.c
@@ -2618,7 +2618,7 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
 			goto out;
 		}
 
-		bitmap_destroy(mddev);
+		md_bitmap_destroy(mddev);
 		mddev->bitmap_info.offset = 0;
 		if (mddev->bitmap_info.file) {
 			struct file *f = mddev->bitmap_info.file;
@@ -2653,15 +2653,20 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
 				goto out;
 			}
 
+			/*
+			 * lockless bitmap shoudle have set bitmap_id
+			 * using bitmap_type, so always ID_BITMAP.
+			 */
+			mddev->bitmap_id = ID_BITMAP;
 			mddev->bitmap_info.offset = offset;
-			rv = bitmap_create(mddev);
+			rv = md_bitmap_create(mddev);
 			if (rv)
 				goto out;
 
 			rv = bitmap_load(mddev);
 			if (rv) {
 				mddev->bitmap_info.offset = 0;
-				bitmap_destroy(mddev);
+				md_bitmap_destroy(mddev);
 				goto out;
 			}
 		}
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 3ce6f9e9d38e..8b1ecc370ad6 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -6447,7 +6447,7 @@ static void md_safemode_timeout(struct timer_list *t)
 
 static int start_dirty_degraded;
 
-static int md_bitmap_create(struct mddev *mddev)
+int md_bitmap_create(struct mddev *mddev)
 {
 	if (mddev->bitmap_id == ID_BITMAP_NONE)
 		return -EINVAL;
@@ -6458,7 +6458,7 @@ static int md_bitmap_create(struct mddev *mddev)
 	return mddev->bitmap_ops->create(mddev);
 }
 
-static void md_bitmap_destroy(struct mddev *mddev)
+void md_bitmap_destroy(struct mddev *mddev)
 {
 	if (!md_bitmap_registered(mddev))
 		return;
diff --git a/drivers/md/md.h b/drivers/md/md.h
index ac84289664cd..ed69244af00d 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -895,6 +895,8 @@ static inline void safe_put_page(struct page *p)
 
 int register_md_submodule(struct md_submodule_head *msh);
 void unregister_md_submodule(struct md_submodule_head *msh);
+int md_bitmap_create(struct mddev *mddev);
+void md_bitmap_destroy(struct mddev *mddev);
 
 extern struct md_thread *md_register_thread(
 	void (*run)(struct md_thread *thread),
-- 
2.53.0

[PATCH v2 2/5] md/md-bitmap: add an extra sysfs argument to md_bitmap_create and destroy

[Su Yue]

For further use, no functional change.

Signed-off-by: Su Yue <glass.su@suse.com>
---
 drivers/md/md-bitmap.c |  6 +++---
 drivers/md/md.c        | 36 ++++++++++++++++++------------------
 drivers/md/md.h        |  4 ++--
 3 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
index 2f24aae05552..ac06c9647bf0 100644
--- a/drivers/md/md-bitmap.c
+++ b/drivers/md/md-bitmap.c
@@ -2618,7 +2618,7 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
 			goto out;
 		}
 
-		md_bitmap_destroy(mddev);
+		md_bitmap_destroy(mddev, true);
 		mddev->bitmap_info.offset = 0;
 		if (mddev->bitmap_info.file) {
 			struct file *f = mddev->bitmap_info.file;
@@ -2659,14 +2659,14 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
 			 */
 			mddev->bitmap_id = ID_BITMAP;
 			mddev->bitmap_info.offset = offset;
-			rv = md_bitmap_create(mddev);
+			rv = md_bitmap_create(mddev, true);
 			if (rv)
 				goto out;
 
 			rv = bitmap_load(mddev);
 			if (rv) {
 				mddev->bitmap_info.offset = 0;
-				md_bitmap_destroy(mddev);
+				md_bitmap_destroy(mddev, true);
 				goto out;
 			}
 		}
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 8b1ecc370ad6..d3c8f77b4fe3 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -678,7 +678,7 @@ static void active_io_release(struct percpu_ref *ref)
 
 static void no_op(struct percpu_ref *r) {}
 
-static bool mddev_set_bitmap_ops(struct mddev *mddev)
+static bool mddev_set_bitmap_ops(struct mddev *mddev, bool create_sysfs)
 {
 	struct bitmap_operations *old = mddev->bitmap_ops;
 	struct md_submodule_head *head;
@@ -703,7 +703,7 @@ static bool mddev_set_bitmap_ops(struct mddev *mddev)
 	mddev->bitmap_ops = (void *)head;
 	xa_unlock(&md_submodule);
 
-	if (!mddev_is_dm(mddev) && mddev->bitmap_ops->group) {
+	if (create_sysfs && !mddev_is_dm(mddev) && mddev->bitmap_ops->group) {
 		if (sysfs_create_group(&mddev->kobj, mddev->bitmap_ops->group))
 			pr_warn("md: cannot register extra bitmap attributes for %s\n",
 				mdname(mddev));
@@ -721,9 +721,9 @@ static bool mddev_set_bitmap_ops(struct mddev *mddev)
 	return false;
 }
 
-static void mddev_clear_bitmap_ops(struct mddev *mddev)
+static void mddev_clear_bitmap_ops(struct mddev *mddev, bool remove_sysfs)
 {
-	if (!mddev_is_dm(mddev) && mddev->bitmap_ops &&
+	if (remove_sysfs && !mddev_is_dm(mddev) && mddev->bitmap_ops &&
 	    mddev->bitmap_ops->group)
 		sysfs_remove_group(&mddev->kobj, mddev->bitmap_ops->group);
 
@@ -6447,24 +6447,24 @@ static void md_safemode_timeout(struct timer_list *t)
 
 static int start_dirty_degraded;
 
-int md_bitmap_create(struct mddev *mddev)
+int md_bitmap_create(struct mddev *mddev, bool create_sysfs)
 {
 	if (mddev->bitmap_id == ID_BITMAP_NONE)
 		return -EINVAL;
 
-	if (!mddev_set_bitmap_ops(mddev))
+	if (!mddev_set_bitmap_ops(mddev, create_sysfs))
 		return -ENOENT;
 
 	return mddev->bitmap_ops->create(mddev);
 }
 
-void md_bitmap_destroy(struct mddev *mddev)
+void md_bitmap_destroy(struct mddev *mddev, bool remove_sysfs)
 {
 	if (!md_bitmap_registered(mddev))
 		return;
 
 	mddev->bitmap_ops->destroy(mddev);
-	mddev_clear_bitmap_ops(mddev);
+	mddev_clear_bitmap_ops(mddev, remove_sysfs);
 }
 
 int md_run(struct mddev *mddev)
@@ -6612,7 +6612,7 @@ int md_run(struct mddev *mddev)
 	}
 	if (err == 0 && pers->sync_request &&
 	    (mddev->bitmap_info.file || mddev->bitmap_info.offset)) {
-		err = md_bitmap_create(mddev);
+		err = md_bitmap_create(mddev, true);
 		if (err)
 			pr_warn("%s: failed to create bitmap (%d)\n",
 				mdname(mddev), err);
@@ -6685,7 +6685,7 @@ int md_run(struct mddev *mddev)
 		pers->free(mddev, mddev->private);
 	mddev->private = NULL;
 	put_pers(pers);
-	md_bitmap_destroy(mddev);
+	md_bitmap_destroy(mddev, true);
 	return err;
 }
 EXPORT_SYMBOL_GPL(md_run);
@@ -6702,7 +6702,7 @@ int do_md_run(struct mddev *mddev)
 	if (md_bitmap_registered(mddev)) {
 		err = mddev->bitmap_ops->load(mddev);
 		if (err) {
-			md_bitmap_destroy(mddev);
+			md_bitmap_destroy(mddev, true);
 			goto out;
 		}
 	}
@@ -6903,7 +6903,7 @@ static void __md_stop(struct mddev *mddev)
 {
 	struct md_personality *pers = mddev->pers;
 
-	md_bitmap_destroy(mddev);
+	md_bitmap_destroy(mddev, true);
 	mddev_detach(mddev);
 	spin_lock(&mddev->lock);
 	mddev->pers = NULL;
@@ -7680,16 +7680,16 @@ static int set_bitmap_file(struct mddev *mddev, int fd)
 	err = 0;
 	if (mddev->pers) {
 		if (fd >= 0) {
-			err = md_bitmap_create(mddev);
+			err = md_bitmap_create(mddev, true);
 			if (!err)
 				err = mddev->bitmap_ops->load(mddev);
 
 			if (err) {
-				md_bitmap_destroy(mddev);
+				md_bitmap_destroy(mddev, true);
 				fd = -1;
 			}
 		} else if (fd < 0) {
-			md_bitmap_destroy(mddev);
+			md_bitmap_destroy(mddev, true);
 		}
 	}
 
@@ -7996,12 +7996,12 @@ static int update_array_info(struct mddev *mddev, mdu_array_info_t *info)
 				mddev->bitmap_info.default_offset;
 			mddev->bitmap_info.space =
 				mddev->bitmap_info.default_space;
-			rv = md_bitmap_create(mddev);
+			rv = md_bitmap_create(mddev, true);
 			if (!rv)
 				rv = mddev->bitmap_ops->load(mddev);
 
 			if (rv)
-				md_bitmap_destroy(mddev);
+				md_bitmap_destroy(mddev, true);
 		} else {
 			struct md_bitmap_stats stats;
 
@@ -8027,7 +8027,7 @@ static int update_array_info(struct mddev *mddev, mdu_array_info_t *info)
 				put_cluster_ops(mddev);
 				mddev->safemode_delay = DEFAULT_SAFEMODE_DELAY;
 			}
-			md_bitmap_destroy(mddev);
+			md_bitmap_destroy(mddev, true);
 			mddev->bitmap_info.offset = 0;
 		}
 	}
diff --git a/drivers/md/md.h b/drivers/md/md.h
index ed69244af00d..4aaba3d7015c 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -895,8 +895,8 @@ static inline void safe_put_page(struct page *p)
 
 int register_md_submodule(struct md_submodule_head *msh);
 void unregister_md_submodule(struct md_submodule_head *msh);
-int md_bitmap_create(struct mddev *mddev);
-void md_bitmap_destroy(struct mddev *mddev);
+int md_bitmap_create(struct mddev *mddev, bool create_sysfs);
+void md_bitmap_destroy(struct mddev *mddev, bool remove_sysfs);
 
 extern struct md_thread *md_register_thread(
 	void (*run)(struct md_thread *thread),
-- 
2.53.0

[PATCH v2 3/5] md/md-bitmap: add dummy bitmap ops for none to fix wrong bitmap offset

[Su Yue]

Before commit fb8cc3b0d9db ("md/md-bitmap: delay registration of bitmap_ops until creating bitmap")
if CONFIG_MD_BITMAP is enabled, both bitmap none, internal and clustered have
the sysfs file bitmap/location.

After the commit, if bitmap is none, bitmap/location doesn't exist anymore.
It breaks 'grow' behavior of a md array of madam with MD_FEATURE_BITMAP_OFFSET.
Take level=mirror and metadata=1.2 as an example:

$ mdadm  --create  /dev/md0 -f --bitmap=none --raid-devices=2 --level=mirror \
  --metadata=1.2 /dev/vdd /dev/vde
$ mdadm --grow /dev/md0 --bitmap=internal
$ cat /sys/block/md0/md/bitmap/location
Before:+8
After: +2

While growing bitmap from none to internal, clustered and llbitmap,
mdadm/Grow.c:Grow_addbitmap() tries to detect bitmap/location first.
1)If bitmap/location exists, it sets bitmap/location after getinfo_super().
2)If bitmap/location doesn't exist, mdadm just calls md_set_array_info() then
mddev->bitmap_info.default_offset will be used.
Situation can be worse if growing none to clustered, bitmap offset of the node
calling `madm --grow` will be changed but the other node are reading bitmap sb from
the old location.

Here introducing a dummy bitmap_operations for ID_BITMAP_NONE to restore sysfs
file bitmap/location for ID_BITMAP_NONE and ID_BITMAP.
location_store() now calls md_bitmap_(create|destroy) with false sysfs parameter then
(add,remove)_internal_bitmap() will be called to manage sysfs entries.

Fixes: fb8cc3b0d9db ("md/md-bitmap: delay registration of bitmap_ops until creating bitmap")
Suggested-by: Yu Kuai <yukuai@fnnas.com>
Signed-off-by: Su Yue <glass.su@suse.com>
---
 drivers/md/md-bitmap.c   | 116 ++++++++++++++++++++++++++++++++++++---
 drivers/md/md-bitmap.h   |   3 +
 drivers/md/md-llbitmap.c |  12 ++++
 drivers/md/md.c          |  16 +++---
 4 files changed, 130 insertions(+), 17 deletions(-)

diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
index ac06c9647bf0..a8a176428c61 100644
--- a/drivers/md/md-bitmap.c
+++ b/drivers/md/md-bitmap.c
@@ -240,6 +240,11 @@ static bool bitmap_enabled(void *data, bool flush)
 	       bitmap->storage.filemap != NULL;
 }
 
+static bool dummy_bitmap_enabled(void *data, bool flush)
+{
+	return false;
+}
+
 /*
  * check a page and, if necessary, allocate it (or hijack it if the alloc fails)
  *
@@ -2201,6 +2206,11 @@ static int bitmap_create(struct mddev *mddev)
 	return 0;
 }
 
+static int dummy_bitmap_create(struct mddev *mddev)
+{
+	return 0;
+}
+
 static int bitmap_load(struct mddev *mddev)
 {
 	int err = 0;
@@ -2594,6 +2604,9 @@ location_show(struct mddev *mddev, char *page)
 	return len;
 }
 
+static int create_internal_group(struct mddev *mddev);
+static void remove_internal_group(struct mddev *mddev);
+
 static ssize_t
 location_store(struct mddev *mddev, const char *buf, size_t len)
 {
@@ -2618,7 +2631,8 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
 			goto out;
 		}
 
-		md_bitmap_destroy(mddev, true);
+		md_bitmap_destroy(mddev, false);
+		remove_internal_group(mddev);
 		mddev->bitmap_info.offset = 0;
 		if (mddev->bitmap_info.file) {
 			struct file *f = mddev->bitmap_info.file;
@@ -2659,14 +2673,22 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
 			 */
 			mddev->bitmap_id = ID_BITMAP;
 			mddev->bitmap_info.offset = offset;
-			rv = md_bitmap_create(mddev, true);
+			rv = md_bitmap_create(mddev, false);
 			if (rv)
 				goto out;
 
+			rv = create_internal_group(mddev);
+			if (rv) {
+				mddev->bitmap_info.offset = 0;
+				bitmap_destroy(mddev);
+				goto out;
+			}
+
 			rv = bitmap_load(mddev);
 			if (rv) {
+				remove_internal_group(mddev);
 				mddev->bitmap_info.offset = 0;
-				md_bitmap_destroy(mddev, true);
+				md_bitmap_destroy(mddev, false);
 				goto out;
 			}
 		}
@@ -2960,8 +2982,7 @@ static struct md_sysfs_entry max_backlog_used =
 __ATTR(max_backlog_used, S_IRUGO | S_IWUSR,
        behind_writes_used_show, behind_writes_used_reset);
 
-static struct attribute *md_bitmap_attrs[] = {
-	&bitmap_location.attr,
+static struct attribute *internal_bitmap_attrs[] = {
 	&bitmap_space.attr,
 	&bitmap_timeout.attr,
 	&bitmap_backlog.attr,
@@ -2972,11 +2993,57 @@ static struct attribute *md_bitmap_attrs[] = {
 	NULL
 };
 
-static struct attribute_group md_bitmap_group = {
+static struct attribute_group internal_bitmap_group = {
 	.name = "bitmap",
-	.attrs = md_bitmap_attrs,
+	.attrs = internal_bitmap_attrs,
 };
 
+/* Only necessary attrs for compatibility */
+static struct attribute *common_bitmap_attrs[] = {
+	&bitmap_location.attr,
+	NULL
+};
+
+static const struct attribute_group common_bitmap_group = {
+	.name = "bitmap",
+	.attrs = common_bitmap_attrs,
+};
+
+static int create_internal_group(struct mddev *mddev)
+{
+	/*
+	 * md_bitmap_group and md_bitmap_common_group are using same name
+	 * 'bitmap'.
+	 */
+	return sysfs_merge_group(&mddev->kobj, &internal_bitmap_group);
+}
+
+static void remove_internal_group(struct mddev *mddev)
+{
+	sysfs_unmerge_group(&mddev->kobj, &internal_bitmap_group);
+}
+
+static int bitmap_register_groups(struct mddev *mddev)
+{
+	int ret;
+
+	ret = sysfs_create_group(&mddev->kobj, &common_bitmap_group);
+
+	if (ret)
+		return ret;
+
+	ret = sysfs_merge_group(&mddev->kobj, &internal_bitmap_group);
+	if (ret)
+		sysfs_remove_group(&mddev->kobj, &common_bitmap_group);
+
+	return ret;
+}
+
+static void bitmap_unregister_groups(struct mddev *mddev)
+{
+	sysfs_unmerge_group(&mddev->kobj, &internal_bitmap_group);
+}
+
 static struct bitmap_operations bitmap_ops = {
 	.head = {
 		.type	= MD_BITMAP,
@@ -3018,21 +3085,52 @@ static struct bitmap_operations bitmap_ops = {
 	.set_pages		= bitmap_set_pages,
 	.free			= md_bitmap_free,
 
-	.group			= &md_bitmap_group,
+	.register_groups	= bitmap_register_groups,
+	.unregister_groups	= bitmap_unregister_groups,
+};
+
+static int none_bitmap_register_groups(struct mddev *mddev)
+{
+	return sysfs_create_group(&mddev->kobj, &common_bitmap_group);
+}
+
+static struct bitmap_operations none_bitmap_ops = {
+	.head = {
+		.type	= MD_BITMAP,
+		.id	= ID_BITMAP_NONE,
+		.name	= "none",
+	},
+
+	.enabled		= dummy_bitmap_enabled,
+	.create			= dummy_bitmap_create,
+	.destroy		= bitmap_destroy,
+	.load			= bitmap_load,
+	.get_stats		= bitmap_get_stats,
+	.free			= md_bitmap_free,
+
+	.register_groups	= none_bitmap_register_groups,
+	.unregister_groups	= NULL,
 };
 
 int md_bitmap_init(void)
 {
+	int ret;
+
 	md_bitmap_wq = alloc_workqueue("md_bitmap", WQ_MEM_RECLAIM | WQ_UNBOUND,
 				       0);
 	if (!md_bitmap_wq)
 		return -ENOMEM;
 
-	return register_md_submodule(&bitmap_ops.head);
+	ret = register_md_submodule(&bitmap_ops.head);
+	if (ret)
+		return ret;
+
+	return register_md_submodule(&none_bitmap_ops.head);
 }
 
 void md_bitmap_exit(void)
 {
 	destroy_workqueue(md_bitmap_wq);
 	unregister_md_submodule(&bitmap_ops.head);
+	unregister_md_submodule(&none_bitmap_ops.head);
 }
diff --git a/drivers/md/md-bitmap.h b/drivers/md/md-bitmap.h
index b42a28fa83a0..10bc6854798c 100644
--- a/drivers/md/md-bitmap.h
+++ b/drivers/md/md-bitmap.h
@@ -125,6 +125,9 @@ struct bitmap_operations {
 	void (*set_pages)(void *data, unsigned long pages);
 	void (*free)(void *data);
 
+	int (*register_groups)(struct mddev *mddev);
+	void (*unregister_groups)(struct mddev *mddev);
+
 	struct attribute_group *group;
 };
 
diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
index bf398d7476b3..9b3ea4f1d268 100644
--- a/drivers/md/md-llbitmap.c
+++ b/drivers/md/md-llbitmap.c
@@ -1561,6 +1561,16 @@ static struct attribute_group md_llbitmap_group = {
 	.attrs = md_llbitmap_attrs,
 };
 
+static int llbitmap_register_groups(struct mddev *mddev)
+{
+	return sysfs_create_group(&mddev->kobj, &md_llbitmap_group);
+}
+
+static void llbitmap_unregister_groups(struct mddev *mddev)
+{
+	sysfs_remove_group(&mddev->kobj, &md_llbitmap_group);
+}
+
 static struct bitmap_operations llbitmap_ops = {
 	.head = {
 		.type	= MD_BITMAP,
@@ -1597,6 +1607,8 @@ static struct bitmap_operations llbitmap_ops = {
 	.dirty_bits		= llbitmap_dirty_bits,
 	.write_all		= llbitmap_write_all,
 
+	.register_groups	= llbitmap_register_groups,
+	.unregister_groups	= llbitmap_unregister_groups,
 	.group			= &md_llbitmap_group,
 };
 
diff --git a/drivers/md/md.c b/drivers/md/md.c
index d3c8f77b4fe3..55a95b227b83 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -683,8 +683,7 @@ static bool mddev_set_bitmap_ops(struct mddev *mddev, bool create_sysfs)
 	struct bitmap_operations *old = mddev->bitmap_ops;
 	struct md_submodule_head *head;
 
-	if (mddev->bitmap_id == ID_BITMAP_NONE ||
-	    (old && old->head.id == mddev->bitmap_id))
+	if (old && old->head.id == mddev->bitmap_id)
 		return true;
 
 	xa_lock(&md_submodule);
@@ -703,8 +702,8 @@ static bool mddev_set_bitmap_ops(struct mddev *mddev, bool create_sysfs)
 	mddev->bitmap_ops = (void *)head;
 	xa_unlock(&md_submodule);
 
-	if (create_sysfs && !mddev_is_dm(mddev) && mddev->bitmap_ops->group) {
-		if (sysfs_create_group(&mddev->kobj, mddev->bitmap_ops->group))
+	if (create_sysfs && !mddev_is_dm(mddev) && mddev->bitmap_ops->register_groups) {
+		if (mddev->bitmap_ops->register_groups(mddev))
 			pr_warn("md: cannot register extra bitmap attributes for %s\n",
 				mdname(mddev));
 		else
@@ -724,8 +723,8 @@ static bool mddev_set_bitmap_ops(struct mddev *mddev, bool create_sysfs)
 static void mddev_clear_bitmap_ops(struct mddev *mddev, bool remove_sysfs)
 {
 	if (remove_sysfs && !mddev_is_dm(mddev) && mddev->bitmap_ops &&
-	    mddev->bitmap_ops->group)
-		sysfs_remove_group(&mddev->kobj, mddev->bitmap_ops->group);
+	    mddev->bitmap_ops->unregister_groups)
+		mddev->bitmap_ops->unregister_groups(mddev);
 
 	mddev->bitmap_ops = NULL;
 }
@@ -6610,8 +6609,9 @@ int md_run(struct mddev *mddev)
 			(unsigned long long)pers->size(mddev, 0, 0) / 2);
 		err = -EINVAL;
 	}
-	if (err == 0 && pers->sync_request &&
-	    (mddev->bitmap_info.file || mddev->bitmap_info.offset)) {
+	if (err == 0 && pers->sync_request) {
+		if (mddev->bitmap_info.offset == 0)
+			mddev->bitmap_id = ID_BITMAP_NONE;
 		err = md_bitmap_create(mddev, true);
 		if (err)
 			pr_warn("%s: failed to create bitmap (%d)\n",
-- 
2.53.0

[PATCH v2 4/5] md: skip ID_BITMAP_NONE when show available bitmap types

[Su Yue]

As none_bitmap_ops is introduced, ID_BITMAP_NONE should be skipped while
iterating md submodules, otherwise:

$ cat /sys/block/md0/md/bitmap_type
[none] bitmap llbitmap [none]

Signed-off-by: Su Yue <glass.su@suse.com>
---
 drivers/md/md.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 55a95b227b83..20a953676319 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -4269,6 +4269,8 @@ bitmap_type_show(struct mddev *mddev, char *page)
 	xa_for_each(&md_submodule, i, head) {
 		if (head->type != MD_BITMAP)
 			continue;
+		if (head->id == ID_BITMAP_NONE)
+			continue;
 
 		if (mddev->bitmap_id == head->id)
 			len += sprintf(page + len, "[%s] ", head->name);
-- 
2.53.0

[PATCH v2 5/5] md/md-bitmap: remove member group from bitmap_operations

[Su Yue]

The member is unused now so remove it.

Signed-off-by: Su Yue <glass.su@suse.com>
---
 drivers/md/md-bitmap.h   | 2 --
 drivers/md/md-llbitmap.c | 1 -
 2 files changed, 3 deletions(-)

diff --git a/drivers/md/md-bitmap.h b/drivers/md/md-bitmap.h
index 10bc6854798c..7fba97a09b66 100644
--- a/drivers/md/md-bitmap.h
+++ b/drivers/md/md-bitmap.h
@@ -127,8 +127,6 @@ struct bitmap_operations {
 
 	int (*register_groups)(struct mddev *mddev);
 	void (*unregister_groups)(struct mddev *mddev);
-
-	struct attribute_group *group;
 };
 
 /* the bitmap API */
diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
index 9b3ea4f1d268..4fb9003cdc49 100644
--- a/drivers/md/md-llbitmap.c
+++ b/drivers/md/md-llbitmap.c
@@ -1609,7 +1609,6 @@ static struct bitmap_operations llbitmap_ops = {
 
 	.register_groups	= llbitmap_register_groups,
 	.unregister_groups	= llbitmap_unregister_groups,
-	.group			= &md_llbitmap_group,
 };
 
 int md_llbitmap_init(void)
-- 
2.53.0

Re: [PATCH v2 1/5] md/md-bitmap: call md_bitmap_create,destroy in location_store

[Li Nan]

在 2026/4/7 18:26, Su Yue 写道:
> If bitmap/location is present, mdadm will call update_array_info()
> while growing bitmap from none to internal via location_store().
> md_bitmap_create() is needed to set mddev->bitmap_ops otherwise
> mddev->bitmap_ops->get_stats() in update_array_info() will trigger
> kernel NULL pointer dereference.
> 
> Fixes: fb8cc3b0d9db ("md/md-bitmap: delay registration of bitmap_ops until creating bitmap")
> Signed-off-by: Su Yue <glass.su@suse.com>
> ---
>   drivers/md/md-bitmap.c | 11 ++++++++---
>   drivers/md/md.c        |  4 ++--
>   drivers/md/md.h        |  2 ++
>   3 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
> index 83378c033c72..2f24aae05552 100644
> --- a/drivers/md/md-bitmap.c
> +++ b/drivers/md/md-bitmap.c
> @@ -2618,7 +2618,7 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
>   			goto out;
>   		}
>   
> -		bitmap_destroy(mddev);
> +		md_bitmap_destroy(mddev);
>   		mddev->bitmap_info.offset = 0;
>   		if (mddev->bitmap_info.file) {
>   			struct file *f = mddev->bitmap_info.file;
> @@ -2653,15 +2653,20 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
>   				goto out;
>   			}
>   
> +			/*
> +			 * lockless bitmap shoudle have set bitmap_id
> +			 * using bitmap_type, so always ID_BITMAP.
> +			 */
> +			mddev->bitmap_id = ID_BITMAP;
>   			mddev->bitmap_info.offset = offset;
> -			rv = bitmap_create(mddev);
> +			rv = md_bitmap_create(mddev);
>   			if (rv)
>   				goto out;
>   
>   			rv = bitmap_load(mddev);

mddev->bitmap_ops->load() should also be used here.

-- 
Thanks,
Nan

Re: [PATCH v2 4/5] md: skip ID_BITMAP_NONE when show available bitmap types

[Li Nan]

在 2026/4/7 18:26, Su Yue 写道:
> As none_bitmap_ops is introduced, ID_BITMAP_NONE should be skipped while
> iterating md submodules, otherwise:
> 
> $ cat /sys/block/md0/md/bitmap_type
> [none] bitmap llbitmap [none]
> 
> Signed-off-by: Su Yue <glass.su@suse.com>
> ---
>   drivers/md/md.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 55a95b227b83..20a953676319 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -4269,6 +4269,8 @@ bitmap_type_show(struct mddev *mddev, char *page)
>   	xa_for_each(&md_submodule, i, head) {
>   		if (head->type != MD_BITMAP)
>   			continue;
> +		if (head->id == ID_BITMAP_NONE)
> +			continue;
>   
>   		if (mddev->bitmap_id == head->id)
>   			len += sprintf(page + len, "[%s] ", head->name);

If we indeed add ID_BITMAP_NONE as patch 3, would it be better to delete
this piece of code instead?

         if (mddev->bitmap_id == ID_BITMAP_NONE)
                 len += sprintf(page + len, "[none] ");
         else
                 len += sprintf(page + len, "none ");

-- 
Thanks,
Nan

Re: [PATCH v2 1/5] md/md-bitmap: call md_bitmap_create,destroy in location_store

[Su Yue]

aOn Mon 13 Apr 2026 at 15:47, Li Nan <linan666@huaweicloud.com> 
wrote:

> 在 2026/4/7 18:26, Su Yue 写道:
>> If bitmap/location is present, mdadm will call 
>> update_array_info()
>> while growing bitmap from none to internal via 
>> location_store().
>> md_bitmap_create() is needed to set mddev->bitmap_ops otherwise
>> mddev->bitmap_ops->get_stats() in update_array_info() will 
>> trigger
>> kernel NULL pointer dereference.
>> Fixes: fb8cc3b0d9db ("md/md-bitmap: delay registration of 
>> bitmap_ops until
>> creating bitmap")
>> Signed-off-by: Su Yue <glass.su@suse.com>
>> ---
>>   drivers/md/md-bitmap.c | 11 ++++++++---
>>   drivers/md/md.c        |  4 ++--
>>   drivers/md/md.h        |  2 ++
>>   3 files changed, 12 insertions(+), 5 deletions(-)
>> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
>> index 83378c033c72..2f24aae05552 100644
>> --- a/drivers/md/md-bitmap.c
>> +++ b/drivers/md/md-bitmap.c
>> @@ -2618,7 +2618,7 @@ location_store(struct mddev *mddev, const 
>> char *buf, size_t len)
>>   			goto out;
>>   		}
>>   -		bitmap_destroy(mddev);
>> +		md_bitmap_destroy(mddev);
>>   		mddev->bitmap_info.offset = 0;
>>   		if (mddev->bitmap_info.file) {
>>   			struct file *f = mddev->bitmap_info.file;
>> @@ -2653,15 +2653,20 @@ location_store(struct mddev *mddev, 
>> const char *buf, size_t len)
>>   				goto out;
>>   			}
>>   +			/*
>> +			 * lockless bitmap shoudle have set bitmap_id
>> +			 * using bitmap_type, so always ID_BITMAP.
>> +			 */
>> +			mddev->bitmap_id = ID_BITMAP;
>>   			mddev->bitmap_info.offset = offset;
>> -			rv = bitmap_create(mddev);
>> +			rv = md_bitmap_create(mddev);
>>   			if (rv)
>>   				goto out;
>>     			rv = bitmap_load(mddev);
>
> mddev->bitmap_ops->load() should also be used here.

/NOD.
location_store() is only used for ID_BITMAP_NONE and ID_BITMAP, so
mddev->bitmap_ops->load() is always bitmap_load().
But for code consistency, mddev->bitmap_ops->load() is better, 
will fix it.

--
Su

Re: [PATCH v2 4/5] md: skip ID_BITMAP_NONE when show available bitmap types

[Su Yue]

On Mon 13 Apr 2026 at 16:15, Li Nan <linan666@huaweicloud.com> 
wrote:

> 在 2026/4/7 18:26, Su Yue 写道:
>> As none_bitmap_ops is introduced, ID_BITMAP_NONE should be 
>> skipped while
>> iterating md submodules, otherwise:
>> $ cat /sys/block/md0/md/bitmap_type
>> [none] bitmap llbitmap [none]
>> Signed-off-by: Su Yue <glass.su@suse.com>
>> ---
>>   drivers/md/md.c | 2 ++
>>   1 file changed, 2 insertions(+)
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 55a95b227b83..20a953676319 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -4269,6 +4269,8 @@ bitmap_type_show(struct mddev *mddev, 
>> char *page)
>>   	xa_for_each(&md_submodule, i, head) {
>>   		if (head->type != MD_BITMAP)
>>   			continue;
>> +		if (head->id == ID_BITMAP_NONE)
>> +			continue;
>>     		if (mddev->bitmap_id == head->id)
>>   			len += sprintf(page + len, "[%s] ", head->name);
>
> If we indeed add ID_BITMAP_NONE as patch 3, would it be better 
> to delete
> this piece of code instead?
>
I dont' think so. There is one case CONFIG_MD_BITMAP is not 
enabled which
means register_md_submodule(&none_bitmap_ops.head) is not called 
but in
mddev_init():

int mddev_init(struct mddev *mddev) 
|
 { 
 |
         int err = 0; 
         |
                                                                                 |
         if (!IS_ENABLED(CONFIG_MD_BITMAP)) 
         |
                 mddev->bitmap_id = ID_BITMAP_NONE;
...
}

--
Su
>
>         if (mddev->bitmap_id == ID_BITMAP_NONE)
>                 len += sprintf(page + len, "[none] ");
>         else
>                 len += sprintf(page + len, "none ");

Re: [PATCH v2 1/5] md/md-bitmap: call md_bitmap_create,destroy in location_store

[Xiao Ni]

On Tue, Apr 7, 2026 at 6:26 PM Su Yue <glass.su@suse.com> wrote:
>
> If bitmap/location is present, mdadm will call update_array_info()
> while growing bitmap from none to internal via location_store().
> md_bitmap_create() is needed to set mddev->bitmap_ops otherwise
> mddev->bitmap_ops->get_stats() in update_array_info() will trigger
> kernel NULL pointer dereference.


Hi Su Yue

How can bitmap/location be present when bitmap is none? Could you
provide the test commands that reproduce this problem?

mdadm -CR /dev/md0 -l1 -n2 /dev/loop0 /dev/loop1 --bitmap=none (There
is not bitmap/location, because bitmap directory is not created)
mdadm /dev/md0 --grow --bitmap=internal
Grow.c md_set_array_info runs
 451             array.state |= (1 << MD_SB_BITMAP_PRESENT);
 452             rv = md_set_array_info(fd, &array);
In kernel space, it runs
 8125             rv = md_bitmap_create(mddev);
 8126             if (!rv)
 8127                 rv = mddev->bitmap_ops->load(mddev);

Best Regards
Xiao

>
> Fixes: fb8cc3b0d9db ("md/md-bitmap: delay registration of bitmap_ops until creating bitmap")
> Signed-off-by: Su Yue <glass.su@suse.com>
> ---
>  drivers/md/md-bitmap.c | 11 ++++++++---
>  drivers/md/md.c        |  4 ++--
>  drivers/md/md.h        |  2 ++
>  3 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
> index 83378c033c72..2f24aae05552 100644
> --- a/drivers/md/md-bitmap.c
> +++ b/drivers/md/md-bitmap.c
> @@ -2618,7 +2618,7 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
>                         goto out;
>                 }
>
> -               bitmap_destroy(mddev);
> +               md_bitmap_destroy(mddev);
>                 mddev->bitmap_info.offset = 0;
>                 if (mddev->bitmap_info.file) {
>                         struct file *f = mddev->bitmap_info.file;
> @@ -2653,15 +2653,20 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
>                                 goto out;
>                         }
>
> +                       /*
> +                        * lockless bitmap shoudle have set bitmap_id
> +                        * using bitmap_type, so always ID_BITMAP.
> +                        */
> +                       mddev->bitmap_id = ID_BITMAP;
>                         mddev->bitmap_info.offset = offset;
> -                       rv = bitmap_create(mddev);
> +                       rv = md_bitmap_create(mddev);
>                         if (rv)
>                                 goto out;
>
>                         rv = bitmap_load(mddev);
>                         if (rv) {
>                                 mddev->bitmap_info.offset = 0;
> -                               bitmap_destroy(mddev);
> +                               md_bitmap_destroy(mddev);
>                                 goto out;
>                         }
>                 }
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 3ce6f9e9d38e..8b1ecc370ad6 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -6447,7 +6447,7 @@ static void md_safemode_timeout(struct timer_list *t)
>
>  static int start_dirty_degraded;
>
> -static int md_bitmap_create(struct mddev *mddev)
> +int md_bitmap_create(struct mddev *mddev)
>  {
>         if (mddev->bitmap_id == ID_BITMAP_NONE)
>                 return -EINVAL;
> @@ -6458,7 +6458,7 @@ static int md_bitmap_create(struct mddev *mddev)
>         return mddev->bitmap_ops->create(mddev);
>  }
>
> -static void md_bitmap_destroy(struct mddev *mddev)
> +void md_bitmap_destroy(struct mddev *mddev)
>  {
>         if (!md_bitmap_registered(mddev))
>                 return;
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index ac84289664cd..ed69244af00d 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -895,6 +895,8 @@ static inline void safe_put_page(struct page *p)
>
>  int register_md_submodule(struct md_submodule_head *msh);
>  void unregister_md_submodule(struct md_submodule_head *msh);
> +int md_bitmap_create(struct mddev *mddev);
> +void md_bitmap_destroy(struct mddev *mddev);
>
>  extern struct md_thread *md_register_thread(
>         void (*run)(struct md_thread *thread),
> --
> 2.53.0
>