2009-07-31 08:56:50

by Xiaotian Feng

[permalink] [raw]
Subject: [PATCH 0/4] gpu/drm: fix memory leak when radeon_driver_load_kms fails

I've noticed a bundle of kmemleak reports on drm. When I look into dmesg,
I found: "[drm:radeon_driver_load_kms] *ERROR* Failed to initialize radeon,
disabling IOCTL".

In radeon_driver_load_kms, if radeon_device_init fails, rdev is not freed.
Then radeon_driver_load_kms as dev->driver->load, if load fails in
drm_get_dev, memories allocated in drm_fill_in_dev will not be freed.

This patchset fixes those memory leaks, after this patchset applied, I didn't
see those drm kmemleak report.


2009-07-31 08:56:23

by Xiaotian Feng

[permalink] [raw]
Subject: [PATCH 1/4] gpu/drm/radeon: fix memory leak in radeon_driver_load_kms

If radeon_device_init fails, radeon_driver_load_kms doesn't release
memory of rdev. This causes kmemleak reports:

unreferenced object 0xffff88022cb53000 (size 4096):
comm "work_for_cpu", pid 97, jiffies 4294672345
backtrace:
[<ffffffff810eb222>] create_object+0x19f/0x2a0
[<ffffffff810eb422>] kmemleak_alloc+0x26/0x4c
[<ffffffff810e363f>] __kmalloc+0x187/0x1b0
[<ffffffffa005f3db>] kzalloc.clone.0+0x13/0x15 [radeon]
[<ffffffffa005f403>] radeon_driver_load_kms+0x26/0xe1 [radeon]
[<ffffffffa0017432>] drm_get_dev+0x37f/0x480 [drm]
[<ffffffffa007f424>] radeon_pci_probe+0x15/0x269 [radeon]
[<ffffffff811f8779>] local_pci_probe+0x17/0x1b
[<ffffffff8105ffbb>] do_work_for_cpu+0x18/0x2a
[<ffffffff81063c38>] kthread+0x8a/0x92
[<ffffffff81012cba>] child_rip+0xa/0x20
[<ffffffffffffffff>] 0xffffffffffffffff

Signed-off-by: Xiaotian Feng <[email protected]>
---
drivers/gpu/drm/radeon/radeon_kms.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
index 4612a7c..df3fdbe 100644
--- a/drivers/gpu/drm/radeon/radeon_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_kms.c
@@ -58,6 +58,8 @@ int radeon_driver_load_kms(struct drm_device *dev, unsigned long flags)
if (r) {
DRM_ERROR("Failed to initialize radeon, disabling IOCTL\n");
radeon_device_fini(rdev);
+ kfree(rdev);
+ dev->dev_private = NULL;
return r;
}
return 0;
--
1.6.2.5

2009-07-31 08:56:32

by Xiaotian Feng

[permalink] [raw]
Subject: [PATCH 4/4] gpu/drm: fix memory leak if dev->driver->load fails

Noticed a bundle of memory leaks after radeon_driver_load_kms fails.
After dev->driver->load failes, drm_get_dev doesn't free memories
allocated from drm_fill_in_dev.

Signed-off-by: Xiaotian Feng <[email protected]>
---
drivers/gpu/drm/drm_stub.c | 14 ++++++++------
1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index fe7f5c4..db9057c 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -448,23 +448,23 @@ int drm_get_dev(struct pci_dev *pdev, const struct pci_device_id *ent,
pci_set_drvdata(pdev, dev);
ret = drm_get_minor(dev, &dev->control, DRM_MINOR_CONTROL);
if (ret)
- goto err_g2;
+ goto err_g3;
}

if ((ret = drm_get_minor(dev, &dev->primary, DRM_MINOR_LEGACY)))
- goto err_g3;
+ goto err_g4;

if (dev->driver->load) {
ret = dev->driver->load(dev, ent->driver_data);
if (ret)
- goto err_g4;
+ goto err_g5;
}

/* setup the grouping for the legacy output */
if (drm_core_check_feature(dev, DRIVER_MODESET)) {
ret = drm_mode_group_init_legacy_group(dev, &dev->primary->mode_group);
if (ret)
- goto err_g4;
+ goto err_g5;
}

list_add_tail(&dev->driver_item, &driver->device_list);
@@ -475,11 +475,13 @@ int drm_get_dev(struct pci_dev *pdev, const struct pci_device_id *ent,

return 0;

-err_g4:
+err_g5:
drm_put_minor(&dev->primary);
-err_g3:
+err_g4:
if (drm_core_check_feature(dev, DRIVER_MODESET))
drm_put_minor(&dev->control);
+err_g3:
+ drm_cleanup_dev(dev);
err_g2:
pci_disable_device(pdev);
err_g1:
--
1.6.2.5

2009-07-31 08:56:36

by Xiaotian Feng

[permalink] [raw]
Subject: [PATCH 2/4] gpu/drm: fix memory leak in drm_fill_in_dev fail path

In drm_fill_in_dev, drm_ht_create/drm_agp_init/drm_gem_init all allocate
memories. but in the fail path, those memory is not freed, then memory
leaks is resulted.

Signed-off-by: Xiaotian Feng <[email protected]>
---
drivers/gpu/drm/drm_stub.c | 25 ++++++++++++++++++++-----
1 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 55bb8a8..8ac1ddb 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -259,7 +259,7 @@ static int drm_fill_in_dev(struct drm_device * dev, struct pci_dev *pdev,
&& (dev->agp == NULL)) {
DRM_ERROR("Cannot initialize the agpgart module.\n");
retcode = -EINVAL;
- goto error_out_unreg;
+ goto error_agp;
}
if (drm_core_has_MTRR(dev)) {
if (dev->agp)
@@ -274,7 +274,7 @@ static int drm_fill_in_dev(struct drm_device * dev, struct pci_dev *pdev,
retcode = drm_ctxbitmap_init(dev);
if (retcode) {
DRM_ERROR("Cannot allocate memory for context bitmap.\n");
- goto error_out_unreg;
+ goto error_ctxbitmap;
}

if (driver->driver_features & DRIVER_GEM) {
@@ -282,14 +282,29 @@ static int drm_fill_in_dev(struct drm_device * dev, struct pci_dev *pdev,
if (retcode) {
DRM_ERROR("Cannot initialize graphics execution "
"manager (GEM)\n");
- goto error_out_unreg;
+ goto error_gem;
}
}

return 0;

- error_out_unreg:
- drm_lastclose(dev);
+error_gem:
+ drm_ctxbitmap_cleanup(dev);
+error_ctxbitmap:
+ if (drm_core_has_MTRR(dev) && drm_core_has_AGP(dev) &&
+ dev->agp && dev->agp->agp_mtrr >= 0) {
+ int retval;
+ retval = mtrr_del(dev->agp->agp_mtrr,
+ dev->agp->agp_info.aper_base,
+ dev->agp->agp_info.aper_size * 1024 * 1024);
+ DRM_DEBUG("mtrr_del=%d\n", retval);
+ }
+ if (drm_core_has_AGP(dev) && dev->agp) {
+ kfree(dev->agp);
+ dev->agp = NULL;
+ }
+error_agp:
+ drm_ht_remove(&dev->map_hash);
return retcode;
}

--
1.6.2.5

2009-07-31 08:56:52

by Xiaotian Feng

[permalink] [raw]
Subject: [PATCH 3/4] gpu/drm: introduce drm_cleanup_dev to cleanup memories allocated from drm_fill_in_dev

drm_fill_in_dev may allocate memories for agp/gem/ht, introduce new function
drm_cleanup_dev(derived from drm_put_dev) to cleanup what drm_fill_in_dev.

Signed-off-by: Xiaotian Feng <[email protected]>
---
drivers/gpu/drm/drm_stub.c | 47 +++++++++++++++++++++++++------------------
1 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 8ac1ddb..fe7f5c4 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -308,6 +308,31 @@ error_agp:
return retcode;
}

+static int drm_cleanup_dev(struct drm_device *dev)
+{
+ drm_ctxbitmap_cleanup(dev);
+
+ if (drm_core_has_MTRR(dev) && drm_core_has_AGP(dev) &&
+ dev->agp && dev->agp->agp_mtrr >= 0) {
+ int retval;
+ retval = mtrr_del(dev->agp->agp_mtrr,
+ dev->agp->agp_info.aper_base,
+ dev->agp->agp_info.aper_size * 1024 * 1024);
+ DRM_DEBUG("mtrr_del=%d\n", retval);
+ }
+
+ if (drm_core_has_AGP(dev) && dev->agp) {
+ kfree(dev->agp);
+ dev->agp = NULL;
+ }
+
+ drm_ht_remove(&dev->map_hash);
+
+ if (dev->driver->driver_features & DRIVER_GEM)
+ drm_gem_destroy(dev);
+
+ return 0;
+}

/**
* Get a secondary minor number.
@@ -519,37 +544,19 @@ void drm_put_dev(struct drm_device *dev)

drm_lastclose(dev);

- if (drm_core_has_MTRR(dev) && drm_core_has_AGP(dev) &&
- dev->agp && dev->agp->agp_mtrr >= 0) {
- int retval;
- retval = mtrr_del(dev->agp->agp_mtrr,
- dev->agp->agp_info.aper_base,
- dev->agp->agp_info.aper_size * 1024 * 1024);
- DRM_DEBUG("mtrr_del=%d\n", retval);
- }
-
if (dev->driver->unload)
dev->driver->unload(dev);

- if (drm_core_has_AGP(dev) && dev->agp) {
- kfree(dev->agp);
- dev->agp = NULL;
- }
-
list_for_each_entry_safe(r_list, list_temp, &dev->maplist, head)
drm_rmmap(dev, r_list->map);
- drm_ht_remove(&dev->map_hash);
-
- drm_ctxbitmap_cleanup(dev);

if (drm_core_check_feature(dev, DRIVER_MODESET))
drm_put_minor(&dev->control);

- if (driver->driver_features & DRIVER_GEM)
- drm_gem_destroy(dev);
-
drm_put_minor(&dev->primary);

+ drm_cleanup_dev(dev);
+
if (dev->devname) {
kfree(dev->devname);
dev->devname = NULL;
--
1.6.2.5