Re: [PATCH v3 1/3] md-cluster: fix lock_comm leak and __sendmsg error path

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Missing `wake_up(&cinfo->wait)` after clearing `MD_CLUSTER_SEND_LOCK` on the error path in `lock_comm()`.
- [High] The single-attempt lock release strategy leaves the DLM lock permanently stuck when there is a pending timed-out conversion.
- [Medium] The fallback restoration of `ack_lockres` to `CR` incorrectly executes on all error paths, leading to redundant or synchronously failing requests.

Pre-existing issues:
- [High] `metadata_update_start()` suffers from the exact same lock leak pattern that the patch attempts to fix in `lock_comm()`.
--

commit ab42e35df8bfdb28c9d4472b6f7586349538824c
Author: Cao Guanghui <[email protected]>

md-cluster: fix lock_comm leak and __sendmsg error path

This commit addresses error handling issues in cluster communication. It clears
MD_CLUSTER_SEND_LOCK when lock_token() fails in lock_comm() to avoid hangs.
It also modifies __sendmsg() to attempt restoring ack_lockres to CR and
replaces an infinite loop with a single attempt when unlocking message_lockres.

> diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
> index 24dac6f09da82..fb62fdb492350 100644
> --- a/drivers/md/md-cluster.c
> +++ b/drivers/md/md-cluster.c
> @@ -735,6 +735,8 @@ static int lock_comm(struct md_cluster_info *cinfo, bool mddev_locked)
>  	wait_event(cinfo->wait,
>  		   !test_and_set_bit(MD_CLUSTER_SEND_LOCK, &cinfo->state));
>  	rv = lock_token(cinfo);
> +	if (rv)
> +		clear_bit_unlock(MD_CLUSTER_SEND_LOCK, &cinfo->state);

[Severity: High]
Does this clear operation also require a wake_up(&cinfo->wait)?

When lock_token() fails, this patch clears the lock bit but appears to miss
the wake_up() call. Any thread currently asleep in wait_event() waiting to
acquire MD_CLUSTER_SEND_LOCK could remain asleep indefinitely because
waitqueues require an explicit wakeup.

>  	if (set_bit)
>  		clear_bit_unlock(MD_CLUSTER_HOLDING_MUTEX_FOR_RECVD, &cinfo->state);
>  	return rv;

[ ... ]

> @@ -801,7 +803,15 @@ static int __sendmsg(struct md_cluster_info *cinfo, struct cluster_msg *cmsg)
>  	}
>  
>  failed_ack:
> -	while ((unlock_error = dlm_unlock_sync(cinfo->message_lockres)))
> +	if (error) {
> +		int ack_ret = dlm_lock_sync(cinfo->ack_lockres, DLM_LOCK_CR);

[Severity: Medium]
Will this unconditional fallback restoration fail synchronously?

If __sendmsg() fails on message_lockres, ack_lockres is still in CR, making
this conversion redundant. However, if it fails on ack_lockres due to a
timeout, the DLM resource is already busy with a pending request. The
immediate dlm_lock_sync() fallback to CR will fail synchronously with -EBUSY
and fail to recover the lock state.

> +
> +		if (ack_ret)
> +			pr_err("md-cluster: failed to restore ACK to CR (%d)\n",
> +			       ack_ret);
> +	}
> +	unlock_error = dlm_unlock_sync(cinfo->message_lockres);
> +	if (unlock_error)
>  		pr_err("md-cluster: failed convert to NL on MESSAGE(%d)\n",
>  			unlock_error);

[Severity: High]
Could this single-attempt lock release strategy leave the DLM lock stuck?

If dlm_lock_sync() times out (for example, when getting EX lock on MESSAGE),
it returns -EBUSY but leaves the lock request active in DLM. This single
dlm_unlock_sync() attempt translates to a new DLM lock request which is
immediately rejected with -EBUSY by DLM due to the pending request. 

Proper recovery might require using dlm_unlock() with DLM_LKF_CANCEL to
cancel the pending operation.


[Severity: High]
This isn't a bug introduced by this patch, but metadata_update_start()
appears to suffer from the exact same lock leak pattern that this patch
attempts to fix in lock_comm().

Looking at metadata_update_start() in drivers/md/md-cluster.c:

	ret = lock_token(cinfo);
	clear_bit_unlock(MD_CLUSTER_HOLDING_MUTEX_FOR_RECVD, &cinfo->state);
	return ret;

If lock_token() fails here, it returns without clearing MD_CLUSTER_SEND_LOCK
and without calling wake_up(&cinfo->wait), which could cause a permanent
cluster-wide hang during metadata updates.

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