linux-kernel - Re: [PATCH 07/23] md/md-bitmap: delay registration of bitmap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d2fabdfd-229d-4043-ad27-61bac1e1f6d2@suse.de>
Date: Tue, 27 May 2025 08:13:56 +0200
From: Hannes Reinecke <hare@...e.de>
To: Yu Kuai <yukuai1@...weicloud.com>, hch@....de, xni@...hat.com,
 colyli@...nel.org, song@...nel.org, yukuai3@...wei.com
Cc: linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-raid@...r.kernel.org, yi.zhang@...wei.com, yangerkun@...wei.com,
 johnny.chenyi@...wei.com
Subject: Re: [PATCH 07/23] md/md-bitmap: delay registration of bitmap_ops
 until creating bitmap

On 5/24/25 08:13, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@...wei.com>
> 
> Currently bitmap_ops is registered while allocating mddev, this is fine
> when there is only one bitmap_ops, however, after introduing a new
> bitmap_ops, user space need a time window to choose which bitmap_ops to
> use while creating new array.
> 
> Signed-off-by: Yu Kuai <yukuai3@...wei.com>
> ---
>   drivers/md/md.c | 86 +++++++++++++++++++++++++++++++------------------
>   1 file changed, 55 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 4eb0c6effd5b..dc4b85f30e13 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -674,39 +674,50 @@ static void no_op(struct percpu_ref *r) {}
>   
>   static bool mddev_set_bitmap_ops(struct mddev *mddev)
>   {
> +	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))
> +		return true;
> +
>   	xa_lock(&md_submodule);
> -	mddev->bitmap_ops = xa_load(&md_submodule, mddev->bitmap_id);
> +	head = xa_load(&md_submodule, mddev->bitmap_id);
>   	xa_unlock(&md_submodule);
>   
> -	if (!mddev->bitmap_ops) {
> -		pr_warn_once("md: can't find bitmap id %d\n", mddev->bitmap_id);
> +	if (WARN_ON_ONCE(!head || head->type != MD_BITMAP)) {
> +		pr_err("md: can't find bitmap id %d\n", mddev->bitmap_id);
>   		return false;
>   	}
>   
> +	if (old && old->group)
> +		sysfs_remove_group(&mddev->kobj, old->group);
> +
> +	mddev->bitmap_ops = (void *)head;
> +	if (mddev->bitmap_ops && mddev->bitmap_ops->group &&
> +	    sysfs_create_group(&mddev->kobj, mddev->bitmap_ops->group))
> +		pr_warn("md: cannot register extra bitmap attributes for %s\n",
> +			mdname(mddev));
> +
>   	return true;
>   }
>   
>   static void mddev_clear_bitmap_ops(struct mddev *mddev)
>   {
> +	if (mddev->bitmap_ops && mddev->bitmap_ops->group)
> +		sysfs_remove_group(&mddev->kobj, mddev->bitmap_ops->group);
> +
>   	mddev->bitmap_ops = NULL;
>   }
>   
>   int mddev_init(struct mddev *mddev)
>   {
> -	mddev->bitmap_id = ID_BITMAP;
> -
> -	if (!mddev_set_bitmap_ops(mddev))
> -		return -EINVAL;
> -
>   	if (percpu_ref_init(&mddev->active_io, active_io_release,
> -			    PERCPU_REF_ALLOW_REINIT, GFP_KERNEL)) {
> -		mddev_clear_bitmap_ops(mddev);
> +			    PERCPU_REF_ALLOW_REINIT, GFP_KERNEL))
>   		return -ENOMEM;
> -	}
>   
>   	if (percpu_ref_init(&mddev->writes_pending, no_op,
>   			    PERCPU_REF_ALLOW_REINIT, GFP_KERNEL)) {
> -		mddev_clear_bitmap_ops(mddev);
>   		percpu_ref_exit(&mddev->active_io);
>   		return -ENOMEM;
>   	}
> @@ -734,6 +745,7 @@ int mddev_init(struct mddev *mddev)
>   	mddev->resync_min = 0;
>   	mddev->resync_max = MaxSector;
>   	mddev->level = LEVEL_NONE;
> +	mddev->bitmap_id = ID_BITMAP;
>   
>   	INIT_WORK(&mddev->sync_work, md_start_sync);
>   	INIT_WORK(&mddev->del_work, mddev_delayed_delete);
> @@ -744,7 +756,6 @@ EXPORT_SYMBOL_GPL(mddev_init);
>   
>   void mddev_destroy(struct mddev *mddev)
>   {
> -	mddev_clear_bitmap_ops(mddev);
>   	percpu_ref_exit(&mddev->active_io);
>   	percpu_ref_exit(&mddev->writes_pending);
>   }
> @@ -6093,11 +6104,6 @@ struct mddev *md_alloc(dev_t dev, char *name)
>   		return ERR_PTR(error);
>   	}
>   
> -	if (md_bitmap_registered(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));
> -
>   	kobject_uevent(&mddev->kobj, KOBJ_ADD);
>   	mddev->sysfs_state = sysfs_get_dirent_safe(mddev->kobj.sd, "array_state");
>   	mddev->sysfs_level = sysfs_get_dirent_safe(mddev->kobj.sd, "level");

But now you've killed udev event processing.
Once the 'add' event is sent _all_ sysfs attributes must be present,
otherwise you'll have a race condition where udev is checking for
attributes which are present only later.

So when moving things around ensure to move the kobject_uevent() call, too.

(ideally you would set the sysfs attributes when calling 'add_device()',
but not sure if that's possible here.)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@...e.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich