RE: [PATCH] EDAC/sb_edac: Fix pci_vtd reference leak in haswell_mci_bind_devs()

[Leo Zhuo]

> From: Wentao Liang <vulab@xxxxxxxxxxx>
> [...]
> Subject: [PATCH] EDAC/sb_edac: Fix pci_vtd reference leak in
> haswell_mci_bind_devs()
> 
> 
> In haswell_mci_bind_devs(), a reference to the VT-d device is taken with
> pci_get_device() and stored in pvt->info.pci_vtd.  If one of the required devices is
> missing, the function returns -ENODEV without putting that reference, and the
> caller frees the mem_ctl_info along with its private data, leaking the reference.
> 
> Put the device reference before returning on the error path.
> 
> Fixes: 50d1bb93672f ("sb_edac: add support for Haswell based systems")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Wentao Liang <vulab@xxxxxxxxxxx>
> ---
>  drivers/edac/sb_edac.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c index
> 7b282dfd093f..5075c703e19e 100644
> --- a/drivers/edac/sb_edac.c
> +++ b/drivers/edac/sb_edac.c
> @@ -2846,6 +2846,8 @@ static int haswell_mci_bind_devs(struct mem_ctl_info
> *mci,
>         return 0;
> 
>  enodev:
> +       pci_dev_put(pvt->info.pci_vtd);
> +       pvt->info.pci_vtd = NULL;

The fix looks incomplete: it only releases pci_vtd only when haswell_mci_bind_devs() returns -ENODEV. 
The reference still leaks if registration fails later or during normal driver removal. 
Broadwell's path has the same issue.

A complete fix should release pci_vtd whenever the MCI is freed, i.e, covering both Haswell and Broadwell 
registration failures and normal removal. And ensure pci_dev_put (pci_vtd) is called exactly once.

How about the following fix:

diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c
index 6e248855a549..d0d01efc2fc9 100644
--- a/drivers/edac/sb_edac.c
+++ b/drivers/edac/sb_edac.c
@@ -3315,6 +3315,7 @@ static struct notifier_block sbridge_mce_dec = {
 static void sbridge_unregister_mci(struct sbridge_dev *sbridge_dev)
 {
        struct mem_ctl_info *mci = sbridge_dev->mci;
+       struct sbridge_pvt *pvt;

        if (unlikely(!mci || !mci->pvt_info)) {
                edac_dbg(0, "MC: dev = %p\n", &sbridge_dev->pdev[0]->dev);
@@ -3323,6 +3324,7 @@ static void sbridge_unregister_mci(struct sbridge_dev *sbridge_dev)
                return;
        }

+       pvt = mci->pvt_info;
        edac_dbg(0, "MC: mci = %p, dev = %p\n",
                 mci, &sbridge_dev->pdev[0]->dev);

@@ -3331,6 +3333,7 @@ static void sbridge_unregister_mci(struct sbridge_dev *sbridge_dev)

        edac_dbg(1, "%s: free mci struct\n", mci->ctl_name);
        kfree(mci->ctl_name);
+       pci_dev_put(pvt->info.pci_vtd);
        edac_mc_free(mci);
        sbridge_dev->mci = NULL;
 }
@@ -3531,6 +3534,7 @@ static int sbridge_register_mci(struct sbridge_dev *sbridge_dev, enum type type)
 fail:
        kfree(mci->ctl_name);
 fail0:
+       pci_dev_put(pvt->info.pci_vtd);
        edac_mc_free(mci);
        sbridge_dev->mci = NULL;
        return rc;
		
>         sbridge_printk(KERN_ERR, "Some needed devices are missing\n");
>         return -ENODEV;
>  }
> --
> 2.34.1
>