Re: [PATCH v3 1/3] hisi_acc_vfio_pci: fix live migration enable conditions for PF passthrough

[liulongfang <liulongfang@huawei.com>]

On 2026/8/31 17:35, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] `hisi_acc_vfio_pci_close_device()` accesses and frees migration file descriptors locklessly, racing with AER reset handling and leading to double `fput()` and Use-After-Free.
> --
> 
> commit 63de12e55959c8c98f32c474a3df763dbb67aa2c
> Author: Longfang Liu <liulongfang@huawei.com>
> Subject: hisi_acc_vfio_pci: fix live migration enable conditions for PF passthrough
> 
> This commit fixes live migration enable conditions for PF passthrough by
> adding validity checks for the PF QM pointer. It ensures operations exit
> or return an error directly when the pointer is invalid to avoid crashes.
> 
>> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
>> index 86362ec424a50..e95d0ab0f11ab 100644
>> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
>> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> [ ... ]
>> @@ -1174,8 +1179,12 @@ static void hisi_acc_vf_pci_aer_reset_done(struct pci_dev *pdev)
>>  	struct hisi_acc_vf_core_device *hisi_acc_vdev = hisi_acc_drvdata(pdev);
>>  	struct hisi_qm *qm = hisi_acc_vdev->pf_qm;
>>  
>> -	if (hisi_acc_vdev->set_reset_flag)
>> -		clear_bit(QM_RESETTING, &qm->misc_ctl);
>> +	if (hisi_acc_vdev->set_reset_flag) {
>> +		if (qm && qm->io_base)
>> +			clear_bit(QM_RESETTING, &qm->misc_ctl);
>> +		else
>> +			dev_err(&pdev->dev, "PF QM not available for reset done\n");
>> +	}
>>  
>>  	if (!hisi_acc_vdev->core_device.vdev.mig_ops)
>>  		return;
> 
> [Severity: High]
> This is a pre-existing issue, but I have a question about how the AER reset
> interacts with device closing.
> 
> In hisi_acc_vf_pci_aer_reset_done(), the code eventually calls
> hisi_acc_vf_reset() while holding state_mutex:
> 
> 	mutex_lock(&hisi_acc_vdev->state_mutex);
> 	hisi_acc_vf_reset(hisi_acc_vdev);
> 	mutex_unlock(&hisi_acc_vdev->state_mutex);
> 
> Inside hisi_acc_vf_reset(), it calls hisi_acc_vf_disable_fds(), which reads
> and modifies hisi_acc_vdev->resuming_migf and hisi_acc_vdev->saving_migf.
> 
> However, in hisi_acc_vfio_pci_close_device(), hisi_acc_vf_disable_fds() is
> called without holding state_mutex:
> 
> static void hisi_acc_vfio_pci_close_device(struct vfio_device *core_vdev)
> {
> 	...
> 	hisi_acc_vf_disable_fds(hisi_acc_vdev);
> 	mutex_lock(&hisi_acc_vdev->open_mutex);
> 
> Can this lead to a race condition where a user closes the device concurrently
> with an AER reset?
> 
> If both threads call hisi_acc_vf_disable_fds() simultaneously, they might both
> see a non-NULL migration file descriptor:
> 
> static void hisi_acc_vf_disable_fds(struct hisi_acc_vf_core_device *hisi_acc_vdev)
> {
> 	if (hisi_acc_vdev->resuming_migf) {
> 		hisi_acc_debug_migf_copy(hisi_acc_vdev, hisi_acc_vdev->resuming_migf);
> 		hisi_acc_vf_disable_fd(hisi_acc_vdev->resuming_migf);
> 		fput(hisi_acc_vdev->resuming_migf->filp);
> 		hisi_acc_vdev->resuming_migf = NULL;
> 	}
>         ...
> }
> 
> Would this result in a double fput() and a use-after-free on the migration
> file object since the read and modification are not protected by a common lock?
>

The subsequent patch fixes the issue introduced by adding the parameter check here.

Thanks.
Longfang.