On Fri, 22 Nov 2013 16:10:49 +0000 Ben Hutchings <bhutchings@xxxxxxxxxxxxxx>
wrote:
> Coverity reported a new defect after this change (commit 60aaf9338545) -
> passing a singleton pointer as the worker_groups parameter to
> alloc_thread_groups() which expects an array pointer. But actually it
> looks like it should still be a singleton pointer and
> alloc_thread_groups() is broken:
>
> - conf->worker_groups = kzalloc(sizeof(struct r5worker_group) *
> - conf->group_cnt, GFP_NOIO);
> - if (!conf->worker_groups || !workers) {
> + workers = kzalloc(size * *group_cnt, GFP_NOIO);
> + *worker_groups = kzalloc(sizeof(struct r5worker_group) *
> + *group_cnt, GFP_NOIO);
> + if (!*worker_groups || !workers) {
> kfree(workers);
> - kfree(conf->worker_groups);
> - conf->worker_groups = NULL;
> + kfree(*worker_groups);
> return -ENOMEM;
> }
>
> - for (i = 0; i < conf->group_cnt; i++) {
> + for (i = 0; i < *group_cnt; i++) {
> struct r5worker_group *group;
>
> - group = &conf->worker_groups[i];
> + group = worker_groups[i];
>
> This assignment should, I think, be:
>
> group = &(*worker_groups)[i];
>
> or equivalently:
>
> group = *worker_groups + i;
>
> Ben.
>
Thanks Ben! I completely agree with the analysis.
Following patch queue for submission later in the week.
NeilBrown
From: NeilBrown <neilb@xxxxxxx>
Date: Mon, 25 Nov 2013 11:12:43 +1100
Subject: [PATCH] md/raid5: fix new memory-reference bug in alloc_thread_groups.
In alloc_thread_groups, worker_groups is a pointer to an array,
not an array of pointers.
So
worker_groups[i]
is wrong. It should be
&(*worker_groups)[i]
Found-by: coverity
Fixes: 60aaf9338545
Reported-by: Ben Hutchings <bhutchings@xxxxxxxxxxxxxx>
Cc: majianpeng <majianpeng@xxxxxxxxx>
Signed-off-by: NeilBrown <neilb@xxxxxxx>
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 47da0af6322b..676d8b7c5117 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -5471,7 +5471,7 @@ static int alloc_thread_groups(struct r5conf *conf, int cnt,
for (i = 0; i < *group_cnt; i++) {
struct r5worker_group *group;
- group = worker_groups[i];
+ group = &(*worker_groups)[i];
INIT_LIST_HEAD(&group->handle_list);
group->conf = conf;
group->workers = workers + i * cnt;
Attachment:
signature.asc
Description: PGP signature
