Re: [PATCH v3 8/9] s390/vfio-ap: Fix NULL deref in status_show() during queue probe

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ABBA deadlock in the probe error path between kernfs active reference and guests_lock.
--

--- Patch [8]: [PATCH v3 8/9] s390/vfio-ap: Fix NULL deref in status_show() during queue probe ---
commit dd5ddcd73e238630c53e21354f9251a158e8bee2
Author: Anthony Krowiak <[email protected]>

s390/vfio-ap: Fix NULL deref in status_show() during queue probe

This commit fixes a NULL pointer dereference in status_show() during queue
probe by acquiring update locks before creating the sysfs attribute group.
This ensures any concurrent read blocks until driver data is set and locks
are released.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index cb1e5db69d8a7..3652ba792be73 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2419,14 +2419,17 @@ void vfio_ap_mdev_unregister(void)
>  
>  int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
>  {
> -	int ret;
> +	int ret, apqn;
>  	struct vfio_ap_queue *q;
>  	DECLARE_BITMAP(apm_filtered, AP_DEVICES);
>  	struct ap_matrix_mdev *matrix_mdev;
>  
> +	apqn = to_ap_queue(&apdev->device)->qid;
> +	matrix_mdev = get_update_locks_by_apqn(apqn);
> +
>  	ret = sysfs_create_group(&apdev->device.kobj, &vfio_queue_attr_group);
>  	if (ret)
> -		return ret;
> +		goto err_release_locks;
>  
>  	q = kzalloc_obj(*q);
>  	if (!q) {
> @@ -2434,11 +2437,10 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
>  		goto err_remove_group;
>  	}
[ ... ]
> @@ -2468,6 +2470,8 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
>  
>  err_remove_group:
>  	sysfs_remove_group(&apdev->device.kobj, &vfio_queue_attr_group);

[Severity: High]
Does this create an ABBA deadlock on the error path?

If kzalloc_obj() fails, we jump to err_remove_group and call
sysfs_remove_group() while still holding matrix_dev->guests_lock (acquired
via get_update_locks_by_apqn()).

If a concurrent reader opens the 'status' sysfs file before we hit the error
path, it acquires a kernfs active reference and blocks on:

status_show()
    mutex_lock(&matrix_dev->guests_lock);

Then sysfs_remove_group() blocks in kernfs_drain() waiting for the reader to
release its active reference, resulting in a deadlock.

> +err_release_locks:
> +	release_update_locks_for_mdev(matrix_mdev);
>  	return ret;
>  }

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