[drivers/pci] Possible memleak in pci_bus_set_aer_ops

[drivers/pci] Possible memleak in pci_bus_set_aer_ops

[Zijie Zhao]

Dear PCI Developers,

We are curious whether the function `pci_bus_set_aer_ops` might have a memory leak.

The function is https://elixir.bootlin.com/linux/v6.8/source/drivers/pci/pcie/aer_inject.c#L297
and the relevant code is
```
static int pci_bus_set_aer_ops(struct pci_bus *bus)
{
	struct pci_ops *ops;
	struct pci_bus_ops *bus_ops;
	unsigned long flags;

	bus_ops = kmalloc(sizeof(*bus_ops), GFP_KERNEL);
	if (!bus_ops)
		return -ENOMEM;
	ops = pci_bus_set_ops(bus, &aer_inj_pci_ops);
	spin_lock_irqsave(&inject_lock, flags);
	if (ops == &aer_inj_pci_ops)
		goto out;
	pci_bus_ops_init(bus_ops, bus, ops);
	list_add(&bus_ops->list, &pci_bus_ops_list);
	bus_ops = NULL;
out:
	spin_unlock_irqrestore(&inject_lock, flags);
	kfree(bus_ops);
	return 0;
}
```

Here if the goto statement does not jump to `out`, the `bus_ops` will be assigned with `NULL` and then `kfree(bus_ops)` will not free the allocated memory.

Please kindly correct us if we missed any key information. Looking forward to your response!

Best,
Zijie

Re: [drivers/pci] Possible memleak in pci_bus_set_aer_ops

[Kuppuswamy Sathyanarayanan]

On 3/13/24 7:20 PM, Zijie Zhao wrote:
> Dear PCI Developers,
>
> We are curious whether the function `pci_bus_set_aer_ops` might have a memory leak.
>
> The function is https://elixir.bootlin.com/linux/v6.8/source/drivers/pci/pcie/aer_inject.c#L297
> and the relevant code is
> ```
> static int pci_bus_set_aer_ops(struct pci_bus *bus)
> {
> 	struct pci_ops *ops;
> 	struct pci_bus_ops *bus_ops;
> 	unsigned long flags;
>
> 	bus_ops = kmalloc(sizeof(*bus_ops), GFP_KERNEL);
> 	if (!bus_ops)
> 		return -ENOMEM;
> 	ops = pci_bus_set_ops(bus, &aer_inj_pci_ops);
> 	spin_lock_irqsave(&inject_lock, flags);
> 	if (ops == &aer_inj_pci_ops)
> 		goto out;
> 	pci_bus_ops_init(bus_ops, bus, ops);
> 	list_add(&bus_ops->list, &pci_bus_ops_list);
> 	bus_ops = NULL;
> out:
> 	spin_unlock_irqrestore(&inject_lock, flags);
> 	kfree(bus_ops);
> 	return 0;
> }
> ```
>
> Here if the goto statement does not jump to `out`, the `bus_ops` will be assigned with `NULL` and then `kfree(bus_ops)` will not free the allocated memory.
>
> Please kindly correct us if we missed any key information. Looking forward to your response!

I think it is a valid issue that needs to be fixed. If you would like, please send a patch to fix it.

>
> Best,
> Zijie
>
-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

[PATCH] fix memory leak in pci_bus_set_aer_ops

[Zijie Zhao]

Remove unnecessary bus_ops = NULL assignment in pci_bus_set_aer_ops to
prevent a memory leak when ops are equal to &aer_inj_pci_ops. This change
ensures allocated bus_ops memory is properly freed.

Signed-off-by: Zijie Zhao <zzjas98@gmail.com>
---
 drivers/pci/pcie/aer_inject.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pci/pcie/aer_inject.c b/drivers/pci/pcie/aer_inject.c
index 2dab275d252f..0c84fadbfd2e 100644
--- a/drivers/pci/pcie/aer_inject.c
+++ b/drivers/pci/pcie/aer_inject.c
@@ -309,7 +309,6 @@ static int pci_bus_set_aer_ops(struct pci_bus *bus)
 		goto out;
 	pci_bus_ops_init(bus_ops, bus, ops);
 	list_add(&bus_ops->list, &pci_bus_ops_list);
-	bus_ops = NULL;
 out:
 	spin_unlock_irqrestore(&inject_lock, flags);
 	kfree(bus_ops);
-- 
2.34.1

Re: [drivers/pci] Possible memleak in pci_bus_set_aer_ops

[Kuppuswamy Sathyanarayanan]

On 3/13/24 7:40 PM, Kuppuswamy Sathyanarayanan wrote:
> On 3/13/24 7:20 PM, Zijie Zhao wrote:
>> Dear PCI Developers,
>>
>> We are curious whether the function `pci_bus_set_aer_ops` might have a memory leak.
>>
>> The function is https://elixir.bootlin.com/linux/v6.8/source/drivers/pci/pcie/aer_inject.c#L297
>> and the relevant code is
>> ```
>> static int pci_bus_set_aer_ops(struct pci_bus *bus)
>> {
>> 	struct pci_ops *ops;
>> 	struct pci_bus_ops *bus_ops;
>> 	unsigned long flags;
>>
>> 	bus_ops = kmalloc(sizeof(*bus_ops), GFP_KERNEL);
>> 	if (!bus_ops)
>> 		return -ENOMEM;
>> 	ops = pci_bus_set_ops(bus, &aer_inj_pci_ops);
>> 	spin_lock_irqsave(&inject_lock, flags);
>> 	if (ops == &aer_inj_pci_ops)
>> 		goto out;
>> 	pci_bus_ops_init(bus_ops, bus, ops);
>> 	list_add(&bus_ops->list, &pci_bus_ops_list);
>> 	bus_ops = NULL;
>> out:
>> 	spin_unlock_irqrestore(&inject_lock, flags);
>> 	kfree(bus_ops);
>> 	return 0;
>> }
>> ```
>>
>> Here if the goto statement does not jump to `out`, the `bus_ops` will be assigned with `NULL` and then `kfree(bus_ops)` will not free the allocated memory.
>>
>> Please kindly correct us if we missed any key information. Looking forward to your response!
> I think it is a valid issue that needs to be fixed. If you would like, please send a patch to fix it.

Sorry, I misread it. I think it is not a issue. For a valid case, the bus_ops is
added to pci_bus_ops_list, which is freed in module exit function. Ignore
my previous comments.

>
>> Best,
>> Zijie
>>
-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer