Message ID | Y1vNNSSWK1EkcohT@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 l7csp811901wru; Fri, 28 Oct 2022 05:55:04 -0700 (PDT) X-Google-Smtp-Source: AMsMyM7iDNtprOQU8IMCN4YYzMGiYvhyMRMR0wP6aE2x4svZb/1PioeQ6F8YINluMNY/zrSB6fiI X-Received: by 2002:a17:907:dac:b0:78e:17a0:d1cb with SMTP id go44-20020a1709070dac00b0078e17a0d1cbmr47333796ejc.618.1666961704725; Fri, 28 Oct 2022 05:55:04 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666961704; cv=none; d=google.com; s=arc-20160816; b=oXEwNMFbjj3obYClkG3Wl1U8sCaYP9UyrCmCPCbExVMHUtC3cOJPHSLdQGYupzl88o SwEy0E4vitjYfSrWgVCv2hLxrY+gvKRDA5b7Trtj9M7qLvDHnmbXQOWvRJGdKLf49Upg WGB9WJk06EZgSTVE/5hTe3BhZ+KCSqPEPhPeq+fUseBWc8D5e4bS7Rsm9c7Z32a6l79X RkXl6PJHQChkOLrpL1MPwS+IHknbhUafZHgf6ziWwqyyCexEqzCKjDeNwWyJ7yvWoZgI m8mVqowKt73CskpX/z/tSuJa91qII662cEu3TUxyh8VDkC5hLMAg+tw3dyKkIQeS+ju+ v0LQ== 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=smOq0RXpnr/nkZedSZpXrPtwOXjHYzB0S+Zcvjd34qc=; b=EqB/1zp1zf3ujMIh/kl1vsuF9ilhOFtIAo1jvRqP3UdqwcJdtVNkYmGRy9wwXd0ag2 O5Vd/vpdfNg77A5OByzxsXyA/TROzDf7Y9ACdVnHTwePoOOFDCufKneUmKK/+N5rLTcq XW99duyfctgcS0d/YXcHqv8SdCom0eiGjkehbyqPooPjeW7c0wjfwA3lhGz+fVOVMj8M CEywdYnaCMHRSY86ROziKc8GnbpTGeNYeKpiUILkpbh3zR72fTP5Bm8FXIS+uc2tJmu8 MGj/wHXYy73VOn6IObOhbInDr6Gcc7uSKcHo6j9mW9m13MxTcw3kwaIVLS8w1Nb+94S3 L7BA== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@mailo.com header.s=mailo header.b=Ekxlr7Dr; 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 eb10-20020a0564020d0a00b00458a0aa9216si3354136edb.634.2022.10.28.05.54.40; Fri, 28 Oct 2022 05:55:04 -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=Ekxlr7Dr; 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 S229695AbiJ1Mig (ORCPT <rfc822;chrisfriedt@gmail.com> + 99 others); Fri, 28 Oct 2022 08:38:36 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33412 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229597AbiJ1Mie (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 28 Oct 2022 08:38:34 -0400 Received: from msg-4.mailo.com (msg-4.mailo.com [213.182.54.15]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B26C01D4DCC for <linux-kernel@vger.kernel.org>; Fri, 28 Oct 2022 05:38:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=mailo.com; s=mailo; t=1666960698; bh=B5OaSZ8HsPRmrMQQFyz9KT8sdbr9Ub7sfHqu4Tovee8=; h=X-EA-Auth:Date:From:To:Subject:Message-ID:MIME-Version: Content-Type; b=Ekxlr7DrdfyemSZ0COtOM0B0w5kwQriCAHGa4y4lvSmfe4CMBWu+DzjOc2Hl7+uLx GUoK3UGw7L/9luMrAgX2ObKa0rHloNNIQUS0OpZgc6j1JN2XzpoMHrSqZUt9xDZzOX E4ODFRtppCXlPDwyGcimnB9fHbSgiGVvzisG62Xs= Received: by b-2.in.mailobj.net [192.168.90.12] with ESMTP via [213.182.55.206] Fri, 28 Oct 2022 14:38:18 +0200 (CEST) X-EA-Auth: VkRt+j6nGRWQq6EtMOp8yVWLs+3ZU5k2Lm4NBQLPnnZrKfXl+WSKVQSq7rMV6nqg+He7Dm8gvOBToPU9ngJiMtCWFCKBgdLm Date: Fri, 28 Oct 2022 18:08:13 +0530 From: Deepak R Varma <drv@mailo.com> To: outreachy@lists.linux.dev, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org Subject: [PATCH] staging: wlan-ng: Use flexible-array for one / zero-length arrays Message-ID: <Y1vNNSSWK1EkcohT@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?1747936036260151160?= X-GMAIL-MSGID: =?utf-8?q?1747936036260151160?= |
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
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
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 >
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
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
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 >
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
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 --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 */