[PATCH v2] PCI: vmd: Fix wrong kfree() in vmd_msi_free()

[PATCH v2] PCI: vmd: Fix wrong kfree() in vmd_msi_free()

[Nam Cao]

vmd_msi_alloc() allocates struct vmd_irq and stashes it into
irq_data->chip_data associated with the VMD's interrupt domain.
vmd_msi_free() extracts the pointer by calling irq_get_chip_data() and
frees it.

irq_get_chip_data() returns the chip_data associated with the top interrupt
domain. This worked in the past, because VMD's interrupt domain was the top
domain.

But since commit d7d8ab87e3e7 ("PCI: vmd: Switch to
msi_create_parent_irq_domain()") changed the interrupt domain hierarchy,
VMD's interrupt domain is not the top domain anymore. irq_get_chip_data()
now returns the chip_data at the MSI devices' interrupt domains. It is
therefore broken for vmd_msi_free() to kfree() this chip_data.

Fix this issue, correctly extract the chip_data associated with the VMD's
interrupt domain.

Fixes: d7d8ab87e3e7 ("PCI: vmd: Switch to msi_create_parent_irq_domain()")
Reported-by: Kenneth Crudup <kenny@panix.com>
Closes: https://lore.kernel.org/linux-pci/dfa40e48-8840-4e61-9fda-25cdb3ad81c1@panix.com/
Reported-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
Closes: https://lore.kernel.org/linux-pci/ed53280ed15d1140700b96cca2734bf327ee92539e5eb68e80f5bbbf0f01@linux.gnuweeb.org/
Tested-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
Tested-by: Kenneth Crudup <kenny@panix.com>
Signed-off-by: Nam Cao <namcao@linutronix.de>
---
v2: Fix typo and describe the change more precisely
---
 drivers/pci/controller/vmd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 9bbb0ff4cc15..b679c7f28f51 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -280,10 +280,12 @@ static int vmd_msi_alloc(struct irq_domain *domain, unsigned int virq,
 static void vmd_msi_free(struct irq_domain *domain, unsigned int virq,
 			 unsigned int nr_irqs)
 {
+	struct irq_data *irq_data;
 	struct vmd_irq *vmdirq;
 
 	for (int i = 0; i < nr_irqs; ++i) {
-		vmdirq = irq_get_chip_data(virq + i);
+		irq_data = irq_domain_get_irq_data(domain, virq + i);
+		vmdirq = irq_data->chip_data;
 
 		synchronize_srcu(&vmdirq->irq->srcu);
 
-- 
2.39.5

Re: [PATCH v2] PCI: vmd: Fix wrong kfree() in vmd_msi_free()

[Manivannan Sadhasivam]

On Thu, Aug 07, 2025 at 10:10:51AM GMT, Nam Cao wrote:
> vmd_msi_alloc() allocates struct vmd_irq and stashes it into
> irq_data->chip_data associated with the VMD's interrupt domain.
> vmd_msi_free() extracts the pointer by calling irq_get_chip_data() and
> frees it.
> 
> irq_get_chip_data() returns the chip_data associated with the top interrupt
> domain. This worked in the past, because VMD's interrupt domain was the top
> domain.
> 
> But since commit d7d8ab87e3e7 ("PCI: vmd: Switch to
> msi_create_parent_irq_domain()") changed the interrupt domain hierarchy,
> VMD's interrupt domain is not the top domain anymore. irq_get_chip_data()
> now returns the chip_data at the MSI devices' interrupt domains. It is
> therefore broken for vmd_msi_free() to kfree() this chip_data.
> 
> Fix this issue, correctly extract the chip_data associated with the VMD's
> interrupt domain.
> 
> Fixes: d7d8ab87e3e7 ("PCI: vmd: Switch to msi_create_parent_irq_domain()")
> Reported-by: Kenneth Crudup <kenny@panix.com>
> Closes: https://lore.kernel.org/linux-pci/dfa40e48-8840-4e61-9fda-25cdb3ad81c1@panix.com/
> Reported-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> Closes: https://lore.kernel.org/linux-pci/ed53280ed15d1140700b96cca2734bf327ee92539e5eb68e80f5bbbf0f01@linux.gnuweeb.org/
> Tested-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> Tested-by: Kenneth Crudup <kenny@panix.com>
> Signed-off-by: Nam Cao <namcao@linutronix.de>

Acked-by: Manivannan Sadhasivam <mani@kernel.org>

- Mani

> ---
> v2: Fix typo and describe the change more precisely
> ---
>  drivers/pci/controller/vmd.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 9bbb0ff4cc15..b679c7f28f51 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -280,10 +280,12 @@ static int vmd_msi_alloc(struct irq_domain *domain, unsigned int virq,
>  static void vmd_msi_free(struct irq_domain *domain, unsigned int virq,
>  			 unsigned int nr_irqs)
>  {
> +	struct irq_data *irq_data;
>  	struct vmd_irq *vmdirq;
>  
>  	for (int i = 0; i < nr_irqs; ++i) {
> -		vmdirq = irq_get_chip_data(virq + i);
> +		irq_data = irq_domain_get_irq_data(domain, virq + i);
> +		vmdirq = irq_data->chip_data;
>  
>  		synchronize_srcu(&vmdirq->irq->srcu);
>  
> -- 
> 2.39.5
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2] PCI: vmd: Fix wrong kfree() in vmd_msi_free()

[Thomas Gleixner]

On Thu, Aug 07 2025 at 10:10, Nam Cao wrote:
> vmd_msi_alloc() allocates struct vmd_irq and stashes it into
> irq_data->chip_data associated with the VMD's interrupt domain.
> vmd_msi_free() extracts the pointer by calling irq_get_chip_data() and
> frees it.
>
> irq_get_chip_data() returns the chip_data associated with the top interrupt
> domain. This worked in the past, because VMD's interrupt domain was the top
> domain.
>
> But since commit d7d8ab87e3e7 ("PCI: vmd: Switch to
> msi_create_parent_irq_domain()") changed the interrupt domain hierarchy,
> VMD's interrupt domain is not the top domain anymore. irq_get_chip_data()
> now returns the chip_data at the MSI devices' interrupt domains. It is
> therefore broken for vmd_msi_free() to kfree() this chip_data.
>
> Fix this issue, correctly extract the chip_data associated with the VMD's
> interrupt domain.
>
> Fixes: d7d8ab87e3e7 ("PCI: vmd: Switch to msi_create_parent_irq_domain()")
> Reported-by: Kenneth Crudup <kenny@panix.com>
> Closes: https://lore.kernel.org/linux-pci/dfa40e48-8840-4e61-9fda-25cdb3ad81c1@panix.com/
> Reported-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> Closes: https://lore.kernel.org/linux-pci/ed53280ed15d1140700b96cca2734bf327ee92539e5eb68e80f5bbbf0f01@linux.gnuweeb.org/
> Tested-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> Tested-by: Kenneth Crudup <kenny@panix.com>
> Signed-off-by: Nam Cao <namcao@linutronix.de>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Re: [PATCH v2] PCI: vmd: Fix wrong kfree() in vmd_msi_free()

[Bjorn Helgaas]

On Thu, Aug 07, 2025 at 10:10:51AM +0200, Nam Cao wrote:
> vmd_msi_alloc() allocates struct vmd_irq and stashes it into
> irq_data->chip_data associated with the VMD's interrupt domain.
> vmd_msi_free() extracts the pointer by calling irq_get_chip_data() and
> frees it.
> 
> irq_get_chip_data() returns the chip_data associated with the top interrupt
> domain. This worked in the past, because VMD's interrupt domain was the top
> domain.
> 
> But since commit d7d8ab87e3e7 ("PCI: vmd: Switch to
> msi_create_parent_irq_domain()") changed the interrupt domain hierarchy,
> VMD's interrupt domain is not the top domain anymore. irq_get_chip_data()
> now returns the chip_data at the MSI devices' interrupt domains. It is
> therefore broken for vmd_msi_free() to kfree() this chip_data.
> 
> Fix this issue, correctly extract the chip_data associated with the VMD's
> interrupt domain.
> 
> Fixes: d7d8ab87e3e7 ("PCI: vmd: Switch to msi_create_parent_irq_domain()")
> Reported-by: Kenneth Crudup <kenny@panix.com>
> Closes: https://lore.kernel.org/linux-pci/dfa40e48-8840-4e61-9fda-25cdb3ad81c1@panix.com/
> Reported-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> Closes: https://lore.kernel.org/linux-pci/ed53280ed15d1140700b96cca2734bf327ee92539e5eb68e80f5bbbf0f01@linux.gnuweeb.org/
> Tested-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> Tested-by: Kenneth Crudup <kenny@panix.com>
> Signed-off-by: Nam Cao <namcao@linutronix.de>

Applied to pci/for-linus for v6.17, thanks!

I assume you checked the other msi_create_parent_irq_domain() changes
for similar problems?

> ---
> v2: Fix typo and describe the change more precisely
> ---
>  drivers/pci/controller/vmd.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 9bbb0ff4cc15..b679c7f28f51 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -280,10 +280,12 @@ static int vmd_msi_alloc(struct irq_domain *domain, unsigned int virq,
>  static void vmd_msi_free(struct irq_domain *domain, unsigned int virq,
>  			 unsigned int nr_irqs)
>  {
> +	struct irq_data *irq_data;
>  	struct vmd_irq *vmdirq;
>  
>  	for (int i = 0; i < nr_irqs; ++i) {
> -		vmdirq = irq_get_chip_data(virq + i);
> +		irq_data = irq_domain_get_irq_data(domain, virq + i);
> +		vmdirq = irq_data->chip_data;
>  
>  		synchronize_srcu(&vmdirq->irq->srcu);
>  
> -- 
> 2.39.5
> 

Re: [PATCH v2] PCI: vmd: Fix wrong kfree() in vmd_msi_free()

[Nam Cao]

Bjorn Helgaas <helgaas@kernel.org> writes:
> On Thu, Aug 07, 2025 at 10:10:51AM +0200, Nam Cao wrote:
>> vmd_msi_alloc() allocates struct vmd_irq and stashes it into
>> irq_data->chip_data associated with the VMD's interrupt domain.
>> vmd_msi_free() extracts the pointer by calling irq_get_chip_data() and
>> frees it.
>> 
>> irq_get_chip_data() returns the chip_data associated with the top interrupt
>> domain. This worked in the past, because VMD's interrupt domain was the top
>> domain.
>> 
>> But since commit d7d8ab87e3e7 ("PCI: vmd: Switch to
>> msi_create_parent_irq_domain()") changed the interrupt domain hierarchy,
>> VMD's interrupt domain is not the top domain anymore. irq_get_chip_data()
>> now returns the chip_data at the MSI devices' interrupt domains. It is
>> therefore broken for vmd_msi_free() to kfree() this chip_data.
>> 
>> Fix this issue, correctly extract the chip_data associated with the VMD's
>> interrupt domain.
...
>
> Applied to pci/for-linus for v6.17, thanks!
>
> I assume you checked the other msi_create_parent_irq_domain() changes
> for similar problems?

Not before you reminded me :(

But yes, none of the similar PCI patches has the same problem.

Nam

Re: [PATCH v2] PCI: vmd: Fix wrong kfree() in vmd_msi_free()

[Bjorn Helgaas]

On Thu, Aug 07, 2025 at 07:43:17PM +0200, Nam Cao wrote:
> Bjorn Helgaas <helgaas@kernel.org> writes:
> > On Thu, Aug 07, 2025 at 10:10:51AM +0200, Nam Cao wrote:
> >> vmd_msi_alloc() allocates struct vmd_irq and stashes it into
> >> irq_data->chip_data associated with the VMD's interrupt domain.
> >> vmd_msi_free() extracts the pointer by calling irq_get_chip_data() and
> >> frees it.
> >> 
> >> irq_get_chip_data() returns the chip_data associated with the top interrupt
> >> domain. This worked in the past, because VMD's interrupt domain was the top
> >> domain.
> >> 
> >> But since commit d7d8ab87e3e7 ("PCI: vmd: Switch to
> >> msi_create_parent_irq_domain()") changed the interrupt domain hierarchy,
> >> VMD's interrupt domain is not the top domain anymore. irq_get_chip_data()
> >> now returns the chip_data at the MSI devices' interrupt domains. It is
> >> therefore broken for vmd_msi_free() to kfree() this chip_data.
> >> 
> >> Fix this issue, correctly extract the chip_data associated with the VMD's
> >> interrupt domain.
> ...
> >
> > Applied to pci/for-linus for v6.17, thanks!
> >
> > I assume you checked the other msi_create_parent_irq_domain() changes
> > for similar problems?
> 
> Not before you reminded me :(
> 
> But yes, none of the similar PCI patches has the same problem.

Great, thanks for checking!  I'll try to get this in before v6.17-rc1
since many people will start using that.

Bjorn