diff mbox series

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

Message ID 20230522212114.gonna.589-kees@kernel.org (mailing list archive)
State Mainlined
Headers show
Series [v2] md/raid5: Convert stripe_head's "dev" to flexible array member | expand

Commit Message

Kees Cook May 22, 2023, 9:21 p.m. UTC
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 <hch@infradead.org>
Cc: linux-raid@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
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/20230517233313.never.130-kees@kernel.org/
v2: use new struct_size_t() helper from
    https://lore.kernel.org/lkml/20230522211810.never.421-kees@kernel.org/
---
 drivers/md/raid5.c | 4 ++--
 drivers/md/raid5.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

Comments

Christoph Hellwig May 23, 2023, 5:55 a.m. UTC | #1
On Mon, May 22, 2023 at 02:21:15PM -0700, Kees Cook wrote:
> -	} 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 <hch@lst.de>
Song Liu May 23, 2023, 5:43 p.m. UTC | #2
On Mon, May 22, 2023 at 2:21 PM Kees Cook <keescook@chromium.org> wrote:
>
> 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 <hch@infradead.org>
> Cc: linux-raid@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> 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/20230517233313.never.130-kees@kernel.org/
> v2: use new struct_size_t() helper from
>     https://lore.kernel.org/lkml/20230522211810.never.421-kees@kernel.org/

LTGM. Thanks!

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

Song
Kees Cook May 30, 2023, 11 p.m. UTC | #3
On Tue, May 23, 2023 at 10:43:52AM -0700, Song Liu wrote:
> On Mon, May 22, 2023 at 2:21 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > 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 <hch@infradead.org>
> > Cc: linux-raid@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > 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/20230517233313.never.130-kees@kernel.org/
> > v2: use new struct_size_t() helper from
> >     https://lore.kernel.org/lkml/20230522211810.never.421-kees@kernel.org/
> 
> LTGM. Thanks!

Thanks!

> 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.
Song Liu May 30, 2023, 11:39 p.m. UTC | #4
On Tue, May 30, 2023 at 4:00 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, May 23, 2023 at 10:43:52AM -0700, Song Liu wrote:
> > On Mon, May 22, 2023 at 2:21 PM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > 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 <hch@infradead.org>
> > > Cc: linux-raid@vger.kernel.org
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > ---
> > > 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/20230517233313.never.130-kees@kernel.org/
> > > v2: use new struct_size_t() helper from
> > >     https://lore.kernel.org/lkml/20230522211810.never.421-kees@kernel.org/
> >
> > LTGM. Thanks!
>
> Thanks!
>
> > 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 <song@kernel.org>

Thanks,
Song
diff mbox series

Patch

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 4739ed891e75..64865f9dd3f5 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -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;
diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
index e873938a6125..6cfc74162b41 100644
--- a/drivers/md/raid5.h
+++ b/drivers/md/raid5.h
@@ -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