[07/12] wifi: mwifiex: fix array of flexible structures warnings

From: Johannes Berg <johannes.berg@intel.com>

There are two, just change them to have a "u8 data[]" type
member, and add casts where needed. No binary changes.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 drivers/net/wireless/marvell/mwifiex/fw.h      | 4 ++--
 drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

Hi,

On Sun, Sep 04, 2022 at 09:29:07PM +0200, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> There are two, just change them to have a "u8 data[]" type
> member, and add casts where needed. No binary changes.

Hmm, what exact warning are you looking at? This one?
https://clang.llvm.org/docs/DiagnosticsReference.html#wflexible-array-extensions

It's a little hard to suggest alternatives (or understand why this is
the only/best way) if I don't know what the alleged bug/warning is.

> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  drivers/net/wireless/marvell/mwifiex/fw.h      | 4 ++--
>  drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h
> index 26a48d8f49be..b4f945a549f7 100644
> --- a/drivers/net/wireless/marvell/mwifiex/fw.h
> +++ b/drivers/net/wireless/marvell/mwifiex/fw.h
> @@ -2104,7 +2104,7 @@ struct mwifiex_fw_mef_entry {
>  struct host_cmd_ds_mef_cfg {
>  	__le32 criteria;
>  	__le16 num_entries;
> -	struct mwifiex_fw_mef_entry mef_entry[];
> +	u8 mef_entry_data[];

Do you actually need this part of the change? The 'mef_entry' (or
'mef_entry_data') field is not actually used anywhere now, but I can't
tell what kind of warning is involved.

But also see the next comment:

>  } __packed;
>  
>  #define CONNECTION_TYPE_INFRA   0
> @@ -2254,7 +2254,7 @@ struct coalesce_receive_filt_rule {
>  struct host_cmd_ds_coalesce_cfg {
>  	__le16 action;
>  	__le16 num_of_rules;
> -	struct coalesce_receive_filt_rule rule[];
> +	u8 rule_data[];

These kinds of changes seem to be losing some valuable information. At a
minimum, it would be nice to leave a comment that points at the intended
struct; but ideally, we'd be able to still get the type safety from
actually using the struct, instead of relying on casts and/or u8/void.

But I don't know if that's possible, as I'm not familiar with the
compiler warning involved.

Brian

On Tue, 2022-09-06 at 15:20 -0700, Brian Norris wrote:
> Hi,
> 
> On Sun, Sep 04, 2022 at 09:29:07PM +0200, Johannes Berg wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> > 
> > There are two, just change them to have a "u8 data[]" type
> > member, and add casts where needed. No binary changes.
> 
> Hmm, what exact warning are you looking at? This one?
> https://clang.llvm.org/docs/DiagnosticsReference.html#wflexible-array-extensions
> 

I think gcc also has one with W=1 now?

But yes, it's similar to that one. I was looking at Jakub's netdev test
builds here:

https://patchwork.hopto.org/static/nipa/673828/12964988/build_32bit/stderr

../drivers/net/wireless/marvell/mwifiex/fw.h:2107:46: warning: array of flexible structures
../drivers/net/wireless/marvell/mwifiex/fw.h:2257:47: warning: array of flexible structures

> > @@ -2104,7 +2104,7 @@ struct mwifiex_fw_mef_entry {
> >  struct host_cmd_ds_mef_cfg {
> >  	__le32 criteria;
> >  	__le16 num_entries;
> > -	struct mwifiex_fw_mef_entry mef_entry[];
> > +	u8 mef_entry_data[];
> 
> Do you actually need this part of the change? The 'mef_entry' (or
> 'mef_entry_data') field is not actually used anywhere now, but I can't
> tell what kind of warning is involved.

But it itself is variable, so the compiler is (rightfully, IMHO) saying
that you can't have an array of something that has a variable size, even
if it's a variable array.

> >  struct host_cmd_ds_coalesce_cfg {
> >  	__le16 action;
> >  	__le16 num_of_rules;
> > -	struct coalesce_receive_filt_rule rule[];
> > +	u8 rule_data[];
> 
> These kinds of changes seem to be losing some valuable information. At a
> minimum, it would be nice to leave a comment that points at the intended
> struct; but ideally, we'd be able to still get the type safety from
> actually using the struct, instead of relying on casts and/or u8/void.

All fair points. This was the way we typically do this in e.g.
ieee80211.h ("u8 variable[]", not "struct element elems[]"), but YMMV.

I thought later after Kalle asked about making it a single entry such as

  struct coalesce_receive_filt_rule first_rule;

but then the sizeof() is messed up and then the change has to be much
more careful.

Anyway, I was mostly trying to remove some warnings in drivers that
don't really have a very active maintainer anymore, since we have so
many in wireless it sometimes makes it hard to see which ones are new.

No particular feelings about this patch :-)

johannes

On Wed, Sep 07, 2022 at 08:57:28AM +0200, Johannes Berg wrote:
> On Tue, 2022-09-06 at 15:20 -0700, Brian Norris wrote:
> > On Sun, Sep 04, 2022 at 09:29:07PM +0200, Johannes Berg wrote:
> > > From: Johannes Berg <johannes.berg@intel.com>
> > > 
> > > There are two, just change them to have a "u8 data[]" type
> > > member, and add casts where needed. No binary changes.
> > 
> > Hmm, what exact warning are you looking at? This one?
> > https://clang.llvm.org/docs/DiagnosticsReference.html#wflexible-array-extensions
> > 
> 
> I think gcc also has one with W=1 now?
> 
> But yes, it's similar to that one. I was looking at Jakub's netdev test
> builds here:
> 
> https://patchwork.hopto.org/static/nipa/673828/12964988/build_32bit/stderr
> 
> ../drivers/net/wireless/marvell/mwifiex/fw.h:2107:46: warning: array of flexible structures
> ../drivers/net/wireless/marvell/mwifiex/fw.h:2257:47: warning: array of flexible structures

I think you're actually looking at a sparse warning:

https://lore.kernel.org/linux-sparse/20200930231828.66751-12-luc.vanoostenryck@gmail.com/

I can convince clang to enable the warning I mentioned, but it's not on
by default, even with W=1. When enabled, it complains similarly, as well
as about a ton of other code too.

> > > @@ -2104,7 +2104,7 @@ struct mwifiex_fw_mef_entry {
> > >  struct host_cmd_ds_mef_cfg {
> > >  	__le32 criteria;
> > >  	__le16 num_entries;
> > > -	struct mwifiex_fw_mef_entry mef_entry[];
> > > +	u8 mef_entry_data[];
> > 
> > Do you actually need this part of the change? The 'mef_entry' (or
> > 'mef_entry_data') field is not actually used anywhere now, but I can't
> > tell what kind of warning is involved.
> 
> But it itself is variable, so the compiler is (rightfully, IMHO) saying
> that you can't have an array of something that has a variable size, even
> if it's a variable array.

OK.

I suppose this warning makes sense when you're truly treating these as
arrays (of size more than 1). And in this case (mwifiex_cmd_mef_cfg()
and mwifiex_cmd_append_rpn_expression()), the code to (mostly?) properly
handle the flexible array is pretty ugly anyway, and doesn't really use
the type safety much (lots of casting through u8* and similar). So this
isn't really the best example to try to retain.

(mwifiex_cmd_coalesce_cfg() has some similarly ugly casting and math.)

But if the "array" is just an optional extension to a struct, and we
expect at most a single element, then I don't think the construct is
fundamentally wrong. It might still be hard to get correct in all cases
(e.g., ensuring the right buffer size), but it still feels like a decent
way to describe things.

> > >  struct host_cmd_ds_coalesce_cfg {
> > >  	__le16 action;
> > >  	__le16 num_of_rules;
> > > -	struct coalesce_receive_filt_rule rule[];
> > > +	u8 rule_data[];
> > 
> > These kinds of changes seem to be losing some valuable information. At a
> > minimum, it would be nice to leave a comment that points at the intended
> > struct; but ideally, we'd be able to still get the type safety from
> > actually using the struct, instead of relying on casts and/or u8/void.
> 
> All fair points. This was the way we typically do this in e.g.
> ieee80211.h ("u8 variable[]", not "struct element elems[]"), but YMMV.
> 
> I thought later after Kalle asked about making it a single entry such as
> 
>   struct coalesce_receive_filt_rule first_rule;
> 
> but then the sizeof() is messed up and then the change has to be much
> more careful.
> 
> Anyway, I was mostly trying to remove some warnings in drivers that
> don't really have a very active maintainer anymore, since we have so
> many in wireless it sometimes makes it hard to see which ones are new.

Sure, I guess it's good to clean some of these up.

Looking around, I don't see a great alternative, and per some of my
above notes, I don't think these are beautiful as-is.

> No particular feelings about this patch :-)

After a little more look, I'm fine with this patch. I'd probably
appreciate it better if there was a comment of some kind in the struct
definition, and perhaps mention sparse's -Wflexible-array-array. But
either way:

Reviewed-by: Brian Norris <briannorris@chromium.org>

Thanks.

On Fri, 2022-09-09 at 13:45 -0700, Brian Norris wrote:

> > I think gcc also has one with W=1 now?
> > 
> > But yes, it's similar to that one. I was looking at Jakub's netdev test
> > builds here:
> > 
> > https://patchwork.hopto.org/static/nipa/673828/12964988/build_32bit/stderr
> > 
> > ../drivers/net/wireless/marvell/mwifiex/fw.h:2107:46: warning: array of flexible structures
> > ../drivers/net/wireless/marvell/mwifiex/fw.h:2257:47: warning: array of flexible structures
> 
> I think you're actually looking at a sparse warning:
> 
> https://lore.kernel.org/linux-sparse/20200930231828.66751-12-luc.vanoostenryck@gmail.com/
> 
> I can convince clang to enable the warning I mentioned, but it's not on
> by default, even with W=1. When enabled, it complains similarly, as well
> as about a ton of other code too.

Hm. I _think_ I saw it locally with just W=1, not C=1, but then that
would've been with gcc, probably 12.2 from Fedora. But I didn't try
again now, don't have much time.

> I suppose this warning makes sense when you're truly treating these as
> arrays (of size more than 1). And in this case (mwifiex_cmd_mef_cfg()
> and mwifiex_cmd_append_rpn_expression()), the code to (mostly?) properly
> handle the flexible array is pretty ugly anyway, and doesn't really use
> the type safety much (lots of casting through u8* and similar). So this
> isn't really the best example to try to retain.

Heh, fair point.

> (mwifiex_cmd_coalesce_cfg() has some similarly ugly casting and math.)

Yeah it kind of has to though, given that it's a variable-sized buffer
with variable-sized entries...

> But if the "array" is just an optional extension to a struct, and we
> expect at most a single element, then I don't think the construct is
> fundamentally wrong. It might still be hard to get correct in all cases
> (e.g., ensuring the right buffer size), but it still feels like a decent
> way to describe things.

Fair enough. It's like 0 or 1, maybe, but of course there's no way to
describe that to the compiler and say "I promise to never access [1] (or
higher)" :)

I guess the only real alternative would be to leave it as a _single_
entry, and then fix up all the sizeof() of the containing struct, if
any, to be offsetof(..., first_entry).

But that sort of describes a single entry should be present, which it
might not be.

> After a little more look, I'm fine with this patch. I'd probably
> appreciate it better if there was a comment of some kind in the struct
> definition, and perhaps mention sparse's -Wflexible-array-array.
> 

Sure, I guess it makes sense to also figure out more precisely where the
warning appeared.

I'm running off for a trip for two weeks now-ish, so not going to send
it in the short term. If you wanted to do it instead, no objections, or
if Kalle wants to just add a comment while applying, or whatever.

In any case there are so many warnings in the drivers that still ought
to _have_ a maintainer (and I'm squinting at you for ath too, Kalle)
that it's pretty much irrelevant to fix this one quickly ;-)

johannes