| Message ID | ZgMpynzZ8FltPCi3@neat (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [next] Bluetooth: L2CAP: Avoid -Wflex-array-member-not-at-end warnings | expand |
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 >
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 >> > >
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);
-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(-)