md/raid5: Convert stripe_head's "dev" to flexible array member

PATCH In review Kees Cook 3 participants 5 messages May 22, 2023 – May 30, 2023 MD RAID

Share & tools

Kees Cook

author

2023-05-22 21:21 UTC #0 lore raw

PATCH v2 1/1 md/raid5: Convert stripe_head's "dev" to flexible array member

Replace old-style 1-element array of "dev" in struct stripe_head with modern C99 flexible array. In the future, we can additionally annotate it with the run-time size, found in the "disks" member.

Cc Christoph Hellwig <[email protected]>

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
It looks like this memory calculation:

        memory = conf->min_nr_stripes * (sizeof(struct stripe_head) +
                 max_disks * ((sizeof(struct bio) + PAGE_SIZE))) / 1024;

... was already buggy (i.e. it included the single "dev" bytes in the result). However, I'm not entirely sure if that is the right analysis, since "dev" is not related to struct bio nor PAGE_SIZE?

v1: https://lore.kernel.org/all/[email protected]/
v2: use new struct_size_t() helper from

    https://lore.kernel.org/lkml/[email protected]/
---
 drivers/md/raid5.c | 4 ++--
 drivers/md/raid5.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

drivers/md/raid5.c +2 -2

index 4739ed891e75..64865f9dd3f5 100644
@@ -2433,7 +2433,7 @@ static int grow_stripes(struct r5conf *conf, int num)
 
 	conf->active_name = 0;
 	sc = kmem_cache_create(conf->cache_name[conf->active_name],
-			       sizeof(struct stripe_head)+(devs-1)*sizeof(struct r5dev),
+			       struct_size_t(struct stripe_head, dev, devs),
 			       0, 0, NULL);
 	if (!sc)
 		return 1;

@@ -2559,7 +2559,7 @@ static int resize_stripes(struct r5conf *conf, int newsize)
 
 	/* Step 1 */
 	sc = kmem_cache_create(conf->cache_name[1-conf->active_name],
-			       sizeof(struct stripe_head)+(newsize-1)*sizeof(struct r5dev),
+			       struct_size_t(struct stripe_head, dev, newsize),
 			       0, 0, NULL);
 	if (!sc)
 		return -ENOMEM;

drivers/md/raid5.h +1 -1

index e873938a6125..6cfc74162b41 100644
@@ -268,7 +268,7 @@ struct stripe_head {
 		unsigned long	flags;
 		u32		log_checksum;
 		unsigned short	write_hint;
-	} dev[1]; /* allocated with extra space depending of RAID geometry */
+	} dev[]; /* allocated with extra space depending of RAID geometry */
 };
 
 /* stripe_head_state - collects and tracks the dynamic state of a stripe_head

Signature · 1 line

2.34.1

Christoph Hellwig

Reviewed-by on v2 1/1

2023-05-23 05:56 UTC #1 lore raw

drivers/md/raid5.h struct stripe_head { · line 271

Quoted 2 lines · Kees Cook

drivers/md/raid5.h +1 -1

-	} dev[1]; /* allocated with extra space depending of RAID geometry */
+	} dev[]; /* allocated with extra space depending of RAID geometry */

I still think this should be:

	/* allocated depending of RAID geometry */

now or dropped entirely, as the extra only made sense when it always had that magic one.

The actual code changes looks good to me:

Reviewed-by Christoph Hellwig <[email protected]>

Song Liu

maintainer

2023-05-23 17:44 UTC #2 lore raw

In reply to Kees Cook

Quoted 14 lines · Kees Cook

Replace old-style 1-element array of "dev" in struct stripe_head with modern C99 flexible array. In the future, we can additionally annotate it with the run-time size, found in the "disks" member.
Cc Christoph Hellwig <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
It looks like this memory calculation:
        memory = conf->min_nr_stripes * (sizeof(struct stripe_head) +
                 max_disks * ((sizeof(struct bio) + PAGE_SIZE))) / 1024;

Quoted 7 lines

... was already buggy (i.e. it included the single "dev" bytes in the result). However, I'm not entirely sure if that is the right analysis, since "dev" is not related to struct bio nor PAGE_SIZE?
v1: https://lore.kernel.org/all/[email protected]/
v2: use new struct_size_t() helper from
    https://lore.kernel.org/lkml/[email protected]/

LTGM. Thanks!

I will hold this for a while until struct_size_t() merged.

Song

Kees Cook

author

2023-05-30 23:00 UTC #3 lore raw

Quoted 19 lines · Song Liu

Quoted 18 lines · Kees Cook
Replace old-style 1-element array of "dev" in struct stripe_head with modern C99 flexible array. In the future, we can additionally annotate it with the run-time size, found in the "disks" member.
Cc Christoph Hellwig <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
It looks like this memory calculation:
        memory = conf->min_nr_stripes * (sizeof(struct stripe_head) +
                 max_disks * ((sizeof(struct bio) + PAGE_SIZE))) / 1024;
... was already buggy (i.e. it included the single "dev" bytes in the result). However, I'm not entirely sure if that is the right analysis, since "dev" is not related to struct bio nor PAGE_SIZE?

Quoted 5 lines

Quoted 3 lines
v1: https://lore.kernel.org/all/[email protected]/
v2: use new struct_size_t() helper from
    https://lore.kernel.org/lkml/[email protected]/
LTGM. Thanks!

Thanks!

Quoted 1 line

I will hold this for a while until struct_size_t() merged.

Since my tree will introduce struct_size_t(), I will just carry it there.

Signature · 1 line

Kees Cook

Song Liu

maintainer Acked-by

2023-05-30 23:39 UTC #4 lore raw

Quoted 25 lines · Kees Cook

Quoted 23 lines · Song Liu
Quoted 21 lines · Kees Cook
Replace old-style 1-element array of "dev" in struct stripe_head with modern C99 flexible array. In the future, we can additionally annotate it with the run-time size, found in the "disks" member.
Cc Christoph Hellwig <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
It looks like this memory calculation:
        memory = conf->min_nr_stripes * (sizeof(struct stripe_head) +
                 max_disks * ((sizeof(struct bio) + PAGE_SIZE))) / 1024;
... was already buggy (i.e. it included the single "dev" bytes in the result). However, I'm not entirely sure if that is the right analysis, since "dev" is not related to struct bio nor PAGE_SIZE?
v1: https://lore.kernel.org/all/[email protected]/
v2: use new struct_size_t() helper from
    https://lore.kernel.org/lkml/[email protected]/

Quoted 8 lines

Quoted 1 line
LTGM. Thanks!
Thanks!
Quoted 1 line
I will hold this for a while until struct_size_t() merged.
Since my tree will introduce struct_size_t(), I will just carry it there.

Sounds good! You can add

Acked-by Song Liu <[email protected]>

Thanks,
Song