Re: [PATCH v4 1/2] hisi_acc_vfio_pci: fix NULL dereference in reset_prepare on PF passthrough

[liulongfang]

On 2026/9/23 6:59, Alex Williamson wrote:
> On Mon, 21 Sep 2026 16:15:12 +0800
> liulongfang <liulongfang@xxxxxxxxxx> wrote:
> 
>> On 2026/9/20 22:26, Alex Williamson wrote:
>>> On Fri, 18 Sep 2026 16:42:43 +0800
>>> Longfang Liu <liulongfang@xxxxxxxxxx> wrote:
>>>   
>>>> When a PF is bound to the driver via driver_override and passed
>>>> through to a VM, its pf_qm stays NULL. The PCI error handler
>>>> reset_prepare() runs during open_device through
>>>> pci_try_reset_function(), before the mig_ops gate, and dereferences
>>>> the NULL pf_qm for the timeout log, crashing the kernel.
>>>>
>>>> Move the mig_ops check to the entry of reset_prepare() and
>>>> aer_reset_done() so non-migration devices skip the QM_RESETTING
>>>> coordination. Also clear set_reset_flag together with QM_RESETTING
>>>> in aer_reset_done(); the flag was never cleared before, so a later
>>>> timed-out reset could release a foreign lock. Replace
>>>> pci_iov_vf_id() >= 0 with pdev->is_virtfn in probe() for clearer
>>>> on-VF gating.
>>>>
>>>> Fixes: b0eed085903e ("hisi_acc_vfio_pci: Add support for VFIO live migration")
>>>> Fixes: a22099ed7936f ("hisi_acc_vfio_pci: fix VF reset timeout issue")
>>>> Signed-off-by: Longfang Liu <liulongfang@xxxxxxxxxx>
>>>> ---
>>>>  .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c    | 19 ++++++++++++-------
>>>>  1 file changed, 12 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
>>>> index 86362ec424a5..8ff69c8d1ff7 100644
>>>> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
>>>> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
>>>> @@ -1154,9 +1154,14 @@ static void hisi_acc_vf_pci_reset_prepare(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;
>>>> -	struct device *dev = &qm->pdev->dev;
>>>> +	struct device *dev = &pdev->dev;
>>>>  	u32 delay = 0;
>>>>  
>>>> +	if (!hisi_acc_vdev->core_device.vdev.mig_ops) {
>>>> +		dev_err(dev, "device not support migration\n");
>>>> +		return;
>>>> +	}  
>>>
>>> Why is not supporting migration worthy of a dev_err()?!  Resets are a
>>> normal operation.  Generating a log to nag the lack of migration
>>> support on every reset is unacceptable.  
>>
>> How about changing it to a dev_info() then? Since users will notice errors when initiating
>> a migration, an explicit log is necessary to pinpoint the cause and facilitate subsequent
>> troubleshooting.
> 
> No, reset is a normal operation, it has nothing to do with migration.
> Migration support is discoverable.  If you want something in the log,
> it should be there once, at probe time.
>

OK, I'll add a dev_info() log in the probe function.

>>>> +
>>>>  	/* All reset requests need to be queued for processing */
>>>>  	while (test_and_set_bit(QM_RESETTING, &qm->misc_ctl)) {
>>>>  		msleep(1);
>>>> @@ -1174,12 +1179,14 @@ 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->core_device.vdev.mig_ops)
>>>>  		return;
>>>>  
>>>> +	if (hisi_acc_vdev->set_reset_flag) {
>>>> +		clear_bit(QM_RESETTING, &qm->misc_ctl);
>>>> +		hisi_acc_vdev->set_reset_flag = false;
>>>> +	}
>>>> +
>>>>  	mutex_lock(&hisi_acc_vdev->state_mutex);
>>>>  	hisi_acc_vf_reset(hisi_acc_vdev);
>>>>  	mutex_unlock(&hisi_acc_vdev->state_mutex);
>>>> @@ -1670,13 +1677,11 @@ static int hisi_acc_vfio_pci_probe(struct pci_dev *pdev, const struct pci_device
>>>>  	struct hisi_acc_vf_core_device *hisi_acc_vdev;
>>>>  	const struct vfio_device_ops *ops = &hisi_acc_vfio_pci_ops;
>>>>  	struct hisi_qm *pf_qm;
>>>> -	int vf_id;
>>>>  	int ret;
>>>>  
>>>>  	pf_qm = hisi_acc_get_pf_qm(pdev);
>>>>  	if (pf_qm && pf_qm->ver >= QM_HW_V3) {
>>>> -		vf_id = pci_iov_vf_id(pdev);
>>>> -		if (vf_id >= 0)
>>>> +		if (pdev->is_virtfn)
>>>>  			ops = &hisi_acc_vfio_pci_migrn_ops;
>>>>  		else
>>>>  			pci_warn(pdev, "migration support failed, continue with generic interface\n");  
>>>
>>> I still reject the redundant is_virtfn check here.  I don't find
>>> support for the claim that it's a convention among variant drivers, nor
>>> does it do anything here or to the next patch.  pf_qm is
>>> deterministically NULL for pdev->is_virtfn.  Thanks,
>>>  
>>
>> Even if left unchanged here, a later patch will anyway replace this complex logic with an is_virtfn check
>> to handle the pci_resource_len inspection during probe.
> 
> Are you talking about patch 2/2 that leaves it like this:
> 
> 	pf_qm = hisi_acc_get_pf_qm(pdev);
> 	if (pf_qm && pf_qm->ver >= QM_HW_V3 && pdev->is_virtfn) {
>

OK,  I will remove pdev->is_virtfn in the next version.

Thanks.
Longfang.

> Where once again I quote the source of pf_qm:
> 
> static struct hisi_qm *hisi_acc_get_pf_qm(struct pci_dev *pdev)
> {
> 	struct hisi_qm  *pf_qm;
> 	struct pci_driver *pf_driver;
>                 
> 	if (!pdev->is_virtfn)
> 		return NULL;
> 
> Please stop wasting time on this.
> .
>