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);
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#L11...
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#L11...
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
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
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
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
linux-stable-mirror@lists.linaro.org
-
Andi Shyti -
Greg KH -
Ma Ke -
Markus Elfring