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

On 8/12/26 8:16 AM, [email protected] wrote:
> 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.

release_update_locks_for_mdev() needs to be called before sysfs_remove_group

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