diff mbox series

[next] ath11k: Replace one-element array with flexible-array member

Message ID 20210823172159.GA25800@embeddedor (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series [next] ath11k: Replace one-element array with flexible-array member | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Gustavo A. R. Silva Aug. 23, 2021, 5:21 p.m. UTC
There is a regular need in the kernel to provide a way to declare having a
dynamically sized set of trailing elements in a structure. Kernel code
should always use "flexible array members"[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].

Refactor the code a bit according to the use of a flexible-array member in
struct scan_chan_list_params instead of a one-element array, and use the
struct_size() helper.

Also, save 25 (too many) bytes that were being allocated:

$ pahole -C channel_param drivers/net/wireless/ath/ath11k/reg.o
struct channel_param {
	u8                         chan_id;              /*     0     1 */
	u8                         pwr;                  /*     1     1 */
	u32                        mhz;                  /*     2     4 */

	/* Bitfield combined with next fields */

	u32                        half_rate:1;          /*     4:16  4 */
	u32                        quarter_rate:1;       /*     4:17  4 */
	u32                        dfs_set:1;            /*     4:18  4 */
	u32                        dfs_set_cfreq2:1;     /*     4:19  4 */
	u32                        is_chan_passive:1;    /*     4:20  4 */
	u32                        allow_ht:1;           /*     4:21  4 */
	u32                        allow_vht:1;          /*     4:22  4 */
	u32                        allow_he:1;           /*     4:23  4 */
	u32                        set_agile:1;          /*     4:24  4 */
	u32                        psc_channel:1;        /*     4:25  4 */

	/* XXX 6 bits hole, try to pack */

	u32                        phy_mode;             /*     8     4 */
	u32                        cfreq1;               /*    12     4 */
	u32                        cfreq2;               /*    16     4 */
	char                       maxpower;             /*    20     1 */
	char                       minpower;             /*    21     1 */
	char                       maxregpower;          /*    22     1 */
	u8                         antennamax;           /*    23     1 */
	u8                         reg_class_id;         /*    24     1 */

	/* size: 25, cachelines: 1, members: 21 */
	/* sum members: 23 */
	/* sum bitfield members: 10 bits, bit holes: 1, sum bit holes: 6 bits */
	/* last cacheline: 25 bytes */
} __attribute__((__packed__));

as previously, sizeof(struct scan_chan_list_params) was 32 bytes:

$ pahole -C scan_chan_list_params drivers/net/wireless/ath/ath11k/reg.o
struct scan_chan_list_params {
	u32                        pdev_id;              /*     0     4 */
	u16                        nallchans;            /*     4     2 */
	struct channel_param       ch_param[1];          /*     6    25 */

	/* size: 32, cachelines: 1, members: 3 */
	/* padding: 1 */
	/* last cacheline: 32 bytes */
};

and now with the flexible array transformation it is just 8 bytes:

$ pahole -C scan_chan_list_params drivers/net/wireless/ath/ath11k/reg.o
struct scan_chan_list_params {
	u32                        pdev_id;              /*     0     4 */
	u16                        nallchans;            /*     4     2 */
	struct channel_param       ch_param[];           /*     6     0 */

	/* size: 8, cachelines: 1, members: 3 */
	/* padding: 2 */
	/* last cacheline: 8 bytes */
};

This helps with the ongoing efforts to globally enable -Warray-bounds and
get us closer to being able to tighten the FORTIFY_SOURCE routines on
memcpy().

This issue was found with the help of Coccinelle and audited and fixed,
manually.

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

Link: https://github.com/KSPP/linux/issues/79
Link: https://github.com/KSPP/linux/issues/109
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/net/wireless/ath/ath11k/reg.c | 7 ++-----
 drivers/net/wireless/ath/ath11k/wmi.c | 2 +-
 drivers/net/wireless/ath/ath11k/wmi.h | 2 +-
 3 files changed, 4 insertions(+), 7 deletions(-)

Comments

Gustavo A. R. Silva Sept. 12, 2021, 7:44 p.m. UTC | #1
Hi Kalle,

I wonder if you can take this patch, please.

Thanks
--
Gustavo

On 8/23/21 12:21, Gustavo A. R. Silva wrote:
> There is a regular need in the kernel to provide a way to declare having a
> dynamically sized set of trailing elements in a structure. Kernel code
> should always use "flexible array members"[1] for these cases. The older
> style of one-element or zero-length arrays should no longer be used[2].
> 
> Refactor the code a bit according to the use of a flexible-array member in
> struct scan_chan_list_params instead of a one-element array, and use the
> struct_size() helper.
> 
> Also, save 25 (too many) bytes that were being allocated:
> 
> $ pahole -C channel_param drivers/net/wireless/ath/ath11k/reg.o
> struct channel_param {
> 	u8                         chan_id;              /*     0     1 */
> 	u8                         pwr;                  /*     1     1 */
> 	u32                        mhz;                  /*     2     4 */
> 
> 	/* Bitfield combined with next fields */
> 
> 	u32                        half_rate:1;          /*     4:16  4 */
> 	u32                        quarter_rate:1;       /*     4:17  4 */
> 	u32                        dfs_set:1;            /*     4:18  4 */
> 	u32                        dfs_set_cfreq2:1;     /*     4:19  4 */
> 	u32                        is_chan_passive:1;    /*     4:20  4 */
> 	u32                        allow_ht:1;           /*     4:21  4 */
> 	u32                        allow_vht:1;          /*     4:22  4 */
> 	u32                        allow_he:1;           /*     4:23  4 */
> 	u32                        set_agile:1;          /*     4:24  4 */
> 	u32                        psc_channel:1;        /*     4:25  4 */
> 
> 	/* XXX 6 bits hole, try to pack */
> 
> 	u32                        phy_mode;             /*     8     4 */
> 	u32                        cfreq1;               /*    12     4 */
> 	u32                        cfreq2;               /*    16     4 */
> 	char                       maxpower;             /*    20     1 */
> 	char                       minpower;             /*    21     1 */
> 	char                       maxregpower;          /*    22     1 */
> 	u8                         antennamax;           /*    23     1 */
> 	u8                         reg_class_id;         /*    24     1 */
> 
> 	/* size: 25, cachelines: 1, members: 21 */
> 	/* sum members: 23 */
> 	/* sum bitfield members: 10 bits, bit holes: 1, sum bit holes: 6 bits */
> 	/* last cacheline: 25 bytes */
> } __attribute__((__packed__));
> 
> as previously, sizeof(struct scan_chan_list_params) was 32 bytes:
> 
> $ pahole -C scan_chan_list_params drivers/net/wireless/ath/ath11k/reg.o
> struct scan_chan_list_params {
> 	u32                        pdev_id;              /*     0     4 */
> 	u16                        nallchans;            /*     4     2 */
> 	struct channel_param       ch_param[1];          /*     6    25 */
> 
> 	/* size: 32, cachelines: 1, members: 3 */
> 	/* padding: 1 */
> 	/* last cacheline: 32 bytes */
> };
> 
> and now with the flexible array transformation it is just 8 bytes:
> 
> $ pahole -C scan_chan_list_params drivers/net/wireless/ath/ath11k/reg.o
> struct scan_chan_list_params {
> 	u32                        pdev_id;              /*     0     4 */
> 	u16                        nallchans;            /*     4     2 */
> 	struct channel_param       ch_param[];           /*     6     0 */
> 
> 	/* size: 8, cachelines: 1, members: 3 */
> 	/* padding: 2 */
> 	/* last cacheline: 8 bytes */
> };
> 
> This helps with the ongoing efforts to globally enable -Warray-bounds and
> get us closer to being able to tighten the FORTIFY_SOURCE routines on
> memcpy().
> 
> This issue was found with the help of Coccinelle and audited and fixed,
> manually.
> 
> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> [2] https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-length-and-one-element-arrays
> 
> Link: https://github.com/KSPP/linux/issues/79
> Link: https://github.com/KSPP/linux/issues/109
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>  drivers/net/wireless/ath/ath11k/reg.c | 7 ++-----
>  drivers/net/wireless/ath/ath11k/wmi.c | 2 +-
>  drivers/net/wireless/ath/ath11k/wmi.h | 2 +-
>  3 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/reg.c b/drivers/net/wireless/ath/ath11k/reg.c
> index e1a1df169034..c83d265185f1 100644
> --- a/drivers/net/wireless/ath/ath11k/reg.c
> +++ b/drivers/net/wireless/ath/ath11k/reg.c
> @@ -97,7 +97,6 @@ int ath11k_reg_update_chan_list(struct ath11k *ar)
>  	struct channel_param *ch;
>  	enum nl80211_band band;
>  	int num_channels = 0;
> -	int params_len;
>  	int i, ret;
>  
>  	bands = hw->wiphy->bands;
> @@ -117,10 +116,8 @@ int ath11k_reg_update_chan_list(struct ath11k *ar)
>  	if (WARN_ON(!num_channels))
>  		return -EINVAL;
>  
> -	params_len = sizeof(struct scan_chan_list_params) +
> -			num_channels * sizeof(struct channel_param);
> -	params = kzalloc(params_len, GFP_KERNEL);
> -
> +	params = kzalloc(struct_size(params, ch_param, num_channels),
> +			 GFP_KERNEL);
>  	if (!params)
>  		return -ENOMEM;
>  
> diff --git a/drivers/net/wireless/ath/ath11k/wmi.c b/drivers/net/wireless/ath/ath11k/wmi.c
> index 6c253eae9d06..0a634ba04509 100644
> --- a/drivers/net/wireless/ath/ath11k/wmi.c
> +++ b/drivers/net/wireless/ath/ath11k/wmi.c
> @@ -2285,7 +2285,7 @@ int ath11k_wmi_send_scan_chan_list_cmd(struct ath11k *ar,
>  	u16 num_send_chans, num_sends = 0, max_chan_limit = 0;
>  	u32 *reg1, *reg2;
>  
> -	tchan_info = &chan_list->ch_param[0];
> +	tchan_info = chan_list->ch_param;
>  	while (chan_list->nallchans) {
>  		len = sizeof(*cmd) + TLV_HDR_SIZE;
>  		max_chan_limit = (wmi->wmi_ab->max_msg_len[ar->pdev_idx] - len) /
> diff --git a/drivers/net/wireless/ath/ath11k/wmi.h b/drivers/net/wireless/ath/ath11k/wmi.h
> index d35c47e0b19d..d9c83726f65d 100644
> --- a/drivers/net/wireless/ath/ath11k/wmi.h
> +++ b/drivers/net/wireless/ath/ath11k/wmi.h
> @@ -3608,7 +3608,7 @@ struct wmi_stop_scan_cmd {
>  struct scan_chan_list_params {
>  	u32 pdev_id;
>  	u16 nallchans;
> -	struct channel_param ch_param[1];
> +	struct channel_param ch_param[];
>  };
>  
>  struct wmi_scan_chan_list_cmd {
>
Kalle Valo Sept. 14, 2021, 7:07 a.m. UTC | #2
"Gustavo A. R. Silva" <gustavo@embeddedor.com> writes:

> I wonder if you can take this patch, please.

This is in my queue, please do not take ath11k patches.
Gustavo A. R. Silva Sept. 14, 2021, 5:36 p.m. UTC | #3
On 9/14/21 02:07, Kalle Valo wrote:
> "Gustavo A. R. Silva" <gustavo@embeddedor.com> writes:
> 
>> I wonder if you can take this patch, please.
> 
> This is in my queue, please do not take ath11k patches.

Great, and sure thing. :)

Thanks
--
Gustavo
Kalle Valo Sept. 28, 2021, 10:54 a.m. UTC | #4
"Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:

> There is a regular need in the kernel to provide a way to declare having a
> dynamically sized set of trailing elements in a structure. Kernel code
> should always use "flexible array members"[1] for these cases. The older
> style of one-element or zero-length arrays should no longer be used[2].
> 
> Refactor the code a bit according to the use of a flexible-array member in
> struct scan_chan_list_params instead of a one-element array, and use the
> struct_size() helper.
> 
> Also, save 25 (too many) bytes that were being allocated:
> 
> $ pahole -C channel_param drivers/net/wireless/ath/ath11k/reg.o
> struct channel_param {
>         u8                         chan_id;              /*     0     1 */
>         u8                         pwr;                  /*     1     1 */
>         u32                        mhz;                  /*     2     4 */
> 
>         /* Bitfield combined with next fields */
> 
>         u32                        half_rate:1;          /*     4:16  4 */
>         u32                        quarter_rate:1;       /*     4:17  4 */
>         u32                        dfs_set:1;            /*     4:18  4 */
>         u32                        dfs_set_cfreq2:1;     /*     4:19  4 */
>         u32                        is_chan_passive:1;    /*     4:20  4 */
>         u32                        allow_ht:1;           /*     4:21  4 */
>         u32                        allow_vht:1;          /*     4:22  4 */
>         u32                        allow_he:1;           /*     4:23  4 */
>         u32                        set_agile:1;          /*     4:24  4 */
>         u32                        psc_channel:1;        /*     4:25  4 */
> 
>         /* XXX 6 bits hole, try to pack */
> 
>         u32                        phy_mode;             /*     8     4 */
>         u32                        cfreq1;               /*    12     4 */
>         u32                        cfreq2;               /*    16     4 */
>         char                       maxpower;             /*    20     1 */
>         char                       minpower;             /*    21     1 */
>         char                       maxregpower;          /*    22     1 */
>         u8                         antennamax;           /*    23     1 */
>         u8                         reg_class_id;         /*    24     1 */
> 
>         /* size: 25, cachelines: 1, members: 21 */
>         /* sum members: 23 */
>         /* sum bitfield members: 10 bits, bit holes: 1, sum bit holes: 6 bits */
>         /* last cacheline: 25 bytes */
> } __attribute__((__packed__));
> 
> as previously, sizeof(struct scan_chan_list_params) was 32 bytes:
> 
> $ pahole -C scan_chan_list_params drivers/net/wireless/ath/ath11k/reg.o
> struct scan_chan_list_params {
>         u32                        pdev_id;              /*     0     4 */
>         u16                        nallchans;            /*     4     2 */
>         struct channel_param       ch_param[1];          /*     6    25 */
> 
>         /* size: 32, cachelines: 1, members: 3 */
>         /* padding: 1 */
>         /* last cacheline: 32 bytes */
> };
> 
> and now with the flexible array transformation it is just 8 bytes:
> 
> $ pahole -C scan_chan_list_params drivers/net/wireless/ath/ath11k/reg.o
> struct scan_chan_list_params {
>         u32                        pdev_id;              /*     0     4 */
>         u16                        nallchans;            /*     4     2 */
>         struct channel_param       ch_param[];           /*     6     0 */
> 
>         /* size: 8, cachelines: 1, members: 3 */
>         /* padding: 2 */
>         /* last cacheline: 8 bytes */
> };
> 
> This helps with the ongoing efforts to globally enable -Warray-bounds and
> get us closer to being able to tighten the FORTIFY_SOURCE routines on
> memcpy().
> 
> This issue was found with the help of Coccinelle and audited and fixed,
> manually.
> 
> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> [2] https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-length-and-one-element-arrays
> 
> Link: https://github.com/KSPP/linux/issues/79
> Link: https://github.com/KSPP/linux/issues/109
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

Patch applied to ath-next branch of ath.git, thanks.

b2549465cdea ath11k: Replace one-element array with flexible-array member
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath11k/reg.c b/drivers/net/wireless/ath/ath11k/reg.c
index e1a1df169034..c83d265185f1 100644
--- a/drivers/net/wireless/ath/ath11k/reg.c
+++ b/drivers/net/wireless/ath/ath11k/reg.c
@@ -97,7 +97,6 @@  int ath11k_reg_update_chan_list(struct ath11k *ar)
 	struct channel_param *ch;
 	enum nl80211_band band;
 	int num_channels = 0;
-	int params_len;
 	int i, ret;
 
 	bands = hw->wiphy->bands;
@@ -117,10 +116,8 @@  int ath11k_reg_update_chan_list(struct ath11k *ar)
 	if (WARN_ON(!num_channels))
 		return -EINVAL;
 
-	params_len = sizeof(struct scan_chan_list_params) +
-			num_channels * sizeof(struct channel_param);
-	params = kzalloc(params_len, GFP_KERNEL);
-
+	params = kzalloc(struct_size(params, ch_param, num_channels),
+			 GFP_KERNEL);
 	if (!params)
 		return -ENOMEM;
 
diff --git a/drivers/net/wireless/ath/ath11k/wmi.c b/drivers/net/wireless/ath/ath11k/wmi.c
index 6c253eae9d06..0a634ba04509 100644
--- a/drivers/net/wireless/ath/ath11k/wmi.c
+++ b/drivers/net/wireless/ath/ath11k/wmi.c
@@ -2285,7 +2285,7 @@  int ath11k_wmi_send_scan_chan_list_cmd(struct ath11k *ar,
 	u16 num_send_chans, num_sends = 0, max_chan_limit = 0;
 	u32 *reg1, *reg2;
 
-	tchan_info = &chan_list->ch_param[0];
+	tchan_info = chan_list->ch_param;
 	while (chan_list->nallchans) {
 		len = sizeof(*cmd) + TLV_HDR_SIZE;
 		max_chan_limit = (wmi->wmi_ab->max_msg_len[ar->pdev_idx] - len) /
diff --git a/drivers/net/wireless/ath/ath11k/wmi.h b/drivers/net/wireless/ath/ath11k/wmi.h
index d35c47e0b19d..d9c83726f65d 100644
--- a/drivers/net/wireless/ath/ath11k/wmi.h
+++ b/drivers/net/wireless/ath/ath11k/wmi.h
@@ -3608,7 +3608,7 @@  struct wmi_stop_scan_cmd {
 struct scan_chan_list_params {
 	u32 pdev_id;
 	u16 nallchans;
-	struct channel_param ch_param[1];
+	struct channel_param ch_param[];
 };
 
 struct wmi_scan_chan_list_cmd {