Received-SPF: pass (google.com: domain of linux-kernel+bounces-9265-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20231221104343.5557-1-2045gemini@gmail.com> <abc324aa-1ccc-c8fd-1437-a77465f6e4be@huaweicloud.com>
 <CAOPYjvbfGZObUa+P5Bo_syLMpyMNEPU6SNm6xJPSqSZYREmNfw@mail.gmail.com>
In-Reply-To: <CAOPYjvbfGZObUa+P5Bo_syLMpyMNEPU6SNm6xJPSqSZYREmNfw@mail.gmail.com>
From: 20 39 <2045gemini@gmail.com>
Date: Fri, 22 Dec 2023 10:34:58 +0800
Message-ID: <CAOPYjvYhEzeF3vdd9GXCX+k_-OmsE1yP7VNozcMt4vOyFLDAfw@mail.gmail.com>
Subject: Re: [PATCH] md/raid5: fix atomicity violation in raid5_cache_count
To: Yu Kuai <yukuai1@huaweicloud.com>
Cc: song@kernel.org, linux-raid@vger.kernel.org, linux-kernel@vger.kernel.org, 
	baijiaju1990@outlook.com, BassCheck <bass@buaa.edu.cn>, 
	"yukuai (C)" <yukuai3@huawei.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

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.

2. Comment on Unlikely Scenario: Regarding the comment in the code /*
unlikely, but not impossible */, it seems to acknowledge the
possibility of a race condition where conf->max_nr_stripes is less
than conf->min_nr_stripes. This comment implies that previous
developers might have noticed potential issues with race conditions,
even if they are considered unlikely.

3. Use of Local Variables Instead of Locks: In addressing this issue,
my approach doesn't involve introducing additional locks, which could
potentially lead to deadlocks or other concurrency problems. Instead,
I've opted to store the values in local variables to ensure that the
check remains effective and consistent throughout its execution. This
modification aims to prevent reading intermediate values that might
lead to incorrect calculations, without introducing new locking
complexities.

I'm looking forward to any further insights or suggestions you might
have on this matter.

Best regards,
Han


20 39 <2045gemini@gmail.com> =E4=BA=8E2023=E5=B9=B412=E6=9C=8822=E6=97=A5=
=E5=91=A8=E4=BA=94 10:21=E5=86=99=E9=81=93=EF=BC=9A
>
> Hi Kuai,
>
> Thanks for your email and the insightful points you've raised. Let me cla=
rify a few aspects regarding the raid5_cache_count() and raid5_set_cache_si=
ze() functions.
>
> 1. Callback Function in setup_conf(): You mentioned that raid5_cache_coun=
t() 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 initiali=
zed 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 rec=
onfig_mutex isn't necessarily held. The documentation in shrinker.h indicat=
es potential invocation scenarios beyond the initial setup context.
>
> 2. Comment on Unlikely Scenario: Regarding the comment in the code /* unl=
ikely, but not impossible */, it seems to acknowledge the possibility of a =
race condition where conf->max_nr_stripes is less than conf->min_nr_stripes=
. This comment implies that previous developers might have noticed potentia=
l issues with race conditions, even if they are considered unlikely.
>
> 3. Use of Local Variables Instead of Locks: In addressing this issue, my =
approach doesn't involve introducing additional locks, which could potentia=
lly lead to deadlocks or other concurrency problems. Instead, I've opted to=
 store the values in local variables to ensure that the check remains effec=
tive and consistent throughout its execution. This modification aims to pre=
vent reading intermediate values that might lead to incorrect calculations,=
 without introducing new locking complexities.
>
> I'm looking forward to any further insights or suggestions you might have=
 on this matter.
>
> Best regards,
> Han
>
> Yu Kuai <yukuai1@huaweicloud.com> =E4=BA=8E2023=E5=B9=B412=E6=9C=8822=E6=
=97=A5=E5=91=A8=E4=BA=94 09:37=E5=86=99=E9=81=93=EF=BC=9A
>>
>> Hi,
>>
>> =E5=9C=A8 2023/12/21 18:43, Gui-Dong Han =E5=86=99=E9=81=93:
>> > In raid5_cache_count():
>> >       if (conf->max_nr_stripes < conf->min_nr_stripes)
>> >               return 0;
>> >       return conf->max_nr_stripes - conf->min_nr_stripes;
>> > The current check is ineffective, as the values could change immediate=
ly
>> > after being checked.
>> >
>> > In raid5_set_cache_size():
>> >       ...
>> >       conf->min_nr_stripes =3D size;
>> >       ...
>> >       while (size > conf->max_nr_stripes)
>> >               conf->min_nr_stripes =3D conf->max_nr_stripes;
>> >       ...
>> >
>>
>> raid5_cache_count() is called from setup_conf() where reconfig_mtuex is
>> held.
>>
>> raid5_set_cache_size() is called from:
>> 1) raid5_store_stripe_cache_size(), reconfig_mutex is held
>> 2) r5l_start() from raid5_add_disk(), reconfig_mutex is held
>> 3) raid_ctr(), reconfig_mutex is held
>>
>> So, how can they concurrent in the first place?
>>
>> Thanks,
>> Kuai
>>
>> > Due to intermediate value updates in raid5_set_cache_size(), concurren=
t
>> > execution of raid5_cache_count() and raid5_set_cache_size() may lead t=
o
>> > inconsistent reads of conf->max_nr_stripes and conf->min_nr_stripes.
>> > The current checks are ineffective as values could change immediately
>> > after being checked, raising the risk of conf->min_nr_stripes exceedin=
g
>> > conf->max_nr_stripes and potentially causing an integer overflow.
>> >
>> > This possible bug is found by an experimental static analysis tool
>> > developed by our team. This tool analyzes the locking APIs to extract
>> > function pairs that can be concurrently executed, and then analyzes th=
e
>> > instructions in the paired functions to identify possible concurrency =
bugs
>> > including data races and atomicity violations. The above possible bug =
is
>> > reported when our tool analyzes the source code of Linux 6.2.
>> >
>> > To resolve this issue, it is suggested to introduce local variables
>> > 'min_stripes' and 'max_stripes' in raid5_cache_count() to ensure the
>> > values remain stable throughout the check. Adding locks in
>> > raid5_cache_count() fails to resolve atomicity violations, as
>> > raid5_set_cache_size() may hold intermediate values of
>> > conf->min_nr_stripes while unlocked. With this patch applied, our tool=
 no
>> > longer reports the bug, with the kernel configuration allyesconfig for
>> > x86_64. Due to the lack of associated hardware, we cannot test the pat=
ch
>> > in runtime testing, and just verify it according to the code logic.
>> >
>> > Fixes: edbe83ab4c27e ("md/raid5: allow the stripe_cache to grow and ..=
.")
>> > Reported-by: BassCheck <bass@buaa.edu.cn>
>> > Signed-off-by: Gui-Dong Han <2045gemini@gmail.com>
>> > ---
>> >   drivers/md/raid5.c | 7 ++++---
>> >   1 file changed, 4 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> > index 8497880135ee..62ebf33402cc 100644
>> > --- a/drivers/md/raid5.c
>> > +++ b/drivers/md/raid5.c
>> > @@ -7390,11 +7390,12 @@ static unsigned long raid5_cache_count(struct =
shrinker *shrink,
>> >                                      struct shrink_control *sc)
>> >   {
>> >       struct r5conf *conf =3D shrink->private_data;
>> > -
>> > -     if (conf->max_nr_stripes < conf->min_nr_stripes)
>> > +     int max_stripes =3D conf->max_nr_stripes;
>> > +     int min_stripes =3D conf->min_nr_stripes;
>> > +     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)
>> >
>>