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

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

[Ben Hutchings]

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.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2561 bytes --]

On Fri, 22 Nov 2013 16:10:49 +0000 Ben Hutchings <bhutchings@solarflare.com>
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@suse.de>
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@solarflare.com>
Cc: majianpeng <majianpeng@gmail.com>
Signed-off-by: NeilBrown <neilb@suse.de>

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]