| Message ID | 20210823172159.GA25800@embeddedor |
|---|---|
| State | New |
| Headers | show |
| Series | [next] ath11k: Replace one-element array with flexible-array member | expand |
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 { >
"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.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
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
"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 -- https://patchwork.kernel.org/project/linux-wireless/patch/20210823172159.GA25800@embeddedor/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
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 {
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(-)