Messages in this thread | | | Date | Thu, 11 Jan 2024 09:51:25 +0800 | Subject | Re: [PATCH v2] md/raid5: fix atomicity violation in raid5_cache_count | From | Gui-Dong Han <> |
| |
Dear All:
I hope this email finds you well. I hope you haven't missed my previous email, as I understand that everyone has a busy schedule. I just wanted to follow up on my previous message sent. I understand that you may be occupied with other tasks or priorities. However, I would greatly appreciate it if you could spare a few moments to check the patch in my previous email. Your prompt response would be highly valuable to me. Thank you for your attention to this matter, and I look forward to hearing from you soon.
Thanks, Han
On 22/12/2023 下午12:52, Gui-Dong Han wrote: > 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 immediately > after being checked. > > In raid5_set_cache_size(): > ... > conf->min_nr_stripes = size; > ... > while (size > conf->max_nr_stripes) > conf->min_nr_stripes = conf->max_nr_stripes; > ... > > Due to intermediate value updates in raid5_set_cache_size(), concurrent > execution of raid5_cache_count() and raid5_set_cache_size() may lead to > 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 exceeding > 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 the > 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 patch > 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> > > --- > v2: > * In this patch v2, we've updated to use READ_ONCE() instead of direct > reads for accessing max_nr_stripes and min_nr_stripes, since read and > write can concurrent. > Thank Yu Kuai for helpful advice. > --- > drivers/md/raid5.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c > index 8497880135ee..9037e46de0e2 100644 > --- a/drivers/md/raid5.c > +++ b/drivers/md/raid5.c > @@ -7391,10 +7391,12 @@ static unsigned long raid5_cache_count(struct shrinker *shrink, > { > struct r5conf *conf = shrink->private_data; > > - if (conf->max_nr_stripes < conf->min_nr_stripes) > + int max_stripes = READ_ONCE(conf->max_nr_stripes); > + int min_stripes = READ_ONCE(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)
| |