| 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
| ||
|
Message-ID: <beeed703-ed7d-2973-c403-c74994962cb0@huaweicloud.com>
Date: Tue, 27 May 2025 15:43:40 +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 06/23] md/md-bitmap: add a new sysfs api bitmap_type
Hi,
在 2025/05/27 14:10, Hannes Reinecke 写道:
> On 5/24/25 08:13, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3@...wei.com>
>>
>> The api will be used by mdadm to set bitmap_ops while creating new array
>> or assemble array, prepare to add a new bitmap.
>>
>> Currently available options are:
>>
>> cat /sys/block/md0/md/bitmap_type
>> none [bitmap]
>>
>> Signed-off-by: Yu Kuai <yukuai3@...wei.com>
>> ---
>> Documentation/admin-guide/md.rst | 73 ++++++++++++++----------
>> drivers/md/md.c | 96 ++++++++++++++++++++++++++++++--
>> drivers/md/md.h | 2 +
>> 3 files changed, 135 insertions(+), 36 deletions(-)
>>
> [ .. ]
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 311e52d5173d..4eb0c6effd5b 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -672,13 +672,18 @@ static void active_io_release(struct percpu_ref
>> *ref)
>> static void no_op(struct percpu_ref *r) {}
>> -static void mddev_set_bitmap_ops(struct mddev *mddev, enum
>> md_submodule_id id)
>> +static bool mddev_set_bitmap_ops(struct mddev *mddev)
>> {
>> xa_lock(&md_submodule);
>> - mddev->bitmap_ops = xa_load(&md_submodule, id);
>> + mddev->bitmap_ops = 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", id);
>> +
>> + if (!mddev->bitmap_ops) {
>> + pr_warn_once("md: can't find bitmap id %d\n", mddev->bitmap_id);
>> + return false;
>> + }
>> +
>> + return true;
>> }
>> static void mddev_clear_bitmap_ops(struct mddev *mddev)
>> @@ -688,8 +693,10 @@ static void mddev_clear_bitmap_ops(struct mddev
>> *mddev)
>> int mddev_init(struct mddev *mddev)
>> {
>> - /* TODO: support more versions */
>> - mddev_set_bitmap_ops(mddev, ID_BITMAP);
>> + 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)) {
>> @@ -4155,6 +4162,82 @@ new_level_store(struct mddev *mddev, const char
>> *buf, size_t len)
>> static struct md_sysfs_entry md_new_level =
>> __ATTR(new_level, 0664, new_level_show, new_level_store);
>> +static ssize_t
>> +bitmap_type_show(struct mddev *mddev, char *page)
>> +{
>> + struct md_submodule_head *head;
>> + unsigned long i;
>> + ssize_t len = 0;
>> +
>> + if (mddev->bitmap_id == ID_BITMAP_NONE)
>> + len += sprintf(page + len, "[none] ");
>> + else
>> + len += sprintf(page + len, "none ");
>> +
>> + xa_lock(&md_submodule);
>> + xa_for_each(&md_submodule, i, head) {
>> + if (head->type != MD_BITMAP)
>> + continue;
>> +
>> + if (mddev->bitmap_id == head->id)
>> + len += sprintf(page + len, "[%s] ", head->name);
>> + else
>> + len += sprintf(page + len, "%s ", head->name);
>> + }
>> + xa_unlock(&md_submodule);
>> +
>> + len += sprintf(page + len, "\n");
>> + return len;
>> +}
>> +
>> +static ssize_t
>> +bitmap_type_store(struct mddev *mddev, const char *buf, size_t len)
>> +{
>> + struct md_submodule_head *head;
>> + enum md_submodule_id id;
>> + unsigned long i;
>> + int err;
>> +
>> + if (mddev->bitmap_ops)
>> + return -EBUSY;
>> +
> Why isn't this protected by md_submodule lock?
> The lock is taken when updating ->bitmap_ops, so I would
> have expected it to be taken when checking it ...
The design is that when bitmap is created, user can no longer set
bitmap_id, and it's right without the protecting there will be race
window.
>
>> + err = kstrtoint(buf, 10, &id);
>> + if (!err) {
>> + if (id == ID_BITMAP_NONE) {
>> + mddev->bitmap_id = id;
>> + return len;
>> + }
>> +
>> + xa_lock(&md_submodule);
>> + head = xa_load(&md_submodule, id);
>> + xa_unlock(&md_submodule);
>> +
>> + if (head && head->type == MD_BITMAP) {
>> + mddev->bitmap_id = id;
>> + return len;
>> + }
>> + }
>> +
>> + if (cmd_match(buf, "none")) {
>> + mddev->bitmap_id = ID_BITMAP_NONE;
>> + return len;
>> + }
>> +
> That is odd coding. The 'if (!err)' condition above might
> fall through to here, but then we already now that it cannot
> match 'none'.
The first kstrtoint() is trying to convert the input string to int id,
looks like I missed return -EINVAL if the id can't be found in
md_submodule.
> Please invert the logic, first check for 'none', and only
> call kstroint if the match failed.
Sure, this sounds better.
Thanks for the review!
Kuai
>
>> + xa_lock(&md_submodule);
>> + xa_for_each(&md_submodule, i, head) {
>> + if (head->type == MD_BITMAP && cmd_match(buf, head->name)) {
>> + mddev->bitmap_id = head->id;
>> + xa_unlock(&md_submodule);
>> + return len;
>> + }
>> + }
>> + xa_unlock(&md_submodule);
>> + return -ENOENT;
>> +}
>> +
>> +static struct md_sysfs_entry md_bitmap_type =
>> +__ATTR(bitmap_type, 0664, bitmap_type_show, bitmap_type_store);
>> +
>> static ssize_t
>> layout_show(struct mddev *mddev, char *page)
>> {
>> @@ -5719,6 +5802,7 @@ __ATTR(serialize_policy, S_IRUGO | S_IWUSR,
>> serialize_policy_show,
>> static struct attribute *md_default_attrs[] = {
>> &md_level.attr,
>> &md_new_level.attr,
>> + &md_bitmap_type.attr,
>> &md_layout.attr,
>> &md_raid_disks.attr,
>> &md_uuid.attr,
>> diff --git a/drivers/md/md.h b/drivers/md/md.h
>> index 13e3f9ce1b79..bf34c0a36551 100644
>> --- a/drivers/md/md.h
>> +++ b/drivers/md/md.h
>> @@ -40,6 +40,7 @@ enum md_submodule_id {
>> ID_CLUSTER,
>> ID_BITMAP,
>> ID_LLBITMAP, /* TODO */
>> + ID_BITMAP_NONE,
>> };
>> struct md_submodule_head {
>> @@ -565,6 +566,7 @@ struct mddev {
>> struct percpu_ref writes_pending;
>> int sync_checkers; /* # of threads checking
>> writes_pending */
>> + enum md_submodule_id bitmap_id;
>> void *bitmap; /* the bitmap for the device */
>> struct bitmap_operations *bitmap_ops;
>> struct {
>
> Cheers,
>
> Hannes
Powered by blists - more mailing lists