Skip to contentSkip to conversation

uio.c: solve memory leak

PATCH In review Cong Ding 3 participants 14 messages Nov 29, 2012 – Jul 25, 2013 Userspace I/O (UIO)

lore
Share & tools
lore

Cong Ding

author
#0 lore raw

PATCH v2 1/1 uio.c: solve memory leak

In version 1, I forgot to modify the same bug in the first loop.

we have to call kobject_put() to clean up the kobject after function kobject_init(), kobject_add(), or kobject_uevent() is called.

Signed-off-by Cong Ding <[email protected]>
---
 drivers/uio/uio.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)
drivers/uio/uio.c +10 -6
index 5110f36..79774d3 100644
@@ -291,10 +291,10 @@ static int uio_dev_add_attributes(struct uio_device *idev)
 		mem->map = map;
 		ret = kobject_add(&map->kobj, idev->map_dir, "map%d", mi);
 		if (ret)
-			goto err_map;
+			goto err_map_kobj;
 		ret = kobject_uevent(&map->kobj, KOBJ_ADD);
 		if (ret)
-			goto err_map;
+			goto err_map_kobj;
 	}
 
 	for (pi = 0; pi < MAX_UIO_PORT_REGIONS; pi++) {

@@ -317,23 +317,27 @@ static int uio_dev_add_attributes(struct uio_device *idev)
 		ret = kobject_add(&portio->kobj, idev->portio_dir,
 							"port%d", pi);
 		if (ret)
-			goto err_portio;
+			goto err_portio_kobj;
 		ret = kobject_uevent(&portio->kobj, KOBJ_ADD);
 		if (ret)
-			goto err_portio;
+			goto err_portio_kobj;
 	}
 
 	return 0;
 
 err_portio:
-	for (pi--; pi >= 0; pi--) {
+	pi--;
+err_portio_kobj:
+	for (; pi >= 0; pi--) {
 		port = &idev->info->port[pi];
 		portio = port->portio;
 		kobject_put(&portio->kobj);
 	}
 	kobject_put(idev->portio_dir);
 err_map:
-	for (mi--; mi>=0; mi--) {
+	mi--;
+err_map_kobj:
+	for (; mi >= 0; mi--) {
 		mem = &idev->info->mem[mi];
 		map = mem->map;
 		kobject_put(&map->kobj);
Signature · 1 line
1.7.4.5
#1 lore raw
Quoted 4 lines · Cong Ding

In version 1, I forgot to modify the same bug in the first loop.

we have to call kobject_put() to clean up the kobject after function kobject_init(), kobject_add(), or kobject_uevent() is called.

Yes, thanks. Incredible how that code looks like... There's still another bug: The memory allocated with kzalloc is never freed. More tomorrow, have to go to sleep now.

Thanks,
Hans

Quoted 59 lines
Signed-off-by Cong Ding <[email protected]>
---
 drivers/uio/uio.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)
drivers/uio/uio.c +10 -6
index 5110f36..79774d3 100644
@@ -291,10 +291,10 @@ static int uio_dev_add_attributes(struct uio_device *idev)
 		mem->map = map;
 		ret = kobject_add(&map->kobj, idev->map_dir, "map%d", mi);
 		if (ret)
-			goto err_map;
+			goto err_map_kobj;
 		ret = kobject_uevent(&map->kobj, KOBJ_ADD);
 		if (ret)
-			goto err_map;
+			goto err_map_kobj;
 	}
 
 	for (pi = 0; pi < MAX_UIO_PORT_REGIONS; pi++) {

@@ -317,23 +317,27 @@ static int uio_dev_add_attributes(struct uio_device *idev)
 		ret = kobject_add(&portio->kobj, idev->portio_dir,
 							"port%d", pi);
 		if (ret)
-			goto err_portio;
+			goto err_portio_kobj;
 		ret = kobject_uevent(&portio->kobj, KOBJ_ADD);
 		if (ret)
-			goto err_portio;
+			goto err_portio_kobj;
 	}
 
 	return 0;
 
 err_portio:
-	for (pi--; pi >= 0; pi--) {
+	pi--;
+err_portio_kobj:
+	for (; pi >= 0; pi--) {
 		port = &idev->info->port[pi];
 		portio = port->portio;
 		kobject_put(&portio->kobj);
 	}
 	kobject_put(idev->portio_dir);
 err_map:
-	for (mi--; mi>=0; mi--) {
+	mi--;
+err_map_kobj:
+	for (; mi >= 0; mi--) {
 		mem = &idev->info->mem[mi];
 		map = mem->map;
 		kobject_put(&map->kobj);
Signature · 1 line
1.7.4.5
author
#2 lore raw

Hi Hans, I think the memory allocated with kzalloc is properly freed by calling kobject_put.

I can give a simple explanation.

1) when we call kobject_init, the parameter portio_attr_type is passed in. portio_attr_type includes a function pointer to portio_release, which releases the memory of portio.

2) when we call kobject_put, kref_put is called with the pointer of function kobject_release.
3) kref_put calls kref_sub, with the same pointer of function kobject_release.
4) and kref_put calls the function kboject_release if atomic_sub_and_test returns true

5) let's look at what kobject_release is. it calls kobject_cleanup, and kobject_cleanup calls t->release(kobj) where t->release is exactly the function we passed in through portio_init at step (1). so function portio_release is called, and the memory allocated with kzalloc is freed.

If there are anything wrong in my analysis, please feel free to let me know.

Personally, I suggest to add a function to create and release uio_portio, which is similar as kobject_create and kobject_put in file lib/kobject.c. In this way, it avoid other readers thinking the memory is not freed (and we should add some comments here). For example, uio_portio_create call kzalloc and kboject_init, and returns uio_portio, which is similar as function kobject_create; and uio_portio_release calls kobject_put to release the memory. And we do same thing for uio_map.

The usage here is quite strange, but it works. If I write this function from zero, I will use a pointer to kobject in uio_portio struct instead of kobject struct itself. In this case I can call kobject_create instead of kobject_init, and then we do both kzalloc(uio_portio) and kfree(uio_portio) in the file uio.c.

Best,
Cong

Quoted 2 lines · Hans J. Koch

There's still another bug: The memory allocated with kzalloc is never freed.

author
#3 lore raw

ping Hans, did you have any comment on this?

- cong

Quoted 44 lines · Cong Ding

Hi Hans, I think the memory allocated with kzalloc is properly freed by calling kobject_put.

I can give a simple explanation.

1) when we call kobject_init, the parameter portio_attr_type is passed in. portio_attr_type includes a function pointer to portio_release, which releases the memory of portio.

2) when we call kobject_put, kref_put is called with the pointer of function kobject_release.
3) kref_put calls kref_sub, with the same pointer of function kobject_release.
4) and kref_put calls the function kboject_release if atomic_sub_and_test returns true

5) let's look at what kobject_release is. it calls kobject_cleanup, and kobject_cleanup calls t->release(kobj) where t->release is exactly the function we passed in through portio_init at step (1). so function portio_release is called, and the memory allocated with kzalloc is freed.

If there are anything wrong in my analysis, please feel free to let me know.

Personally, I suggest to add a function to create and release uio_portio, which is similar as kobject_create and kobject_put in file lib/kobject.c. In this way, it avoid other readers thinking the memory is not freed (and we should add some comments here). For example, uio_portio_create call kzalloc and kboject_init, and returns uio_portio, which is similar as function kobject_create; and uio_portio_release calls kobject_put to release the memory. And we do same thing for uio_map.

The usage here is quite strange, but it works. If I write this function from zero, I will use a pointer to kobject in uio_portio struct instead of kobject struct itself. In this case I can call kobject_create instead of kobject_init, and then we do both kzalloc(uio_portio) and kfree(uio_portio) in the file uio.c.

Best,
Cong

Quoted 2 lines · Hans J. Koch

There's still another bug: The memory allocated with kzalloc is never freed.

#4 lore raw
Quoted 1 line · Cong Ding

ping Hans, did you have any comment on this?

Sounds right what you say. Is your patch v2 your final solution, or would you like to come up with v3?

Thanks a lot for your patience and your thorough analysis.

Hans

Quoted 49 lines

- cong

Quoted 44 lines · Cong Ding

Hi Hans, I think the memory allocated with kzalloc is properly freed by calling kobject_put.

I can give a simple explanation.

1) when we call kobject_init, the parameter portio_attr_type is passed in. portio_attr_type includes a function pointer to portio_release, which releases the memory of portio.

2) when we call kobject_put, kref_put is called with the pointer of function kobject_release.
3) kref_put calls kref_sub, with the same pointer of function kobject_release.
4) and kref_put calls the function kboject_release if atomic_sub_and_test returns true

5) let's look at what kobject_release is. it calls kobject_cleanup, and kobject_cleanup calls t->release(kobj) where t->release is exactly the function we passed in through portio_init at step (1). so function portio_release is called, and the memory allocated with kzalloc is freed.

If there are anything wrong in my analysis, please feel free to let me know.

Personally, I suggest to add a function to create and release uio_portio, which is similar as kobject_create and kobject_put in file lib/kobject.c. In this way, it avoid other readers thinking the memory is not freed (and we should add some comments here). For example, uio_portio_create call kzalloc and kboject_init, and returns uio_portio, which is similar as function kobject_create; and uio_portio_release calls kobject_put to release the memory. And we do same thing for uio_map.

The usage here is quite strange, but it works. If I write this function from zero, I will use a pointer to kobject in uio_portio struct instead of kobject struct itself. In this case I can call kobject_create instead of kobject_init, and then we do both kzalloc(uio_portio) and kfree(uio_portio) in the file uio.c.

Best,
Cong

Quoted 2 lines · Hans J. Koch

There's still another bug: The memory allocated with kzalloc is never freed.

author
#5 lore raw
Quoted 7 lines · Hans J. Koch
Quoted 1 line · Cong Ding

ping Hans, did you have any comment on this?

Sounds right what you say. Is your patch v2 your final solution, or would you like to come up with v3?

Thanks a lot for your patience and your thorough analysis.

If you don't have more comment or objection, it would be the final version.

Thanks

- cong

Quoted 46 lines
Quoted 46 lines · Cong Ding
Quoted 44 lines

Hi Hans, I think the memory allocated with kzalloc is properly freed by calling kobject_put.

I can give a simple explanation.

1) when we call kobject_init, the parameter portio_attr_type is passed in. portio_attr_type includes a function pointer to portio_release, which releases the memory of portio.

2) when we call kobject_put, kref_put is called with the pointer of function kobject_release.
3) kref_put calls kref_sub, with the same pointer of function kobject_release.
4) and kref_put calls the function kboject_release if atomic_sub_and_test returns true

5) let's look at what kobject_release is. it calls kobject_cleanup, and kobject_cleanup calls t->release(kobj) where t->release is exactly the function we passed in through portio_init at step (1). so function portio_release is called, and the memory allocated with kzalloc is freed.

If there are anything wrong in my analysis, please feel free to let me know.

Personally, I suggest to add a function to create and release uio_portio, which is similar as kobject_create and kobject_put in file lib/kobject.c. In this way, it avoid other readers thinking the memory is not freed (and we should add some comments here). For example, uio_portio_create call kzalloc and kboject_init, and returns uio_portio, which is similar as function kobject_create; and uio_portio_release calls kobject_put to release the memory. And we do same thing for uio_map.

The usage here is quite strange, but it works. If I write this function from zero, I will use a pointer to kobject in uio_portio struct instead of kobject struct itself. In this case I can call kobject_create instead of kobject_init, and then we do both kzalloc(uio_portio) and kfree(uio_portio) in the file uio.c.

Best,
Cong

Quoted 2 lines · Hans J. Koch

There's still another bug: The memory allocated with kzalloc is never freed.

#6 lore raw
In reply to Cong Ding
Quoted 6 lines · Cong Ding

In version 1, I forgot to modify the same bug in the first loop.

we have to call kobject_put() to clean up the kobject after function kobject_init(), kobject_add(), or kobject_uevent() is called.

Signed-off-by Cong Ding <[email protected]>
Signed-off-by "Hans J. Koch" <[email protected]>
Quoted 57 lines
---
 drivers/uio/uio.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)
drivers/uio/uio.c +10 -6
index 5110f36..79774d3 100644
@@ -291,10 +291,10 @@ static int uio_dev_add_attributes(struct uio_device *idev)
 		mem->map = map;
 		ret = kobject_add(&map->kobj, idev->map_dir, "map%d", mi);
 		if (ret)
-			goto err_map;
+			goto err_map_kobj;
 		ret = kobject_uevent(&map->kobj, KOBJ_ADD);
 		if (ret)
-			goto err_map;
+			goto err_map_kobj;
 	}
 
 	for (pi = 0; pi < MAX_UIO_PORT_REGIONS; pi++) {

@@ -317,23 +317,27 @@ static int uio_dev_add_attributes(struct uio_device *idev)
 		ret = kobject_add(&portio->kobj, idev->portio_dir,
 							"port%d", pi);
 		if (ret)
-			goto err_portio;
+			goto err_portio_kobj;
 		ret = kobject_uevent(&portio->kobj, KOBJ_ADD);
 		if (ret)
-			goto err_portio;
+			goto err_portio_kobj;
 	}
 
 	return 0;
 
 err_portio:
-	for (pi--; pi >= 0; pi--) {
+	pi--;
+err_portio_kobj:
+	for (; pi >= 0; pi--) {
 		port = &idev->info->port[pi];
 		portio = port->portio;
 		kobject_put(&portio->kobj);
 	}
 	kobject_put(idev->portio_dir);
 err_map:
-	for (mi--; mi>=0; mi--) {
+	mi--;
+err_map_kobj:
+	for (; mi >= 0; mi--) {
 		mem = &idev->info->mem[mi];
 		map = mem->map;
 		kobject_put(&map->kobj);
Signature · 1 line
1.7.4.5
author
#7 lore raw
Quoted 9 lines · Hans J. Koch
Quoted 6 lines · Cong Ding

In version 1, I forgot to modify the same bug in the first loop.

we have to call kobject_put() to clean up the kobject after function kobject_init(), kobject_add(), or kobject_uevent() is called.

Signed-off-by Cong Ding <[email protected]>
Signed-off-by "Hans J. Koch" <[email protected]>

Hi Greg, is this patch stil in the queue?

- cong

Quoted 57 lines
Quoted 57 lines
---
 drivers/uio/uio.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)
drivers/uio/uio.c +10 -7
index 5110f36..79774d3 100644
@@ -291,10 +291,10 @@ static int uio_dev_add_attributes(struct uio_device *idev)
              mem->map = map;
              ret = kobject_add(&map->kobj, idev->map_dir, "map%d", mi);
              if (ret)
-                     goto err_map;
+                     goto err_map_kobj;
              ret = kobject_uevent(&map->kobj, KOBJ_ADD);
              if (ret)
-                     goto err_map;
+                     goto err_map_kobj;
      }
 
      for (pi = 0; pi < MAX_UIO_PORT_REGIONS; pi++) {

@@ -317,23 +317,27 @@ static int uio_dev_add_attributes(struct uio_device *idev)
              ret = kobject_add(&portio->kobj, idev->portio_dir,
                                                      "port%d", pi);
              if (ret)
-                     goto err_portio;
+                     goto err_portio_kobj;
              ret = kobject_uevent(&portio->kobj, KOBJ_ADD);
              if (ret)
-                     goto err_portio;
+                     goto err_portio_kobj;
      }
 
      return 0;
 
 err_portio:
-     for (pi--; pi >= 0; pi--) {
+     pi--;
+err_portio_kobj:
+     for (; pi >= 0; pi--) {
              port = &idev->info->port[pi];
              portio = port->portio;
              kobject_put(&portio->kobj);
      }
      kobject_put(idev->portio_dir);
 err_map:
-     for (mi--; mi>=0; mi--) {
+     mi--;
+err_map_kobj:
+     for (; mi >= 0; mi--) {
              mem = &idev->info->mem[mi];
              map = mem->map;
              kobject_put(&map->kobj);
--

1.7.4.5

maintainer
#8 lore raw
Quoted 4 lines · Cong Ding
Quoted 3 lines · Hans J. Koch
Quoted 2 lines · Cong Ding

In version 1, I forgot to modify the same bug in the first loop.

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

we have to call kobject_put() to clean up the kobject after function kobject_init(), kobject_add(), or kobject_uevent() is called.

Signed-off-by Cong Ding <[email protected]>
Signed-off-by "Hans J. Koch" <[email protected]>

Hi Greg, is this patch stil in the queue?

Hans is queueing up UIO patches to send to me, I'm waiting for him to send them as I don't have any in my trees.

thanks,

greg k-h

#9 lore raw
Quoted 8 lines · Greg Kroah-Hartman
Quoted 7 lines · Cong Ding
Quoted 6 lines · Hans J. Koch
Quoted 5 lines · Cong Ding

In version 1, I forgot to modify the same bug in the first loop.

we have to call kobject_put() to clean up the kobject after function kobject_init(), kobject_add(), or kobject_uevent() is called.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Cong Ding <[email protected]>
Signed-off-by "Hans J. Koch" <[email protected]>

Hi Greg, is this patch stil in the queue?

Hans is queueing up UIO patches to send to me, I'm waiting for him to send them as I don't have any in my trees.

I'll set that up tonight or tomorrow. Sorry, I was delayed by illness and a lot of other work.

Thanks,
Hans

Quoted 5 lines

thanks,

greg k-h

author
#10 lore raw
Quoted 13 lines · Hans J. Koch
Quoted 12 lines · Greg Kroah-Hartman
Quoted 11 lines · Cong Ding
Quoted 9 lines · Hans J. Koch
Quoted 6 lines · Cong Ding

In version 1, I forgot to modify the same bug in the first loop.

we have to call kobject_put() to clean up the kobject after function kobject_init(), kobject_add(), or kobject_uevent() is called.

Signed-off-by Cong Ding <[email protected]>
Signed-off-by "Hans J. Koch" <[email protected]>
Quoted 7 lines
Quoted 4 lines
Quoted 1 line

Hi Greg, is this patch stil in the queue?

Hans is queueing up UIO patches to send to me, I'm waiting for him to send them as I don't have any in my trees.

I'll set that up tonight or tomorrow. Sorry, I was delayed by illness and a lot of other work.

Hi Hans, what's this going on?

Thanks, - cong

author
#11 lore raw
Quoted 16 lines · Cong Ding
Quoted 15 lines · Hans J. Koch
Quoted 14 lines · Greg Kroah-Hartman
Quoted 12 lines · Cong Ding
Quoted 9 lines · Hans J. Koch
Quoted 6 lines · Cong Ding

In version 1, I forgot to modify the same bug in the first loop.

we have to call kobject_put() to clean up the kobject after function kobject_init(), kobject_add(), or kobject_uevent() is called.

Signed-off-by Cong Ding <[email protected]>
Signed-off-by "Hans J. Koch" <[email protected]>

Hi Greg, is this patch stil in the queue?

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

Hans is queueing up UIO patches to send to me, I'm waiting for him to send them as I don't have any in my trees.

I'll set that up tonight or tomorrow. Sorry, I was delayed by illness and a lot of other work.

Hi Hans, what's this going on?

Thanks, - cong

ping

author
#12 lore raw
Quoted 17 lines · Cong Ding
Quoted 16 lines · Cong Ding
Quoted 15 lines · Hans J. Koch
Quoted 14 lines · Greg Kroah-Hartman
Quoted 12 lines · Cong Ding
Quoted 9 lines · Hans J. Koch
Quoted 6 lines · Cong Ding

In version 1, I forgot to modify the same bug in the first loop.

we have to call kobject_put() to clean up the kobject after function kobject_init(), kobject_add(), or kobject_uevent() is called.

Signed-off-by Cong Ding <[email protected]>
Signed-off-by "Hans J. Koch" <[email protected]>

Hi Greg, is this patch stil in the queue?

Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

Hans is queueing up UIO patches to send to me, I'm waiting for him to send them as I don't have any in my trees.

I'll set that up tonight or tomorrow. Sorry, I was delayed by illness and a lot of other work.

Hi Hans, what's this going on?

Thanks, - cong

Hi Hans, it's another month. What's the progress of this patch?
- cong

author
#13 lore raw
Quoted 21 lines · Cong Ding
Quoted 20 lines · Cong Ding
Quoted 19 lines · Cong Ding
Quoted 18 lines · Hans J. Koch
Quoted 16 lines · Greg Kroah-Hartman
Quoted 4 lines · Cong Ding
Quoted 3 lines · Hans J. Koch
Quoted 2 lines · Cong Ding

In version 1, I forgot to modify the same bug in the first loop.

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

we have to call kobject_put() to clean up the kobject after function kobject_init(), kobject_add(), or kobject_uevent() is called.

Signed-off-by Cong Ding <[email protected]>
Signed-off-by "Hans J. Koch" <[email protected]>

Hi Greg, is this patch stil in the queue?

Hans is queueing up UIO patches to send to me, I'm waiting for him to send them as I don't have any in my trees.

Quoted 6 lines
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines

I'll set that up tonight or tomorrow. Sorry, I was delayed by illness and a lot of other work.

Hi Hans, what's this going on?

Thanks, - cong

Hi Hans, it's another month. What's the progress of this patch?

still no action to this this patch? it was signed-off by Hans 7 months ago!
- cong