Re: [PATCH v6 09/15] s390/vfio-ap: Add method to set a new guest AP configuration

On 7/27/26 2:11 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Latent NULL pointer dereference in `collect_queues_by_apid` due to contradictory logic.
> --
>
> --- Patch [9]: [PATCH v6 09/15] s390/vfio-ap: Add method to set a new guest AP configuration ---
> commit 5b979f09ef9429e09eace5977dabbd22cc832bbf
> Author: Anthony Krowiak <[email protected]>
>
> s390/vfio-ap: Add method to set a new guest AP configuration
>
> This commit adds a new vfio_ap_set_new_config function to set a guest's AP
> configuration. It refactors code from ap_config_store to handle changes to
> the sysfs ap_config attribute and support live guest migration.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index d05372b50d2fc..0b49be22d5d8b 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> [ ... ]
>
>> -static void collect_queues_to_reset(struct ap_matrix_mdev *matrix_mdev,
>> -				    unsigned long apid,
>> -				    struct list_head *qlist)
>> +static void collect_queues_by_apid(struct ap_matrix_mdev *matrix_mdev,
>> +				   unsigned long apid,
>> +				   struct list_head *qlist)
>>   {
>>   	struct vfio_ap_queue *q;
>>   	unsigned long  apqi;
>>   
>>   	for_each_set_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm, AP_DOMAINS) {
>> -		q = vfio_ap_mdev_get_queue(matrix_mdev, AP_MKQID(apid, apqi));
>> +		q = matrix_mdev ?
>> +				vfio_ap_mdev_get_queue(matrix_mdev, AP_MKQID(apid, apqi)) :
>> +				vfio_ap_find_queue(AP_MKQID(apid, apqi));
> [Severity: Medium]
> Will this result in a NULL pointer dereference if matrix_mdev is NULL?
>
> The loop condition unconditionally accesses matrix_mdev->shadow_apcb.aqm.
> However, the ternary operator inside the loop (matrix_mdev ?) suggests
> that matrix_mdev could be NULL.
>
> If matrix_mdev can indeed be NULL, the unconditional dereference in
> for_each_set_bit_inv() will crash before the check is evaluated. If
> matrix_mdev is never NULL in practice, is the ternary check necessary?

Every caller passes matrix_mdev obtained from container context that is
always non-NULL at those call sites. The ternary is dead code — matrix_mdev
is never NULL here and will be removed.

>
>>   		if (q)
>>   			list_add_tail(&q->reset_qnode, qlist);
>>   	}