diff mbox series

staging: wlan-ng: Use flexible-array for one / zero-length arrays

Message ID Y1vNNSSWK1EkcohT@ubunlion
State New
Headers
Series staging: wlan-ng: Use flexible-array for one / zero-length arrays |

Commit Message

Deepak R Varma Oct. 28, 2022, 12:38 p.m. UTC 
  Flexible-array member should be used instead of one or zero member to
meet the need for having a dynamically sized trailing elements in a
structure. Refer to links [1] and [2] for detailed guidance on this
suggestion.

[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2] https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays

Issue identified using coccicheck.

Signed-off-by: Deepak R Varma <drv@mailo.com>
---
 drivers/staging/wlan-ng/p80211mgmt.h  | 8 ++++----
 drivers/staging/wlan-ng/p80211types.h | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

--
2.34.1

Comments

Dan Carpenter Oct. 28, 2022, 1:33 p.m. UTC | #1 
On Fri, Oct 28, 2022 at 06:08:13PM +0530, Deepak R Varma wrote:
> Flexible-array member should be used instead of one or zero member to
> meet the need for having a dynamically sized trailing elements in a
> structure. Refer to links [1] and [2] for detailed guidance on this
> suggestion.
> 
> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> [2] https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
> 
> Issue identified using coccicheck.
> 
> Signed-off-by: Deepak R Varma <drv@mailo.com>
> ---
>  drivers/staging/wlan-ng/p80211mgmt.h  | 8 ++++----
>  drivers/staging/wlan-ng/p80211types.h | 2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/wlan-ng/p80211mgmt.h b/drivers/staging/wlan-ng/p80211mgmt.h
> index 1ef30d3f3159..d6fe52de2c8f 100644
> --- a/drivers/staging/wlan-ng/p80211mgmt.h
> +++ b/drivers/staging/wlan-ng/p80211mgmt.h
> @@ -229,14 +229,14 @@ struct wlan_ie {
>  struct wlan_ie_ssid {
>  	u8 eid;
>  	u8 len;
> -	u8 ssid[1];		/* may be zero, ptrs may overlap */
> +	u8 ssid[];		/* may be zero, ptrs may overlap */

How have you ensured that changing this from a 1 byte array to a zero
byte array does not break anything?  It's not uncommon for a people
to do math like "size - 1 + length".  The "- 1" would be to take the
1 element into consideration.

I was trying to read through this code to check your work, but then
you sent a second patch which also does not explain how you are auditing
your changes.  Can you go a bit slower?

regards,
dan carpenter
Deepak R Varma Oct. 28, 2022, 1:41 p.m. UTC | #2 
On Fri, Oct 28, 2022 at 04:33:59PM +0300, Dan Carpenter wrote:
> On Fri, Oct 28, 2022 at 06:08:13PM +0530, Deepak R Varma wrote:
> > Flexible-array member should be used instead of one or zero member to
> > meet the need for having a dynamically sized trailing elements in a
> > structure. Refer to links [1] and [2] for detailed guidance on this
> > suggestion.
> >
> > [1] https://en.wikipedia.org/wiki/Flexible_array_member
> > [2] https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
> >
> > Issue identified using coccicheck.
> >
> > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > ---
> >  drivers/staging/wlan-ng/p80211mgmt.h  | 8 ++++----
> >  drivers/staging/wlan-ng/p80211types.h | 2 +-
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/wlan-ng/p80211mgmt.h b/drivers/staging/wlan-ng/p80211mgmt.h
> > index 1ef30d3f3159..d6fe52de2c8f 100644
> > --- a/drivers/staging/wlan-ng/p80211mgmt.h
> > +++ b/drivers/staging/wlan-ng/p80211mgmt.h
> > @@ -229,14 +229,14 @@ struct wlan_ie {
> >  struct wlan_ie_ssid {
> >  	u8 eid;
> >  	u8 len;
> > -	u8 ssid[1];		/* may be zero, ptrs may overlap */
> > +	u8 ssid[];		/* may be zero, ptrs may overlap */
>
> How have you ensured that changing this from a 1 byte array to a zero
> byte array does not break anything?  It's not uncommon for a people
> to do math like "size - 1 + length".  The "- 1" would be to take the
> 1 element into consideration.

Hi Dan,
I did a code review to understand how this structure member is used and did not
find any computations you mentioned. I would certainly like to receive your feedback
as well.

>
> I was trying to read through this code to check your work, but then
> you sent a second patch which also does not explain how you are auditing
> your changes.  Can you go a bit slower?

My apologies for rushing patches in. I will hold on for feedback on these
patches before turning in any new patch involving similar change. I hope it is
okay to send a different type of patch though. Please correct if I am wrong.

Thank you for looking into the patches. Appreciate your time and feedback.

./drv


>
> regards,
> dan carpenter
>
Dan Carpenter Oct. 28, 2022, 2:14 p.m. UTC | #3 
On Fri, Oct 28, 2022 at 07:11:00PM +0530, Deepak R Varma wrote:
> On Fri, Oct 28, 2022 at 04:33:59PM +0300, Dan Carpenter wrote:
> > On Fri, Oct 28, 2022 at 06:08:13PM +0530, Deepak R Varma wrote:
> > > Flexible-array member should be used instead of one or zero member to
> > > meet the need for having a dynamically sized trailing elements in a
> > > structure. Refer to links [1] and [2] for detailed guidance on this
> > > suggestion.
> > >
> > > [1] https://en.wikipedia.org/wiki/Flexible_array_member  
> > > [2] https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays  
> > >
> > > Issue identified using coccicheck.
> > >
> > > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > > ---
> > >  drivers/staging/wlan-ng/p80211mgmt.h  | 8 ++++----
> > >  drivers/staging/wlan-ng/p80211types.h | 2 +-
> > >  2 files changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/staging/wlan-ng/p80211mgmt.h b/drivers/staging/wlan-ng/p80211mgmt.h
> > > index 1ef30d3f3159..d6fe52de2c8f 100644
> > > --- a/drivers/staging/wlan-ng/p80211mgmt.h
> > > +++ b/drivers/staging/wlan-ng/p80211mgmt.h
> > > @@ -229,14 +229,14 @@ struct wlan_ie {
> > >  struct wlan_ie_ssid {
> > >  	u8 eid;
> > >  	u8 len;
> > > -	u8 ssid[1];		/* may be zero, ptrs may overlap */
> > > +	u8 ssid[];		/* may be zero, ptrs may overlap */
> >
> > How have you ensured that changing this from a 1 byte array to a zero
> > byte array does not break anything?  It's not uncommon for a people
> > to do math like "size - 1 + length".  The "- 1" would be to take the
> > 1 element into consideration.
> 
> Hi Dan,
> I did a code review to understand how this structure member is used and did not
> find any computations you mentioned. I would certainly like to receive your feedback
> as well.
> 

That would be useful information for the commit message.  Even if it
goes under the --- cut off.

---
Compile tested only.  I audited all fourty two uses of the wlan_ie_ssid
struct and nothing checks sizeof(struct wlan_ie_ssid) and changing this
from 1 to 0 will not break anything.

> >
> > I was trying to read through this code to check your work, but then
> > you sent a second patch which also does not explain how you are auditing
> > your changes.  Can you go a bit slower?
> 
> My apologies for rushing patches in. I will hold on for feedback on these
> patches before turning in any new patch involving similar change. I hope it is
> okay to send a different type of patch though. Please correct if I am wrong.

I'm not saying you have to wait for days for reviewers to get through
your patches...  I guess what I'm really trying to say is please group
your patches together.  Pavel Skripkin's review comments seem reasonable
so if the patches are grouped then he only has to reply once but now
someone has to reply to every email.

Also if you write 10 patches maybe you will notice or think of something
part way through and which forces you to re-write the earlier patches.

When you send patches every ten minutes then everyone can see you have
not tested it at all.  I don't really expect people to test their
staging patches, but let's not make it obvious.  Wait overnight so
people think "Ah, maybe they were tested".  It gives people (false)
confidence that this is quality work.

regards,
dan carpenter
Dan Carpenter Oct. 28, 2022, 2:36 p.m. UTC | #4 
On Fri, Oct 28, 2022 at 06:08:13PM +0530, Deepak R Varma wrote:
> Flexible-array member should be used instead of one or zero member to
> meet the need for having a dynamically sized trailing elements in a
> structure. Refer to links [1] and [2] for detailed guidance on this
> suggestion.
> 
> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> [2] https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
> 
> Issue identified using coccicheck.
> 
> Signed-off-by: Deepak R Varma <drv@mailo.com>
> ---
>  drivers/staging/wlan-ng/p80211mgmt.h  | 8 ++++----
>  drivers/staging/wlan-ng/p80211types.h | 2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/wlan-ng/p80211mgmt.h b/drivers/staging/wlan-ng/p80211mgmt.h
> index 1ef30d3f3159..d6fe52de2c8f 100644
> --- a/drivers/staging/wlan-ng/p80211mgmt.h
> +++ b/drivers/staging/wlan-ng/p80211mgmt.h
> @@ -229,14 +229,14 @@ struct wlan_ie {
>  struct wlan_ie_ssid {
>  	u8 eid;
>  	u8 len;
> -	u8 ssid[1];		/* may be zero, ptrs may overlap */
> +	u8 ssid[];		/* may be zero, ptrs may overlap */
>  } __packed;

When you're writing this change look at where struct wlan_ie_ssid is
used.  It's included in five different structs:

	struct wlan_ie_ssid *ssid;

But none of those *ssid pointers are used.  So instead of doing this
just delete the wlan_ie_ssid struct and delete every reference to it.
Send that as a patch by itself and then repeat that process for the next
struct.

regards,
dan carpenter
Deepak R Varma Oct. 28, 2022, 2:44 p.m. UTC | #5 
On Fri, Oct 28, 2022 at 05:36:41PM +0300, Dan Carpenter wrote:
> On Fri, Oct 28, 2022 at 06:08:13PM +0530, Deepak R Varma wrote:
> > Flexible-array member should be used instead of one or zero member to
> > meet the need for having a dynamically sized trailing elements in a
> > structure. Refer to links [1] and [2] for detailed guidance on this
> > suggestion.
> >
> > [1] https://en.wikipedia.org/wiki/Flexible_array_member
> > [2] https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
> >
> > Issue identified using coccicheck.
> >
> > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > ---
> >  drivers/staging/wlan-ng/p80211mgmt.h  | 8 ++++----
> >  drivers/staging/wlan-ng/p80211types.h | 2 +-
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/wlan-ng/p80211mgmt.h b/drivers/staging/wlan-ng/p80211mgmt.h
> > index 1ef30d3f3159..d6fe52de2c8f 100644
> > --- a/drivers/staging/wlan-ng/p80211mgmt.h
> > +++ b/drivers/staging/wlan-ng/p80211mgmt.h
> > @@ -229,14 +229,14 @@ struct wlan_ie {
> >  struct wlan_ie_ssid {
> >  	u8 eid;
> >  	u8 len;
> > -	u8 ssid[1];		/* may be zero, ptrs may overlap */
> > +	u8 ssid[];		/* may be zero, ptrs may overlap */
> >  } __packed;
>
> When you're writing this change look at where struct wlan_ie_ssid is
> used.  It's included in five different structs:
>
> 	struct wlan_ie_ssid *ssid;
>
> But none of those *ssid pointers are used.  So instead of doing this
> just delete the wlan_ie_ssid struct and delete every reference to it.
> Send that as a patch by itself and then repeat that process for the next
> struct.

Thank you Dan. I had the same observation, but was not sure if that clean up
should be included as part of current patch scope. Now I will, as you advised.

Also, there are a few function declarations that I do not think are used in the
code anywhere.
   File: drivers/staging/wlan-ng/p80211mgmt.h
   Line number : 500 through 519

Can you please comment if it is safe to remove these functions and include this
clean-up as well in the proposed patch?

Thank you,
./drv

>
> regards,
> dan carpenter
>
Dan Carpenter Oct. 28, 2022, 2:48 p.m. UTC | #6 
On Fri, Oct 28, 2022 at 08:14:53PM +0530, Deepak R Varma wrote:
> Also, there are a few function declarations that I do not think are used in the
> code anywhere.
>    File: drivers/staging/wlan-ng/p80211mgmt.h
>    Line number : 500 through 519
> 
> Can you please comment if it is safe to remove these functions and include this
> clean-up as well in the proposed patch?

Always feel free to delete code that is not used.

(But send the patch tomorrow and as part of a patchset).

regards,
dan carpenter
Deepak R Varma Nov. 1, 2022, 1:56 p.m. UTC | #7 
On Fri, Oct 28, 2022 at 05:36:41PM +0300, Dan Carpenter wrote:
> On Fri, Oct 28, 2022 at 06:08:13PM +0530, Deepak R Varma wrote:
> > Flexible-array member should be used instead of one or zero member to
> > meet the need for having a dynamically sized trailing elements in a
> > structure. Refer to links [1] and [2] for detailed guidance on this
> > suggestion.
> >
> > [1] https://en.wikipedia.org/wiki/Flexible_array_member
> > [2] https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
> >
> > Issue identified using coccicheck.
> >
> > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > ---
> >  drivers/staging/wlan-ng/p80211mgmt.h  | 8 ++++----
> >  drivers/staging/wlan-ng/p80211types.h | 2 +-
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/wlan-ng/p80211mgmt.h b/drivers/staging/wlan-ng/p80211mgmt.h
> > index 1ef30d3f3159..d6fe52de2c8f 100644
> > --- a/drivers/staging/wlan-ng/p80211mgmt.h
> > +++ b/drivers/staging/wlan-ng/p80211mgmt.h
> > @@ -229,14 +229,14 @@ struct wlan_ie {
> >  struct wlan_ie_ssid {
> >  	u8 eid;
> >  	u8 len;
> > -	u8 ssid[1];		/* may be zero, ptrs may overlap */
> > +	u8 ssid[];		/* may be zero, ptrs may overlap */
> >  } __packed;
>
> When you're writing this change look at where struct wlan_ie_ssid is
> used.  It's included in five different structs:
>
> 	struct wlan_ie_ssid *ssid;
>
> But none of those *ssid pointers are used.  So instead of doing this
> just delete the wlan_ie_ssid struct and delete every reference to it.
> Send that as a patch by itself and then repeat that process for the next
> struct.

Hello Dan and Pavel,
I have built a patch set based on your feedback on this (and the other similar
patch for r8188eu driver). I have only covered one driver for now and would wait
for feedback before I do the same thing for the other driver.

Thank you,
./drv

>
> regards,
> dan carpenter
>
diff mbox series

Patch

diff --git a/drivers/staging/wlan-ng/p80211mgmt.h b/drivers/staging/wlan-ng/p80211mgmt.h
index 1ef30d3f3159..d6fe52de2c8f 100644
--- a/drivers/staging/wlan-ng/p80211mgmt.h
+++ b/drivers/staging/wlan-ng/p80211mgmt.h
@@ -229,14 +229,14 @@  struct wlan_ie {
 struct wlan_ie_ssid {
 	u8 eid;
 	u8 len;
-	u8 ssid[1];		/* may be zero, ptrs may overlap */
+	u8 ssid[];		/* may be zero, ptrs may overlap */
 } __packed;

 /*-- Supported Rates  -----------------------------*/
 struct wlan_ie_supp_rates {
 	u8 eid;
 	u8 len;
-	u8 rates[1];		/* had better be at LEAST one! */
+	u8 rates[];		/* had better be at LEAST one! */
 } __packed;

 /*-- FH Parameter Set  ----------------------------*/
@@ -274,7 +274,7 @@  struct wlan_ie_tim {
 	u8 dtim_cnt;
 	u8 dtim_period;
 	u8 bitmap_ctl;
-	u8 virt_bm[1];
+	u8 virt_bm[];
 } __packed;

 /*-- IBSS Parameter Set ---------------------------*/
@@ -288,7 +288,7 @@  struct wlan_ie_ibss_parms {
 struct wlan_ie_challenge {
 	u8 eid;
 	u8 len;
-	u8 challenge[1];
+	u8 challenge[];
 } __packed;

 /*-------------------------------------------------*/
diff --git a/drivers/staging/wlan-ng/p80211types.h b/drivers/staging/wlan-ng/p80211types.h
index 6486612a8f31..b2ffd09881b0 100644
--- a/drivers/staging/wlan-ng/p80211types.h
+++ b/drivers/staging/wlan-ng/p80211types.h
@@ -234,7 +234,7 @@  struct p80211pstr32 {
 /* MAC address array */
 struct p80211macarray {
 	u32 cnt;
-	u8 data[1][MAXLEN_PSTR6];
+	u8 data[][MAXLEN_PSTR6];
 } __packed;

 /* prototype template */