Message ID | Y1vRivFfRD6VoBt/@ubunlion |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp814917wru; Fri, 28 Oct 2022 06:01:16 -0700 (PDT) X-Google-Smtp-Source: AMsMyM6bPYsdf4y88KsCdluLpeV5kwj2xJXdArRFiI1SrNDkfz1RWEk4yXzxCThCu/liqFDQQn5O X-Received: by 2002:a17:90b:1e4c:b0:213:32a5:a778 with SMTP id pi12-20020a17090b1e4c00b0021332a5a778mr16606058pjb.172.1666962075469; Fri, 28 Oct 2022 06:01:15 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666962075; cv=none; d=google.com; s=arc-20160816; b=r9HgD5zu7CdSwjhWOcFPo+VlLyg76grLFAWZWtYo/9dxMDoSYBsGpU2f6KEoYPYszs W7jvObxYqODYQvPlTe2MjE57+zLAkY15U5Mx1eVmMLV2eZBvp1kmK/6XOF34RVqryHKP WIXLc1iPrHVoFUc67ekHkswqimwd5Jan7OmfaWdX7s+f/X08QQvxPl48bSNlMnBUkmUn 0HSLRJ3HXevlz/1K/8CUtsXZrLaQKS/NaMMY8lFCFEKh/oalL+V4xX3eueAYkjVlumQ6 kbqNDBfVWB3uTEellHT7FuvMs/fNxyu12oAWSoMfTNZbNSDTF0wx3aoN2Fk1bsiqQmpd lU2w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-disposition:mime-version:message-id :subject:to:from:date:dkim-signature; bh=d5CO6E1a+Ron91gS12mA5wVipcQAFDDlTLXaUi4t5v0=; b=a9DlGIVna3qh+tajDQPgrAmll9QrZPVGbVz8fQwLfkxMXt8V4fpq/bJL9JFp4sA4Sj vlGFq6xV7ZG9NyMmxzlvpekqXZ6x35pgrmNa8mg5NOo9GPTG+lVBkkYQIaQSVrgBrRBF ZFIlFGkFZNEyeG/t5FtrqZWs5GpCWuR+gcx6kGrhsilp6KTxdIZlmwMaZ3/O2X5LF/c7 31fLAQ3UCCs44hXzp/2toKWyfaKTX6cqGsLbcFrmZcdjZ4dffNZrJrAHu48OEX/XpDIC /CII1bJ5xjsYPb2/F40/6ESg9OJQmM02ZeVQ6u6PzXzDL/bC7YaSYwjmyymComDwy61H jOtQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@mailo.com header.s=mailo header.b=fw9vbUfl; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=mailo.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id s22-20020a056a00179600b00536ee478380si6286783pfg.7.2022.10.28.06.01.00; Fri, 28 Oct 2022 06:01:15 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=fail header.i=@mailo.com header.s=mailo header.b=fw9vbUfl; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=mailo.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229822AbiJ1M5F (ORCPT <rfc822;chrisfriedt@gmail.com> + 99 others); Fri, 28 Oct 2022 08:57:05 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35640 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229817AbiJ1M5D (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 28 Oct 2022 08:57:03 -0400 Received: from msg-2.mailo.com (msg-2.mailo.com [213.182.54.12]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3A60E1BFB89 for <linux-kernel@vger.kernel.org>; Fri, 28 Oct 2022 05:57:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=mailo.com; s=mailo; t=1666961809; bh=lEKw0VqbwNuC4XyGnQEaICgfcwjW+MBtFkkBkwVWc8g=; h=X-EA-Auth:Date:From:To:Subject:Message-ID:MIME-Version: Content-Type; b=fw9vbUfl0GLTtNJvI3oQ8w344HMaf8qj6dQ0A2U8lEACBgT05HOSlvlL/0Du57HMU dh5/jiMiA+0EEkTO0Jmy8C7lwvYRZEbk8aMeqvM70sjoNRSnXgtW4aPgEMD813vXyt rg7CbB9o6tVOrbVOtlx1ZC/7fgwIhIIJnH1Q7xq8= Received: by b-1.in.mailobj.net [192.168.90.11] with ESMTP via [213.182.55.206] Fri, 28 Oct 2022 14:56:48 +0200 (CEST) X-EA-Auth: pe0cS9Nh5+DFLxMvYTNXZ7QsyNRGy9Cnw4vNHRK9I2lj3ljt7IUlsKPP45XMfG19xntHkZm9KYARn2cEYHeWGmaiTImTctmY Date: Fri, 28 Oct 2022 18:26:42 +0530 From: Deepak R Varma <drv@mailo.com> To: outreachy@lists.linux.dev, Larry Finger <Larry.Finger@lwfinger.net>, Phillip Potter <phil@philpotter.co.uk>, Pavel Skripkin <paskripkin@gmail.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org Subject: [PATCH] staging: r8188eu: Use flexible-array for one length array member Message-ID: <Y1vRivFfRD6VoBt/@ubunlion> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1747936425100567102?= X-GMAIL-MSGID: =?utf-8?q?1747936425100567102?= |
Series |
staging: r8188eu: Use flexible-array for one length array member
|
|
Commit Message
Deepak R Varma
Oct. 28, 2022, 12:56 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/r8188eu/include/odm.h | 2 +-
drivers/staging/r8188eu/include/wlan_bssdef.h | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
--
2.34.1
Comments
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
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 >
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
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 >
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
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 > >
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);