* [PATCH] staging: r8188eu: Use flexible-array for one length array member
@ 2022-10-28 12:56 Deepak R Varma
2022-10-28 14:03 ` Pavel Skripkin
0 siblings, 1 reply; 7+ messages in thread
From: Deepak R Varma @ 2022-10-28 12:56 UTC (permalink / raw)
To: outreachy, Larry Finger, Phillip Potter, Pavel Skripkin,
Greg Kroah-Hartman, linux-staging, linux-kernel
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/r8188eu/include/odm.h | 2 +-
drivers/staging/r8188eu/include/wlan_bssdef.h | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/r8188eu/include/odm.h b/drivers/staging/r8188eu/include/odm.h
index 89b01dd614ba..e2a9de5b9323 100644
--- a/drivers/staging/r8188eu/include/odm.h
+++ b/drivers/staging/r8188eu/include/odm.h
@@ -166,7 +166,7 @@ struct odm_ra_info {
struct ijk_matrix_regs_set {
bool bIQKDone;
- s32 Value[1][IQK_Matrix_REG_NUM];
+ s32 Value[][IQK_Matrix_REG_NUM];
};
struct odm_rf_cal {
diff --git a/drivers/staging/r8188eu/include/wlan_bssdef.h b/drivers/staging/r8188eu/include/wlan_bssdef.h
index 831c465df500..33177de194eb 100644
--- a/drivers/staging/r8188eu/include/wlan_bssdef.h
+++ b/drivers/staging/r8188eu/include/wlan_bssdef.h
@@ -179,7 +179,7 @@ struct ndis_802_11_status_ind {
struct ndis_802_11_auth_evt {
struct ndis_802_11_status_ind Status;
- struct ndis_802_11_auth_req Request[1];
+ struct ndis_802_11_auth_req Request[];
};
struct ndis_802_11_test {
@@ -291,7 +291,7 @@ struct pmkid_candidate {
struct ndis_802_11_pmkid_list {
u32 Version; /* Version of the structure */
u32 NumCandidates; /* No. of pmkid candidates */
- struct pmkid_candidate CandidateList[1];
+ struct pmkid_candidate CandidateList[];
};
struct ndis_802_11_auth_encrypt {
@@ -304,7 +304,7 @@ struct ndis_802_11_cap {
u32 Version;
u32 NoOfPMKIDs;
u32 NoOfAuthEncryptPairsSupported;
- struct ndis_802_11_auth_encrypt AuthenticationEncryptionSupported[1];
+ struct ndis_802_11_auth_encrypt AuthenticationEncryptionSupported[];
};
u8 key_2char2num(u8 hch, u8 lch);
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: r8188eu: Use flexible-array for one length array member
2022-10-28 12:56 [PATCH] staging: r8188eu: Use flexible-array for one length array member Deepak R Varma
@ 2022-10-28 14:03 ` Pavel Skripkin
2022-10-28 14:11 ` Deepak R Varma
0 siblings, 1 reply; 7+ messages in thread
From: Pavel Skripkin @ 2022-10-28 14:03 UTC (permalink / raw)
To: Deepak R Varma, outreachy, Larry Finger, Phillip Potter,
Greg Kroah-Hartman, linux-staging, linux-kernel
Hi Deepak R,
Deepak R Varma <drv@mailo.com> says:
> 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/r8188eu/include/odm.h | 2 +-
> drivers/staging/r8188eu/include/wlan_bssdef.h | 6 +++---
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/include/odm.h b/drivers/staging/r8188eu/include/odm.h
> index 89b01dd614ba..e2a9de5b9323 100644
> --- a/drivers/staging/r8188eu/include/odm.h
> +++ b/drivers/staging/r8188eu/include/odm.h
> @@ -166,7 +166,7 @@ struct odm_ra_info {
>
> struct ijk_matrix_regs_set {
> bool bIQKDone;
> - s32 Value[1][IQK_Matrix_REG_NUM];
> + s32 Value[][IQK_Matrix_REG_NUM];
> };
>
you are changing the actual size of the struct. Wondering if you have
tested this patch somehow
> struct odm_rf_cal {
> diff --git a/drivers/staging/r8188eu/include/wlan_bssdef.h b/drivers/staging/r8188eu/include/wlan_bssdef.h
> index 831c465df500..33177de194eb 100644
> --- a/drivers/staging/r8188eu/include/wlan_bssdef.h
> +++ b/drivers/staging/r8188eu/include/wlan_bssdef.h
> @@ -179,7 +179,7 @@ struct ndis_802_11_status_ind {
>
> struct ndis_802_11_auth_evt {
> struct ndis_802_11_status_ind Status;
> - struct ndis_802_11_auth_req Request[1];
> + struct ndis_802_11_auth_req Request[];
> };
>
this structure seems to be unused. Better to remove it instead of
maintaining the old code
> struct ndis_802_11_test {
> @@ -291,7 +291,7 @@ struct pmkid_candidate {
> struct ndis_802_11_pmkid_list {
> u32 Version; /* Version of the structure */
> u32 NumCandidates; /* No. of pmkid candidates */
> - struct pmkid_candidate CandidateList[1];
> + struct pmkid_candidate CandidateList[];
> };
this one as well
>
> struct ndis_802_11_auth_encrypt {
> @@ -304,7 +304,7 @@ struct ndis_802_11_cap {
> u32 Version;
> u32 NoOfPMKIDs;
> u32 NoOfAuthEncryptPairsSupported;
> - struct ndis_802_11_auth_encrypt AuthenticationEncryptionSupported[1];
> + struct ndis_802_11_auth_encrypt AuthenticationEncryptionSupported[];
> };
>
> u8 key_2char2num(u8 hch, u8 lch);
> --
> 2.34.1
>
and this one as well
>
>
With regards,
Pavel Skripkin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: r8188eu: Use flexible-array for one length array member
2022-10-28 14:03 ` Pavel Skripkin
@ 2022-10-28 14:11 ` Deepak R Varma
2022-10-28 14:43 ` Dan Carpenter
0 siblings, 1 reply; 7+ messages in thread
From: Deepak R Varma @ 2022-10-28 14:11 UTC (permalink / raw)
To: Pavel Skripkin
Cc: outreachy, Larry Finger, Phillip Potter, Greg Kroah-Hartman,
linux-staging, linux-kernel
On Fri, Oct 28, 2022 at 05:03:25PM +0300, Pavel Skripkin wrote:
> Hi Deepak R,
>
> Deepak R Varma <drv@mailo.com> says:
> > 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/r8188eu/include/odm.h | 2 +-
> > drivers/staging/r8188eu/include/wlan_bssdef.h | 6 +++---
> > 2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/r8188eu/include/odm.h b/drivers/staging/r8188eu/include/odm.h
> > index 89b01dd614ba..e2a9de5b9323 100644
> > --- a/drivers/staging/r8188eu/include/odm.h
> > +++ b/drivers/staging/r8188eu/include/odm.h
> > @@ -166,7 +166,7 @@ struct odm_ra_info {
> >
> > struct ijk_matrix_regs_set {
> > bool bIQKDone;
> > - s32 Value[1][IQK_Matrix_REG_NUM];
> > + s32 Value[][IQK_Matrix_REG_NUM];
> > };
> >
>
> you are changing the actual size of the struct. Wondering if you have tested
> this patch somehow
Hello Pavel,
Thank you for reviewing the patch. I build the module post making the changes an
ensured that the build is successful. However, I am not sure how to check the
changes I am proposing. Could you please direct me to some information on how to
test patches? Is there some documentation generic/driver-specific that I can
refer to?
>
> > struct odm_rf_cal {
> > diff --git a/drivers/staging/r8188eu/include/wlan_bssdef.h b/drivers/staging/r8188eu/include/wlan_bssdef.h
> > index 831c465df500..33177de194eb 100644
> > --- a/drivers/staging/r8188eu/include/wlan_bssdef.h
> > +++ b/drivers/staging/r8188eu/include/wlan_bssdef.h
> > @@ -179,7 +179,7 @@ struct ndis_802_11_status_ind {
> >
> > struct ndis_802_11_auth_evt {
> > struct ndis_802_11_status_ind Status;
> > - struct ndis_802_11_auth_req Request[1];
> > + struct ndis_802_11_auth_req Request[];
> > };
> >
>
> this structure seems to be unused. Better to remove it instead of
> maintaining the old code
Sounds good. I agree and will do as suggested.
>
> > struct ndis_802_11_test {
> > @@ -291,7 +291,7 @@ struct pmkid_candidate {
> > struct ndis_802_11_pmkid_list {
> > u32 Version; /* Version of the structure */
> > u32 NumCandidates; /* No. of pmkid candidates */
> > - struct pmkid_candidate CandidateList[1];
> > + struct pmkid_candidate CandidateList[];
> > };
>
> this one as well
Yes, same here.
>
> >
> > struct ndis_802_11_auth_encrypt {
> > @@ -304,7 +304,7 @@ struct ndis_802_11_cap {
> > u32 Version;
> > u32 NoOfPMKIDs;
> > u32 NoOfAuthEncryptPairsSupported;
> > - struct ndis_802_11_auth_encrypt AuthenticationEncryptionSupported[1];
> > + struct ndis_802_11_auth_encrypt AuthenticationEncryptionSupported[];
> > };
> >
> > u8 key_2char2num(u8 hch, u8 lch);
> > --
> > 2.34.1
> >
>
> and this one as well
Yes, will do.
Thank you again for your feedback and suggestions.
./drv
>
> >
> >
>
>
>
>
>
> With regards,
> Pavel Skripkin
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: r8188eu: Use flexible-array for one length array member
2022-10-28 14:11 ` Deepak R Varma
@ 2022-10-28 14:43 ` Dan Carpenter
2022-11-02 19:19 ` Deepak R Varma
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2022-10-28 14:43 UTC (permalink / raw)
To: Deepak R Varma
Cc: Pavel Skripkin, outreachy, Larry Finger, Phillip Potter,
Greg Kroah-Hartman, linux-staging, linux-kernel
On Fri, Oct 28, 2022 at 07:41:54PM +0530, Deepak R Varma wrote:
> On Fri, Oct 28, 2022 at 05:03:25PM +0300, Pavel Skripkin wrote:
> > Hi Deepak R,
> >
> > Deepak R Varma <drv@mailo.com> says:
> > > 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/r8188eu/include/odm.h | 2 +-
> > > drivers/staging/r8188eu/include/wlan_bssdef.h | 6 +++---
> > > 2 files changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/staging/r8188eu/include/odm.h b/drivers/staging/r8188eu/include/odm.h
> > > index 89b01dd614ba..e2a9de5b9323 100644
> > > --- a/drivers/staging/r8188eu/include/odm.h
> > > +++ b/drivers/staging/r8188eu/include/odm.h
> > > @@ -166,7 +166,7 @@ struct odm_ra_info {
> > >
> > > struct ijk_matrix_regs_set {
> > > bool bIQKDone;
> > > - s32 Value[1][IQK_Matrix_REG_NUM];
> > > + s32 Value[][IQK_Matrix_REG_NUM];
> > > };
> > >
> >
> > you are changing the actual size of the struct. Wondering if you have tested
> > this patch somehow
>
> Hello Pavel,
> Thank you for reviewing the patch. I build the module post making the changes an
> ensured that the build is successful. However, I am not sure how to check the
> changes I am proposing. Could you please direct me to some information on how to
> test patches? Is there some documentation generic/driver-specific that I can
> refer to?
You just have to look at every place where it is used and especially
look at where it is allocated. It is only used in one place:
struct ijk_matrix_regs_set IQKMatrixRegSetting;
But that is in the middle of a struct and generally it doesn't make
sense to have a flex array in the middle of a struct. So investigating
further, we see that it really is a one element array:
Do a grep:
pDM_Odm->RFCalibrateInfo.IQKMatrixRegSetting[ChannelMappedIndex].Value[0][1]);
The first element is always zero. So this patch introduces memory
corruption.
The code is messy and should be cleaned up, of course.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: r8188eu: Use flexible-array for one length array member
2022-10-28 14:43 ` Dan Carpenter
@ 2022-11-02 19:19 ` Deepak R Varma
2022-11-03 5:22 ` Dan Carpenter
0 siblings, 1 reply; 7+ messages in thread
From: Deepak R Varma @ 2022-11-02 19:19 UTC (permalink / raw)
To: Dan Carpenter, Pavel Skripkin
Cc: outreachy, Larry Finger, Phillip Potter, Greg Kroah-Hartman,
linux-staging, linux-kernel
On Fri, Oct 28, 2022 at 05:43:03PM +0300, Dan Carpenter wrote:
> On Fri, Oct 28, 2022 at 07:41:54PM +0530, Deepak R Varma wrote:
> > On Fri, Oct 28, 2022 at 05:03:25PM +0300, Pavel Skripkin wrote:
> > > Hi Deepak R,
> > >
> > > Deepak R Varma <drv@mailo.com> says:
> > > > 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/r8188eu/include/odm.h | 2 +-
> > > > drivers/staging/r8188eu/include/wlan_bssdef.h | 6 +++---
> > > > 2 files changed, 4 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/r8188eu/include/odm.h b/drivers/staging/r8188eu/include/odm.h
> > > > index 89b01dd614ba..e2a9de5b9323 100644
> > > > --- a/drivers/staging/r8188eu/include/odm.h
> > > > +++ b/drivers/staging/r8188eu/include/odm.h
> > > > @@ -166,7 +166,7 @@ struct odm_ra_info {
> > > >
> > > > struct ijk_matrix_regs_set {
> > > > bool bIQKDone;
> > > > - s32 Value[1][IQK_Matrix_REG_NUM];
> > > > + s32 Value[][IQK_Matrix_REG_NUM];
> > > > };
> > > >
> > >
> > > you are changing the actual size of the struct. Wondering if you have tested
> > > this patch somehow
> >
> > Hello Pavel,
> > Thank you for reviewing the patch. I build the module post making the changes an
> > ensured that the build is successful. However, I am not sure how to check the
> > changes I am proposing. Could you please direct me to some information on how to
> > test patches? Is there some documentation generic/driver-specific that I can
> > refer to?
>
> You just have to look at every place where it is used and especially
> look at where it is allocated. It is only used in one place:
>
> struct ijk_matrix_regs_set IQKMatrixRegSetting;
>
> But that is in the middle of a struct and generally it doesn't make
> sense to have a flex array in the middle of a struct. So investigating
> further, we see that it really is a one element array:
>
> Do a grep:
>
> pDM_Odm->RFCalibrateInfo.IQKMatrixRegSetting[ChannelMappedIndex].Value[0][1]);
>
> The first element is always zero. So this patch introduces memory
> corruption.
>
> The code is messy and should be cleaned up, of course.
Hello Pavel and Dan,
I looked at the surround code and places where the proposed patch will have an
impact. As you rightly pointed out this array can't be treated as a flexible
array since it fits right in the middle of another struct.
I am wondering if ijk_matrix_regs_set (or IQKMatrixRegSetting) is a one element
array, why is it declared as a two dimensional array? Can this be a simple one
dimensional array instead? Is this the cleanup you are referring to when you say
"code is messy and should be cleaned up"?
Thank you,
./drv
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: r8188eu: Use flexible-array for one length array member
2022-11-02 19:19 ` Deepak R Varma
@ 2022-11-03 5:22 ` Dan Carpenter
2022-11-03 5:25 ` Deepak R Varma
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2022-11-03 5:22 UTC (permalink / raw)
To: Deepak R Varma
Cc: Pavel Skripkin, outreachy, Larry Finger, Phillip Potter,
Greg Kroah-Hartman, linux-staging, linux-kernel
On Thu, Nov 03, 2022 at 12:49:38AM +0530, Deepak R Varma wrote:
> I am wondering if ijk_matrix_regs_set (or IQKMatrixRegSetting) is a one element
> array, why is it declared as a two dimensional array? Can this be a simple one
> dimensional array instead? Is this the cleanup you are referring to when you say
> "code is messy and should be cleaned up"?
Yes. You have interpreted that correctly.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: r8188eu: Use flexible-array for one length array member
2022-11-03 5:22 ` Dan Carpenter
@ 2022-11-03 5:25 ` Deepak R Varma
0 siblings, 0 replies; 7+ messages in thread
From: Deepak R Varma @ 2022-11-03 5:25 UTC (permalink / raw)
To: Dan Carpenter
Cc: Pavel Skripkin, outreachy, Larry Finger, Phillip Potter,
Greg Kroah-Hartman, linux-staging, linux-kernel
On Thu, Nov 03, 2022 at 08:22:14AM +0300, Dan Carpenter wrote:
> On Thu, Nov 03, 2022 at 12:49:38AM +0530, Deepak R Varma wrote:
> > I am wondering if ijk_matrix_regs_set (or IQKMatrixRegSetting) is a one element
> > array, why is it declared as a two dimensional array? Can this be a simple one
> > dimensional array instead? Is this the cleanup you are referring to when you say
> > "code is messy and should be cleaned up"?
>
> Yes. You have interpreted that correctly.
Thank you for the confirmation. This will be an interesting correction. I will
review and submit a patch shortly.
Thank you,
./drv
>
> regards,
> dan carpenter
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-11-03 5:25 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-28 12:56 [PATCH] staging: r8188eu: Use flexible-array for one length array member Deepak R Varma
2022-10-28 14:03 ` Pavel Skripkin
2022-10-28 14:11 ` Deepak R Varma
2022-10-28 14:43 ` Dan Carpenter
2022-11-02 19:19 ` Deepak R Varma
2022-11-03 5:22 ` Dan Carpenter
2022-11-03 5:25 ` Deepak R Varma
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.