linux-kernel - Re: [PATCH] md/raid5: fix atomicity violation in raid5_cache

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: <ae9fc764-4765-8c13-8d59-56bc8582c094@huaweicloud.com>
Date: Fri, 22 Dec 2023 10:53:16 +0800
From: Yu Kuai <yukuai1@...weicloud.com>
To: 20 39 <2045gemini@...il.com>, Yu Kuai <yukuai1@...weicloud.com>
Cc: song@...nel.org, linux-raid@...r.kernel.org,
 linux-kernel@...r.kernel.org, baijiaju1990@...look.com,
 BassCheck <bass@...a.edu.cn>, "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH] md/raid5: fix atomicity violation in raid5_cache_count

Hi,

在 2023/12/22 10:34, 20 39 写道:
> Hi Kuai,
> 
> Thank you for your patience. This email is essentially the same as my
> previous one, only now adjusted to plain text format. I apologize for
> any inconvenience caused earlier.
> 
> Thanks for your email and the insightful points you've raised. Let me
> clarify a few aspects regarding the raid5_cache_count() and
> raid5_set_cache_size() functions.
> 
> 1. Callback Function in setup_conf(): You mentioned that
> raid5_cache_count() is called from setup_conf() where reconfig_mutex
> is held. While this is true, it's important to note that
> raid5_cache_count() is actually initialized as a callback function in
> setup_conf(), as described in /include/linux/shrinker.h. This means it
> could be invoked later in a context where the reconfig_mutex isn't
> necessarily held. The documentation in shrinker.h indicates potential
> invocation scenarios beyond the initial setup context.

Yes, you're right. I misread the code. Then this patch looks good to me,
just one nit below.

>>>> @@ -7390,11 +7390,12 @@ static unsigned long raid5_cache_count(struct shrinker *shrink,
>>>>                                       struct shrink_control *sc)
>>>>    {
>>>>        struct r5conf *conf = shrink->private_data;
>>>> -
>>>> -     if (conf->max_nr_stripes < conf->min_nr_stripes)
>>>> +     int max_stripes = conf->max_nr_stripes;
>>>> +     int min_stripes = conf->min_nr_stripes;

Since read and write can concurrent, I'll suggest to use READ_ONCE() and
WRITE_ONCE() for max/min_nr_stripes.

Thanks,
Kuai
>>>> +     if (max_stripes < min_stripes)
>>>>                /* unlikely, but not impossible */
>>>>                return 0;
>>>> -     return conf->max_nr_stripes - conf->min_nr_stripes;
>>>> +     return max_stripes - min_stripes;
>>>>    }
>>>>
>>>>    static struct r5conf *setup_conf(struct mddev *mddev)
>>>>
>>>
> .
>