Re: [PATCH 3/5] s390/vfio-ap: Fix use of wrong lock in mdev probe function

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Data race and list corruption due to mismatched locks on list traversal and mutation.
--

--- Patch [3]: [PATCH 3/5] s390/vfio-ap: Fix use of wrong lock in mdev probe function ---
commit 6ed976a494c7af5de9033ac808583fad9a0cb4f7
Author: Anthony Krowiak <[email protected]>

s390/vfio-ap: Fix use of wrong lock in mdev probe function

This commit changes the lock used to guard the addition of a newly created
ap_matrix_mdev object to matrix_dev->mdev_list in vfio_ap_mdev_probe()
from mdevs_lock to guests_lock.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index e382e5a1cb996..a472de00bc149 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -803,9 +803,9 @@ static int vfio_ap_mdev_probe(struct mdev_device *mdev)
>  	matrix_mdev->req_trigger = NULL;
>  	matrix_mdev->cfg_chg_trigger = NULL;
>  	dev_set_drvdata(&mdev->dev, matrix_mdev);
> -	mutex_lock(&matrix_dev->mdevs_lock);
> +	mutex_lock(&matrix_dev->guests_lock);
>  	list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
> -	mutex_unlock(&matrix_dev->mdevs_lock);

[Severity: High]
Does changing this lock to guests_lock create a data race when traversing
the list in other functions?

For example, when a userspace process reads the status attribute,
status_show() only acquires mdevs_lock before calling
vfio_ap_mdev_for_queue():

drivers/s390/crypto/vfio_ap_ops.c:status_show() {
    ...
	mutex_lock(&matrix_dev->mdevs_lock);
	q = dev_get_drvdata(&apdev->device);
	matrix_mdev = vfio_ap_mdev_for_queue(q);
    ...
}

Which then iterates over the list relying on the assumption that writers
will hold mdevs_lock:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_for_queue() {
    ...
	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
    ...
}

If vfio_ap_mdev_probe() is called concurrently with status_show(), could
this cause list corruption or a use-after-free panic since the reader and
writer are no longer synchronized with the same lock?

> +	mutex_unlock(&matrix_dev->guests_lock);
>  	return 0;
>  
>  err_put_vdev:

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