Re: md/raid5: Use conf->device_lock protect changing of multi-thread resources.

On Fri, 22 Nov 2013 16:10:49 +0000 Ben Hutchings [off-list ref]
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 <redacted>
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 <redacted>
Cc: majianpeng <redacted>
Signed-off-by: NeilBrown <redacted>

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;