Skip to contentSkip to conversation

dmaengine: idxd: Fix memory leak in idxd_alloc()

PATCH Applied Rafael Mendonca 4 participants 7 messages Sep 14, 2022 – Sep 29, 2022

Subsystems (2)
dmaengine Intel IDXD driver
lore
Share & tools
lore

Rafael Mendonca

author
#0 lore raw

PATCH 1/1 dmaengine: idxd: Fix memory leak in idxd_alloc()

If the IDA id allocation fails, then the allocated memory for the idxd_device struct doesn't get freed before returning NULL, which leads to a memleak.

Fixes 47c16ac27d4c ("dmaengine: idxd: fix idxd conf_dev 'struct device' lifetime")
Signed-off-by Rafael Mendonca <[email protected]>
---
 drivers/dma/idxd/init.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/dma/idxd/init.c +3 -1
index aa3478257ddb..fdc97519b8fb 100644
@@ -445,8 +445,10 @@ static struct idxd_device *idxd_alloc(struct pci_dev *pdev, struct idxd_driver_d
 	idxd->data = data;
 	idxd_dev_set_type(&idxd->idxd_dev, idxd->data->type);
 	idxd->id = ida_alloc(&idxd_ida, GFP_KERNEL);
-	if (idxd->id < 0)
+	if (idxd->id < 0) {
+		kfree(idxd);
 		return NULL;
+	}
 
 	device_initialize(conf_dev);
 	conf_dev->parent = dev;
Signature · 1 line
2.34.1
#1 lore raw
Quoted 6 lines · Rafael Mendonca

If the IDA id allocation fails, then the allocated memory for the idxd_device struct doesn't get freed before returning NULL, which leads to a memleak.

Fixes 47c16ac27d4c ("dmaengine: idxd: fix idxd conf_dev 'struct device' lifetime")
Signed-off-by Rafael Mendonca <[email protected]>

I think there needs to be a kfree(idxd) where it checks rc < 0 after the call to dev_set_name() as well, yes?

Regards,
Jerry

Quoted 23 lines
---
 drivers/dma/idxd/init.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/dma/idxd/init.c +3 -1
index aa3478257ddb..fdc97519b8fb 100644
@@ -445,8 +445,10 @@ static struct idxd_device *idxd_alloc(struct pci_dev *pdev, struct idxd_driver_d
 	idxd->data = data;
 	idxd_dev_set_type(&idxd->idxd_dev, idxd->data->type);
 	idxd->id = ida_alloc(&idxd_ida, GFP_KERNEL);
-	if (idxd->id < 0)
+	if (idxd->id < 0) {
+		kfree(idxd);
 		return NULL;
+	}
 
 	device_initialize(conf_dev);
 	conf_dev->parent = dev;
Signature · 1 line
2.34.1
reviewer
#2 lore raw
Quoted 8 lines · Jerry Snitselaar
Quoted 6 lines · Rafael Mendonca

If the IDA id allocation fails, then the allocated memory for the idxd_device struct doesn't get freed before returning NULL, which leads to a memleak.

Fixes 47c16ac27d4c ("dmaengine: idxd: fix idxd conf_dev 'struct device' lifetime")
Signed-off-by Rafael Mendonca <[email protected]>

I think there needs to be a kfree(idxd) where it checks rc < 0 after the call to dev_set_name() as well, yes?

The idxd_conf_device_release() should take care of freeing idxd with the put_device(). So I think we are good here.

Quoted 27 lines

Regards,
Jerry

Quoted 23 lines
---
  drivers/dma/idxd/init.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
drivers/dma/idxd/init.c +3 -1
index aa3478257ddb..fdc97519b8fb 100644
@@ -445,8 +445,10 @@ static struct idxd_device *idxd_alloc(struct pci_dev *pdev, struct idxd_driver_d
  	idxd->data = data;
  	idxd_dev_set_type(&idxd->idxd_dev, idxd->data->type);
  	idxd->id = ida_alloc(&idxd_ida, GFP_KERNEL);
-	if (idxd->id < 0)
+	if (idxd->id < 0) {
+		kfree(idxd);
  		return NULL;
+	}
 
  	device_initialize(conf_dev);
  	conf_dev->parent = dev;
Signature · 1 line
2.34.1
#3 lore raw
Quoted 7 lines · Dave Jiang
Quoted 5 lines · Jerry Snitselaar
Quoted 4 lines · Rafael Mendonca

If the IDA id allocation fails, then the allocated memory for the idxd_device struct doesn't get freed before returning NULL, which leads to a memleak.

Quoted 5 lines
Quoted 3 lines
Quoted 2 lines
Fixes 47c16ac27d4c ("dmaengine: idxd: fix idxd conf_dev 'struct device' lifetime")
Signed-off-by Rafael Mendonca <[email protected]>

I think there needs to be a kfree(idxd) where it checks rc < 0 after the call to dev_set_name() as well, yes?

The idxd_conf_device_release() should take care of freeing idxd with the put_device(). So I think we are good here.

Ah, right. Thanks.

Jerry

Quoted 26 lines
Quoted 26 lines

Regards,
Jerry

Quoted 22 lines
---
  drivers/dma/idxd/init.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
drivers/dma/idxd/init.c +3 -1
index aa3478257ddb..fdc97519b8fb 100644
@@ -445,8 +445,10 @@ static struct idxd_device *idxd_alloc(struct pci_dev *pdev, struct idxd_driver_d
  	idxd->data = data;
  	idxd_dev_set_type(&idxd->idxd_dev, idxd->data->type);
  	idxd->id = ida_alloc(&idxd_ida, GFP_KERNEL);
-	if (idxd->id < 0)
+	if (idxd->id < 0) {
+		kfree(idxd);
  		return NULL;
+	}
  	device_initialize(conf_dev);
  	conf_dev->parent = dev;
Signature · 1 line
2.34.1
reviewer Reviewed-by on v1 1/1
#4 lore raw
In reply to Rafael Mendonca
Quoted 6 lines · Rafael Mendonca

If the IDA id allocation fails, then the allocated memory for the idxd_device struct doesn't get freed before returning NULL, which leads to a memleak.

Fixes 47c16ac27d4c ("dmaengine: idxd: fix idxd conf_dev 'struct device' lifetime")
Signed-off-by Rafael Mendonca <[email protected]>

Thanks!

Reviewed-by Dave Jiang <[email protected]>
Quoted 20 lines
---
  drivers/dma/idxd/init.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
drivers/dma/idxd/init.c +3 -1
index aa3478257ddb..fdc97519b8fb 100644
@@ -445,8 +445,10 @@ static struct idxd_device *idxd_alloc(struct pci_dev *pdev, struct idxd_driver_d
  	idxd->data = data;
  	idxd_dev_set_type(&idxd->idxd_dev, idxd->data->type);
  	idxd->id = ida_alloc(&idxd_ida, GFP_KERNEL);
-	if (idxd->id < 0)
+	if (idxd->id < 0) {
+		kfree(idxd);
  		return NULL;
+	}
 
  	device_initialize(conf_dev);
  	conf_dev->parent = dev;
Reviewed-by on v1 1/1
#5 lore raw
In reply to Rafael Mendonca
Quoted 6 lines · Rafael Mendonca

If the IDA id allocation fails, then the allocated memory for the idxd_device struct doesn't get freed before returning NULL, which leads to a memleak.

Fixes 47c16ac27d4c ("dmaengine: idxd: fix idxd conf_dev 'struct device' lifetime")
Signed-off-by Rafael Mendonca <[email protected]>
Reviewed-by Jerry Snitselaar <[email protected]
Quoted 23 lines
---
 drivers/dma/idxd/init.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/dma/idxd/init.c +3 -1
index aa3478257ddb..fdc97519b8fb 100644
@@ -445,8 +445,10 @@ static struct idxd_device *idxd_alloc(struct pci_dev *pdev, struct idxd_driver_d
 	idxd->data = data;
 	idxd_dev_set_type(&idxd->idxd_dev, idxd->data->type);
 	idxd->id = ida_alloc(&idxd_ida, GFP_KERNEL);
-	if (idxd->id < 0)
+	if (idxd->id < 0) {
+		kfree(idxd);
 		return NULL;
+	}
 
 	device_initialize(conf_dev);
 	conf_dev->parent = dev;
Signature · 1 line
2.34.1
maintainer Applied
#6 lore raw
In reply to Rafael Mendonca
Quoted 3 lines · Rafael Mendonca

If the IDA id allocation fails, then the allocated memory for the idxd_device struct doesn't get freed before returning NULL, which leads to a memleak.

Applied, thanks

Signature · 1 line
~Vinod