Re: [PATCH v2 7/8] s390/vfio-ap: Fix required lock not held during display of sysfs status attribute
From: Anthony Krowiak
Date: Tue Aug 11 2026 - 16:20:04 EST
On 8/11/26 2:55 PM, Matthew Rosato wrote:
On 8/11/26 2:51 PM, Anthony Krowiak wrote:
It only found 1 against this patch. Quoting:
On 8/11/26 2:20 PM, Matthew Rosato wrote:
On 8/11/26 2:03 PM, Matthew Rosato wrote:Can you be more specific as to which Sashiko finding you are talking about?
On 8/10/26 3:22 PM, Anthony Krowiak wrote:Also same idea here, I don't believe the pre-existing finding from
The status_show function that supports display of the statusPlease see my comment on patch 3.
attribute of
the devices in /sys/bus/ap/devices calls the vfio_ap_mdev_for_queue
function which iterates the matrix_dev->mdev_list to find the object
representing the queue device whose status is to be displayed. In
order to
traverse this list, the matrix_dev->guests_lock mutex must be held
which is
not the case.
To fix this, the guests_lock mutex is taken prior to taking the
matrix_dev->mdevs_lock mutex in the status_show function. It is taken
there rather than the vfio_ap_mdev_for_queue function - where it is
needed - because it must be taken prior to the mdevs_lock mutex in
order to
adhere to the proper locking order and prevent a lockdep splat; also
because the mdevs_lock is needed there to access fields within
the matrix_mdev object in that function.
Fixes: f139862b92cf ("s390/vfio-ap: add status attribute to AP queue
device's sysfs dir")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Anthony Krowiak <akrowiak@xxxxxxxxxxxxx>
Sashiko against this patch is resolved by this series, so have a look
and consider a follow-on patch if it's a valid report.
This is a pre-existing issue, but can this sequence lead to a NULL pointer
dereference if status_show() is called during device probing?
If a userspace process reads the status sysfs attribute before
vfio_ap_mdev_probe_queue() has finished setting the device driver data:
vfio_ap_mdev_probe_queue() {
...
ret = sysfs_create_group(&apdev->device.kobj, &vfio_queue_attr_group);
if (ret)
return ret;
q = kzalloc(...);
...
dev_set_drvdata(&apdev->device, q);
}
Userspace could trigger status_show() while the driver data is still NULL.
The dev_get_drvdata() call above would return NULL for q, which is then
passed to vfio_ap_mdev_for_queue() without any checks.
Inside vfio_ap_mdev_for_queue(), the uninitialized q is unconditionally
dereferenced:
vfio_ap_mdev_for_queue() {
struct ap_matrix_mdev *matrix_mdev;
unsigned long apid = AP_QID_CARD(q->apqn);
...
}
Does the sysfs group creation need to be delayed until after the driver
data is fully initialized and set, or should status_show() check if q is NULL?
I have a fix for this, shall I include it in this series or send it as an individual
patch?