[PATCH v3 0/9] s390/vfio-ap: Fix bugs in vfio_ap device driver callback functions

[PATCH v3 0/9] s390/vfio-ap: Fix bugs in vfio_ap device driver callback functions

[Anthony Krowiak]

During review of patches by the Sashiko AI, several pre-existing bugs were
discovered. This 9-patch series fixes those bugs

Change log v2 => v3:
~~~~~~~~~~~~~~~~~~~
Patch 3: Fix use of wrong lock in mdev probe function
Patch 7: Fix required lock not held during display of sysfs status
attribute 
* Squashed these two patches into a new patch entitled "Fix missing lock
  required to access list of ap_matrix_mdev objects"
* Added two new patches that fix pre-existing bugs:
  ~ s390/vfio-ap: Fix NULL deref in status_show() during queue probe
  ~ s390/vfio-ap: Fix memory leak when queue removed from host AP config

Anthony Krowiak (9):
  s390/vfio-ap: Fix stale do_remove flag across iterations in
    vfio_ap_mdev_cfg_remove
  s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for
    NULL
  s390/vfio-ap: Fix missing lock required to access list of
    ap_matrix_mdev objects
  s390/vfio-ap: Fix required lock not held during update of
    ap_matrix_mdev object
  s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove
  s390/vfio-ap: fix potential use of uninitialized apm_filtered bitmap
  s390/vfio-ap: Fix hot-unplug skipped when last AP adapter or domain
    removed
  s390/vfio-ap: Fix NULL deref in status_show() during queue probe
  s390/vfio-ap: Fix memory leak when queue removed from host AP config

 drivers/s390/crypto/vfio_ap_ops.c | 110 ++++++++++++++++++++----------
 1 file changed, 75 insertions(+), 35 deletions(-)

-- 
2.53.0

[PATCH v3 1/9] s390/vfio-ap: Fix stale do_remove flag across iterations in vfio_ap_mdev_cfg_remove

[Anthony Krowiak]

The do_remove flag in vfio_ap_mdev_cfg_remove() is initialised to zero
before the loop that iterates over the list of matrix mdevs, but is
never reset at the start of each iteration. Since do_remove is
OR-accumulated across iterations, a positive result from one mdev
carries over to subsequent mdevs.

The fix is to set the do_remove flag with the first call to bitmap_and;
for example: do_remove = bitmap_an rather than do_remove |= bitmap_and.

Fixes: eeb386aeb5b7 ("s390/vfio-ap: handle config changed and scan complete notification")
Cc: stable@vger.kernel.org
Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 44b3a1dcc1b3..845c86ba8bc3 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2603,15 +2603,15 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
 	DECLARE_BITMAP(aprem, AP_DEVICES);
 	DECLARE_BITMAP(aqrem, AP_DOMAINS);
 	DECLARE_BITMAP(cdrem, AP_DOMAINS);
-	int do_remove = 0;
+	int do_remove;
 
 	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
 		mutex_lock(&matrix_mdev->kvm->lock);
 		mutex_lock(&matrix_dev->mdevs_lock);
 
-		do_remove |= bitmap_and(aprem, ap_remove,
-					  matrix_mdev->matrix.apm,
-					  AP_DEVICES);
+		do_remove = bitmap_and(aprem, ap_remove,
+				       matrix_mdev->matrix.apm,
+				       AP_DEVICES);
 		do_remove |= bitmap_and(aqrem, aq_remove,
 					  matrix_mdev->matrix.aqm,
 					  AP_DOMAINS);
-- 
2.53.0

[PATCH v3 2/9] s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for NULL

[Anthony Krowiak]

The ap_driver structure has two fields which are function pointers to
callbacks:

* .on_config_changed: called at the start of the AP bus scan function to
                      notify the device driver that the host AP
                      configuration has changed and the associated AP
                      devices will be added or removed accordingly. This
                      gives the implementor a chance to evaluate the
                      configuration changes and respond to them before
                      the associated devices are added or removed.

* .on_scan_complete:  Called at the end of the AP bus scan function to
                      notify the device driver that the host AP
                      configuration has changed and the AP devices have
                      been added or removed accordingly. This gives the
                      implementor the opportunity to respond to the
                      changes after the associated devices are added or
                      removed.

These two callbacks are implemented in the vfio_ap device driver via the
vfio_ap_on_cfg_changed and vfio_ap_on_scan_complete functions respectively.

Within the call stack of these two callback functions the
matrix_mdev->kvm->lock mutex is taken without checking whether
matrix_mdev->kvm is NULL or not. If matrix_mdev->kvm has never been set,
trying to take the lock will trigger a NULL pointer dereference. This patch
adds checks for matrix_mdev->kvm == NULL before taking the
matrix_mdev->kvm->lock mutex.

Note that the matrix_mdev->kvm->lock mutex taken in the
vfio_ap_mdev_hot_plug_config function is moved to the calling function
along with the matrix_dev->mdevs_lock which is needed there to access
the fields of the matrix_mdev. It makes little sense to make the change
the check for matrix_mdev->kvm there before taking the kvm->lock
mutex only to have to move it out via another patch, so it is done in
this patch.

It is important to make note of the following:
1. The matrix_dev->guests_lock is acquired at the start of both callback
   functions. This ensures that matrix_mdev will not be removed via the
   vfio_ap_mdev_remove function because it too takes matrix_dev_guests_lock
   before removing the object; so, matrix_mdev will be available for the
   duration of the callback functions.

2. The matrix_dev->mdevs_lock mutex must be taken in order to access
   fields within the matrix_mdev structure

3. matrix_mdev->kvm->lock mutex must be taken before the
   matrix_dev->mdevs_lock to prevent a lockdep splat.

4: The kvm->lock must be held while plugging the guest's AP configuration
   into its SIE state description via the vfio_ap_mdev_update_guest_apcb
   function.

5. The vfio_ap_mdev_update_guest_apcb checks matrix_mdev->kvm to verify it
   is not NULL before doing the hot plug of the guest's AP configuration.

Fixes: eeb386aeb5b7c ("s390/vfio-ap: handle config changed and scan complete notification")
Cc: stable@vger.kernel.org
Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 33 ++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 845c86ba8bc3..e382e5a1cb99 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2606,7 +2606,13 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
 	int do_remove;
 
 	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
-		mutex_lock(&matrix_mdev->kvm->lock);
+		/*
+		 * The mdevs_lock must be held to access fields within matrix_mdev,
+		 * and kvm->lock must be taken before mdevs_lock to satisfy the lock
+		 * ordering requirement and prevent a lockdep splat.
+		 */
+		if (matrix_mdev->kvm)
+			mutex_lock(&matrix_mdev->kvm->lock);
 		mutex_lock(&matrix_dev->mdevs_lock);
 
 		do_remove = bitmap_and(aprem, ap_remove,
@@ -2624,7 +2630,8 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
 						    cdrem);
 
 		mutex_unlock(&matrix_dev->mdevs_lock);
-		mutex_unlock(&matrix_mdev->kvm->lock);
+		if (matrix_mdev->kvm)
+			mutex_unlock(&matrix_mdev->kvm->lock);
 	}
 }
 
@@ -2821,9 +2828,6 @@ static void vfio_ap_mdev_hot_plug_cfg(struct ap_matrix_mdev *matrix_mdev)
 	DECLARE_BITMAP(apm_filtered, AP_DEVICES);
 	bool filter_domains, filter_adapters, filter_cdoms, do_hotplug = false;
 
-	mutex_lock(&matrix_mdev->kvm->lock);
-	mutex_lock(&matrix_dev->mdevs_lock);
-
 	filter_adapters = bitmap_intersects(matrix_mdev->matrix.apm,
 					    matrix_mdev->apm_add, AP_DEVICES);
 	filter_domains = bitmap_intersects(matrix_mdev->matrix.aqm,
@@ -2841,9 +2845,6 @@ static void vfio_ap_mdev_hot_plug_cfg(struct ap_matrix_mdev *matrix_mdev)
 		vfio_ap_mdev_update_guest_apcb(matrix_mdev);
 
 	reset_queues_for_apids(matrix_mdev, apm_filtered);
-
-	mutex_unlock(&matrix_dev->mdevs_lock);
-	mutex_unlock(&matrix_mdev->kvm->lock);
 }
 
 void vfio_ap_on_scan_complete(struct ap_config_info *new_config_info,
@@ -2854,15 +2855,29 @@ void vfio_ap_on_scan_complete(struct ap_config_info *new_config_info,
 	mutex_lock(&matrix_dev->guests_lock);
 
 	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
+		/*
+		 * The mdevs_lock must be held to access fields within matrix_mdev,
+		 * and kvm->lock must be taken before mdevs_lock to satisfy the lock
+		 * ordering requirement and prevent a lockdep splat.
+		 */
+		if (matrix_mdev->kvm)
+			mutex_lock(&matrix_mdev->kvm->lock);
+		mutex_lock(&matrix_dev->mdevs_lock);
+
 		if (bitmap_empty(matrix_mdev->apm_add, AP_DEVICES) &&
 		    bitmap_empty(matrix_mdev->aqm_add, AP_DOMAINS) &&
 		    bitmap_empty(matrix_mdev->adm_add, AP_DOMAINS))
-			continue;
+			goto do_unlock;
 
 		vfio_ap_mdev_hot_plug_cfg(matrix_mdev);
 		bitmap_clear(matrix_mdev->apm_add, 0, AP_DEVICES);
 		bitmap_clear(matrix_mdev->aqm_add, 0, AP_DOMAINS);
 		bitmap_clear(matrix_mdev->adm_add, 0, AP_DOMAINS);
+
+do_unlock:
+		mutex_unlock(&matrix_dev->mdevs_lock);
+		if (matrix_mdev->kvm)
+			mutex_unlock(&matrix_mdev->kvm->lock);
 	}
 
 	mutex_unlock(&matrix_dev->guests_lock);
-- 
2.53.0

[PATCH v3 3/9] s390/vfio-ap: Fix missing lock required to access list of ap_matrix_mdev objects

[Anthony Krowiak]

In order to traverse or add/remove ap_matrix_mdev objects in the
matrix_dev->mdev_list, the matrix_dev->guests_lock mutex must be held.
There are two functions that access the list without holding the mutex:

vfio_ap_mdev_probe function
~~~~~~~~~~~~~~~~~~~~~~~~~~~
The vfio_ap_mdev_probe function uses the matrix_dev->mdevs_lock
mutex to guard the add of a newly created ap_matrix_mdev object to the
matrix_dev->mdev_list. This mutex does not protect list access; its purpose
is to guard against concurrent access to fields contained in an
ap_matrix_mdev object. This could lead to kernel memory corruption or
use-after-free if another mdev is created or removed concurrently.

The adding of an ap_matrix_mdev object to matrix_dev->mdev_list
is now guarded by the matrix_dev->guests_lock which is the correct
way to protect against concurrent mdev_list access.

vfio_ap_mdev_for_queue function
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The status_show function that supports display of the status 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.

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.

See the vfio-ap-locking.rst in the linux kernel tree.

Fixes: 2c1ee8983aa3 ("s390/vfio-ap: prepare for dynamic update of guest's APCB on queue probe/remove")
Cc: stable@vger.kernel.org
Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index e382e5a1cb99..5c14ded4fd8e 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);
+	mutex_unlock(&matrix_dev->guests_lock);
 	return 0;
 
 err_put_vdev:
@@ -2297,6 +2297,8 @@ static struct ap_matrix_mdev *vfio_ap_mdev_for_queue(struct vfio_ap_queue *q)
 	unsigned long apid = AP_QID_CARD(q->apqn);
 	unsigned long apqi = AP_QID_QUEUE(q->apqn);
 
+	lockdep_assert_held(&matrix_dev->guests_lock);
+
 	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
 		if (test_bit_inv(apid, matrix_mdev->matrix.apm) &&
 		    test_bit_inv(apqi, matrix_mdev->matrix.aqm))
@@ -2316,6 +2318,7 @@ static ssize_t status_show(struct device *dev,
 	struct ap_matrix_mdev *matrix_mdev;
 	struct ap_device *apdev = to_ap_dev(dev);
 
+	mutex_lock(&matrix_dev->guests_lock);
 	mutex_lock(&matrix_dev->mdevs_lock);
 	q = dev_get_drvdata(&apdev->device);
 	matrix_mdev = vfio_ap_mdev_for_queue(q);
@@ -2343,6 +2346,7 @@ static ssize_t status_show(struct device *dev,
 	}
 
 	mutex_unlock(&matrix_dev->mdevs_lock);
+	mutex_unlock(&matrix_dev->guests_lock);
 
 	return nchars;
 }
-- 
2.53.0

[PATCH v3 4/9] s390/vfio-ap: Fix required lock not held during update of ap_matrix_mdev object

[Anthony Krowiak]

In the vfio_ap_mdev_cfg_add function, the apm_add, aqm_add and adm_add
fields of an ap_matrix_mdev object fields are modified while not holding
the matrix_dev->mdevs_lock. This lock must be held while making these
to guard against a race condition with another caller that may be
concurrently modifying these fields or any of the fields in the
matrix_mdev->matrix.

Fixes: eeb386aeb5b7c ("s390/vfio-ap: handle config changed and scan complete notification")
Cc: stable@vger.kernel.org
Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 5c14ded4fd8e..8a1ecb891504 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2760,12 +2760,20 @@ static void vfio_ap_mdev_cfg_add(unsigned long *apm_add, unsigned long *aqm_add,
 	vfio_ap_filter_apid_by_qtype(apm_add, aqm_add);
 
 	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
+		/*
+		 * The mdevs_lock must be held in order to access fields
+		 * within matrix_mdev
+		 */
+		mutex_lock(&matrix_dev->mdevs_lock);
+
 		bitmap_and(matrix_mdev->apm_add,
 			   matrix_mdev->matrix.apm, apm_add, AP_DEVICES);
 		bitmap_and(matrix_mdev->aqm_add,
 			   matrix_mdev->matrix.aqm, aqm_add, AP_DOMAINS);
 		bitmap_and(matrix_mdev->adm_add,
 			   matrix_mdev->matrix.adm, adm_add, AP_DEVICES);
+
+		mutex_unlock(&matrix_dev->mdevs_lock);
 	}
 }
 
-- 
2.53.0

[PATCH v3 5/9] s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove

[Anthony Krowiak]

The vfio_ap_config_remove function uses the bitmap_andnot function to clear
bits from the matrix_mdev->matrix.adm bitmap (specifies the control domains
assigned to the mdev). This prevents the explicitly unplugged control
domains from being removed the KVM guest. The bitmap_and function is used
instead.

Fixes: eeb386aeb5b7c ("s390/vfio-ap: handle config changed and scan complete notification")
Cc: stable@vger.kernel.org
Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 8a1ecb891504..b6d0c988cc81 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2625,9 +2625,9 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
 		do_remove |= bitmap_and(aqrem, aq_remove,
 					  matrix_mdev->matrix.aqm,
 					  AP_DOMAINS);
-		do_remove |= bitmap_andnot(cdrem, cd_remove,
-					     matrix_mdev->matrix.adm,
-					     AP_DOMAINS);
+		do_remove |= bitmap_and(cdrem, cd_remove,
+					matrix_mdev->matrix.adm,
+					AP_DOMAINS);
 
 		if (do_remove)
 			vfio_ap_mdev_hot_unplug_cfg(matrix_mdev, aprem, aqrem,
-- 
2.53.0

[PATCH v3 6/9] s390/vfio-ap: fix potential use of uninitialized apm_filtered bitmap

[Anthony Krowiak]

The DECLARE_BITMAP(apm_filtered, AP_DEVICES) macro allocates the bitmap
on the stack without zero-initializing it.

In vfio_ap_mdev_hot_plug_cfg(), the vfio_ap_mdev_filter_matrix() function
is only called to initialize and populate apm_filtered if either
filter_adapters or filter_domains is true. If the hot plug configuration
change only adds control domains (meaning filter_cdoms is true, but
filter_adapters and filter_domains are both false),
vfio_ap_mdev_filter_matrix() is bypassed.

Consequently, apm_filtered is passed to reset_queues_for_apids() with
uninitialized stack garbage. This can cause reset_queues_for_apids() to
interpret arbitrary stack garbage bits as valid APIDs to reset, potentially
performing unintended guest hardware queue resets.

Fix this by zero-initializing the apm_filtered bitmap at the beginning of
vfio_ap_mdev_hot_plug_cfg() using bitmap_zero().

Fixes: eeb386aeb5b7c ("s390/vfio-ap: handle config changed and scan complete notification")
Cc: stable@vger.kernel.org
Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index b6d0c988cc81..cdc9bf5c5e53 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2840,6 +2840,8 @@ static void vfio_ap_mdev_hot_plug_cfg(struct ap_matrix_mdev *matrix_mdev)
 	DECLARE_BITMAP(apm_filtered, AP_DEVICES);
 	bool filter_domains, filter_adapters, filter_cdoms, do_hotplug = false;
 
+	bitmap_zero(apm_filtered, AP_DEVICES);
+
 	filter_adapters = bitmap_intersects(matrix_mdev->matrix.apm,
 					    matrix_mdev->apm_add, AP_DEVICES);
 	filter_domains = bitmap_intersects(matrix_mdev->matrix.aqm,
-- 
2.53.0

[PATCH v3 7/9] s390/vfio-ap: Fix hot-unplug skipped when last AP adapter or domain removed

[Anthony Krowiak]

The vfio_ap_mdev_hot_unplug_cfg() function uses the return value of
bitmap_andnot() to determine whether the guest APCB needs to be updated.
However, bitmap_andnot() returns false when the resulting destination
bitmap is empty. This means that if the only adapter, domain or control
domain assigned to an mdev is removed from the host's AP configuration,
the bit is correctly cleared from the shadow APCB, but bitmap_andnot()
returns false because the result is an empty bitmap. Consequently,
do_hotplug remains 0 and vfio_ap_mdev_update_guest_apcb() is never called,
leaving the KVM guest with stale hardware access to the unplugged AP
devices.

Fix this by replacing the bitmap_andnot() return value check with
bitmap_intersects() to determine whether the shadow APCB actually
overlaps with the removal mask. If there is an intersection, call
bitmap_andnot() solely for its side effect of clearing the bits, then
unconditionally set do_hotplug to trigger the guest APCB update.

Fixes: eeb386aeb5b7c ("s390/vfio-ap: handle config changed and scan complete notification")
Cc: stable@vger.kernel.org
Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index cdc9bf5c5e53..cb1e5db69d8a 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2563,24 +2563,28 @@ static void vfio_ap_mdev_hot_unplug_cfg(struct ap_matrix_mdev *matrix_mdev,
 					unsigned long *aqrem,
 					unsigned long *cdrem)
 {
-	int do_hotplug = 0;
+	bool do_hotplug = false;
 
-	if (!bitmap_empty(aprem, AP_DEVICES)) {
-		do_hotplug |= bitmap_andnot(matrix_mdev->shadow_apcb.apm,
-					    matrix_mdev->shadow_apcb.apm,
-					    aprem, AP_DEVICES);
+	if (bitmap_intersects(matrix_mdev->shadow_apcb.apm, aprem, AP_DEVICES)) {
+		bitmap_andnot(matrix_mdev->shadow_apcb.apm,
+			      matrix_mdev->shadow_apcb.apm,
+			      aprem, AP_DEVICES);
+		do_hotplug = true;
 	}
 
-	if (!bitmap_empty(aqrem, AP_DOMAINS)) {
-		do_hotplug |= bitmap_andnot(matrix_mdev->shadow_apcb.aqm,
-					    matrix_mdev->shadow_apcb.aqm,
-					    aqrem, AP_DEVICES);
+	if (bitmap_intersects(matrix_mdev->shadow_apcb.aqm, aqrem, AP_DOMAINS)) {
+		bitmap_andnot(matrix_mdev->shadow_apcb.aqm,
+			      matrix_mdev->shadow_apcb.aqm,
+			      aqrem, AP_DOMAINS);
+		do_hotplug = true;
 	}
 
-	if (!bitmap_empty(cdrem, AP_DOMAINS))
-		do_hotplug |= bitmap_andnot(matrix_mdev->shadow_apcb.adm,
-					    matrix_mdev->shadow_apcb.adm,
-					    cdrem, AP_DOMAINS);
+	if (bitmap_intersects(matrix_mdev->shadow_apcb.adm, cdrem, AP_DOMAINS)) {
+		bitmap_andnot(matrix_mdev->shadow_apcb.adm,
+			      matrix_mdev->shadow_apcb.adm,
+			      cdrem, AP_DOMAINS);
+		do_hotplug = true;
+	}
 
 	if (do_hotplug)
 		vfio_ap_mdev_update_guest_apcb(matrix_mdev);
-- 
2.53.0

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

[Anthony Krowiak]

When vfio_ap_mdev_probe_queue() creates the sysfs attribute group,
the queue's driver data has not yet been set. A concurrent read of
the 'status' attribute can therefore call dev_get_drvdata() and
get NULL, which is then passed directly to
vfio_ap_mdev_for_queue() where q->apqn is unconditionally
dereferenced, causing a NULL pointer dereference.

Fix this by acquiring the update locks before calling
sysfs_create_group(). The status_show() function acquires
guests_lock before reading the driver data, so any concurrent
read will block until after dev_set_drvdata() has been called
and the update locks are released.

As a bonus, the APQN no longer needs to be read from the queue
struct after allocation — it can be read directly from apdev
before allocation and stored in a local variable, which is then
assigned to q->apqn once the allocation succeeds.

Fixes: 260f3ea141382 ("s390/vfio-ap: move probe and remove callbacks to vfio_ap_ops.c")
Cc: stable@vger.kernel.org
Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index cb1e5db69d8a..3652ba792be7 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;
 	}
 
-	q->apqn = to_ap_queue(&apdev->device)->qid;
+	q->apqn = apqn;
 	q->saved_isc = VFIO_AP_ISC_INVALID;
 	memset(&q->reset_status, 0, sizeof(q->reset_status));
 	INIT_WORK(&q->reset_work, apq_reset_check);
-	matrix_mdev = get_update_locks_by_apqn(q->apqn);
 
 	if (matrix_mdev) {
 		vfio_ap_mdev_link_queue(matrix_mdev, q);
@@ -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);
+err_release_locks:
+	release_update_locks_for_mdev(matrix_mdev);
 	return ret;
 }
 
-- 
2.53.0

[PATCH v3 9/9] s390/vfio-ap: Fix memory leak when queue removed from host AP config

[Anthony Krowiak]

When an adapter or domain is removed from the host's AP configuration, the
AP bus invokes vfio_ap_on_cfg_changed() to notify the vfio_ap device
driver. For each ap_matrix_mdev object to which the adapter or domain
is assigned, vfio_ap_mdev_hot_unplug_cfg() is called and removes the
adapter or domain from the matrix_mdev->shadow_apcb (i.e., the guest's AP
configuration) and hot unplugs it if a guest is using it. The new host
AP configuration (sans adapter or domain) is then stored in
matrix_dev->info.

When the AP bus subsequently unbinds the physical queue devices associated
with the adapter or domain that has been removed, it invokes
vfio_ap_mdev_remove_queue() for each queue removed. At this point, the
adapter or domain will no longer be assigned to the
matrix_mdev->shadow_apcb or the matrix_dev->info object because they would
have been removed by vfio_ap_on_cfg_changed(). Consequently,
vfio_ap_mdev_reset_queue(q) is bypassed and kfree(q) is called
without executing vfio_ap_free_aqic_resources(). This indefinitely pins
guest memory (q->saved_iova) and leaks KVM GISC resources (q->saved_isc).

Note that resetting the queue would fail with an invalid APQN error due to
the fact the queue is not longer in the host's AP configuration; however,
it is still necessary to free the AQIC resources. The fix here is to call
vfio_ap_free_aqic_resources if the adapter or domain is neither in
matrix_mdev->shadow_apcb or matrix_dev->info.

Fixes: b9bd10c43456d ("s390/vfio-ap: do not reset queue removed from host config")
Cc: stable@vger.kernel.org
Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 3652ba792be7..faab5e9179ff 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2507,12 +2507,15 @@ void vfio_ap_mdev_remove_queue(struct ap_device *apdev)
 	/*
 	 * If the queue is not in the host's AP configuration, then resetting
 	 * it will fail with response code 01, (APQN not valid); so, let's make
-	 * sure it is in the host's config.
+	 * sure it is in the host's config. If it is not, then free the KVM GISC
+	 * resources.
 	 */
 	if (test_bit_inv(apid, (unsigned long *)matrix_dev->info.apm) &&
 	    test_bit_inv(apqi, (unsigned long *)matrix_dev->info.aqm)) {
 		vfio_ap_mdev_reset_queue(q);
 		flush_work(&q->reset_work);
+	} else {
+		vfio_ap_free_aqic_resources(q);
 	}
 
 done:
-- 
2.53.0

Re: [PATCH v3 0/9] s390/vfio-ap: Fix bugs in vfio_ap device driver callback functions

[Christian Borntraeger]

Am 12.08.26 um 13:51 schrieb Anthony Krowiak:
> During review of patches by the Sashiko AI, several pre-existing bugs were
> discovered. This 9-patch series fixes those bugs
> 
> Change log v2 => v3:
> ~~~~~~~~~~~~~~~~~~~
> Patch 3: Fix use of wrong lock in mdev probe function
> Patch 7: Fix required lock not held during display of sysfs status
> attribute
> * Squashed these two patches into a new patch entitled "Fix missing lock
>    required to access list of ap_matrix_mdev objects"
> * Added two new patches that fix pre-existing bugs:
>    ~ s390/vfio-ap: Fix NULL deref in status_show() during queue probe
>    ~ s390/vfio-ap: Fix memory leak when queue removed from host AP config
> 
> Anthony Krowiak (9):
>    s390/vfio-ap: Fix stale do_remove flag across iterations in
>      vfio_ap_mdev_cfg_remove
>    s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for
>      NULL
>    s390/vfio-ap: Fix missing lock required to access list of
>      ap_matrix_mdev objects
>    s390/vfio-ap: Fix required lock not held during update of
>      ap_matrix_mdev object
>    s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove
>    s390/vfio-ap: fix potential use of uninitialized apm_filtered bitmap
>    s390/vfio-ap: Fix hot-unplug skipped when last AP adapter or domain
>      removed
>    s390/vfio-ap: Fix NULL deref in status_show() during queue probe
>    s390/vfio-ap: Fix memory leak when queue removed from host AP config
> 
>   drivers/s390/crypto/vfio_ap_ops.c | 110 ++++++++++++++++++++----------
>   1 file changed, 75 insertions(+), 35 deletions(-)
> 


Sashiko has one new finding for patch8. Everything else is unrelated.
Will you do a respin for this?
I plan to take this for the next merge window.

Re: [PATCH v3 0/9] s390/vfio-ap: Fix bugs in vfio_ap device driver callback functions

[Matthew Rosato]

On 8/12/26 11:30 AM, Christian Borntraeger wrote:
> 
> Am 12.08.26 um 13:51 schrieb Anthony Krowiak:
>> During review of patches by the Sashiko AI, several pre-existing bugs
>> were
>> discovered. This 9-patch series fixes those bugs
>>
>> Change log v2 => v3:
>> ~~~~~~~~~~~~~~~~~~~
>> Patch 3: Fix use of wrong lock in mdev probe function
>> Patch 7: Fix required lock not held during display of sysfs status
>> attribute
>> * Squashed these two patches into a new patch entitled "Fix missing lock
>>    required to access list of ap_matrix_mdev objects"
>> * Added two new patches that fix pre-existing bugs:
>>    ~ s390/vfio-ap: Fix NULL deref in status_show() during queue probe
>>    ~ s390/vfio-ap: Fix memory leak when queue removed from host AP config
>>
>> Anthony Krowiak (9):
>>    s390/vfio-ap: Fix stale do_remove flag across iterations in
>>      vfio_ap_mdev_cfg_remove
>>    s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for
>>      NULL
>>    s390/vfio-ap: Fix missing lock required to access list of
>>      ap_matrix_mdev objects
>>    s390/vfio-ap: Fix required lock not held during update of
>>      ap_matrix_mdev object
>>    s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove
>>    s390/vfio-ap: fix potential use of uninitialized apm_filtered bitmap
>>    s390/vfio-ap: Fix hot-unplug skipped when last AP adapter or domain
>>      removed
>>    s390/vfio-ap: Fix NULL deref in status_show() during queue probe
>>    s390/vfio-ap: Fix memory leak when queue removed from host AP config
>>
>>   drivers/s390/crypto/vfio_ap_ops.c | 110 ++++++++++++++++++++----------
>>   1 file changed, 75 insertions(+), 35 deletions(-)
>>
> 
> 
> Sashiko has one new finding for patch8. Everything else is unrelated.
> Will you do a respin for this?

I am going thru this version now but yes I would like a respin that
fixes patch 8.

> I plan to take this for the next merge window.

I am also going thru all of the Sashiko reports, most are already
resolved by this series.  But I will send Tony a pruned list to
investigate after this series vs adding more fixes to this one.

Re: [PATCH v3 0/9] s390/vfio-ap: Fix bugs in vfio_ap device driver callback functions

[Christian Borntraeger]

Am 12.08.26 um 17:36 schrieb Matthew Rosato:
> On 8/12/26 11:30 AM, Christian Borntraeger wrote:
>>
>> Am 12.08.26 um 13:51 schrieb Anthony Krowiak:
>>> During review of patches by the Sashiko AI, several pre-existing bugs
>>> were
>>> discovered. This 9-patch series fixes those bugs
>>>
>>> Change log v2 => v3:
>>> ~~~~~~~~~~~~~~~~~~~
>>> Patch 3: Fix use of wrong lock in mdev probe function
>>> Patch 7: Fix required lock not held during display of sysfs status
>>> attribute
>>> * Squashed these two patches into a new patch entitled "Fix missing lock
>>>     required to access list of ap_matrix_mdev objects"
>>> * Added two new patches that fix pre-existing bugs:
>>>     ~ s390/vfio-ap: Fix NULL deref in status_show() during queue probe
>>>     ~ s390/vfio-ap: Fix memory leak when queue removed from host AP config
>>>
>>> Anthony Krowiak (9):
>>>     s390/vfio-ap: Fix stale do_remove flag across iterations in
>>>       vfio_ap_mdev_cfg_remove
>>>     s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for
>>>       NULL
>>>     s390/vfio-ap: Fix missing lock required to access list of
>>>       ap_matrix_mdev objects
>>>     s390/vfio-ap: Fix required lock not held during update of
>>>       ap_matrix_mdev object
>>>     s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove
>>>     s390/vfio-ap: fix potential use of uninitialized apm_filtered bitmap
>>>     s390/vfio-ap: Fix hot-unplug skipped when last AP adapter or domain
>>>       removed
>>>     s390/vfio-ap: Fix NULL deref in status_show() during queue probe
>>>     s390/vfio-ap: Fix memory leak when queue removed from host AP config
>>>
>>>    drivers/s390/crypto/vfio_ap_ops.c | 110 ++++++++++++++++++++----------
>>>    1 file changed, 75 insertions(+), 35 deletions(-)
>>>
>>
>>
>> Sashiko has one new finding for patch8. Everything else is unrelated.
>> Will you do a respin for this?
> 
> I am going thru this version now but yes I would like a respin that
> fixes patch 8.
> 
>> I plan to take this for the next merge window.
> 
> I am also going thru all of the Sashiko reports, most are already
> resolved by this series.  But I will send Tony a pruned list to
> investigate after this series vs adding more fixes to this one.

Let me know when this is ready. Apart from patch 8 I am happy.

Re: [PATCH v3 3/9] s390/vfio-ap: Fix missing lock required to access list of ap_matrix_mdev objects

[Matthew Rosato]

On 8/12/26 7:51 AM, Anthony Krowiak wrote:
> In order to traverse or add/remove ap_matrix_mdev objects in the
> matrix_dev->mdev_list, the matrix_dev->guests_lock mutex must be held.
> There are two functions that access the list without holding the mutex:
> 
> vfio_ap_mdev_probe function
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> The vfio_ap_mdev_probe function uses the matrix_dev->mdevs_lock
> mutex to guard the add of a newly created ap_matrix_mdev object to the
> matrix_dev->mdev_list. This mutex does not protect list access; its purpose
> is to guard against concurrent access to fields contained in an
> ap_matrix_mdev object. This could lead to kernel memory corruption or
> use-after-free if another mdev is created or removed concurrently.
> 
> The adding of an ap_matrix_mdev object to matrix_dev->mdev_list
> is now guarded by the matrix_dev->guests_lock which is the correct
> way to protect against concurrent mdev_list access.
> 
> vfio_ap_mdev_for_queue function
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> The status_show function that supports display of the status 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.
> 
> 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.
> 
> See the vfio-ap-locking.rst in the linux kernel tree.
> 
> Fixes: 2c1ee8983aa3 ("s390/vfio-ap: prepare for dynamic update of guest's APCB on queue probe/remove")
> Cc: stable@vger.kernel.org
> Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

Re: [PATCH v3 9/9] s390/vfio-ap: Fix memory leak when queue removed from host AP config

[Matthew Rosato]

On 8/12/26 7:51 AM, Anthony Krowiak wrote:
> When an adapter or domain is removed from the host's AP configuration, the
> AP bus invokes vfio_ap_on_cfg_changed() to notify the vfio_ap device
> driver. For each ap_matrix_mdev object to which the adapter or domain
> is assigned, vfio_ap_mdev_hot_unplug_cfg() is called and removes the
> adapter or domain from the matrix_mdev->shadow_apcb (i.e., the guest's AP
> configuration) and hot unplugs it if a guest is using it. The new host
> AP configuration (sans adapter or domain) is then stored in
> matrix_dev->info.
> 
> When the AP bus subsequently unbinds the physical queue devices associated
> with the adapter or domain that has been removed, it invokes
> vfio_ap_mdev_remove_queue() for each queue removed. At this point, the
> adapter or domain will no longer be assigned to the
> matrix_mdev->shadow_apcb or the matrix_dev->info object because they would
> have been removed by vfio_ap_on_cfg_changed(). Consequently,
> vfio_ap_mdev_reset_queue(q) is bypassed and kfree(q) is called
> without executing vfio_ap_free_aqic_resources(). This indefinitely pins
> guest memory (q->saved_iova) and leaks KVM GISC resources (q->saved_isc).
> 
> Note that resetting the queue would fail with an invalid APQN error due to
> the fact the queue is not longer in the host's AP configuration; however,
> it is still necessary to free the AQIC resources. The fix here is to call
> vfio_ap_free_aqic_resources if the adapter or domain is neither in
> matrix_mdev->shadow_apcb or matrix_dev->info.
> 
> Fixes: b9bd10c43456d ("s390/vfio-ap: do not reset queue removed from host config")
> Cc: stable@vger.kernel.org
> Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>