* [PATCH][next] Bluetooth: L2CAP: Avoid -Wflex-array-member-not-at-end warnings
@ 2024-03-26 20:02 Gustavo A. R. Silva
2024-03-26 21:12 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 3+ messages in thread
From: Gustavo A. R. Silva @ 2024-03-26 20:02 UTC (permalink / raw)
To: Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: linux-bluetooth, netdev, linux-kernel, Gustavo A. R. Silva,
linux-hardening
-Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
ready to enable it globally.
There are currently a couple of objects (`req` and `rsp`), in a couple
of structures, that contain flexible structures (`struct l2cap_ecred_conn_req`
and `struct l2cap_ecred_conn_rsp`), for example:
struct l2cap_ecred_rsp_data {
struct {
struct l2cap_ecred_conn_rsp rsp;
__le16 scid[L2CAP_ECRED_MAX_CID];
} __packed pdu;
int count;
};
in the struct above, `struct l2cap_ecred_conn_rsp` is a flexible
structure:
struct l2cap_ecred_conn_rsp {
__le16 mtu;
__le16 mps;
__le16 credits;
__le16 result;
__le16 dcid[];
};
So, in order to avoid ending up with a flexible-array member in the
middle of another structure, we use the `struct_group_tagged()` (and
`__struct_group()` when the flexible structure is `__packed`) helper
to separate the flexible array from the rest of the members in the
flexible structure:
struct l2cap_ecred_conn_rsp {
struct_group_tagged(l2cap_ecred_conn_rsp_hdr, hdr,
... the rest of members
);
__le16 dcid[];
};
With the change described above, we now declare objects of the type of
the tagged struct, in this example `struct l2cap_ecred_conn_rsp_hdr`,
without embedding flexible arrays in the middle of other structures:
struct l2cap_ecred_rsp_data {
struct {
struct l2cap_ecred_conn_rsp_hdr rsp;
__le16 scid[L2CAP_ECRED_MAX_CID];
} __packed pdu;
int count;
};
Also, when the flexible-array member needs to be accessed, we use
`container_of()` to retrieve a pointer to the flexible structure.
We also use the `DEFINE_RAW_FLEX()` helper for a couple of on-stack
definitions of a flexible structure where the size of the flexible-array
member is known at compile-time.
So, with these changes, fix the following warnings:
net/bluetooth/l2cap_core.c:1260:45: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
net/bluetooth/l2cap_core.c:3740:45: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
net/bluetooth/l2cap_core.c:4999:45: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
net/bluetooth/l2cap_core.c:7116:47: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
Link: https://github.com/KSPP/linux/issues/202
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
Hi!
I wonder if `struct l2cap_ecred_conn_rsp` should also be `__packed`.
Thanks
- Gustavo
include/net/bluetooth/l2cap.h | 20 +++++++++------
net/bluetooth/l2cap_core.c | 46 ++++++++++++++++-------------------
2 files changed, 33 insertions(+), 33 deletions(-)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index a4278aa618ab..92a143517d83 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -463,18 +463,22 @@ struct l2cap_le_credits {
#define L2CAP_ECRED_MAX_CID 5
struct l2cap_ecred_conn_req {
- __le16 psm;
- __le16 mtu;
- __le16 mps;
- __le16 credits;
+ __struct_group(l2cap_ecred_conn_req_hdr, hdr, __packed,
+ __le16 psm;
+ __le16 mtu;
+ __le16 mps;
+ __le16 credits;
+ );
__le16 scid[];
} __packed;
struct l2cap_ecred_conn_rsp {
- __le16 mtu;
- __le16 mps;
- __le16 credits;
- __le16 result;
+ struct_group_tagged(l2cap_ecred_conn_rsp_hdr, hdr,
+ __le16 mtu;
+ __le16 mps;
+ __le16 credits;
+ __le16 result;
+ );
__le16 dcid[];
};
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 467b242d8be0..bf087eca489e 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1257,7 +1257,7 @@ static void l2cap_le_connect(struct l2cap_chan *chan)
struct l2cap_ecred_conn_data {
struct {
- struct l2cap_ecred_conn_req req;
+ struct l2cap_ecred_conn_req_hdr req;
__le16 scid[5];
} __packed pdu;
struct l2cap_chan *chan;
@@ -3737,7 +3737,7 @@ static void l2cap_ecred_list_defer(struct l2cap_chan *chan, void *data)
struct l2cap_ecred_rsp_data {
struct {
- struct l2cap_ecred_conn_rsp rsp;
+ struct l2cap_ecred_conn_rsp_hdr rsp;
__le16 scid[L2CAP_ECRED_MAX_CID];
} __packed pdu;
int count;
@@ -3746,6 +3746,8 @@ struct l2cap_ecred_rsp_data {
static void l2cap_ecred_rsp_defer(struct l2cap_chan *chan, void *data)
{
struct l2cap_ecred_rsp_data *rsp = data;
+ struct l2cap_ecred_conn_rsp *rsp_flex =
+ container_of(&rsp->pdu.rsp, struct l2cap_ecred_conn_rsp, hdr);
if (test_bit(FLAG_ECRED_CONN_REQ_SENT, &chan->flags))
return;
@@ -3755,7 +3757,7 @@ static void l2cap_ecred_rsp_defer(struct l2cap_chan *chan, void *data)
/* Include all channels pending with the same ident */
if (!rsp->pdu.rsp.result)
- rsp->pdu.rsp.dcid[rsp->count++] = cpu_to_le16(chan->scid);
+ rsp_flex->dcid[rsp->count++] = cpu_to_le16(chan->scid);
else
l2cap_chan_del(chan, ECONNRESET);
}
@@ -4995,10 +4997,7 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
u8 *data)
{
struct l2cap_ecred_conn_req *req = (void *) data;
- struct {
- struct l2cap_ecred_conn_rsp rsp;
- __le16 dcid[L2CAP_ECRED_MAX_CID];
- } __packed pdu;
+ DEFINE_RAW_FLEX(struct l2cap_ecred_conn_rsp, pdu, dcid, L2CAP_ECRED_MAX_CID);
struct l2cap_chan *chan, *pchan;
u16 mtu, mps;
__le16 psm;
@@ -5017,7 +5016,7 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
cmd_len -= sizeof(*req);
num_scid = cmd_len / sizeof(u16);
- if (num_scid > ARRAY_SIZE(pdu.dcid)) {
+ if (num_scid > L2CAP_ECRED_MAX_CID) {
result = L2CAP_CR_LE_INVALID_PARAMS;
goto response;
}
@@ -5046,7 +5045,7 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
BT_DBG("psm 0x%2.2x mtu %u mps %u", __le16_to_cpu(psm), mtu, mps);
- memset(&pdu, 0, sizeof(pdu));
+ memset(pdu, 0, sizeof(*pdu));
/* Check if we have socket listening on psm */
pchan = l2cap_global_chan_by_psm(BT_LISTEN, psm, &conn->hcon->src,
@@ -5072,8 +5071,8 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
BT_DBG("scid[%d] 0x%4.4x", i, scid);
- pdu.dcid[i] = 0x0000;
- len += sizeof(*pdu.dcid);
+ pdu->dcid[i] = 0x0000;
+ len += sizeof(*pdu->dcid);
/* Check for valid dynamic CID range */
if (scid < L2CAP_CID_DYN_START || scid > L2CAP_CID_LE_DYN_END) {
@@ -5107,13 +5106,13 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
l2cap_ecred_init(chan, __le16_to_cpu(req->credits));
/* Init response */
- if (!pdu.rsp.credits) {
- pdu.rsp.mtu = cpu_to_le16(chan->imtu);
- pdu.rsp.mps = cpu_to_le16(chan->mps);
- pdu.rsp.credits = cpu_to_le16(chan->rx_credits);
+ if (!pdu->credits) {
+ pdu->mtu = cpu_to_le16(chan->imtu);
+ pdu->mps = cpu_to_le16(chan->mps);
+ pdu->credits = cpu_to_le16(chan->rx_credits);
}
- pdu.dcid[i] = cpu_to_le16(chan->scid);
+ pdu->dcid[i] = cpu_to_le16(chan->scid);
__set_chan_timer(chan, chan->ops->get_sndtimeo(chan));
@@ -5135,13 +5134,13 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
l2cap_chan_put(pchan);
response:
- pdu.rsp.result = cpu_to_le16(result);
+ pdu->result = cpu_to_le16(result);
if (defer)
return 0;
l2cap_send_cmd(conn, cmd->ident, L2CAP_ECRED_CONN_RSP,
- sizeof(pdu.rsp) + len, &pdu);
+ sizeof(*pdu) + len, pdu);
return 0;
}
@@ -7112,14 +7111,11 @@ EXPORT_SYMBOL_GPL(l2cap_chan_connect);
static void l2cap_ecred_reconfigure(struct l2cap_chan *chan)
{
struct l2cap_conn *conn = chan->conn;
- struct {
- struct l2cap_ecred_reconf_req req;
- __le16 scid;
- } pdu;
+ DEFINE_RAW_FLEX(struct l2cap_ecred_reconf_req, pdu, scid, 1);
- pdu.req.mtu = cpu_to_le16(chan->imtu);
- pdu.req.mps = cpu_to_le16(chan->mps);
- pdu.scid = cpu_to_le16(chan->scid);
+ pdu->mtu = cpu_to_le16(chan->imtu);
+ pdu->mps = cpu_to_le16(chan->mps);
+ pdu->scid[0] = cpu_to_le16(chan->scid);
chan->ident = l2cap_get_ident(conn);
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH][next] Bluetooth: L2CAP: Avoid -Wflex-array-member-not-at-end warnings
2024-03-26 20:02 [PATCH][next] Bluetooth: L2CAP: Avoid -Wflex-array-member-not-at-end warnings Gustavo A. R. Silva
@ 2024-03-26 21:12 ` Luiz Augusto von Dentz
2024-03-26 21:57 ` Gustavo A. R. Silva
0 siblings, 1 reply; 3+ messages in thread
From: Luiz Augusto von Dentz @ 2024-03-26 21:12 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Marcel Holtmann, Johan Hedberg, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, linux-bluetooth, netdev,
linux-kernel, linux-hardening
Hi Gustavo,
On Tue, Mar 26, 2024 at 4:02 PM Gustavo A. R. Silva
<gustavoars@kernel.org> wrote:
>
> -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
> ready to enable it globally.
>
> There are currently a couple of objects (`req` and `rsp`), in a couple
> of structures, that contain flexible structures (`struct l2cap_ecred_conn_req`
> and `struct l2cap_ecred_conn_rsp`), for example:
>
> struct l2cap_ecred_rsp_data {
> struct {
> struct l2cap_ecred_conn_rsp rsp;
> __le16 scid[L2CAP_ECRED_MAX_CID];
> } __packed pdu;
> int count;
> };
>
> in the struct above, `struct l2cap_ecred_conn_rsp` is a flexible
> structure:
>
> struct l2cap_ecred_conn_rsp {
> __le16 mtu;
> __le16 mps;
> __le16 credits;
> __le16 result;
> __le16 dcid[];
> };
>
> So, in order to avoid ending up with a flexible-array member in the
> middle of another structure, we use the `struct_group_tagged()` (and
> `__struct_group()` when the flexible structure is `__packed`) helper
> to separate the flexible array from the rest of the members in the
> flexible structure:
>
> struct l2cap_ecred_conn_rsp {
> struct_group_tagged(l2cap_ecred_conn_rsp_hdr, hdr,
>
> ... the rest of members
>
> );
> __le16 dcid[];
> };
Wouldn't it be better, more readable at least, to not define the
l2cap_ecred_conn_rsp_hdr inside thought? Instead just do:
struct_group_tagged(l2cap_ecred_conn_rsp_hdr, hdr,
...
};
struct l2cap_ecred_conn_rsp {
struct l2cap_ecred_conn_rsp_hdr hdr;
__le16 dcid[];
};
Or was this done this way in order to maintain the same fields?
> With the change described above, we now declare objects of the type of
> the tagged struct, in this example `struct l2cap_ecred_conn_rsp_hdr`,
> without embedding flexible arrays in the middle of other structures:
>
> struct l2cap_ecred_rsp_data {
> struct {
> struct l2cap_ecred_conn_rsp_hdr rsp;
> __le16 scid[L2CAP_ECRED_MAX_CID];
> } __packed pdu;
> int count;
> };
>
> Also, when the flexible-array member needs to be accessed, we use
> `container_of()` to retrieve a pointer to the flexible structure.
>
> We also use the `DEFINE_RAW_FLEX()` helper for a couple of on-stack
> definitions of a flexible structure where the size of the flexible-array
> member is known at compile-time.
>
> So, with these changes, fix the following warnings:
> net/bluetooth/l2cap_core.c:1260:45: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> net/bluetooth/l2cap_core.c:3740:45: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> net/bluetooth/l2cap_core.c:4999:45: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> net/bluetooth/l2cap_core.c:7116:47: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
>
> Link: https://github.com/KSPP/linux/issues/202
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>
> Hi!
>
> I wonder if `struct l2cap_ecred_conn_rsp` should also be `__packed`.
>
> Thanks
> - Gustavo
>
> include/net/bluetooth/l2cap.h | 20 +++++++++------
> net/bluetooth/l2cap_core.c | 46 ++++++++++++++++-------------------
> 2 files changed, 33 insertions(+), 33 deletions(-)
>
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index a4278aa618ab..92a143517d83 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -463,18 +463,22 @@ struct l2cap_le_credits {
> #define L2CAP_ECRED_MAX_CID 5
>
> struct l2cap_ecred_conn_req {
> - __le16 psm;
> - __le16 mtu;
> - __le16 mps;
> - __le16 credits;
> + __struct_group(l2cap_ecred_conn_req_hdr, hdr, __packed,
> + __le16 psm;
> + __le16 mtu;
> + __le16 mps;
> + __le16 credits;
> + );
> __le16 scid[];
> } __packed;
>
> struct l2cap_ecred_conn_rsp {
> - __le16 mtu;
> - __le16 mps;
> - __le16 credits;
> - __le16 result;
> + struct_group_tagged(l2cap_ecred_conn_rsp_hdr, hdr,
> + __le16 mtu;
> + __le16 mps;
> + __le16 credits;
> + __le16 result;
> + );
> __le16 dcid[];
> };
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 467b242d8be0..bf087eca489e 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -1257,7 +1257,7 @@ static void l2cap_le_connect(struct l2cap_chan *chan)
>
> struct l2cap_ecred_conn_data {
> struct {
> - struct l2cap_ecred_conn_req req;
> + struct l2cap_ecred_conn_req_hdr req;
> __le16 scid[5];
> } __packed pdu;
Can't we just use DEFINE_RAW_FLEX for the pdu field above?
> struct l2cap_chan *chan;
> @@ -3737,7 +3737,7 @@ static void l2cap_ecred_list_defer(struct l2cap_chan *chan, void *data)
>
> struct l2cap_ecred_rsp_data {
> struct {
> - struct l2cap_ecred_conn_rsp rsp;
> + struct l2cap_ecred_conn_rsp_hdr rsp;
> __le16 scid[L2CAP_ECRED_MAX_CID];
> } __packed pdu;
Ditto.
> int count;
> @@ -3746,6 +3746,8 @@ struct l2cap_ecred_rsp_data {
> static void l2cap_ecred_rsp_defer(struct l2cap_chan *chan, void *data)
> {
> struct l2cap_ecred_rsp_data *rsp = data;
> + struct l2cap_ecred_conn_rsp *rsp_flex =
> + container_of(&rsp->pdu.rsp, struct l2cap_ecred_conn_rsp, hdr);
>
> if (test_bit(FLAG_ECRED_CONN_REQ_SENT, &chan->flags))
> return;
> @@ -3755,7 +3757,7 @@ static void l2cap_ecred_rsp_defer(struct l2cap_chan *chan, void *data)
>
> /* Include all channels pending with the same ident */
> if (!rsp->pdu.rsp.result)
> - rsp->pdu.rsp.dcid[rsp->count++] = cpu_to_le16(chan->scid);
> + rsp_flex->dcid[rsp->count++] = cpu_to_le16(chan->scid);
> else
> l2cap_chan_del(chan, ECONNRESET);
> }
> @@ -4995,10 +4997,7 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
> u8 *data)
> {
> struct l2cap_ecred_conn_req *req = (void *) data;
> - struct {
> - struct l2cap_ecred_conn_rsp rsp;
> - __le16 dcid[L2CAP_ECRED_MAX_CID];
> - } __packed pdu;
> + DEFINE_RAW_FLEX(struct l2cap_ecred_conn_rsp, pdu, dcid, L2CAP_ECRED_MAX_CID);
> struct l2cap_chan *chan, *pchan;
> u16 mtu, mps;
> __le16 psm;
> @@ -5017,7 +5016,7 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
> cmd_len -= sizeof(*req);
> num_scid = cmd_len / sizeof(u16);
>
> - if (num_scid > ARRAY_SIZE(pdu.dcid)) {
> + if (num_scid > L2CAP_ECRED_MAX_CID) {
> result = L2CAP_CR_LE_INVALID_PARAMS;
> goto response;
> }
> @@ -5046,7 +5045,7 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
>
> BT_DBG("psm 0x%2.2x mtu %u mps %u", __le16_to_cpu(psm), mtu, mps);
>
> - memset(&pdu, 0, sizeof(pdu));
> + memset(pdu, 0, sizeof(*pdu));
>
> /* Check if we have socket listening on psm */
> pchan = l2cap_global_chan_by_psm(BT_LISTEN, psm, &conn->hcon->src,
> @@ -5072,8 +5071,8 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
>
> BT_DBG("scid[%d] 0x%4.4x", i, scid);
>
> - pdu.dcid[i] = 0x0000;
> - len += sizeof(*pdu.dcid);
> + pdu->dcid[i] = 0x0000;
> + len += sizeof(*pdu->dcid);
>
> /* Check for valid dynamic CID range */
> if (scid < L2CAP_CID_DYN_START || scid > L2CAP_CID_LE_DYN_END) {
> @@ -5107,13 +5106,13 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
> l2cap_ecred_init(chan, __le16_to_cpu(req->credits));
>
> /* Init response */
> - if (!pdu.rsp.credits) {
> - pdu.rsp.mtu = cpu_to_le16(chan->imtu);
> - pdu.rsp.mps = cpu_to_le16(chan->mps);
> - pdu.rsp.credits = cpu_to_le16(chan->rx_credits);
> + if (!pdu->credits) {
> + pdu->mtu = cpu_to_le16(chan->imtu);
> + pdu->mps = cpu_to_le16(chan->mps);
> + pdu->credits = cpu_to_le16(chan->rx_credits);
> }
>
> - pdu.dcid[i] = cpu_to_le16(chan->scid);
> + pdu->dcid[i] = cpu_to_le16(chan->scid);
>
> __set_chan_timer(chan, chan->ops->get_sndtimeo(chan));
>
> @@ -5135,13 +5134,13 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
> l2cap_chan_put(pchan);
>
> response:
> - pdu.rsp.result = cpu_to_le16(result);
> + pdu->result = cpu_to_le16(result);
>
> if (defer)
> return 0;
>
> l2cap_send_cmd(conn, cmd->ident, L2CAP_ECRED_CONN_RSP,
> - sizeof(pdu.rsp) + len, &pdu);
> + sizeof(*pdu) + len, pdu);
>
> return 0;
> }
> @@ -7112,14 +7111,11 @@ EXPORT_SYMBOL_GPL(l2cap_chan_connect);
> static void l2cap_ecred_reconfigure(struct l2cap_chan *chan)
> {
> struct l2cap_conn *conn = chan->conn;
> - struct {
> - struct l2cap_ecred_reconf_req req;
> - __le16 scid;
> - } pdu;
> + DEFINE_RAW_FLEX(struct l2cap_ecred_reconf_req, pdu, scid, 1);
>
> - pdu.req.mtu = cpu_to_le16(chan->imtu);
> - pdu.req.mps = cpu_to_le16(chan->mps);
> - pdu.scid = cpu_to_le16(chan->scid);
> + pdu->mtu = cpu_to_le16(chan->imtu);
> + pdu->mps = cpu_to_le16(chan->mps);
> + pdu->scid[0] = cpu_to_le16(chan->scid);
>
> chan->ident = l2cap_get_ident(conn);
>
> --
> 2.34.1
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH][next] Bluetooth: L2CAP: Avoid -Wflex-array-member-not-at-end warnings
2024-03-26 21:12 ` Luiz Augusto von Dentz
@ 2024-03-26 21:57 ` Gustavo A. R. Silva
0 siblings, 0 replies; 3+ messages in thread
From: Gustavo A. R. Silva @ 2024-03-26 21:57 UTC (permalink / raw)
To: Luiz Augusto von Dentz, Gustavo A. R. Silva
Cc: Marcel Holtmann, Johan Hedberg, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, linux-bluetooth, netdev,
linux-kernel, linux-hardening
On 3/26/24 15:12, Luiz Augusto von Dentz wrote:
> Hi Gustavo,
>
> On Tue, Mar 26, 2024 at 4:02 PM Gustavo A. R. Silva
> <gustavoars@kernel.org> wrote:
>>
>> -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
>> ready to enable it globally.
>>
>> There are currently a couple of objects (`req` and `rsp`), in a couple
>> of structures, that contain flexible structures (`struct l2cap_ecred_conn_req`
>> and `struct l2cap_ecred_conn_rsp`), for example:
>>
>> struct l2cap_ecred_rsp_data {
>> struct {
>> struct l2cap_ecred_conn_rsp rsp;
>> __le16 scid[L2CAP_ECRED_MAX_CID];
>> } __packed pdu;
>> int count;
>> };
>>
>> in the struct above, `struct l2cap_ecred_conn_rsp` is a flexible
>> structure:
>>
>> struct l2cap_ecred_conn_rsp {
>> __le16 mtu;
>> __le16 mps;
>> __le16 credits;
>> __le16 result;
>> __le16 dcid[];
>> };
>>
>> So, in order to avoid ending up with a flexible-array member in the
>> middle of another structure, we use the `struct_group_tagged()` (and
>> `__struct_group()` when the flexible structure is `__packed`) helper
>> to separate the flexible array from the rest of the members in the
>> flexible structure:
>>
>> struct l2cap_ecred_conn_rsp {
>> struct_group_tagged(l2cap_ecred_conn_rsp_hdr, hdr,
>>
>> ... the rest of members
>>
>> );
>> __le16 dcid[];
>> };
>
> Wouldn't it be better, more readable at least, to not define the
> l2cap_ecred_conn_rsp_hdr inside thought? Instead just do:
>
> struct_group_tagged(l2cap_ecred_conn_rsp_hdr, hdr,
> ...
> };
>
> struct l2cap_ecred_conn_rsp {
> struct l2cap_ecred_conn_rsp_hdr hdr;
> __le16 dcid[];
> };
>
> Or was this done this way in order to maintain the same fields?
Exactly. But I can change it if people consider that's better.
>
>> With the change described above, we now declare objects of the type of
>> the tagged struct, in this example `struct l2cap_ecred_conn_rsp_hdr`,
>> without embedding flexible arrays in the middle of other structures:
>>
>> struct l2cap_ecred_rsp_data {
>> struct {
>> struct l2cap_ecred_conn_rsp_hdr rsp;
>> __le16 scid[L2CAP_ECRED_MAX_CID];
>> } __packed pdu;
>> int count;
>> };
>>
>> Also, when the flexible-array member needs to be accessed, we use
>> `container_of()` to retrieve a pointer to the flexible structure.
>>
>> We also use the `DEFINE_RAW_FLEX()` helper for a couple of on-stack
>> definitions of a flexible structure where the size of the flexible-array
>> member is known at compile-time.
>>
>> So, with these changes, fix the following warnings:
>> net/bluetooth/l2cap_core.c:1260:45: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
>> net/bluetooth/l2cap_core.c:3740:45: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
>> net/bluetooth/l2cap_core.c:4999:45: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
>> net/bluetooth/l2cap_core.c:7116:47: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
>>
>> Link: https://github.com/KSPP/linux/issues/202
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>> ---
>>
>> Hi!
>>
>> I wonder if `struct l2cap_ecred_conn_rsp` should also be `__packed`.
>>
>> Thanks
>> - Gustavo
>>
>> include/net/bluetooth/l2cap.h | 20 +++++++++------
>> net/bluetooth/l2cap_core.c | 46 ++++++++++++++++-------------------
>> 2 files changed, 33 insertions(+), 33 deletions(-)
>>
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index a4278aa618ab..92a143517d83 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -463,18 +463,22 @@ struct l2cap_le_credits {
>> #define L2CAP_ECRED_MAX_CID 5
>>
>> struct l2cap_ecred_conn_req {
>> - __le16 psm;
>> - __le16 mtu;
>> - __le16 mps;
>> - __le16 credits;
>> + __struct_group(l2cap_ecred_conn_req_hdr, hdr, __packed,
>> + __le16 psm;
>> + __le16 mtu;
>> + __le16 mps;
>> + __le16 credits;
>> + );
>> __le16 scid[];
>> } __packed;
>>
>> struct l2cap_ecred_conn_rsp {
>> - __le16 mtu;
>> - __le16 mps;
>> - __le16 credits;
>> - __le16 result;
>> + struct_group_tagged(l2cap_ecred_conn_rsp_hdr, hdr,
>> + __le16 mtu;
>> + __le16 mps;
>> + __le16 credits;
>> + __le16 result;
>> + );
>> __le16 dcid[];
>> };
>>
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index 467b242d8be0..bf087eca489e 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -1257,7 +1257,7 @@ static void l2cap_le_connect(struct l2cap_chan *chan)
>>
>> struct l2cap_ecred_conn_data {
>> struct {
>> - struct l2cap_ecred_conn_req req;
>> + struct l2cap_ecred_conn_req_hdr req;
>> __le16 scid[5];
>> } __packed pdu;
>
> Can't we just use DEFINE_RAW_FLEX for the pdu field above?
No; DEFINE_FLEX() and DEFINE_RAW_FLEX() are for on-stack code only.
Thanks
--
Gustavo
>
>> struct l2cap_chan *chan;
>> @@ -3737,7 +3737,7 @@ static void l2cap_ecred_list_defer(struct l2cap_chan *chan, void *data)
>>
>> struct l2cap_ecred_rsp_data {
>> struct {
>> - struct l2cap_ecred_conn_rsp rsp;
>> + struct l2cap_ecred_conn_rsp_hdr rsp;
>> __le16 scid[L2CAP_ECRED_MAX_CID];
>> } __packed pdu;
>
> Ditto.
>
>> int count;
>> @@ -3746,6 +3746,8 @@ struct l2cap_ecred_rsp_data {
>> static void l2cap_ecred_rsp_defer(struct l2cap_chan *chan, void *data)
>> {
>> struct l2cap_ecred_rsp_data *rsp = data;
>> + struct l2cap_ecred_conn_rsp *rsp_flex =
>> + container_of(&rsp->pdu.rsp, struct l2cap_ecred_conn_rsp, hdr);
>>
>> if (test_bit(FLAG_ECRED_CONN_REQ_SENT, &chan->flags))
>> return;
>> @@ -3755,7 +3757,7 @@ static void l2cap_ecred_rsp_defer(struct l2cap_chan *chan, void *data)
>>
>> /* Include all channels pending with the same ident */
>> if (!rsp->pdu.rsp.result)
>> - rsp->pdu.rsp.dcid[rsp->count++] = cpu_to_le16(chan->scid);
>> + rsp_flex->dcid[rsp->count++] = cpu_to_le16(chan->scid);
>> else
>> l2cap_chan_del(chan, ECONNRESET);
>> }
>> @@ -4995,10 +4997,7 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
>> u8 *data)
>> {
>> struct l2cap_ecred_conn_req *req = (void *) data;
>> - struct {
>> - struct l2cap_ecred_conn_rsp rsp;
>> - __le16 dcid[L2CAP_ECRED_MAX_CID];
>> - } __packed pdu;
>> + DEFINE_RAW_FLEX(struct l2cap_ecred_conn_rsp, pdu, dcid, L2CAP_ECRED_MAX_CID);
>> struct l2cap_chan *chan, *pchan;
>> u16 mtu, mps;
>> __le16 psm;
>> @@ -5017,7 +5016,7 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
>> cmd_len -= sizeof(*req);
>> num_scid = cmd_len / sizeof(u16);
>>
>> - if (num_scid > ARRAY_SIZE(pdu.dcid)) {
>> + if (num_scid > L2CAP_ECRED_MAX_CID) {
>> result = L2CAP_CR_LE_INVALID_PARAMS;
>> goto response;
>> }
>> @@ -5046,7 +5045,7 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
>>
>> BT_DBG("psm 0x%2.2x mtu %u mps %u", __le16_to_cpu(psm), mtu, mps);
>>
>> - memset(&pdu, 0, sizeof(pdu));
>> + memset(pdu, 0, sizeof(*pdu));
>>
>> /* Check if we have socket listening on psm */
>> pchan = l2cap_global_chan_by_psm(BT_LISTEN, psm, &conn->hcon->src,
>> @@ -5072,8 +5071,8 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
>>
>> BT_DBG("scid[%d] 0x%4.4x", i, scid);
>>
>> - pdu.dcid[i] = 0x0000;
>> - len += sizeof(*pdu.dcid);
>> + pdu->dcid[i] = 0x0000;
>> + len += sizeof(*pdu->dcid);
>>
>> /* Check for valid dynamic CID range */
>> if (scid < L2CAP_CID_DYN_START || scid > L2CAP_CID_LE_DYN_END) {
>> @@ -5107,13 +5106,13 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
>> l2cap_ecred_init(chan, __le16_to_cpu(req->credits));
>>
>> /* Init response */
>> - if (!pdu.rsp.credits) {
>> - pdu.rsp.mtu = cpu_to_le16(chan->imtu);
>> - pdu.rsp.mps = cpu_to_le16(chan->mps);
>> - pdu.rsp.credits = cpu_to_le16(chan->rx_credits);
>> + if (!pdu->credits) {
>> + pdu->mtu = cpu_to_le16(chan->imtu);
>> + pdu->mps = cpu_to_le16(chan->mps);
>> + pdu->credits = cpu_to_le16(chan->rx_credits);
>> }
>>
>> - pdu.dcid[i] = cpu_to_le16(chan->scid);
>> + pdu->dcid[i] = cpu_to_le16(chan->scid);
>>
>> __set_chan_timer(chan, chan->ops->get_sndtimeo(chan));
>>
>> @@ -5135,13 +5134,13 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
>> l2cap_chan_put(pchan);
>>
>> response:
>> - pdu.rsp.result = cpu_to_le16(result);
>> + pdu->result = cpu_to_le16(result);
>>
>> if (defer)
>> return 0;
>>
>> l2cap_send_cmd(conn, cmd->ident, L2CAP_ECRED_CONN_RSP,
>> - sizeof(pdu.rsp) + len, &pdu);
>> + sizeof(*pdu) + len, pdu);
>>
>> return 0;
>> }
>> @@ -7112,14 +7111,11 @@ EXPORT_SYMBOL_GPL(l2cap_chan_connect);
>> static void l2cap_ecred_reconfigure(struct l2cap_chan *chan)
>> {
>> struct l2cap_conn *conn = chan->conn;
>> - struct {
>> - struct l2cap_ecred_reconf_req req;
>> - __le16 scid;
>> - } pdu;
>> + DEFINE_RAW_FLEX(struct l2cap_ecred_reconf_req, pdu, scid, 1);
>>
>> - pdu.req.mtu = cpu_to_le16(chan->imtu);
>> - pdu.req.mps = cpu_to_le16(chan->mps);
>> - pdu.scid = cpu_to_le16(chan->scid);
>> + pdu->mtu = cpu_to_le16(chan->imtu);
>> + pdu->mps = cpu_to_le16(chan->mps);
>> + pdu->scid[0] = cpu_to_le16(chan->scid);
>>
>> chan->ident = l2cap_get_ident(conn);
>>
>> --
>> 2.34.1
>>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-03-26 21:57 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-26 20:02 [PATCH][next] Bluetooth: L2CAP: Avoid -Wflex-array-member-not-at-end warnings Gustavo A. R. Silva
2024-03-26 21:12 ` Luiz Augusto von Dentz
2024-03-26 21:57 ` Gustavo A. R. Silva
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).