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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
>
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 <[email protected]>
> > > ---
> > > 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