Re: [PATCH] cxl/mbox: Serialize RAW Get/Set Feature commands

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing lock acquisition in kernel-managed Feature paths defeats the intended synchronization.
- [Low] The patch references an undeclared struct member `feat_mutex`, resulting in an unconditional build failure.
--

--- Patch [1]: [PATCH] cxl/mbox: Serialize RAW Get/Set Feature commands ---
commit ffea860f8f4c719a69a797f95e4162563fecddae
Author: Richard Cheng <[email protected]>

cxl/mbox: Serialize RAW Get/Set Feature commands

This commit modifies handle_mailbox_cmd_from_user() to acquire feat_mutex
around RAW Get and Set Feature mailbox commands. It aims to serialize these
userspace commands against kernel-managed Feature operations.

Link: https://lore.kernel.org/linux-cxl/[email protected]/

> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 79543103ac298..b94b8514f6786 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -594,6 +594,8 @@ static int handle_mailbox_cmd_from_user(struct cxl_mailbox *cxl_mbox,
>  					u32 *retval)
>  {
>  	struct device *dev = cxl_mbox->host;
> +	bool is_feature_cmd = mbox_cmd->opcode == CXL_MBOX_OP_GET_FEATURE ||
> +			      mbox_cmd->opcode == CXL_MBOX_OP_SET_FEATURE;
>  	int rc;
>  
>  	dev_dbg(dev,
> @@ -603,7 +605,11 @@ static int handle_mailbox_cmd_from_user(struct cxl_mailbox *cxl_mbox,
>  		cxl_mem_opcode_to_name(mbox_cmd->opcode),
>  		mbox_cmd->opcode, mbox_cmd->size_in);
>  
> +	if (is_feature_cmd)
> +		mutex_lock(&cxl_mbox->feat_mutex);

[Severity: Low]
Does this code compile? Looking at struct cxl_mailbox in
include/cxl/mailbox.h, it doesn't appear to have a feat_mutex member
declared:

include/cxl/mailbox.h:
struct cxl_mailbox {
	struct device *host;
	DECLARE_BITMAP(enabled_cmds, CXL_MEM_COMMAND_ID_MAX);
	DECLARE_BITMAP(exclusive_cmds, CXL_MEM_COMMAND_ID_MAX);
	size_t payload_size;
	struct mutex mbox_mutex;
	struct rcuwait mbox_wait;
	int (*mbox_send)(struct cxl_mailbox *cxl_mbox, struct cxl_mbox_cmd *cmd);
	enum cxl_features_capability feat_cap;
};

[Severity: High]
The commit message indicates this lock is meant to serialize RAW commands
against kernel-managed Feature helper loops like cxl_get_feature().

However, looking at cxl_get_feature() in drivers/cxl/core/features.c, it
loops over cxl_internal_send_cmd() without acquiring feat_mutex:

drivers/cxl/core/features.c:cxl_get_feature() {
	...
	do {
		...
		rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
		...
	} while (data_rcvd_size < feat_out_size);
}

Does this missing lock acquisition in the kernel-managed paths defeat the
intended synchronization, allowing interleaved execution if userspace submits
RAW commands concurrently?

>  	rc = cxl_mbox->mbox_send(cxl_mbox, mbox_cmd);
> +	if (is_feature_cmd)
> +		mutex_unlock(&cxl_mbox->feat_mutex);
>  	if (rc)
>  		goto out;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1