Skip to contentSkip to conversation

ath10k: Replace zero-length array with flexible-array

PATCH In review Gustavo A. R. Silva 3 participants 4 messages May 7, 2020 – May 11, 2020

Subsystems (3)
Networking Network drivers Qualcomm Atheros ATH10K wireless driver
lore
Share & tools
lore

Gustavo A. R. Silva

author
#0 lore raw

PATCH v2 1/1 ath10k: Replace zero-length array with flexible-array

The current codebase makes use of the zero-length array language extension to the C90 standard, but the preferred mechanism to declare variable-length types such as these ones is a flexible array member[1][2], introduced in C99:

struct foo {
        int stuff;
        struct boo array[];
};

By making use of the mechanism above, we will get a compiler warning in case the flexible array does not occur last in the structure, which will help us prevent some kind of undefined behavior bugs from being inadvertently introduced[3] to the codebase from now on.

Also, notice that, dynamic memory allocations won't be affected by this change:

"Flexible array members have incomplete type, and so the sizeof operator may not be applied. As a quirk of the original implementation of zero-length arrays, sizeof evaluates to zero."[1]

sizeof(flexible-array-member) triggers a warning because flexible array members have incomplete type[1]. There are some instances of code in which the sizeof operator is being incorrectly/erroneously applied to zero-length arrays and the result is zero. Such instances may be hiding some bugs. So, this work (flexible-array member conversions) will also help to get completely rid of those sorts of issues.

This issue was found with the help of Coccinelle.

[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
Changes in v2:
- Rebase on top of ath.git master branch.

 drivers/net/wireless/ath/ath10k/ce.h       |  2 +-
 drivers/net/wireless/ath/ath10k/core.h     |  2 +-
 drivers/net/wireless/ath/ath10k/coredump.h |  4 +--
 drivers/net/wireless/ath/ath10k/debug.h    |  2 +-
 drivers/net/wireless/ath/ath10k/htt.h      | 42 +++++++++++-----------
 drivers/net/wireless/ath/ath10k/hw.h       |  2 +-
 drivers/net/wireless/ath/ath10k/wmi-tlv.h  |  6 ++--
 drivers/net/wireless/ath/ath10k/wmi.h      | 42 +++++++++++-----------
 8 files changed, 51 insertions(+), 51 deletions(-)
drivers/net/wireless/ath/ath10k/ce.h +1 -1
index 9711f0eb9117..75df79d43120 100644
@@ -110,7 +110,7 @@ struct ath10k_ce_ring {
 	struct ce_desc_64 *shadow_base;
 
 	/* keep last */
-	void *per_transfer_context[0];
+	void *per_transfer_context[];
 };
 
 struct ath10k_ce_pipe {
drivers/net/wireless/ath/ath10k/core.h +1 -1
index ceac76553b8f..5c18f6c20462 100644
@@ -1262,7 +1262,7 @@ struct ath10k {
 	int coex_gpio_pin;
 
 	/* must be last */
-	u8 drv_priv[0] __aligned(sizeof(void *));
+	u8 drv_priv[] __aligned(sizeof(void *));
 };
 
 static inline bool ath10k_peer_stats_enabled(struct ath10k *ar)
drivers/net/wireless/ath/ath10k/coredump.h +2 -2
index 8bf03e8c1d3a..e760ce1a5f1e 100644
@@ -88,7 +88,7 @@ struct ath10k_dump_file_data {
 	u8 unused[128];
 
 	/* struct ath10k_tlv_dump_data + more */
-	u8 data[0];
+	u8 data[];
 } __packed;
 
 struct ath10k_dump_ram_data_hdr {

@@ -100,7 +100,7 @@ struct ath10k_dump_ram_data_hdr {
 	/* length of payload data, not including this header */
 	__le32 length;
 
-	u8 data[0];
+	u8 data[];
 };
 
 /* magic number to fill the holes not copied due to sections in regions */
drivers/net/wireless/ath/ath10k/debug.h +1 -1
index 4cbfd9279d6f..997c1c80aba7 100644
@@ -65,7 +65,7 @@ struct ath10k_pktlog_hdr {
 	__le16 log_type; /* Type of log information foll this header */
 	__le16 size; /* Size of variable length log information in bytes */
 	__le32 timestamp;
-	u8 payload[0];
+	u8 payload[];
 } __packed;
 
 /* FIXME: How to calculate the buffer size sanely? */
drivers/net/wireless/ath/ath10k/htt.h +21 -21

165 lines not on this page · raw message

drivers/net/wireless/ath/ath10k/hw.h +1 -1
index d9907a4648a8..f16edcb9f326 100644
@@ -165,7 +165,7 @@ enum qca9377_chip_id_rev {
 struct ath10k_fw_ie {
 	__le32 id;
 	__le32 len;
-	u8 data[0];
+	u8 data[];
 };
 
 enum ath10k_fw_ie_type {
drivers/net/wireless/ath/ath10k/wmi-tlv.h +3 -3
index 6e0537dabd1d..e77b97ca5c7f 100644
@@ -1637,7 +1637,7 @@ wmi_tlv_svc_map_ext(const __le32 *in, unsigned long *out, size_t len)
 struct wmi_tlv {
 	__le16 len;
 	__le16 tag;
-	u8 value[0];
+	u8 value[];
 } __packed;
 
 struct ath10k_mgmt_tx_pkt_addr {

@@ -2037,7 +2037,7 @@ struct wmi_tlv_bcn_tx_status_ev {
 struct wmi_tlv_bcn_prb_info {
 	__le32 caps;
 	__le32 erp;
-	u8 ies[0];
+	u8 ies[];
 } __packed;
 
 struct wmi_tlv_bcn_tmpl_cmd {

@@ -2068,7 +2068,7 @@ struct wmi_tlv_diag_item {
 	__le16 len;
 	__le32 timestamp;
 	__le32 code;
-	u8 payload[0];
+	u8 payload[];
 } __packed;
 
 struct wmi_tlv_diag_data_ev {
drivers/net/wireless/ath/ath10k/wmi.h +21 -21

164 lines not on this page · raw message

Signature · 1 line
2.26.2
#1 lore raw

On 2020-05-06 21:11, Gustavo A. R. Silva wrote:
[...]

drivers/net/wireless/ath/ath10k/htt.h struct htt_tx_fetch_ind { · line 1679

Quoted 9 lines

static inline struct htt_stats_conf_item *htt_stats_conf_next_item(

drivers/net/wireless/ath/ath10k/htt.h +1 -1
@@ -1674,7 +1674,7 @@ struct htt_tx_fetch_ind {
 	__le16 num_resp_ids;
 	__le16 num_records;
 	struct htt_tx_fetch_record records[0];
-	__le32 resp_ids[0]; /* ath10k_htt_get_tx_fetch_ind_resp_ids() */
+	__le32 resp_ids[]; /* ath10k_htt_get_tx_fetch_ind_resp_ids() */
 } __packed;

Missed to handle records[0].

-Rajkumar

_______________________________________________
ath10k mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/ath10k

#2 lore raw
In reply to Gustavo A. R. Silva
Quoted 29 lines · Gustavo A. R. Silva

The current codebase makes use of the zero-length array language extension to the C90 standard, but the preferred mechanism to declare variable-length types such as these ones is a flexible array member[1][2], introduced in C99:

struct foo {
        int stuff;
        struct boo array[];
};

By making use of the mechanism above, we will get a compiler warning in case the flexible array does not occur last in the structure, which will help us prevent some kind of undefined behavior bugs from being inadvertently introduced[3] to the codebase from now on.

Also, notice that, dynamic memory allocations won't be affected by this change:

"Flexible array members have incomplete type, and so the sizeof operator may not be applied. As a quirk of the original implementation of zero-length arrays, sizeof evaluates to zero."[1]

sizeof(flexible-array-member) triggers a warning because flexible array members have incomplete type[1]. There are some instances of code in which the sizeof operator is being incorrectly/erroneously applied to zero-length arrays and the result is zero. Such instances may be hiding some bugs. So, this work (flexible-array member conversions) will also help to get completely rid of those sorts of issues.

Quoted 8 lines

This issue was found with the help of Coccinelle.

[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")

Signed-off-by Gustavo A. R. Silva <[email protected]>
Signed-off-by Kalle Valo <[email protected]>

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

d3ed0cf047cf ath10k: Replace zero-length array with flexible-array

Signature · 3 lines
https://patchwork.kernel.org/patch/11532369/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
#3 lore raw
In reply to Gustavo A. R. Silva
Quoted 29 lines · Gustavo A. R. Silva

The current codebase makes use of the zero-length array language extension to the C90 standard, but the preferred mechanism to declare variable-length types such as these ones is a flexible array member[1][2], introduced in C99:

struct foo {
        int stuff;
        struct boo array[];
};

By making use of the mechanism above, we will get a compiler warning in case the flexible array does not occur last in the structure, which will help us prevent some kind of undefined behavior bugs from being inadvertently introduced[3] to the codebase from now on.

Also, notice that, dynamic memory allocations won't be affected by this change:

"Flexible array members have incomplete type, and so the sizeof operator may not be applied. As a quirk of the original implementation of zero-length arrays, sizeof evaluates to zero."[1]

sizeof(flexible-array-member) triggers a warning because flexible array members have incomplete type[1]. There are some instances of code in which the sizeof operator is being incorrectly/erroneously applied to zero-length arrays and the result is zero. Such instances may be hiding some bugs. So, this work (flexible-array member conversions) will also help to get completely rid of those sorts of issues.

Quoted 8 lines

This issue was found with the help of Coccinelle.

[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")

Signed-off-by Gustavo A. R. Silva <[email protected]>
Signed-off-by Kalle Valo <[email protected]>

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

d3ed0cf047cf ath10k: Replace zero-length array with flexible-array

Signature · 8 lines
https://patchwork.kernel.org/patch/11532369/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

_______________________________________________
ath10k mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/ath10k