Re: [PATCH v2 2/3] md-cluster: propagate update_size() errors to callers

[sashiko-bot@kernel.org]

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

New issues:
- [High] Self-deadlock in error path of update_size() when calling md_update_sb() while holding cluster locks.
- [High] The patch fails its stated goal by returning success when a cluster capacity revert occurs.
- [High] Early cluster communication error paths return an error without reverting the local array size, leaving the cluster in a split-brain state.
- [Medium] The patch propagates hardcoded `-1` returns from update_size() which translates to `-EPERM` for userspace, violating standard kernel error codes.

Pre-existing issues:
- [High] lock_comm() leaks the MD_CLUSTER_SEND_LOCK bit on lock_token() failure, causing permanent hangs for future cluster operations.
--

commit 10a3e68b8ff2647112ced9ea72fb738bc9b14bb0
Author: Cao Guanghui <caoguanghui@kylinos.cn>

md-cluster: propagate update_size() errors to callers

This patch updates the update_size() callback in the md_cluster_ops structure
to return an integer instead of void. This allows error codes from cluster
operations to be propagated up to the caller in md.c, instead of failing
silently.

> diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
> index 9f404660a1fb7..bc4118d8c5a9f 100644
> --- a/drivers/md/md-cluster.c
> +++ b/drivers/md/md-cluster.c
> @@ -1292,7 +1292,7 @@ static int cluster_check_sync_size(struct mddev *mddev)
[ ... ]
> -static void update_size(struct mddev *mddev, sector_t old_dev_sectors)
> +static int update_size(struct mddev *mddev, sector_t old_dev_sectors)
>  {
>  	struct md_cluster_info *cinfo = mddev->cluster_info;
>  	struct cluster_msg cmsg;
> @@ -1303,7 +1303,7 @@ static void update_size(struct mddev *mddev, sector_t old_dev_sectors)
>  	md_update_sb(mddev, 1);
>  	if (lock_comm(cinfo, 1)) {

[Severity: High]
This is a pre-existing issue, but does lock_comm() leak the
MD_CLUSTER_SEND_LOCK bit on failure?

Looking at lock_comm(), if test_and_set_bit(MD_CLUSTER_SEND_LOCK) succeeds,
but lock_token() fails, lock_comm() directly returns the error without
clearing the bit:

lock_comm() {
    ...
    wait_event(cinfo->wait,
               !test_and_set_bit(MD_CLUSTER_SEND_LOCK, &cinfo->state));
    rv = lock_token(cinfo);
    if (set_bit)
        clear_bit_unlock(MD_CLUSTER_HOLDING_MUTEX_FOR_RECVD, &cinfo->state);
    return rv;
}

If this occurs, will subsequent cluster operations permanently hang in
wait_event() waiting for the bit to clear?

>  		pr_err("%s: lock_comm failed\n", __func__);
> -		return;
> +		return -1;

[Severity: Medium]
Could returning -1 here cause userspace to see a confusing -EPERM error?

Since the patch changes this callback to return int, using standard error
codes like -EIO, -EAGAIN, or -ENODEV might be more appropriate than a
hardcoded -1.

[ ... ]
> @@ -1325,12 +1325,12 @@ static void update_size(struct mddev *mddev, sector_t old_dev_sectors)
>  			pr_err("%s:%d: failed to send METADATA_UPDATED msg\n",
>  			       __func__, __LINE__);
>  			unlock_comm(cinfo);
> -			return;
> +			return ret;
>  		}
>  	} else {
>  		pr_err("md-cluster: No good device id found to send\n");
>  		unlock_comm(cinfo);
> -		return;
> +		return -1;

[Severity: Medium]
Does returning -1 here also similarly risk propagating -EPERM to userspace
for an internal configuration or device availability error?

>  	}
>  
>  	/*
[ ... ]
>  	if (cluster_check_sync_size(mddev) == 0) {
>  		memset(&cmsg, 0, sizeof(cmsg));
>  		cmsg.type = cpu_to_le32(CHANGE_CAPACITY);
>  		ret = __sendmsg(cinfo, &cmsg);
>  		if (ret)
>  			pr_err("%s:%d: failed to send CHANGE_CAPACITY msg\n",
>  			       __func__, __LINE__);
>  		set_capacity_and_notify(mddev->gendisk, mddev->array_sectors);
>  	} else {
>  		/* revert to previous sectors */
>  		ret = mddev->pers->resize(mddev, old_dev_sectors);
>  		if (ret)
>  			pr_err("%s:%d: failed to revert array size\n",
>  			       __func__, __LINE__);
>  		md_update_sb(mddev, 1);

[Severity: High]
Could this call to md_update_sb() cause a self-deadlock?

The current thread holds the cluster send lock because lock_comm() was called
successfully at the start of update_size(). When md_update_sb() is called, it
triggers a cluster metadata update:

md_update_sb() -> mddev->cluster_ops->metadata_update_start()

The metadata_update_start() path appears to wait for MD_CLUSTER_SEND_LOCK to
be cleared. Since the lock is held by the same thread, will it wait forever?

>  		ret = __sendmsg(cinfo, &cmsg);

[Severity: High]
Does this __sendmsg() call overwrite the failure state from the capacity
revert?

If the cluster sync size check fails, the result of the local revert
operation is stored in 'ret' right above. By reassigning 'ret' to the result
of __sendmsg() here, would a successful broadcast incorrectly return 0
(success) to userspace instead of propagating the failure of the resize?

>  		if (ret)
>  			pr_err("%s:%d: failed to send METADATA_UPDATED msg\n",
>  			       __func__, __LINE__);
>  	}
>  	unlock_comm(cinfo);
> +	return ret;
>  }

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 680b34a63cb3d..b723fd9eccc0b 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -8023,7 +8023,7 @@ static int update_size(struct mddev *mddev, sector_t num_sectors)
>  	rv = mddev->pers->resize(mddev, num_sectors);
>  	if (!rv) {
>  		if (mddev_is_clustered(mddev))
> -			mddev->cluster_ops->update_size(mddev, old_dev_sectors);
> +			rv = mddev->cluster_ops->update_size(mddev, old_dev_sectors);

[Severity: High]
If the cluster update_size() operation fails early (for example, if lock_comm
fails or it cannot find an active raid device), we now propagate the error
back to userspace.

However, the local array has already been resized by mddev->pers->resize()
right above this call. Does this local resize need to be reverted if the
cluster update fails, to prevent a split-brain state where the local node has
a different capacity than the rest of the cluster?

>  		else if (!mddev_is_dm(mddev))
>  			set_capacity_and_notify(mddev->gendisk,
>  						mddev->array_sectors);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827061200.79753-1-ghuicao@163.com?part=2