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

[Alex Williamson]

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.

> >> +
> >>  	/* 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) {

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.