In i2c_amd_probe(), amd_mp2_find_device() utilizes
driver_find_next_device() which internally calls driver_find_device()
to locate the matching device. driver_find_device() increments the
reference count of the found device by calling get_device(), but
amd_mp2_find_device() fails to call put_device() to decrement the
reference count before returning. This results in a reference count
leak of the PCI device each time i2c_amd_probe() is executed, which
may prevent the device from being properly released and cause a memory
leak.

Found by code review.

Cc: stable@vger.kernel.org
Fixes: 529766e0a011 ("i2c: Add drivers for the AMD PCIe MP2 I2C controller")
Signed-off-by: Ma Ke <make24@iscas.ac.cn>
---
Changes in v2:
- modified the missing initialization in the patch. Sorry for the omission.
---
 drivers/i2c/busses/i2c-amd-mp2-pci.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-amd-mp2-pci.c b/drivers/i2c/busses/i2c-amd-mp2-pci.c
index ef7370d3dbea..60edbabc2986 100644
--- a/drivers/i2c/busses/i2c-amd-mp2-pci.c
+++ b/drivers/i2c/busses/i2c-amd-mp2-pci.c
@@ -458,13 +458,16 @@ struct amd_mp2_dev *amd_mp2_find_device(void)
 {
 	struct device *dev;
 	struct pci_dev *pci_dev;
+	struct amd_mp2_dev *mp2_dev;
 
 	dev = driver_find_next_device(&amd_mp2_pci_driver.driver, NULL);
 	if (!dev)
 		return NULL;
 
 	pci_dev = to_pci_dev(dev);
-	return (struct amd_mp2_dev *)pci_get_drvdata(pci_dev);
+	mp2_dev = (struct amd_mp2_dev *)pci_get_drvdata(pci_dev);
+	put_device(dev);
+	return mp2_dev;
 }
 EXPORT_SYMBOL_GPL(amd_mp2_find_device);
 
-- 
2.17.1

Hi,

> diff --git a/drivers/i2c/busses/i2c-amd-mp2-pci.c b/drivers/i2c/busses/i2c-amd-mp2-pci.c
> index ef7370d3dbea..60edbabc2986 100644
> --- a/drivers/i2c/busses/i2c-amd-mp2-pci.c
> +++ b/drivers/i2c/busses/i2c-amd-mp2-pci.c
> @@ -458,13 +458,16 @@ struct amd_mp2_dev *amd_mp2_find_device(void)
>  {
>  	struct device *dev;
>  	struct pci_dev *pci_dev;
> +	struct amd_mp2_dev *mp2_dev;
>  
>  	dev = driver_find_next_device(&amd_mp2_pci_driver.driver, NULL);
>  	if (!dev)
>  		return NULL;
>  
>  	pci_dev = to_pci_dev(dev);
> -	return (struct amd_mp2_dev *)pci_get_drvdata(pci_dev);
> +	mp2_dev = (struct amd_mp2_dev *)pci_get_drvdata(pci_dev);
> +	put_device(dev);
> +	return mp2_dev;

the patch is good, but I don't think you need to declare mp2_dev
because to_pci_dev(dev) should work even without hodling the
reference of dev.

I also have to agree with Markus that something like:

struct device *dev __free(put_device) = ...; /* it can also be NULL */

would work nicer.

Thanks,
Andi

>  }
>  EXPORT_SYMBOL_GPL(amd_mp2_find_device);
>  
> -- 
> 2.17.1
>

On Thu, 2 Oct 2025 at 07:56, Andi Shyti <andi.shyti@kernel.org> wrote:
> Hi,
> 
> > diff --git a/drivers/i2c/busses/i2c-amd-mp2-pci.c b/drivers/i2c/busses/i2c-amd-mp2-pci.c
> > index ef7370d3dbea..60edbabc2986 100644
> > --- a/drivers/i2c/busses/i2c-amd-mp2-pci.c
> > +++ b/drivers/i2c/busses/i2c-amd-mp2-pci.c
> > @@ -458,13 +458,16 @@ struct amd_mp2_dev *amd_mp2_find_device(void)
> >  {
> >  	struct device *dev;
> >  	struct pci_dev *pci_dev;
> > +	struct amd_mp2_dev *mp2_dev;
> >  
> >  	dev = driver_find_next_device(&amd_mp2_pci_driver.driver, NULL);
> >  	if (!dev)
> >  		return NULL;
> >  
> >  	pci_dev = to_pci_dev(dev);
> > -	return (struct amd_mp2_dev *)pci_get_drvdata(pci_dev);
> > +	mp2_dev = (struct amd_mp2_dev *)pci_get_drvdata(pci_dev);
> > +	put_device(dev);
> > +	return mp2_dev;
> 
> the patch is good, but I don't think you need to declare mp2_dev
> because to_pci_dev(dev) should work even without hodling the
> reference of dev.
Thank you for your feedback. The declaration of the temporary variable
mp2_dev in the patch may be necessary because we need to save the 
result of pci_get_drvdata(pci_dev) before calling put_device(dev). If 
we do not do this and instead call put_device(dev) first before 
returning pci_get_drvdata(pci_dev), it may lead to the deallocation of
dev, thereby invalidating pci_dev. Accessing its driver data at this 
point could potentially trigger a use-after-free error. Therefore, the
temporary variable ensures that the driver data remains safely 
accessible even after the reference is released. So perhaps we need to
retain the declaration of this temporary variable?

Best regards,
Ma Ke
> 
> I also have to agree with Markus that something like:
> 
> struct device *dev __free(put_device) = ...; /* it can also be NULL */
> 
> would work nicer.
> 
> Thanks,
> Andi
> 
> >  }
> >  EXPORT_SYMBOL_GPL(amd_mp2_find_device);
> >  
> > -- 
> > 2.17.1
> >

Hi again,

On Thu, Oct 02, 2025 at 01:56:30AM +0200, Andi Shyti wrote:
> Hi,
> 
> > diff --git a/drivers/i2c/busses/i2c-amd-mp2-pci.c b/drivers/i2c/busses/i2c-amd-mp2-pci.c
> > index ef7370d3dbea..60edbabc2986 100644
> > --- a/drivers/i2c/busses/i2c-amd-mp2-pci.c
> > +++ b/drivers/i2c/busses/i2c-amd-mp2-pci.c
> > @@ -458,13 +458,16 @@ struct amd_mp2_dev *amd_mp2_find_device(void)
> >  {
> >  	struct device *dev;
> >  	struct pci_dev *pci_dev;
> > +	struct amd_mp2_dev *mp2_dev;
> >  
> >  	dev = driver_find_next_device(&amd_mp2_pci_driver.driver, NULL);
> >  	if (!dev)
> >  		return NULL;
> >  
> >  	pci_dev = to_pci_dev(dev);
> > -	return (struct amd_mp2_dev *)pci_get_drvdata(pci_dev);
> > +	mp2_dev = (struct amd_mp2_dev *)pci_get_drvdata(pci_dev);
> > +	put_device(dev);
> > +	return mp2_dev;
> 
> the patch is good, but I don't think you need to declare mp2_dev
> because to_pci_dev(dev) should work even without hodling the
> reference of dev.
> 
> I also have to agree with Markus that something like:
> 
> struct device *dev __free(put_device) = ...; /* it can also be NULL */

sorry, please ignore this last comment, because if !dev we
shouldn't call put_device().

Andi

> In i2c_amd_probe(), amd_mp2_find_device() utilizes
> driver_find_next_device() which internally calls driver_find_device()
> to locate the matching device. driver_find_device() increments the
> reference count of the found device by calling get_device(), but
> amd_mp2_find_device() fails to call put_device() to decrement the
> reference count before returning. This results in a reference count
> leak of the PCI device each time i2c_amd_probe() is executed, which
> may prevent the device from being properly released and cause a memory
> leak.

Under which circumstances would you become interested to apply an attribute
like “__free(put_device)”?
https://elixir.bootlin.com/linux/v6.17-rc7/source/include/linux/device.h#L1180

Regards,
Markus

On Sun, Sep 28, 2025 at 01:00:13PM +0200, Markus Elfring wrote:
> > In i2c_amd_probe(), amd_mp2_find_device() utilizes
> > driver_find_next_device() which internally calls driver_find_device()
> > to locate the matching device. driver_find_device() increments the
> > reference count of the found device by calling get_device(), but
> > amd_mp2_find_device() fails to call put_device() to decrement the
> > reference count before returning. This results in a reference count
> > leak of the PCI device each time i2c_amd_probe() is executed, which
> > may prevent the device from being properly released and cause a memory
> > leak.
> 
> Under which circumstances would you become interested to apply an attribute
> like “__free(put_device)”?
> https://elixir.bootlin.com/linux/v6.17-rc7/source/include/linux/device.h#L1180
> 
> Regards,
> Markus
> 

Hi,

This is the semi-friendly patch-bot of Greg Kroah-Hartman.

Markus, you seem to have sent a nonsensical or otherwise pointless
review comment to a patch submission on a Linux kernel developer mailing
list.  I strongly suggest that you not do this anymore.  Please do not
bother developers who are actively working to produce patches and
features with comments that, in the end, are a waste of time.

Patch submitter, please ignore Markus's suggestion; you do not need to
follow it at all.  The person/bot/AI that sent it is being ignored by
almost all Linux kernel maintainers for having a persistent pattern of
behavior of producing distracting and pointless commentary, and
inability to adapt to feedback.  Please feel free to also ignore emails
from them.

thanks,

greg k-h's patch email bot