Message-ID: <5a438394-ae88-4d72-a56e-6886d06b0a84@embeddedor.com>
Date: Fri, 13 Sep 2024 10:10:40 +0200
From: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To: "Gustavo A. R. Silva" <gustavoars@...nel.org>,
 Bjorn Andersson <andersson@...nel.org>,
 Mathieu Poirier <mathieu.poirier@...aro.org>
Cc: linux-arm-msm@...r.kernel.org, linux-remoteproc@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH][next] rpmsg: glink: Avoid -Wflex-array-member-not-at-end
 warnings

Hi all,

Friendly ping: who can take this, please? 🙂

Thanks
-Gustavo

On 07/08/24 17:19, Gustavo A. R. Silva wrote:
> -Wflex-array-member-not-at-end was introduced in GCC-14, and we are
> getting ready to enable it, globally.
> 
> So, in order to avoid ending up with a flexible-array member in the
> middle of multiple other structs, we use the `__struct_group()`
> helper to create a new tagged `struct glink_msg_hdr`. This structure
> groups together all the members of the flexible `struct glink_msg`
> except the flexible array.
> 
> As a result, the array is effectively separated from the rest of the
> members without modifying the memory layout of the flexible structure.
> We then change the type of the middle struct members currently causing
> trouble from `struct glink_msg` to `struct glink_msg_hdr`.
> 
> We also want to ensure that when new members need to be added to the
> flexible structure, they are always included within the newly created
> tagged struct. For this, we use `static_assert()`. This ensures that the
> memory layout for both the flexible structure and the new tagged struct
> is the same after any changes.
> 
> This approach avoids having to implement `struct glink_msg_hdr` as a
> completely separate structure, thus preventing having to maintain two
> independent but basically identical structures, closing the door to
> potential bugs in the future.
> 
> We also use `container_of()` whenever we need to retrieve a pointer to
> the flexible structure, through which we can access the flexible-array
> member, if necessary.
> 
> Additionally, we use the `DEFINE_RAW_FLEX()` helper for an on-stack
> definition of a flexible structure where the size for the flexible-array
> member is known at compile-time.
> 
> So, with these changes, fix the following warnings:
> drivers/rpmsg/qcom_glink_native.c:51:26: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> drivers/rpmsg/qcom_glink_native.c:459:34: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> drivers/rpmsg/qcom_glink_native.c:846:34: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> drivers/rpmsg/qcom_glink_native.c:968:34: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> drivers/rpmsg/qcom_glink_native.c:1380:34: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> 
> Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
> ---
>   drivers/rpmsg/qcom_glink_native.c | 42 +++++++++++++++++--------------
>   1 file changed, 23 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 82d460ff4777..ed89b810f262 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -30,11 +30,16 @@
>   #define RPM_GLINK_CID_MAX	65536
>   
>   struct glink_msg {
> -	__le16 cmd;
> -	__le16 param1;
> -	__le32 param2;
> +	/* New members MUST be added within the __struct_group() macro below. */
> +	__struct_group(glink_msg_hdr, hdr, __packed,
> +		__le16 cmd;
> +		__le16 param1;
> +		__le32 param2;
> +	);
>   	u8 data[];
>   } __packed;
> +static_assert(offsetof(struct glink_msg, data) == sizeof(struct glink_msg_hdr),
> +	      "struct member likely outside of __struct_group()");
>   
>   /**
>    * struct glink_defer_cmd - deferred incoming control message
> @@ -48,7 +53,7 @@ struct glink_msg {
>   struct glink_defer_cmd {
>   	struct list_head node;
>   
> -	struct glink_msg msg;
> +	struct glink_msg_hdr msg;
>   	u8 data[];
>   };
>   
> @@ -455,12 +460,9 @@ static void qcom_glink_intent_req_abort(struct glink_channel *channel)
>   static int qcom_glink_send_open_req(struct qcom_glink *glink,
>   				    struct glink_channel *channel)
>   {
> -	struct {
> -		struct glink_msg msg;
> -		u8 name[GLINK_NAME_SIZE];
> -	} __packed req;
> +	DEFINE_RAW_FLEX(struct glink_msg, req, data, GLINK_NAME_SIZE);
>   	int name_len = strlen(channel->name) + 1;
> -	int req_len = ALIGN(sizeof(req.msg) + name_len, 8);
> +	int req_len = ALIGN(sizeof(*req) + name_len, 8);
>   	int ret;
>   	unsigned long flags;
>   
> @@ -476,12 +478,12 @@ static int qcom_glink_send_open_req(struct qcom_glink *glink,
>   
>   	channel->lcid = ret;
>   
> -	req.msg.cmd = cpu_to_le16(GLINK_CMD_OPEN);
> -	req.msg.param1 = cpu_to_le16(channel->lcid);
> -	req.msg.param2 = cpu_to_le32(name_len);
> -	strcpy(req.name, channel->name);
> +	req->cmd = cpu_to_le16(GLINK_CMD_OPEN);
> +	req->param1 = cpu_to_le16(channel->lcid);
> +	req->param2 = cpu_to_le32(name_len);
> +	strcpy(req->data, channel->name);
>   
> -	ret = qcom_glink_tx(glink, &req, req_len, NULL, 0, true);
> +	ret = qcom_glink_tx(glink, req, req_len, NULL, 0, true);
>   	if (ret)
>   		goto remove_idr;
>   
> @@ -826,7 +828,9 @@ static int qcom_glink_rx_defer(struct qcom_glink *glink, size_t extra)
>   
>   	INIT_LIST_HEAD(&dcmd->node);
>   
> -	qcom_glink_rx_peek(glink, &dcmd->msg, 0, sizeof(dcmd->msg) + extra);
> +	qcom_glink_rx_peek(glink,
> +			   container_of(&dcmd->msg, struct glink_msg, hdr), 0,
> +			   sizeof(dcmd->msg) + extra);
>   
>   	spin_lock(&glink->rx_lock);
>   	list_add_tail(&dcmd->node, &glink->rx_queue);
> @@ -843,7 +847,7 @@ static int qcom_glink_rx_data(struct qcom_glink *glink, size_t avail)
>   	struct glink_core_rx_intent *intent;
>   	struct glink_channel *channel;
>   	struct {
> -		struct glink_msg msg;
> +		struct glink_msg_hdr msg;
>   		__le32 chunk_size;
>   		__le32 left_size;
>   	} __packed hdr;
> @@ -965,7 +969,7 @@ static void qcom_glink_handle_intent(struct qcom_glink *glink,
>   	};
>   
>   	struct {
> -		struct glink_msg msg;
> +		struct glink_msg_hdr msg;
>   		struct intent_pair intents[];
>   	} __packed * msg;
>   
> @@ -1377,7 +1381,7 @@ static int __qcom_glink_send(struct glink_channel *channel,
>   	struct glink_core_rx_intent *tmp;
>   	int iid = 0;
>   	struct {
> -		struct glink_msg msg;
> +		struct glink_msg_hdr msg;
>   		__le32 chunk_size;
>   		__le32 left_size;
>   	} __packed req;
> @@ -1685,7 +1689,7 @@ static void qcom_glink_work(struct work_struct *work)
>   		list_del(&dcmd->node);
>   		spin_unlock_irqrestore(&glink->rx_lock, flags);
>   
> -		msg = &dcmd->msg;
> +		msg = container_of(&dcmd->msg, struct glink_msg, hdr);
>   		cmd = le16_to_cpu(msg->cmd);
>   		param1 = le16_to_cpu(msg->param1);
>   		param2 = le32_to_cpu(msg->param2);

Powered by blists - more mailing lists