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: <d95b41a0-c1aa-8b8b-d959-05e41eee3a7f@huaweicloud.com>
Date: Tue, 27 May 2025 15:53:06 +0800
From: Yu Kuai <yukuai1@...weicloud.com>
To: Hannes Reinecke <hare@...e.de>, Yu Kuai <yukuai1@...weicloud.com>,
 hch@....de, xni@...hat.com, colyli@...nel.org, song@...nel.org
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, "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH 07/23] md/md-bitmap: delay registration of bitmap_ops
 until creating bitmap

Hi,

在 2025/05/27 14:13, Hannes Reinecke 写道:
> 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.

I do not expect the bitmap entries are checked by udev, otherwise this
set can introduce regressions since the bitmap entries are no longer
existed after using the new biltmap.

And the above KOBJ_ADD uevent is used for mddev->kobj, right? In this
case, we're creating new entries under mddev->kobj, should this be
KOBJ_CHANGE?

Thanks,
Kuai

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