[PATCH v4 0/4] perf/x86/intel/uncore: Fix PCI device reference count leak

[PATCH v4 0/4] perf/x86/intel/uncore: Fix PCI device reference count leak

[Xiongfeng Wang]

pci_get_device() will increase the reference count for the returned
pci_dev, and also decrease the reference count for the input parameter
*from* if it is not NULL. We add missing pci_dev_put() to decrease the
reference count.


ChangeLog:
v1->v2:
  move pci_dev_put() to the last of function sad_cfg_iio_topology()
  add fixes for snr_uncore_mmio_map(), hswep_has_limit_sbox() and __uncore_imc_init_box()
v2->v3:
  split one patch into several patches, one patch for one fix
v3->v4:
  change 'pci_dev_put(pdev)' to 'pci_dev_put(dev)' in the first patch

Xiongfeng Wang (4):
  perf/x86/intel/uncore: Fix reference count leak in
    sad_cfg_iio_topology()
  perf/x86/intel/uncore: Fix reference count leak in
    hswep_has_limit_sbox()
  perf/x86/intel/uncore: Fix reference count leak in
    snr_uncore_mmio_map()
  perf/x86/intel/uncore: Fix reference count leak in
    __uncore_imc_init_box()

 arch/x86/events/intel/uncore_snb.c   |  3 +++
 arch/x86/events/intel/uncore_snbep.c | 11 ++++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

-- 
2.20.1

[PATCH v4 1/4] perf/x86/intel/uncore: Fix reference count leak in sad_cfg_iio_topology()

[Xiongfeng Wang]

pci_get_device() will increase the reference count for the returned
pci_dev, and also decrease the reference count for the input parameter
*from* if it is not NULL.

If we break the loop in sad_cfg_iio_topology() with 'dev' not NULL. We
need to call pci_dev_put() to decrease the reference count. Since
pci_dev_put() can handle the NULL input parameter, we can just add one
pci_dev_put() right before 'return ret'.

Fixes: c1777be3646b ("perf/x86/intel/uncore: Enable I/O stacks to IIO PMON mapping on SNR")
Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
---
 arch/x86/events/intel/uncore_snbep.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
index ed869443efb2..76fedc8e12dd 100644
--- a/arch/x86/events/intel/uncore_snbep.c
+++ b/arch/x86/events/intel/uncore_snbep.c
@@ -4492,6 +4492,8 @@ static int sad_cfg_iio_topology(struct intel_uncore_type *type, u8 *sad_pmon_map
 		type->topology = NULL;
 	}
 
+	pci_dev_put(dev);
+
 	return ret;
 }
 
-- 
2.20.1

[PATCH v4 2/4] perf/x86/intel/uncore: Fix reference count leak in hswep_has_limit_sbox()

[Xiongfeng Wang]

pci_get_device() will increase the reference count for the returned
pci_dev. We need to call pci_dev_put() to decrease the reference count
before exiting from hswep_has_limit_sbox(). Add pci_dev_put() for both
normal and error path.

Fixes: 9d480158ee86 ("perf/x86/intel/uncore: Remove uncore extra PCI dev HSWEP_PCI_PCU_3")
Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
---
 arch/x86/events/intel/uncore_snbep.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
index 76fedc8e12dd..3fc484045ed6 100644
--- a/arch/x86/events/intel/uncore_snbep.c
+++ b/arch/x86/events/intel/uncore_snbep.c
@@ -2891,8 +2891,12 @@ static bool hswep_has_limit_sbox(unsigned int device)
 		return false;
 
 	pci_read_config_dword(dev, HSWEP_PCU_CAPID4_OFFET, &capid4);
-	if (!hswep_get_chop(capid4))
+	if (!hswep_get_chop(capid4)) {
+		pci_dev_put(dev);
 		return true;
+	}
+
+	pci_dev_put(dev);
 
 	return false;
 }
-- 
2.20.1

[PATCH v4 3/4] perf/x86/intel/uncore: Fix reference count leak in snr_uncore_mmio_map()

[Xiongfeng Wang]

pci_get_device() will increase the reference count for the returned
pci_dev, so snr_uncore_get_mc_dev() will return a pci_dev with its
reference count increased. We need to call pci_dev_put() to decrease the
reference count before exiting from snr_uncore_mmio_map(). Add
pci_dev_put() for both normal and error path.

Fixes: ee49532b38dd ("perf/x86/intel/uncore: Add IMC uncore support for Snow Ridge")
Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
---
 arch/x86/events/intel/uncore_snbep.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
index 3fc484045ed6..0481f5135cb1 100644
--- a/arch/x86/events/intel/uncore_snbep.c
+++ b/arch/x86/events/intel/uncore_snbep.c
@@ -4866,9 +4866,12 @@ static int snr_uncore_mmio_map(struct intel_uncore_box *box,
 	box->io_addr = ioremap(addr, type->mmio_map_size);
 	if (!box->io_addr) {
 		pr_warn("perf uncore: Failed to ioremap for %s.\n", type->name);
+		pci_dev_put(pdev);
 		return -EINVAL;
 	}
 
+	pci_dev_put(pdev);
+
 	return 0;
 }
 
-- 
2.20.1

[PATCH v4 4/4] perf/x86/intel/uncore: Fix reference count leak in __uncore_imc_init_box()

[Xiongfeng Wang]

pci_get_device() will increase the reference count for the returned
pci_dev, so tgl_uncore_get_mc_dev() will return a pci_dev with its
reference count increased. We need to call pci_dev_put() to decrease the
reference count before exiting from __uncore_imc_init_box(). Add
pci_dev_put() for both normal and error path.

Fixes: fdb64822443e ("perf/x86: Add Intel Tiger Lake uncore support")
Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
---
 arch/x86/events/intel/uncore_snb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/events/intel/uncore_snb.c b/arch/x86/events/intel/uncore_snb.c
index 1ef4f7861e2e..1f4869227efb 100644
--- a/arch/x86/events/intel/uncore_snb.c
+++ b/arch/x86/events/intel/uncore_snb.c
@@ -1338,6 +1338,7 @@ static void __uncore_imc_init_box(struct intel_uncore_box *box,
 	/* MCHBAR is disabled */
 	if (!(mch_bar & BIT(0))) {
 		pr_warn("perf uncore: MCHBAR is disabled. Failed to map IMC free-running counters.\n");
+		pci_dev_put(pdev);
 		return;
 	}
 	mch_bar &= ~BIT(0);
@@ -1352,6 +1353,8 @@ static void __uncore_imc_init_box(struct intel_uncore_box *box,
 	box->io_addr = ioremap(addr, type->mmio_map_size);
 	if (!box->io_addr)
 		pr_warn("perf uncore: Failed to ioremap for %s.\n", type->name);
+
+	pci_dev_put(pdev);
 }
 
 static void tgl_uncore_imc_freerunning_init_box(struct intel_uncore_box *box)
-- 
2.20.1

Re: [PATCH v4 2/4] perf/x86/intel/uncore: Fix reference count leak in hswep_has_limit_sbox()

[Liang, Kan]

On 2022-11-17 4:43 a.m., Xiongfeng Wang wrote:
> pci_get_device() will increase the reference count for the returned
> pci_dev. We need to call pci_dev_put() to decrease the reference count
> before exiting from hswep_has_limit_sbox(). Add pci_dev_put() for both
> normal and error path.
> 
> Fixes: 9d480158ee86 ("perf/x86/intel/uncore: Remove uncore extra PCI dev HSWEP_PCI_PCU_3")
> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
> ---
>  arch/x86/events/intel/uncore_snbep.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
> index 76fedc8e12dd..3fc484045ed6 100644
> --- a/arch/x86/events/intel/uncore_snbep.c
> +++ b/arch/x86/events/intel/uncore_snbep.c
> @@ -2891,8 +2891,12 @@ static bool hswep_has_limit_sbox(unsigned int device)
>  		return false;
>  
>  	pci_read_config_dword(dev, HSWEP_PCU_CAPID4_OFFET, &capid4);

I think we can move the pci_dev_put here to avoid patching it twice.

Thanks,
Kan

> -	if (!hswep_get_chop(capid4))
> +	if (!hswep_get_chop(capid4)) {
> +		pci_dev_put(dev);
>  		return true;
> +	}
> +
> +	pci_dev_put(dev);
>  
>  	return false;
>  }

Re: [PATCH v4 3/4] perf/x86/intel/uncore: Fix reference count leak in snr_uncore_mmio_map()

[Liang, Kan]

On 2022-11-17 4:43 a.m., Xiongfeng Wang wrote:
> pci_get_device() will increase the reference count for the returned
> pci_dev, so snr_uncore_get_mc_dev() will return a pci_dev with its
> reference count increased. We need to call pci_dev_put() to decrease the
> reference count before exiting from snr_uncore_mmio_map(). Add
> pci_dev_put() for both normal and error path.
> 
> Fixes: ee49532b38dd ("perf/x86/intel/uncore: Add IMC uncore support for Snow Ridge")
> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
> ---
>  arch/x86/events/intel/uncore_snbep.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
> index 3fc484045ed6..0481f5135cb1 100644
> --- a/arch/x86/events/intel/uncore_snbep.c
> +++ b/arch/x86/events/intel/uncore_snbep.c
> @@ -4866,9 +4866,12 @@ static int snr_uncore_mmio_map(struct intel_uncore_box *box,

Similar to the previous comments.
I think we can release the dev immediately once when we do not use it
anymore. So we don't need to patch it everywhere later.

Thanks,
Kan
>  	box->io_addr = ioremap(addr, type->mmio_map_size);
>  	if (!box->io_addr) {
>  		pr_warn("perf uncore: Failed to ioremap for %s.\n", type->name);
> +		pci_dev_put(pdev);
>  		return -EINVAL;
>  	}
>  
> +	pci_dev_put(pdev);
> +
>  	return 0;
>  }
>