| Message ID | ZxvyavDjXDaV9cNg@kspp (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [next] jbd2: Avoid dozens of -Wflex-array-member-not-at-end warnings | expand |
On Fri 25-10-24 13:32:58, Gustavo A. R. Silva wrote: > -Wflex-array-member-not-at-end was introduced in GCC-14, and we > are getting ready to enable it, globally. > > Use the `DEFINE_RAW_FLEX()` helper for an on-stack definition of > a flexible structure (`struct shash_desc`) where the size of the > flexible-array member (`__ctx`) is known at compile-time, and > refactor the rest of the code, accordingly. > > So, with this, fix 77 of the following warnings: > > include/linux/jbd2.h:1800:35: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > --- > include/linux/jbd2.h | 13 +++++-------- > 1 file changed, 5 insertions(+), 8 deletions(-) > > diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h > index 8aef9bb6ad57..ce4560e62d3b 100644 > --- a/include/linux/jbd2.h > +++ b/include/linux/jbd2.h > @@ -1796,22 +1796,19 @@ static inline unsigned long jbd2_log_space_left(journal_t *journal) > static inline u32 jbd2_chksum(journal_t *journal, u32 crc, > const void *address, unsigned int length) > { > - struct { > - struct shash_desc shash; > - char ctx[JBD_MAX_CHECKSUM_SIZE]; > - } desc; > + DEFINE_RAW_FLEX(struct shash_desc, desc, __ctx, 1); Am I missing some magic here or the 1 above should be JBD_MAX_CHECKSUM_SIZE? Honza
On 31/10/24 06:33, Jan Kara wrote: > On Fri 25-10-24 13:32:58, Gustavo A. R. Silva wrote: >> -Wflex-array-member-not-at-end was introduced in GCC-14, and we >> are getting ready to enable it, globally. >> >> Use the `DEFINE_RAW_FLEX()` helper for an on-stack definition of >> a flexible structure (`struct shash_desc`) where the size of the >> flexible-array member (`__ctx`) is known at compile-time, and >> refactor the rest of the code, accordingly. >> >> So, with this, fix 77 of the following warnings: >> >> include/linux/jbd2.h:1800:35: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] >> >> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> >> --- >> include/linux/jbd2.h | 13 +++++-------- >> 1 file changed, 5 insertions(+), 8 deletions(-) >> >> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h >> index 8aef9bb6ad57..ce4560e62d3b 100644 >> --- a/include/linux/jbd2.h >> +++ b/include/linux/jbd2.h >> @@ -1796,22 +1796,19 @@ static inline unsigned long jbd2_log_space_left(journal_t *journal) >> static inline u32 jbd2_chksum(journal_t *journal, u32 crc, >> const void *address, unsigned int length) >> { >> - struct { >> - struct shash_desc shash; >> - char ctx[JBD_MAX_CHECKSUM_SIZE]; >> - } desc; >> + DEFINE_RAW_FLEX(struct shash_desc, desc, __ctx, 1); > > Am I missing some magic here or the 1 above should be > JBD_MAX_CHECKSUM_SIZE? This seems to be 32-bit code, and the element type of the flex-array member `__ctx` is `void *`. Therefore, we have: `sizeof(ctx) == 4` when `char ctx[JBD_MAX_CHECKSUM_SIZE];` To maintain the same size, we tell `DEFINE_RAW_FLEX()` to allocate `1` element for the flex array, as in 32-bit `sizeof(void *) == 4`. -- Gustavo
On Thu 31-10-24 09:54:36, Gustavo A. R. Silva wrote: > On 31/10/24 06:33, Jan Kara wrote: > > On Fri 25-10-24 13:32:58, Gustavo A. R. Silva wrote: > > > -Wflex-array-member-not-at-end was introduced in GCC-14, and we > > > are getting ready to enable it, globally. > > > > > > Use the `DEFINE_RAW_FLEX()` helper for an on-stack definition of > > > a flexible structure (`struct shash_desc`) where the size of the > > > flexible-array member (`__ctx`) is known at compile-time, and > > > refactor the rest of the code, accordingly. > > > > > > So, with this, fix 77 of the following warnings: > > > > > > include/linux/jbd2.h:1800:35: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > > > > > > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > > > --- > > > include/linux/jbd2.h | 13 +++++-------- > > > 1 file changed, 5 insertions(+), 8 deletions(-) > > > > > > diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h > > > index 8aef9bb6ad57..ce4560e62d3b 100644 > > > --- a/include/linux/jbd2.h > > > +++ b/include/linux/jbd2.h > > > @@ -1796,22 +1796,19 @@ static inline unsigned long jbd2_log_space_left(journal_t *journal) > > > static inline u32 jbd2_chksum(journal_t *journal, u32 crc, > > > const void *address, unsigned int length) > > > { > > > - struct { > > > - struct shash_desc shash; > > > - char ctx[JBD_MAX_CHECKSUM_SIZE]; > > > - } desc; > > > + DEFINE_RAW_FLEX(struct shash_desc, desc, __ctx, 1); > > > > Am I missing some magic here or the 1 above should be > > JBD_MAX_CHECKSUM_SIZE? > > This seems to be 32-bit code, and the element type of the flex-array > member `__ctx` is `void *`. Therefore, we have: Why do you think the code is 32-bit? It is used regardless of the architecture... > `sizeof(ctx) == 4` when `char ctx[JBD_MAX_CHECKSUM_SIZE];` > > To maintain the same size, we tell `DEFINE_RAW_FLEX()` to allocate `1` > element for the flex array, as in 32-bit `sizeof(void *) == 4`. So I agree we end up allocating enough space on stack but it is pretty subtle and if JBD_MAX_CHECKSUM_SIZE definition changes, we have a problem. I think we need something like (JBD_MAX_CHECKSUM_SIZE + sizeof(*desc->__ctx) - 1) / sizeof(*desc->__ctx))? Honza
On 31/10/24 15:32, Jan Kara wrote: > On Thu 31-10-24 09:54:36, Gustavo A. R. Silva wrote: >> On 31/10/24 06:33, Jan Kara wrote: >>> On Fri 25-10-24 13:32:58, Gustavo A. R. Silva wrote: >>>> -Wflex-array-member-not-at-end was introduced in GCC-14, and we >>>> are getting ready to enable it, globally. >>>> >>>> Use the `DEFINE_RAW_FLEX()` helper for an on-stack definition of >>>> a flexible structure (`struct shash_desc`) where the size of the >>>> flexible-array member (`__ctx`) is known at compile-time, and >>>> refactor the rest of the code, accordingly. >>>> >>>> So, with this, fix 77 of the following warnings: >>>> >>>> include/linux/jbd2.h:1800:35: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] >>>> >>>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> >>>> --- >>>> include/linux/jbd2.h | 13 +++++-------- >>>> 1 file changed, 5 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h >>>> index 8aef9bb6ad57..ce4560e62d3b 100644 >>>> --- a/include/linux/jbd2.h >>>> +++ b/include/linux/jbd2.h >>>> @@ -1796,22 +1796,19 @@ static inline unsigned long jbd2_log_space_left(journal_t *journal) >>>> static inline u32 jbd2_chksum(journal_t *journal, u32 crc, >>>> const void *address, unsigned int length) >>>> { >>>> - struct { >>>> - struct shash_desc shash; >>>> - char ctx[JBD_MAX_CHECKSUM_SIZE]; >>>> - } desc; >>>> + DEFINE_RAW_FLEX(struct shash_desc, desc, __ctx, 1); >>> >>> Am I missing some magic here or the 1 above should be >>> JBD_MAX_CHECKSUM_SIZE? >> >> This seems to be 32-bit code, and the element type of the flex-array >> member `__ctx` is `void *`. Therefore, we have: > > Why do you think the code is 32-bit? It is used regardless of the > architecture... Right, sorry, I got a bit confused... > >> `sizeof(ctx) == 4` when `char ctx[JBD_MAX_CHECKSUM_SIZE];` >> >> To maintain the same size, we tell `DEFINE_RAW_FLEX()` to allocate `1` >> element for the flex array, as in 32-bit `sizeof(void *) == 4`. > > So I agree we end up allocating enough space on stack but it is pretty > subtle and if JBD_MAX_CHECKSUM_SIZE definition changes, we have a problem. > I think we need something like (JBD_MAX_CHECKSUM_SIZE + sizeof(*desc->__ctx) > - 1) / sizeof(*desc->__ctx))? I see. Well, in that case it'd be something more like: - struct { - struct shash_desc shash; - char ctx[JBD_MAX_CHECKSUM_SIZE]; - } desc; + DEFINE_RAW_FLEX(struct shash_desc, desc, __ctx, + (JBD_MAX_CHECKSUM_SIZE + + sizeof(*((struct shash_desc *)0)->__ctx)) / + sizeof(*((struct shash_desc *)0)->__ctx)); Notice that `desc` is created inside `DEFINE_RAW_FLEX()` Thanks -- Gustavo
On Thu 31-10-24 17:31:34, Gustavo A. R. Silva wrote: > On 31/10/24 15:32, Jan Kara wrote: > > > > > `sizeof(ctx) == 4` when `char ctx[JBD_MAX_CHECKSUM_SIZE];` > > > > > > To maintain the same size, we tell `DEFINE_RAW_FLEX()` to allocate `1` > > > element for the flex array, as in 32-bit `sizeof(void *) == 4`. > > > > So I agree we end up allocating enough space on stack but it is pretty > > subtle and if JBD_MAX_CHECKSUM_SIZE definition changes, we have a problem. > > I think we need something like (JBD_MAX_CHECKSUM_SIZE + sizeof(*desc->__ctx) > > - 1) / sizeof(*desc->__ctx))? > > I see. Well, in that case it'd be something more like: > > - struct { > - struct shash_desc shash; > - char ctx[JBD_MAX_CHECKSUM_SIZE]; > - } desc; > + DEFINE_RAW_FLEX(struct shash_desc, desc, __ctx, > + (JBD_MAX_CHECKSUM_SIZE + > + sizeof(*((struct shash_desc *)0)->__ctx)) / > + sizeof(*((struct shash_desc *)0)->__ctx)); > > Notice that `desc` is created inside `DEFINE_RAW_FLEX()` Right. Thanks for fixing this. The cleanest option then probably is: DEFINE_RAW_FLEX(struct shash_desc, desc, __ctx, DIV_ROUND_UP(JBD_MAX_CHECKSUM_SIZE, sizeof(*((struct shash_desc *)0)->__ctx))) Honza
On 01/11/24 04:15, Jan Kara wrote: > On Thu 31-10-24 17:31:34, Gustavo A. R. Silva wrote: >> On 31/10/24 15:32, Jan Kara wrote: >>> >>>> `sizeof(ctx) == 4` when `char ctx[JBD_MAX_CHECKSUM_SIZE];` >>>> >>>> To maintain the same size, we tell `DEFINE_RAW_FLEX()` to allocate `1` >>>> element for the flex array, as in 32-bit `sizeof(void *) == 4`. >>> >>> So I agree we end up allocating enough space on stack but it is pretty >>> subtle and if JBD_MAX_CHECKSUM_SIZE definition changes, we have a problem. >>> I think we need something like (JBD_MAX_CHECKSUM_SIZE + sizeof(*desc->__ctx) >>> - 1) / sizeof(*desc->__ctx))? >> >> I see. Well, in that case it'd be something more like: >> >> - struct { >> - struct shash_desc shash; >> - char ctx[JBD_MAX_CHECKSUM_SIZE]; >> - } desc; >> + DEFINE_RAW_FLEX(struct shash_desc, desc, __ctx, >> + (JBD_MAX_CHECKSUM_SIZE + >> + sizeof(*((struct shash_desc *)0)->__ctx)) / >> + sizeof(*((struct shash_desc *)0)->__ctx)); >> >> Notice that `desc` is created inside `DEFINE_RAW_FLEX()` > Right. Thanks for fixing this. The cleanest option then probably is: > > DEFINE_RAW_FLEX(struct shash_desc, desc, __ctx, > DIV_ROUND_UP(JBD_MAX_CHECKSUM_SIZE, > sizeof(*((struct shash_desc *)0)->__ctx))) OK. There you go v2: https://lore.kernel.org/linux-hardening/ZyU94w0IALVhc9Jy@kspp/ Thanks a lot for the feedback. :) -- Gustavo
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h index 8aef9bb6ad57..ce4560e62d3b 100644 --- a/include/linux/jbd2.h +++ b/include/linux/jbd2.h @@ -1796,22 +1796,19 @@ static inline unsigned long jbd2_log_space_left(journal_t *journal) static inline u32 jbd2_chksum(journal_t *journal, u32 crc, const void *address, unsigned int length) { - struct { - struct shash_desc shash; - char ctx[JBD_MAX_CHECKSUM_SIZE]; - } desc; + DEFINE_RAW_FLEX(struct shash_desc, desc, __ctx, 1); int err; BUG_ON(crypto_shash_descsize(journal->j_chksum_driver) > JBD_MAX_CHECKSUM_SIZE); - desc.shash.tfm = journal->j_chksum_driver; - *(u32 *)desc.ctx = crc; + desc->tfm = journal->j_chksum_driver; + *(u32 *)desc->__ctx = crc; - err = crypto_shash_update(&desc.shash, address, length); + err = crypto_shash_update(desc, address, length); BUG_ON(err); - return *(u32 *)desc.ctx; + return *(u32 *)desc->__ctx; } /* Return most recent uncommitted transaction */
-Wflex-array-member-not-at-end was introduced in GCC-14, and we are getting ready to enable it, globally. Use the `DEFINE_RAW_FLEX()` helper for an on-stack definition of a flexible structure (`struct shash_desc`) where the size of the flexible-array member (`__ctx`) is known at compile-time, and refactor the rest of the code, accordingly. So, with this, fix 77 of the following warnings: include/linux/jbd2.h:1800:35: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- include/linux/jbd2.h | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)