RE: Fix memory leak in binder

RE: Fix memory leak in binder

[Tu, Xiaobing]

-----Original Message-----
From: Tu, Xiaobing 
Sent: Monday, October 15, 2012 3:03 PM
To: 'ccross@android.com'; 'davej@redhat.com'; 'akpm@linux-foundation.org'; 'mingo@elte.hu'; 'rusty@rustcorp.com.au'; 'a.p.zijlstra@chello.nl'; 'linux-kernel@vger.kernel.org'; 'rostedt@goodmis.org'
Cc: Zhang, Di; Ma, Xindong
Subject: Fix memory leak in binder

After enabling kmemleak and run monkey, following memleak is reported:
unreferenced object 0xeed27f80 (size 64):
  comm "Binder_8", pid 641, jiffies 4294946341 (age 2275.810s)
  hex dump (first 32 bytes):
    4f dd 00 00 84 7f d2 ee 84 7f d2 ee 01 00 00 00  O...............
    00 00 00 00 00 00 00 00 00 aa 4c d7 00 00 00 00  ..........L.....
  backtrace:
    [<c184fabc>] kmemleak_alloc+0x3c/0xa0
    [<c12f391e>] kmem_cache_alloc_trace+0x9e/0x180
    [<c1668bb5>] binder_thread_write+0xcf5/0x23a0
    [<c166b091>] binder_ioctl+0x1f1/0x530
    [<c130dcf6>] do_vfs_ioctl+0x86/0x5e0
    [<c130e282>] sys_ioctl+0x32/0x60
    [<c1872e01>] syscall_call+0x7/0xb
    [<ffffffff>] 0xffffffff

The work item in async_todo list is not freed when binder released.
Also the async transaction should also be freed in binder_release_work.

Signed-off-by: Leon Ma <xindong.ma@intel.com>
Signed-off-by: Di Zhang <di.zhang@intel.com>
---
 drivers/staging/android/binder.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 7df2a89..d23de68 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -1281,6 +1281,7 @@ static void binder_send_failed_reply(struct binder_transaction *t,
 					"already\n", target_thread->proc->pid,
 					target_thread->pid,
 					target_thread->return_error);
+				kfree(t);
 			}
 			return;
 		} else {
@@ -2509,6 +2510,11 @@ static void binder_release_work(struct list_head *list)
 			t = container_of(w, struct binder_transaction, work);
 			if (t->buffer->target_node && !(t->flags & TF_ONE_WAY))
 				binder_send_failed_reply(t, BR_DEAD_REPLY);
+			else {
+				t->buffer->transaction = NULL;
+				kfree(t);
+				binder_stats_deleted(BINDER_STAT_TRANSACTION);
+			}
 		} break;
 		case BINDER_WORK_TRANSACTION_COMPLETE: {
 			kfree(w);
@@ -2982,6 +2988,7 @@ static void binder_deferred_release(struct binder_proc *proc)
 
 		nodes++;
 		rb_erase(&node->rb_node, &proc->nodes);
+		binder_release_work(&node->async_todo);
 		list_del_init(&node->work.entry);
 		if (hlist_empty(&node->refs)) {
 			kfree(node);
-- 
1.7.6

Br
XiaoBing Tu
PSI@System Integration Shanghai

Fix memory leak in binder--version2

[Tu, Xiaobing]

After enabling kmemleak and run monkey, following memleak is reported:
unreferenced object 0xeed27f80 (size 64):
  comm "Binder_8", pid 641, jiffies 4294946341 (age 2275.810s)
  hex dump (first 32 bytes):
    4f dd 00 00 84 7f d2 ee 84 7f d2 ee 01 00 00 00  O...............
    00 00 00 00 00 00 00 00 00 aa 4c d7 00 00 00 00  ..........L.....
  backtrace:
    [<c184fabc>] kmemleak_alloc+0x3c/0xa0
    [<c12f391e>] kmem_cache_alloc_trace+0x9e/0x180
    [<c1668bb5>] binder_thread_write+0xcf5/0x23a0
    [<c166b091>] binder_ioctl+0x1f1/0x530
    [<c130dcf6>] do_vfs_ioctl+0x86/0x5e0
    [<c130e282>] sys_ioctl+0x32/0x60
    [<c1872e01>] syscall_call+0x7/0xb
    [<ffffffff>] 0xffffffff

The work item in async_todo list is not freed when binder released.
Also the async transaction should also be freed in binder_release_work.

Signed-off-by: Leon Ma <xindong.ma@intel.com>
Signed-off-by: Di Zhang <di.zhang@intel.com>
---
 drivers/staging/android/binder.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 7df2a89..022c9f8 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -2509,6 +2509,11 @@ static void binder_release_work(struct list_head *list)
 			t = container_of(w, struct binder_transaction, work);
 			if (t->buffer->target_node && !(t->flags & TF_ONE_WAY))
 				binder_send_failed_reply(t, BR_DEAD_REPLY);
+			else {
+				t->buffer->transaction = NULL;
+				kfree(t);
+				binder_stats_deleted(BINDER_STAT_TRANSACTION);
+			}
 		} break;
 		case BINDER_WORK_TRANSACTION_COMPLETE: {
 			kfree(w);
@@ -2982,6 +2987,7 @@ static void binder_deferred_release(struct binder_proc *proc)
 
 		nodes++;
 		rb_erase(&node->rb_node, &proc->nodes);
+		binder_release_work(&node->async_todo);
 		list_del_init(&node->work.entry);
 		if (hlist_empty(&node->refs)) {
 			kfree(node);
-- 
1.7.6

Re: Fix memory leak in binder--version2

[Greg KH]

On Mon, Oct 15, 2012 at 07:20:01AM +0000, Tu, Xiaobing wrote:
> After enabling kmemleak and run monkey, following memleak is reported:
> unreferenced object 0xeed27f80 (size 64):
>   comm "Binder_8", pid 641, jiffies 4294946341 (age 2275.810s)
>   hex dump (first 32 bytes):
>     4f dd 00 00 84 7f d2 ee 84 7f d2 ee 01 00 00 00  O...............
>     00 00 00 00 00 00 00 00 00 aa 4c d7 00 00 00 00  ..........L.....
>   backtrace:
>     [<c184fabc>] kmemleak_alloc+0x3c/0xa0
>     [<c12f391e>] kmem_cache_alloc_trace+0x9e/0x180
>     [<c1668bb5>] binder_thread_write+0xcf5/0x23a0
>     [<c166b091>] binder_ioctl+0x1f1/0x530
>     [<c130dcf6>] do_vfs_ioctl+0x86/0x5e0
>     [<c130e282>] sys_ioctl+0x32/0x60
>     [<c1872e01>] syscall_call+0x7/0xb
>     [<ffffffff>] 0xffffffff
> 
> The work item in async_todo list is not freed when binder released.
> Also the async transaction should also be freed in binder_release_work.
> 
> Signed-off-by: Leon Ma <xindong.ma@intel.com>
> Signed-off-by: Di Zhang <di.zhang@intel.com>
> ---
>  drivers/staging/android/binder.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)

Nice fix, but next time can you at least use scripts/get_maintainer.pl
to figure out who to send this to?  I'll queue it up soon, but it took
akpm to point me at this for me to notice it.

thanks,

greg k-h

Re: Fix memory leak in binder--version2

[Arve Hjønnevåg]

On Mon, Oct 15, 2012 at 3:52 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Mon, Oct 15, 2012 at 07:20:01AM +0000, Tu, Xiaobing wrote:
>> After enabling kmemleak and run monkey, following memleak is reported:
>> unreferenced object 0xeed27f80 (size 64):
>>   comm "Binder_8", pid 641, jiffies 4294946341 (age 2275.810s)
>>   hex dump (first 32 bytes):
>>     4f dd 00 00 84 7f d2 ee 84 7f d2 ee 01 00 00 00  O...............
>>     00 00 00 00 00 00 00 00 00 aa 4c d7 00 00 00 00  ..........L.....
>>   backtrace:
>>     [<c184fabc>] kmemleak_alloc+0x3c/0xa0
>>     [<c12f391e>] kmem_cache_alloc_trace+0x9e/0x180
>>     [<c1668bb5>] binder_thread_write+0xcf5/0x23a0
>>     [<c166b091>] binder_ioctl+0x1f1/0x530
>>     [<c130dcf6>] do_vfs_ioctl+0x86/0x5e0
>>     [<c130e282>] sys_ioctl+0x32/0x60
>>     [<c1872e01>] syscall_call+0x7/0xb
>>     [<ffffffff>] 0xffffffff
>>
>> The work item in async_todo list is not freed when binder released.
>> Also the async transaction should also be freed in binder_release_work.
>>
>> Signed-off-by: Leon Ma <xindong.ma@intel.com>
>> Signed-off-by: Di Zhang <di.zhang@intel.com>
>> ---
>>  drivers/staging/android/binder.c |    6 ++++++
>>  1 files changed, 6 insertions(+), 0 deletions(-)
>
> Nice fix, but next time can you at least use scripts/get_maintainer.pl
> to figure out who to send this to?  I'll queue it up soon, but it took
> akpm to point me at this for me to notice it.

This patch was just pointer out to me as well. I have a similar fix
queued up at https://android-review.googlesource.com/#/c/43801/ that
is still being tested. It fixes this leak and a theoretical leak of
death notification objects.

-- 
Arve Hjønnevåg

Re: Fix memory leak in binder--version2

[Greg KH]

On Mon, Oct 15, 2012 at 04:55:21PM -0700, Arve Hjønnevåg wrote:
> On Mon, Oct 15, 2012 at 3:52 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Mon, Oct 15, 2012 at 07:20:01AM +0000, Tu, Xiaobing wrote:
> >> After enabling kmemleak and run monkey, following memleak is reported:
> >> unreferenced object 0xeed27f80 (size 64):
> >>   comm "Binder_8", pid 641, jiffies 4294946341 (age 2275.810s)
> >>   hex dump (first 32 bytes):
> >>     4f dd 00 00 84 7f d2 ee 84 7f d2 ee 01 00 00 00  O...............
> >>     00 00 00 00 00 00 00 00 00 aa 4c d7 00 00 00 00  ..........L.....
> >>   backtrace:
> >>     [<c184fabc>] kmemleak_alloc+0x3c/0xa0
> >>     [<c12f391e>] kmem_cache_alloc_trace+0x9e/0x180
> >>     [<c1668bb5>] binder_thread_write+0xcf5/0x23a0
> >>     [<c166b091>] binder_ioctl+0x1f1/0x530
> >>     [<c130dcf6>] do_vfs_ioctl+0x86/0x5e0
> >>     [<c130e282>] sys_ioctl+0x32/0x60
> >>     [<c1872e01>] syscall_call+0x7/0xb
> >>     [<ffffffff>] 0xffffffff
> >>
> >> The work item in async_todo list is not freed when binder released.
> >> Also the async transaction should also be freed in binder_release_work.
> >>
> >> Signed-off-by: Leon Ma <xindong.ma@intel.com>
> >> Signed-off-by: Di Zhang <di.zhang@intel.com>
> >> ---
> >>  drivers/staging/android/binder.c |    6 ++++++
> >>  1 files changed, 6 insertions(+), 0 deletions(-)
> >
> > Nice fix, but next time can you at least use scripts/get_maintainer.pl
> > to figure out who to send this to?  I'll queue it up soon, but it took
> > akpm to point me at this for me to notice it.
> 
> This patch was just pointer out to me as well. I have a similar fix
> queued up at https://android-review.googlesource.com/#/c/43801/ that
> is still being tested. It fixes this leak and a theoretical leak of
> death notification objects.

Ok, should I hold off applying this patch and wait for your patch
instead?

thanks,

greg k-h

[PATCH 1/2] Staging: android: binder: Fix memory leak on thread/process exit

[Arve Hjønnevåg]

If a thread or process exited while a reply, one-way transaction or
death notification was pending, the struct holding the pending work
was leaked.

Change-Id: I2eaafaba1c0ecda3ec0872d449dc16d0721c21e7
Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 drivers/staging/android/binder.c |   28 +++++++++++++++++++++++++++-
 1 files changed, 27 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 85ed6cf..b89799a 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -2548,14 +2548,38 @@ static void binder_release_work(struct list_head *list)
 			struct binder_transaction *t;
 
 			t = container_of(w, struct binder_transaction, work);
-			if (t->buffer->target_node && !(t->flags & TF_ONE_WAY))
+			if (t->buffer->target_node &&
+			    !(t->flags & TF_ONE_WAY)) {
 				binder_send_failed_reply(t, BR_DEAD_REPLY);
+			} else {
+				binder_debug(BINDER_DEBUG_DEAD_TRANSACTION,
+					"binder: undelivered transaction %d\n",
+					t->debug_id);
+				t->buffer->transaction = NULL;
+				kfree(t);
+				binder_stats_deleted(BINDER_STAT_TRANSACTION);
+			}
 		} break;
 		case BINDER_WORK_TRANSACTION_COMPLETE: {
+			binder_debug(BINDER_DEBUG_DEAD_TRANSACTION,
+				"binder: undelivered TRANSACTION_COMPLETE\n");
 			kfree(w);
 			binder_stats_deleted(BINDER_STAT_TRANSACTION_COMPLETE);
 		} break;
+		case BINDER_WORK_DEAD_BINDER_AND_CLEAR:
+		case BINDER_WORK_CLEAR_DEATH_NOTIFICATION: {
+			struct binder_ref_death *death;
+
+			death = container_of(w, struct binder_ref_death, work);
+			binder_debug(BINDER_DEBUG_DEAD_TRANSACTION,
+				"binder: undelivered death notification, %p\n",
+				death->cookie);
+			kfree(death);
+			binder_stats_deleted(BINDER_STAT_DEATH);
+		} break;
 		default:
+			pr_err("binder: unexpected work type, %d, not freed\n",
+			       w->type);
 			break;
 		}
 	}
@@ -3036,6 +3060,7 @@ static void binder_deferred_release(struct binder_proc *proc)
 		nodes++;
 		rb_erase(&node->rb_node, &proc->nodes);
 		list_del_init(&node->work.entry);
+		binder_release_work(&node->async_todo);
 		if (hlist_empty(&node->refs)) {
 			kfree(node);
 			binder_stats_deleted(BINDER_STAT_NODE);
@@ -3074,6 +3099,7 @@ static void binder_deferred_release(struct binder_proc *proc)
 		binder_delete_ref(ref);
 	}
 	binder_release_work(&proc->todo);
+	binder_release_work(&proc->delivered_death);
 	buffers = 0;
 
 	while ((n = rb_first(&proc->allocated_buffers))) {
-- 
1.7.7.3

[PATCH 2/2] Staging: android: binder: Allow using highmem for binder buffers

[Arve Hjønnevåg]

Change-Id: I19be5fd89c194abcb8b041e2dd6c4d71ae7b1cc2
Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 drivers/staging/android/binder.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index b89799a..c831dcc 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -671,7 +671,7 @@ static int binder_update_page_range(struct binder_proc *proc, int allocate,
 		page = &proc->pages[(page_addr - proc->buffer) / PAGE_SIZE];
 
 		BUG_ON(*page);
-		*page = alloc_page(GFP_KERNEL | __GFP_ZERO);
+		*page = alloc_page(GFP_KERNEL | __GFP_HIGHMEM | __GFP_ZERO);
 		if (*page == NULL) {
 			printk(KERN_ERR "binder: %d: binder_alloc_buf failed "
 			       "for page at %p\n", proc->pid, page_addr);
-- 
1.7.7.3

Re: [PATCH 2/2] Staging: android: binder: Allow using highmem for binder buffers

[Greg KH]

On Mon, Oct 15, 2012 at 05:32:42PM -0700, Arve Hjønnevåg wrote:
> Change-Id: I19be5fd89c194abcb8b041e2dd6c4d71ae7b1cc2

Again with the change-id field :(

And please provide some information here, _why_ do we want to allow
highmem for binder buffers?  What does this solve?  Is it a bug or just
a feature enhancement?  Depending on what it is depends on what release
it should go into.

thanks,

greg k-h

Re: [PATCH 1/2] Staging: android: binder: Fix memory leak on thread/process exit

[Greg KH]

On Mon, Oct 15, 2012 at 05:32:41PM -0700, Arve Hjønnevåg wrote:
> If a thread or process exited while a reply, one-way transaction or
> death notification was pending, the struct holding the pending work
> was leaked.
> 
> Change-Id: I2eaafaba1c0ecda3ec0872d449dc16d0721c21e7

What is this field?  Please don't include this in kernel patches, it
forces me to edit the patch by hand :(

Also, this should go to older kernels, right?

greg k-h

Re: [PATCH 1/2] Staging: android: binder: Fix memory leak on thread/process exit

[Arve Hjønnevåg]

On Tue, Oct 16, 2012 at 12:11 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Mon, Oct 15, 2012 at 05:32:41PM -0700, Arve Hjønnevåg wrote:
>> If a thread or process exited while a reply, one-way transaction or
>> death notification was pending, the struct holding the pending work
>> was leaked.
>>
>> Change-Id: I2eaafaba1c0ecda3ec0872d449dc16d0721c21e7
>
> What is this field?  Please don't include this in kernel patches, it
> forces me to edit the patch by hand :(
>

It is a tag generated a git hook to uniquely identify a change through
multiple revisions of that change. If the tag does not already exist
the hook adds it. I removed it from the reposted patches as you
requested, but have you considered leaving it in? It is a useful
search token both for finding different revisions of a patch and for
finding which branch a change has been cherry-picked into.

> Also, this should go to older kernels, right?

It can, but the leak is relatively slow so I don't think it is
critical. I looked at one device that had been running for a while and
it had leaked around 16k per month.

-- 
Arve Hjønnevåg

Re: [PATCH 1/2] Staging: android: binder: Fix memory leak on thread/process exit

[Greg KH]

On Tue, Oct 16, 2012 at 03:45:20PM -0700, Arve Hjønnevåg wrote:
> On Tue, Oct 16, 2012 at 12:11 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Mon, Oct 15, 2012 at 05:32:41PM -0700, Arve Hjønnevåg wrote:
> >> If a thread or process exited while a reply, one-way transaction or
> >> death notification was pending, the struct holding the pending work
> >> was leaked.
> >>
> >> Change-Id: I2eaafaba1c0ecda3ec0872d449dc16d0721c21e7
> >
> > What is this field?  Please don't include this in kernel patches, it
> > forces me to edit the patch by hand :(
> >
> 
> It is a tag generated a git hook to uniquely identify a change through
> multiple revisions of that change. If the tag does not already exist
> the hook adds it. I removed it from the reposted patches as you
> requested, but have you considered leaving it in? It is a useful
> search token both for finding different revisions of a patch and for
> finding which branch a change has been cherry-picked into.

Sorry, I was being facetious, I knew what it really was, but the point
was, why would I?

As has been stated before, you can include information like this in your
patch, but you MUST reference it properly so that others can be able to
figure out what you mean.  A random Change-Id value means nothing given
that lots of different groups use gerrit.  You need a url, or a pointer
to which gerrit instance this refers to before you can include it.

Same goes for bugzilla entries, if you look in the kernel changelog, we
reference lots of different ones, but we use urls to determine which one
we are talking about (suse, red hat, kernel.org, etc.)

thanks,

greg k-h

Re: [PATCH 1/2] Staging: android: binder: Fix memory leak on thread/process exit

[Arve Hjønnevåg]

On Mon, Oct 22, 2012 at 1:00 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Tue, Oct 16, 2012 at 03:45:20PM -0700, Arve Hjønnevåg wrote:
>> On Tue, Oct 16, 2012 at 12:11 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Mon, Oct 15, 2012 at 05:32:41PM -0700, Arve Hjønnevåg wrote:
>> >> If a thread or process exited while a reply, one-way transaction or
>> >> death notification was pending, the struct holding the pending work
>> >> was leaked.
>> >>
>> >> Change-Id: I2eaafaba1c0ecda3ec0872d449dc16d0721c21e7
>> >
>> > What is this field?  Please don't include this in kernel patches, it
>> > forces me to edit the patch by hand :(
>> >
>>
>> It is a tag generated a git hook to uniquely identify a change through
>> multiple revisions of that change. If the tag does not already exist
>> the hook adds it. I removed it from the reposted patches as you
>> requested, but have you considered leaving it in? It is a useful
>> search token both for finding different revisions of a patch and for
>> finding which branch a change has been cherry-picked into.
>
> Sorry, I was being facetious, I knew what it really was, but the point
> was, why would I?
>

So you can use it. If, for instance, you want to see which branches
you cherry-picked this change into you can run "git rev-list --all
--grep I2eaafaba1c0ecda3ec0872d449dc16d0721c21e7 | xargs -rl git
branch -a --contains". If it was more widely used, it would also be
useful for finding mailing list discussion of multiple versions of
patches (even when the title of the patch changes).

> As has been stated before, you can include information like this in your
> patch, but you MUST reference it properly so that others can be able to
> figure out what you mean.  A random Change-Id value means nothing given
> that lots of different groups use gerrit.  You need a url, or a pointer
> to which gerrit instance this refers to before you can include it.
>

There is no single url. The tag was generated by a local git hook
before it was uploaded anywhere. While gerrit looks for this tag and
use it when it is there, the same change can be uploaded to different
gerrit instances. While it would be useful to also include urls to the
gerrit instance(s) that a patch was discussed on, the Change-Id is
useful by itself.

> Same goes for bugzilla entries, if you look in the kernel changelog, we
> reference lots of different ones, but we use urls to determine which one
> we are talking about (suse, red hat, kernel.org, etc.)
>

Those IDs appear to be local to a specific server, so the id without
the url would not be very useful.

-- 
Arve Hjønnevåg

Re: [PATCH 1/2] Staging: android: binder: Fix memory leak on thread/process exit

[Greg KH]

On Mon, Oct 22, 2012 at 05:58:08PM -0700, Arve Hj?nnev?g wrote:
> On Mon, Oct 22, 2012 at 1:00 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Tue, Oct 16, 2012 at 03:45:20PM -0700, Arve Hj?nnev?g wrote:
> >> On Tue, Oct 16, 2012 at 12:11 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> > On Mon, Oct 15, 2012 at 05:32:41PM -0700, Arve Hj?nnev?g wrote:
> >> >> If a thread or process exited while a reply, one-way transaction or
> >> >> death notification was pending, the struct holding the pending work
> >> >> was leaked.
> >> >>
> >> >> Change-Id: I2eaafaba1c0ecda3ec0872d449dc16d0721c21e7
> >> >
> >> > What is this field?  Please don't include this in kernel patches, it
> >> > forces me to edit the patch by hand :(
> >> >
> >>
> >> It is a tag generated a git hook to uniquely identify a change through
> >> multiple revisions of that change. If the tag does not already exist
> >> the hook adds it. I removed it from the reposted patches as you
> >> requested, but have you considered leaving it in? It is a useful
> >> search token both for finding different revisions of a patch and for
> >> finding which branch a change has been cherry-picked into.
> >
> > Sorry, I was being facetious, I knew what it really was, but the point
> > was, why would I?
> >
> 
> So you can use it. If, for instance, you want to see which branches
> you cherry-picked this change into you can run "git rev-list --all
> --grep I2eaafaba1c0ecda3ec0872d449dc16d0721c21e7 | xargs -rl git
> branch -a --contains". If it was more widely used, it would also be
> useful for finding mailing list discussion of multiple versions of
> patches (even when the title of the patch changes).

Yes, if you want a unique identifier for a patch in order to try to
track it through trees, you can do something like that (hint, see Alan
Stern's patches, he has his own identifier for that.)  But don't use the
gerrit id for this, it's not useful and it is confusing.  We have
discussed this in the past, see the lkml archives for details.

> > As has been stated before, you can include information like this in your
> > patch, but you MUST reference it properly so that others can be able to
> > figure out what you mean.  A random Change-Id value means nothing given
> > that lots of different groups use gerrit.  You need a url, or a pointer
> > to which gerrit instance this refers to before you can include it.
> >
> 
> There is no single url. The tag was generated by a local git hook
> before it was uploaded anywhere. While gerrit looks for this tag and
> use it when it is there, the same change can be uploaded to different
> gerrit instances. While it would be useful to also include urls to the
> gerrit instance(s) that a patch was discussed on, the Change-Id is
> useful by itself.
> 
> > Same goes for bugzilla entries, if you look in the kernel changelog, we
> > reference lots of different ones, but we use urls to determine which one
> > we are talking about (suse, red hat, kernel.org, etc.)
> >
> 
> Those IDs appear to be local to a specific server, so the id without
> the url would not be very useful.

Ok, then I don't recommend it be used at all, as we have a unique
identifier, the git commit id in Linus's tree.  That id does follow the
patch around if it goes into any stable kernel tree, so let's stick with
it.

thanks,

greg k-h

Re: Fix memory leak in binder--version2

[Arve Hjønnevåg]

On Mon, Oct 15, 2012 at 4:58 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Mon, Oct 15, 2012 at 04:55:21PM -0700, Arve Hjønnevåg wrote:
>> On Mon, Oct 15, 2012 at 3:52 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Mon, Oct 15, 2012 at 07:20:01AM +0000, Tu, Xiaobing wrote:
>> >> After enabling kmemleak and run monkey, following memleak is reported:
>> >> unreferenced object 0xeed27f80 (size 64):
>> >>   comm "Binder_8", pid 641, jiffies 4294946341 (age 2275.810s)
>> >>   hex dump (first 32 bytes):
>> >>     4f dd 00 00 84 7f d2 ee 84 7f d2 ee 01 00 00 00  O...............
>> >>     00 00 00 00 00 00 00 00 00 aa 4c d7 00 00 00 00  ..........L.....
>> >>   backtrace:
>> >>     [<c184fabc>] kmemleak_alloc+0x3c/0xa0
>> >>     [<c12f391e>] kmem_cache_alloc_trace+0x9e/0x180
>> >>     [<c1668bb5>] binder_thread_write+0xcf5/0x23a0
>> >>     [<c166b091>] binder_ioctl+0x1f1/0x530
>> >>     [<c130dcf6>] do_vfs_ioctl+0x86/0x5e0
>> >>     [<c130e282>] sys_ioctl+0x32/0x60
>> >>     [<c1872e01>] syscall_call+0x7/0xb
>> >>     [<ffffffff>] 0xffffffff
>> >>
>> >> The work item in async_todo list is not freed when binder released.
>> >> Also the async transaction should also be freed in binder_release_work.
>> >>
>> >> Signed-off-by: Leon Ma <xindong.ma@intel.com>
>> >> Signed-off-by: Di Zhang <di.zhang@intel.com>
>> >> ---
>> >>  drivers/staging/android/binder.c |    6 ++++++
>> >>  1 files changed, 6 insertions(+), 0 deletions(-)
>> >
>> > Nice fix, but next time can you at least use scripts/get_maintainer.pl
>> > to figure out who to send this to?  I'll queue it up soon, but it took
>> > akpm to point me at this for me to notice it.
>>
>> This patch was just pointer out to me as well. I have a similar fix
>> queued up at https://android-review.googlesource.com/#/c/43801/ that
>> is still being tested. It fixes this leak and a theoretical leak of
>> death notification objects.
>
> Ok, should I hold off applying this patch and wait for your patch
> instead?
>

I just sent two patches, but they are not fully tested yet.

-- 
Arve Hjønnevåg

Re: Fix memory leak in binder--version2

[Greg KH]

On Mon, Oct 15, 2012 at 05:39:22PM -0700, Arve Hjønnevåg wrote:
> On Mon, Oct 15, 2012 at 4:58 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Mon, Oct 15, 2012 at 04:55:21PM -0700, Arve Hjønnevåg wrote:
> >> On Mon, Oct 15, 2012 at 3:52 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> > On Mon, Oct 15, 2012 at 07:20:01AM +0000, Tu, Xiaobing wrote:
> >> >> After enabling kmemleak and run monkey, following memleak is reported:
> >> >> unreferenced object 0xeed27f80 (size 64):
> >> >>   comm "Binder_8", pid 641, jiffies 4294946341 (age 2275.810s)
> >> >>   hex dump (first 32 bytes):
> >> >>     4f dd 00 00 84 7f d2 ee 84 7f d2 ee 01 00 00 00  O...............
> >> >>     00 00 00 00 00 00 00 00 00 aa 4c d7 00 00 00 00  ..........L.....
> >> >>   backtrace:
> >> >>     [<c184fabc>] kmemleak_alloc+0x3c/0xa0
> >> >>     [<c12f391e>] kmem_cache_alloc_trace+0x9e/0x180
> >> >>     [<c1668bb5>] binder_thread_write+0xcf5/0x23a0
> >> >>     [<c166b091>] binder_ioctl+0x1f1/0x530
> >> >>     [<c130dcf6>] do_vfs_ioctl+0x86/0x5e0
> >> >>     [<c130e282>] sys_ioctl+0x32/0x60
> >> >>     [<c1872e01>] syscall_call+0x7/0xb
> >> >>     [<ffffffff>] 0xffffffff
> >> >>
> >> >> The work item in async_todo list is not freed when binder released.
> >> >> Also the async transaction should also be freed in binder_release_work.
> >> >>
> >> >> Signed-off-by: Leon Ma <xindong.ma@intel.com>
> >> >> Signed-off-by: Di Zhang <di.zhang@intel.com>
> >> >> ---
> >> >>  drivers/staging/android/binder.c |    6 ++++++
> >> >>  1 files changed, 6 insertions(+), 0 deletions(-)
> >> >
> >> > Nice fix, but next time can you at least use scripts/get_maintainer.pl
> >> > to figure out who to send this to?  I'll queue it up soon, but it took
> >> > akpm to point me at this for me to notice it.
> >>
> >> This patch was just pointer out to me as well. I have a similar fix
> >> queued up at https://android-review.googlesource.com/#/c/43801/ that
> >> is still being tested. It fixes this leak and a theoretical leak of
> >> death notification objects.
> >
> > Ok, should I hold off applying this patch and wait for your patch
> > instead?
> >
> 
> I just sent two patches, but they are not fully tested yet.

Ok, care to test them and after that, resend them with the changes I
noted in the changelog section of the patch?

thanks,

greg k-h

[PATCH 1/4] Staging: android: binder: Add some missing binder_stat_br calls

[Arve Hjønnevåg]

Cached thread return errors, death notifications and new looper
requests were not included in the stats.

Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 drivers/staging/android/binder.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 7b0ba92..9e852d0 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -2135,6 +2135,7 @@ retry:
 			if (put_user(thread->return_error2, (uint32_t __user *)ptr))
 				return -EFAULT;
 			ptr += sizeof(uint32_t);
+			binder_stat_br(proc, thread, thread->return_error2);
 			if (ptr == end)
 				goto done;
 			thread->return_error2 = BR_OK;
@@ -2142,6 +2143,7 @@ retry:
 		if (put_user(thread->return_error, (uint32_t __user *)ptr))
 			return -EFAULT;
 		ptr += sizeof(uint32_t);
+		binder_stat_br(proc, thread, thread->return_error);
 		thread->return_error = BR_OK;
 		goto done;
 	}
@@ -2297,6 +2299,7 @@ retry:
 			if (put_user(death->cookie, (void * __user *)ptr))
 				return -EFAULT;
 			ptr += sizeof(void *);
+			binder_stat_br(proc, thread, cmd);
 			binder_debug(BINDER_DEBUG_DEATH_NOTIFICATION,
 				     "binder: %d:%d %s %p\n",
 				      proc->pid, thread->pid,
@@ -2404,6 +2407,7 @@ done:
 			     proc->pid, thread->pid);
 		if (put_user(BR_SPAWN_LOOPER, (uint32_t __user *)buffer))
 			return -EFAULT;
+		binder_stat_br(proc, thread, BR_SPAWN_LOOPER);
 	}
 	return 0;
 }
-- 
1.7.7.3

[PATCH 2/4] Staging: android: binder: Add some tracepoints

[Arve Hjønnevåg]

Add tracepoints:
- ioctl entry and exit
- Main binder lock: lock, locked and unlock
- Command and return buffer opcodes
- Transaction: create and receive
- Transaction buffer: create and free
- Object and file descriptor transfer
- binder_update_page_range

Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 drivers/staging/android/Makefile       |    2 +
 drivers/staging/android/binder.c       |   91 +++++++--
 drivers/staging/android/binder_trace.h |  327 ++++++++++++++++++++++++++++++++
 3 files changed, 400 insertions(+), 20 deletions(-)
 create mode 100644 drivers/staging/android/binder_trace.h

diff --git a/drivers/staging/android/Makefile b/drivers/staging/android/Makefile
index e16fcd5..b35a631 100644
--- a/drivers/staging/android/Makefile
+++ b/drivers/staging/android/Makefile
@@ -1,3 +1,5 @@
+ccflags-y += -I$(src)			# needed for trace events
+
 obj-$(CONFIG_ANDROID_BINDER_IPC)	+= binder.o
 obj-$(CONFIG_ASHMEM)			+= ashmem.o
 obj-$(CONFIG_ANDROID_LOGGER)		+= logger.o
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 9e852d0..b7cfcdb 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -35,8 +35,9 @@
 #include <linux/slab.h>
 
 #include "binder.h"
+#include "binder_trace.h"
 
-static DEFINE_MUTEX(binder_lock);
+static DEFINE_MUTEX(binder_main_lock);
 static DEFINE_MUTEX(binder_deferred_lock);
 static DEFINE_MUTEX(binder_mmap_lock);
 
@@ -411,6 +412,19 @@ static long task_close_fd(struct binder_proc *proc, unsigned int fd)
 	return retval;
 }
 
+static inline void binder_lock(const char *tag)
+{
+	trace_binder_lock(tag);
+	mutex_lock(&binder_main_lock);
+	trace_binder_locked(tag);
+}
+
+static inline void binder_unlock(const char *tag)
+{
+	trace_binder_unlock(tag);
+	mutex_unlock(&binder_main_lock);
+}
+
 static void binder_set_nice(long nice)
 {
 	long min_nice;
@@ -537,6 +551,8 @@ static int binder_update_page_range(struct binder_proc *proc, int allocate,
 	if (end <= start)
 		return 0;
 
+	trace_binder_update_page_range(proc, allocate, start, end);
+
 	if (vma)
 		mm = NULL;
 	else
@@ -1461,6 +1477,9 @@ static void binder_transaction(struct binder_proc *proc,
 	t->code = tr->code;
 	t->flags = tr->flags;
 	t->priority = task_nice(current);
+
+	trace_binder_transaction(reply, t, target_node);
+
 	t->buffer = binder_alloc_buf(target_proc, tr->data_size,
 		tr->offsets_size, !reply && (t->flags & TF_ONE_WAY));
 	if (t->buffer == NULL) {
@@ -1471,6 +1490,7 @@ static void binder_transaction(struct binder_proc *proc,
 	t->buffer->debug_id = t->debug_id;
 	t->buffer->transaction = t;
 	t->buffer->target_node = target_node;
+	trace_binder_transaction_alloc_buf(t->buffer);
 	if (target_node)
 		binder_inc_node(target_node, 1, 0, NULL);
 
@@ -1543,6 +1563,7 @@ static void binder_transaction(struct binder_proc *proc,
 			binder_inc_ref(ref, fp->type == BINDER_TYPE_HANDLE,
 				       &thread->todo);
 
+			trace_binder_transaction_node_to_ref(t, node, ref);
 			binder_debug(BINDER_DEBUG_TRANSACTION,
 				     "        node %d u%p -> ref %d desc %d\n",
 				     node->debug_id, node->ptr, ref->debug_id,
@@ -1567,6 +1588,7 @@ static void binder_transaction(struct binder_proc *proc,
 				fp->binder = ref->node->ptr;
 				fp->cookie = ref->node->cookie;
 				binder_inc_node(ref->node, fp->type == BINDER_TYPE_BINDER, 0, NULL);
+				trace_binder_transaction_ref_to_node(t, ref);
 				binder_debug(BINDER_DEBUG_TRANSACTION,
 					     "        ref %d desc %d -> node %d u%p\n",
 					     ref->debug_id, ref->desc, ref->node->debug_id,
@@ -1580,6 +1602,8 @@ static void binder_transaction(struct binder_proc *proc,
 				}
 				fp->handle = new_ref->desc;
 				binder_inc_ref(new_ref, fp->type == BINDER_TYPE_HANDLE, NULL);
+				trace_binder_transaction_ref_to_ref(t, ref,
+								    new_ref);
 				binder_debug(BINDER_DEBUG_TRANSACTION,
 					     "        ref %d desc %d -> ref %d desc %d (node %d)\n",
 					     ref->debug_id, ref->desc, new_ref->debug_id,
@@ -1619,6 +1643,7 @@ static void binder_transaction(struct binder_proc *proc,
 				goto err_get_unused_fd_failed;
 			}
 			task_fd_install(target_proc, target_fd, file);
+			trace_binder_transaction_fd(t, fp->handle, target_fd);
 			binder_debug(BINDER_DEBUG_TRANSACTION,
 				     "        fd %ld -> %d\n", fp->handle, target_fd);
 			/* TODO: fput? */
@@ -1667,6 +1692,7 @@ err_binder_new_node_failed:
 err_bad_object_type:
 err_bad_offset:
 err_copy_data_failed:
+	trace_binder_transaction_failed_buffer_release(t->buffer);
 	binder_transaction_buffer_release(target_proc, t->buffer, offp);
 	t->buffer->transaction = NULL;
 	binder_free_buf(target_proc, t->buffer);
@@ -1712,6 +1738,7 @@ int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
 		if (get_user(cmd, (uint32_t __user *)ptr))
 			return -EFAULT;
 		ptr += sizeof(uint32_t);
+		trace_binder_command(cmd);
 		if (_IOC_NR(cmd) < ARRAY_SIZE(binder_stats.bc)) {
 			binder_stats.bc[_IOC_NR(cmd)]++;
 			proc->stats.bc[_IOC_NR(cmd)]++;
@@ -1881,6 +1908,7 @@ int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
 				else
 					list_move_tail(buffer->target_node->async_todo.next, &thread->todo);
 			}
+			trace_binder_transaction_buffer_release(buffer);
 			binder_transaction_buffer_release(proc, buffer, NULL);
 			binder_free_buf(proc, buffer);
 			break;
@@ -2089,6 +2117,7 @@ int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
 void binder_stat_br(struct binder_proc *proc, struct binder_thread *thread,
 		    uint32_t cmd)
 {
+	trace_binder_return(cmd);
 	if (_IOC_NR(cmd) < ARRAY_SIZE(binder_stats.br)) {
 		binder_stats.br[_IOC_NR(cmd)]++;
 		proc->stats.br[_IOC_NR(cmd)]++;
@@ -2152,7 +2181,12 @@ retry:
 	thread->looper |= BINDER_LOOPER_STATE_WAITING;
 	if (wait_for_proc_work)
 		proc->ready_threads++;
-	mutex_unlock(&binder_lock);
+
+	binder_unlock(__func__);
+
+	trace_binder_wait_for_work(wait_for_proc_work,
+				   !!thread->transaction_stack,
+				   !list_empty(&thread->todo));
 	if (wait_for_proc_work) {
 		if (!(thread->looper & (BINDER_LOOPER_STATE_REGISTERED |
 					BINDER_LOOPER_STATE_ENTERED))) {
@@ -2176,7 +2210,9 @@ retry:
 		} else
 			ret = wait_event_interruptible(thread->wait, binder_has_thread_work(thread));
 	}
-	mutex_lock(&binder_lock);
+
+	binder_lock(__func__);
+
 	if (wait_for_proc_work)
 		proc->ready_threads--;
 	thread->looper &= ~BINDER_LOOPER_STATE_WAITING;
@@ -2367,6 +2403,7 @@ retry:
 			return -EFAULT;
 		ptr += sizeof(tr);
 
+		trace_binder_transaction_received(t);
 		binder_stat_br(proc, thread, cmd);
 		binder_debug(BINDER_DEBUG_TRANSACTION,
 			     "binder: %d:%d %s %d %d:%d, cmd %d"
@@ -2520,12 +2557,14 @@ static unsigned int binder_poll(struct file *filp,
 	struct binder_thread *thread = NULL;
 	int wait_for_proc_work;
 
-	mutex_lock(&binder_lock);
+	binder_lock(__func__);
+
 	thread = binder_get_thread(proc);
 
 	wait_for_proc_work = thread->transaction_stack == NULL &&
 		list_empty(&thread->todo) && thread->return_error == BR_OK;
-	mutex_unlock(&binder_lock);
+
+	binder_unlock(__func__);
 
 	if (wait_for_proc_work) {
 		if (binder_has_proc_work(proc, thread))
@@ -2553,11 +2592,13 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 
 	/*pr_info("binder_ioctl: %d:%d %x %lx\n", proc->pid, current->pid, cmd, arg);*/
 
+	trace_binder_ioctl(cmd, arg);
+
 	ret = wait_event_interruptible(binder_user_error_wait, binder_stop_on_user_error < 2);
 	if (ret)
-		return ret;
+		goto err_unlocked;
 
-	mutex_lock(&binder_lock);
+	binder_lock(__func__);
 	thread = binder_get_thread(proc);
 	if (thread == NULL) {
 		ret = -ENOMEM;
@@ -2582,6 +2623,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 
 		if (bwr.write_size > 0) {
 			ret = binder_thread_write(proc, thread, (void __user *)bwr.write_buffer, bwr.write_size, &bwr.write_consumed);
+			trace_binder_write_done(ret);
 			if (ret < 0) {
 				bwr.read_consumed = 0;
 				if (copy_to_user(ubuf, &bwr, sizeof(bwr)))
@@ -2591,6 +2633,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		}
 		if (bwr.read_size > 0) {
 			ret = binder_thread_read(proc, thread, (void __user *)bwr.read_buffer, bwr.read_size, &bwr.read_consumed, filp->f_flags & O_NONBLOCK);
+			trace_binder_read_done(ret);
 			if (!list_empty(&proc->todo))
 				wake_up_interruptible(&proc->wait);
 			if (ret < 0) {
@@ -2666,10 +2709,12 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 err:
 	if (thread)
 		thread->looper &= ~BINDER_LOOPER_STATE_NEED_RETURN;
-	mutex_unlock(&binder_lock);
+	binder_unlock(__func__);
 	wait_event_interruptible(binder_user_error_wait, binder_stop_on_user_error < 2);
 	if (ret && ret != -ERESTARTSYS)
 		pr_info("binder: %d:%d ioctl %x %lx returned %d\n", proc->pid, current->pid, cmd, arg, ret);
+err_unlocked:
+	trace_binder_ioctl_done(ret);
 	return ret;
 }
 
@@ -2815,13 +2860,16 @@ static int binder_open(struct inode *nodp, struct file *filp)
 	INIT_LIST_HEAD(&proc->todo);
 	init_waitqueue_head(&proc->wait);
 	proc->default_priority = task_nice(current);
-	mutex_lock(&binder_lock);
+
+	binder_lock(__func__);
+
 	binder_stats_created(BINDER_STAT_PROC);
 	hlist_add_head(&proc->proc_node, &binder_procs);
 	proc->pid = current->group_leader->pid;
 	INIT_LIST_HEAD(&proc->delivered_death);
 	filp->private_data = proc;
-	mutex_unlock(&binder_lock);
+
+	binder_unlock(__func__);
 
 	if (binder_debugfs_dir_entry_proc) {
 		char strbuf[11];
@@ -3001,7 +3049,7 @@ static void binder_deferred_func(struct work_struct *work)
 
 	int defer;
 	do {
-		mutex_lock(&binder_lock);
+		binder_lock(__func__);
 		mutex_lock(&binder_deferred_lock);
 		if (!hlist_empty(&binder_deferred_list)) {
 			proc = hlist_entry(binder_deferred_list.first,
@@ -3028,7 +3076,7 @@ static void binder_deferred_func(struct work_struct *work)
 		if (defer & BINDER_DEFERRED_RELEASE)
 			binder_deferred_release(proc); /* frees proc */
 
-		mutex_unlock(&binder_lock);
+		binder_unlock(__func__);
 		if (files)
 			put_files_struct(files);
 	} while (proc);
@@ -3369,7 +3417,7 @@ static int binder_state_show(struct seq_file *m, void *unused)
 	int do_lock = !binder_debug_no_lock;
 
 	if (do_lock)
-		mutex_lock(&binder_lock);
+		binder_lock(__func__);
 
 	seq_puts(m, "binder state:\n");
 
@@ -3381,7 +3429,7 @@ static int binder_state_show(struct seq_file *m, void *unused)
 	hlist_for_each_entry(proc, pos, &binder_procs, proc_node)
 		print_binder_proc(m, proc, 1);
 	if (do_lock)
-		mutex_unlock(&binder_lock);
+		binder_unlock(__func__);
 	return 0;
 }
 
@@ -3392,7 +3440,7 @@ static int binder_stats_show(struct seq_file *m, void *unused)
 	int do_lock = !binder_debug_no_lock;
 
 	if (do_lock)
-		mutex_lock(&binder_lock);
+		binder_lock(__func__);
 
 	seq_puts(m, "binder stats:\n");
 
@@ -3401,7 +3449,7 @@ static int binder_stats_show(struct seq_file *m, void *unused)
 	hlist_for_each_entry(proc, pos, &binder_procs, proc_node)
 		print_binder_proc_stats(m, proc);
 	if (do_lock)
-		mutex_unlock(&binder_lock);
+		binder_unlock(__func__);
 	return 0;
 }
 
@@ -3412,13 +3460,13 @@ static int binder_transactions_show(struct seq_file *m, void *unused)
 	int do_lock = !binder_debug_no_lock;
 
 	if (do_lock)
-		mutex_lock(&binder_lock);
+		binder_lock(__func__);
 
 	seq_puts(m, "binder transactions:\n");
 	hlist_for_each_entry(proc, pos, &binder_procs, proc_node)
 		print_binder_proc(m, proc, 0);
 	if (do_lock)
-		mutex_unlock(&binder_lock);
+		binder_unlock(__func__);
 	return 0;
 }
 
@@ -3428,11 +3476,11 @@ static int binder_proc_show(struct seq_file *m, void *unused)
 	int do_lock = !binder_debug_no_lock;
 
 	if (do_lock)
-		mutex_lock(&binder_lock);
+		binder_lock(__func__);
 	seq_puts(m, "binder proc state:\n");
 	print_binder_proc(m, proc, 1);
 	if (do_lock)
-		mutex_unlock(&binder_lock);
+		binder_unlock(__func__);
 	return 0;
 }
 
@@ -3527,4 +3575,7 @@ static int __init binder_init(void)
 
 device_initcall(binder_init);
 
+#define CREATE_TRACE_POINTS
+#include "binder_trace.h"
+
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/staging/android/binder_trace.h b/drivers/staging/android/binder_trace.h
new file mode 100644
index 0000000..82a567c
--- /dev/null
+++ b/drivers/staging/android/binder_trace.h
@@ -0,0 +1,327 @@
+/*
+ * Copyright (C) 2012 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM binder
+
+#if !defined(_BINDER_TRACE_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _BINDER_TRACE_H
+
+#include <linux/tracepoint.h>
+
+struct binder_buffer;
+struct binder_node;
+struct binder_proc;
+struct binder_ref;
+struct binder_thread;
+struct binder_transaction;
+
+TRACE_EVENT(binder_ioctl,
+	TP_PROTO(unsigned int cmd, unsigned long arg),
+	TP_ARGS(cmd, arg),
+
+	TP_STRUCT__entry(
+		__field(unsigned int, cmd)
+		__field(unsigned long, arg)
+	),
+	TP_fast_assign(
+		__entry->cmd = cmd;
+		__entry->arg = arg;
+	),
+	TP_printk("cmd=0x%x arg=0x%lx", __entry->cmd, __entry->arg)
+);
+
+DECLARE_EVENT_CLASS(binder_lock_class,
+	TP_PROTO(const char *tag),
+	TP_ARGS(tag),
+	TP_STRUCT__entry(
+		__field(const char *, tag)
+	),
+	TP_fast_assign(
+		__entry->tag = tag;
+	),
+	TP_printk("tag=%s", __entry->tag)
+);
+
+#define DEFINE_BINDER_LOCK_EVENT(name)	\
+DEFINE_EVENT(binder_lock_class, name,	\
+	TP_PROTO(const char *func), \
+	TP_ARGS(func))
+
+DEFINE_BINDER_LOCK_EVENT(binder_lock);
+DEFINE_BINDER_LOCK_EVENT(binder_locked);
+DEFINE_BINDER_LOCK_EVENT(binder_unlock);
+
+DECLARE_EVENT_CLASS(binder_function_return_class,
+	TP_PROTO(int ret),
+	TP_ARGS(ret),
+	TP_STRUCT__entry(
+		__field(int, ret)
+	),
+	TP_fast_assign(
+		__entry->ret = ret;
+	),
+	TP_printk("ret=%d", __entry->ret)
+);
+
+#define DEFINE_BINDER_FUNCTION_RETURN_EVENT(name)	\
+DEFINE_EVENT(binder_function_return_class, name,	\
+	TP_PROTO(int ret), \
+	TP_ARGS(ret))
+
+DEFINE_BINDER_FUNCTION_RETURN_EVENT(binder_ioctl_done);
+DEFINE_BINDER_FUNCTION_RETURN_EVENT(binder_write_done);
+DEFINE_BINDER_FUNCTION_RETURN_EVENT(binder_read_done);
+
+TRACE_EVENT(binder_wait_for_work,
+	TP_PROTO(bool proc_work, bool transaction_stack, bool thread_todo),
+	TP_ARGS(proc_work, transaction_stack, thread_todo),
+
+	TP_STRUCT__entry(
+		__field(bool, proc_work)
+		__field(bool, transaction_stack)
+		__field(bool, thread_todo)
+	),
+	TP_fast_assign(
+		__entry->proc_work = proc_work;
+		__entry->transaction_stack = transaction_stack;
+		__entry->thread_todo = thread_todo;
+	),
+	TP_printk("proc_work=%d transaction_stack=%d thread_todo=%d",
+		  __entry->proc_work, __entry->transaction_stack,
+		  __entry->thread_todo)
+);
+
+TRACE_EVENT(binder_transaction,
+	TP_PROTO(bool reply, struct binder_transaction *t,
+		 struct binder_node *target_node),
+	TP_ARGS(reply, t, target_node),
+	TP_STRUCT__entry(
+		__field(int, debug_id)
+		__field(int, target_node)
+		__field(int, to_proc)
+		__field(int, to_thread)
+		__field(int, reply)
+		__field(unsigned int, code)
+		__field(unsigned int, flags)
+	),
+	TP_fast_assign(
+		__entry->debug_id = t->debug_id;
+		__entry->target_node = target_node ? target_node->debug_id : 0;
+		__entry->to_proc = t->to_proc->pid;
+		__entry->to_thread = t->to_thread ? t->to_thread->pid : 0;
+		__entry->reply = reply;
+		__entry->code = t->code;
+		__entry->flags = t->flags;
+	),
+	TP_printk("transaction=%d dest_node=%d dest_proc=%d dest_thread=%d reply=%d flags=0x%x code=0x%x",
+		  __entry->debug_id, __entry->target_node,
+		  __entry->to_proc, __entry->to_thread,
+		  __entry->reply, __entry->flags, __entry->code)
+);
+
+TRACE_EVENT(binder_transaction_received,
+	TP_PROTO(struct binder_transaction *t),
+	TP_ARGS(t),
+
+	TP_STRUCT__entry(
+		__field(int, debug_id)
+	),
+	TP_fast_assign(
+		__entry->debug_id = t->debug_id;
+	),
+	TP_printk("transaction=%d", __entry->debug_id)
+);
+
+TRACE_EVENT(binder_transaction_node_to_ref,
+	TP_PROTO(struct binder_transaction *t, struct binder_node *node,
+		 struct binder_ref *ref),
+	TP_ARGS(t, node, ref),
+
+	TP_STRUCT__entry(
+		__field(int, debug_id)
+		__field(int, node_debug_id)
+		__field(void __user *, node_ptr)
+		__field(int, ref_debug_id)
+		__field(uint32_t, ref_desc)
+	),
+	TP_fast_assign(
+		__entry->debug_id = t->debug_id;
+		__entry->node_debug_id = node->debug_id;
+		__entry->node_ptr = node->ptr;
+		__entry->ref_debug_id = ref->debug_id;
+		__entry->ref_desc = ref->desc;
+	),
+	TP_printk("transaction=%d node=%d src_ptr=0x%p ==> dest_ref=%d dest_desc=%d",
+		  __entry->debug_id, __entry->node_debug_id, __entry->node_ptr,
+		  __entry->ref_debug_id, __entry->ref_desc)
+);
+
+TRACE_EVENT(binder_transaction_ref_to_node,
+	TP_PROTO(struct binder_transaction *t, struct binder_ref *ref),
+	TP_ARGS(t, ref),
+
+	TP_STRUCT__entry(
+		__field(int, debug_id)
+		__field(int, ref_debug_id)
+		__field(uint32_t, ref_desc)
+		__field(int, node_debug_id)
+		__field(void __user *, node_ptr)
+	),
+	TP_fast_assign(
+		__entry->debug_id = t->debug_id;
+		__entry->ref_debug_id = ref->debug_id;
+		__entry->ref_desc = ref->desc;
+		__entry->node_debug_id = ref->node->debug_id;
+		__entry->node_ptr = ref->node->ptr;
+	),
+	TP_printk("transaction=%d node=%d src_ref=%d src_desc=%d ==> dest_ptr=0x%p",
+		  __entry->debug_id, __entry->node_debug_id,
+		  __entry->ref_debug_id, __entry->ref_desc, __entry->node_ptr)
+);
+
+TRACE_EVENT(binder_transaction_ref_to_ref,
+	TP_PROTO(struct binder_transaction *t, struct binder_ref *src_ref,
+		 struct binder_ref *dest_ref),
+	TP_ARGS(t, src_ref, dest_ref),
+
+	TP_STRUCT__entry(
+		__field(int, debug_id)
+		__field(int, node_debug_id)
+		__field(int, src_ref_debug_id)
+		__field(uint32_t, src_ref_desc)
+		__field(int, dest_ref_debug_id)
+		__field(uint32_t, dest_ref_desc)
+	),
+	TP_fast_assign(
+		__entry->debug_id = t->debug_id;
+		__entry->node_debug_id = src_ref->node->debug_id;
+		__entry->src_ref_debug_id = src_ref->debug_id;
+		__entry->src_ref_desc = src_ref->desc;
+		__entry->dest_ref_debug_id = dest_ref->debug_id;
+		__entry->dest_ref_desc = dest_ref->desc;
+	),
+	TP_printk("transaction=%d node=%d src_ref=%d src_desc=%d ==> dest_ref=%d dest_desc=%d",
+		  __entry->debug_id, __entry->node_debug_id,
+		  __entry->src_ref_debug_id, __entry->src_ref_desc,
+		  __entry->dest_ref_debug_id, __entry->dest_ref_desc)
+);
+
+TRACE_EVENT(binder_transaction_fd,
+	TP_PROTO(struct binder_transaction *t, int src_fd, int dest_fd),
+	TP_ARGS(t, src_fd, dest_fd),
+
+	TP_STRUCT__entry(
+		__field(int, debug_id)
+		__field(int, src_fd)
+		__field(int, dest_fd)
+	),
+	TP_fast_assign(
+		__entry->debug_id = t->debug_id;
+		__entry->src_fd = src_fd;
+		__entry->dest_fd = dest_fd;
+	),
+	TP_printk("transaction=%d src_fd=%d ==> dest_fd=%d",
+		  __entry->debug_id, __entry->src_fd, __entry->dest_fd)
+);
+
+DECLARE_EVENT_CLASS(binder_buffer_class,
+	TP_PROTO(struct binder_buffer *buf),
+	TP_ARGS(buf),
+	TP_STRUCT__entry(
+		__field(int, debug_id)
+		__field(size_t, data_size)
+		__field(size_t, offsets_size)
+	),
+	TP_fast_assign(
+		__entry->debug_id = buf->debug_id;
+		__entry->data_size = buf->data_size;
+		__entry->offsets_size = buf->offsets_size;
+	),
+	TP_printk("transaction=%d data_size=%zd offsets_size=%zd",
+		  __entry->debug_id, __entry->data_size, __entry->offsets_size)
+);
+
+DEFINE_EVENT(binder_buffer_class, binder_transaction_alloc_buf,
+	TP_PROTO(struct binder_buffer *buffer),
+	TP_ARGS(buffer));
+
+DEFINE_EVENT(binder_buffer_class, binder_transaction_buffer_release,
+	TP_PROTO(struct binder_buffer *buffer),
+	TP_ARGS(buffer));
+
+DEFINE_EVENT(binder_buffer_class, binder_transaction_failed_buffer_release,
+	TP_PROTO(struct binder_buffer *buffer),
+	TP_ARGS(buffer));
+
+TRACE_EVENT(binder_update_page_range,
+	TP_PROTO(struct binder_proc *proc, bool allocate,
+		 void *start, void *end),
+	TP_ARGS(proc, allocate, start, end),
+	TP_STRUCT__entry(
+		__field(int, proc)
+		__field(bool, allocate)
+		__field(size_t, offset)
+		__field(size_t, size)
+	),
+	TP_fast_assign(
+		__entry->proc = proc->pid;
+		__entry->allocate = allocate;
+		__entry->offset = start - proc->buffer;
+		__entry->size = end - start;
+	),
+	TP_printk("proc=%d allocate=%d offset=%zu size=%zu",
+		  __entry->proc, __entry->allocate,
+		  __entry->offset, __entry->size)
+);
+
+TRACE_EVENT(binder_command,
+	TP_PROTO(uint32_t cmd),
+	TP_ARGS(cmd),
+	TP_STRUCT__entry(
+		__field(uint32_t, cmd)
+	),
+	TP_fast_assign(
+		__entry->cmd = cmd;
+	),
+	TP_printk("cmd=0x%x %s",
+		  __entry->cmd,
+		  _IOC_NR(__entry->cmd) < ARRAY_SIZE(binder_command_strings) ?
+			  binder_command_strings[_IOC_NR(__entry->cmd)] :
+			  "unknown")
+);
+
+TRACE_EVENT(binder_return,
+	TP_PROTO(uint32_t cmd),
+	TP_ARGS(cmd),
+	TP_STRUCT__entry(
+		__field(uint32_t, cmd)
+	),
+	TP_fast_assign(
+		__entry->cmd = cmd;
+	),
+	TP_printk("cmd=0x%x %s",
+		  __entry->cmd,
+		  _IOC_NR(__entry->cmd) < ARRAY_SIZE(binder_return_strings) ?
+			  binder_return_strings[_IOC_NR(__entry->cmd)] :
+			  "unknown")
+);
+
+#endif /* _BINDER_TRACE_H */
+
+#undef TRACE_INCLUDE_PATH
+#undef TRACE_INCLUDE_FILE
+#define TRACE_INCLUDE_PATH .
+#define TRACE_INCLUDE_FILE binder_trace
+#include <trace/define_trace.h>
-- 
1.7.7.3

[PATCH 3/4] Staging: android: binder: Fix memory leak on thread/process exit

[Arve Hjønnevåg]

If a thread or process exited while a reply, one-way transaction or
death notification was pending, the struct holding the pending work
was leaked.

Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 drivers/staging/android/binder.c |   28 +++++++++++++++++++++++++++-
 1 files changed, 27 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index b7cfcdb..91ce956 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -2460,14 +2460,38 @@ static void binder_release_work(struct list_head *list)
 			struct binder_transaction *t;
 
 			t = container_of(w, struct binder_transaction, work);
-			if (t->buffer->target_node && !(t->flags & TF_ONE_WAY))
+			if (t->buffer->target_node &&
+			    !(t->flags & TF_ONE_WAY)) {
 				binder_send_failed_reply(t, BR_DEAD_REPLY);
+			} else {
+				binder_debug(BINDER_DEBUG_DEAD_TRANSACTION,
+					"binder: undelivered transaction %d\n",
+					t->debug_id);
+				t->buffer->transaction = NULL;
+				kfree(t);
+				binder_stats_deleted(BINDER_STAT_TRANSACTION);
+			}
 		} break;
 		case BINDER_WORK_TRANSACTION_COMPLETE: {
+			binder_debug(BINDER_DEBUG_DEAD_TRANSACTION,
+				"binder: undelivered TRANSACTION_COMPLETE\n");
 			kfree(w);
 			binder_stats_deleted(BINDER_STAT_TRANSACTION_COMPLETE);
 		} break;
+		case BINDER_WORK_DEAD_BINDER_AND_CLEAR:
+		case BINDER_WORK_CLEAR_DEATH_NOTIFICATION: {
+			struct binder_ref_death *death;
+
+			death = container_of(w, struct binder_ref_death, work);
+			binder_debug(BINDER_DEBUG_DEAD_TRANSACTION,
+				"binder: undelivered death notification, %p\n",
+				death->cookie);
+			kfree(death);
+			binder_stats_deleted(BINDER_STAT_DEATH);
+		} break;
 		default:
+			pr_err("binder: unexpected work type, %d, not freed\n",
+			       w->type);
 			break;
 		}
 	}
@@ -2951,6 +2975,7 @@ static void binder_deferred_release(struct binder_proc *proc)
 		nodes++;
 		rb_erase(&node->rb_node, &proc->nodes);
 		list_del_init(&node->work.entry);
+		binder_release_work(&node->async_todo);
 		if (hlist_empty(&node->refs)) {
 			kfree(node);
 			binder_stats_deleted(BINDER_STAT_NODE);
@@ -2989,6 +3014,7 @@ static void binder_deferred_release(struct binder_proc *proc)
 		binder_delete_ref(ref);
 	}
 	binder_release_work(&proc->todo);
+	binder_release_work(&proc->delivered_death);
 	buffers = 0;
 
 	while ((n = rb_first(&proc->allocated_buffers))) {
-- 
1.7.7.3

[PATCH 4/4] Staging: android: binder: Allow using highmem for binder buffers

[Arve Hjønnevåg]

The default kernel mapping for the pages allocated for the binder
buffers is never used. Set the __GFP_HIGHMEM flag when allocating
these pages so we don't needlessly use low memory pages that may
be required elsewhere.

Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 drivers/staging/android/binder.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 91ce956..9fbc06e 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -583,7 +583,7 @@ static int binder_update_page_range(struct binder_proc *proc, int allocate,
 		page = &proc->pages[(page_addr - proc->buffer) / PAGE_SIZE];
 
 		BUG_ON(*page);
-		*page = alloc_page(GFP_KERNEL | __GFP_ZERO);
+		*page = alloc_page(GFP_KERNEL | __GFP_HIGHMEM | __GFP_ZERO);
 		if (*page == NULL) {
 			pr_err("binder: %d: binder_alloc_buf failed "
 			       "for page at %p\n", proc->pid, page_addr);
-- 
1.7.7.3

Re: Fix memory leak in binder--version2

[Arve Hjønnevåg]

On Tue, Oct 16, 2012 at 12:11 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Mon, Oct 15, 2012 at 05:39:22PM -0700, Arve Hjønnevåg wrote:
>> On Mon, Oct 15, 2012 at 4:58 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Mon, Oct 15, 2012 at 04:55:21PM -0700, Arve Hjønnevåg wrote:
>> >> On Mon, Oct 15, 2012 at 3:52 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> >> > On Mon, Oct 15, 2012 at 07:20:01AM +0000, Tu, Xiaobing wrote:
>> >> >> After enabling kmemleak and run monkey, following memleak is reported:
>> >> >> unreferenced object 0xeed27f80 (size 64):
>> >> >>   comm "Binder_8", pid 641, jiffies 4294946341 (age 2275.810s)
>> >> >>   hex dump (first 32 bytes):
>> >> >>     4f dd 00 00 84 7f d2 ee 84 7f d2 ee 01 00 00 00  O...............
>> >> >>     00 00 00 00 00 00 00 00 00 aa 4c d7 00 00 00 00  ..........L.....
>> >> >>   backtrace:
>> >> >>     [<c184fabc>] kmemleak_alloc+0x3c/0xa0
>> >> >>     [<c12f391e>] kmem_cache_alloc_trace+0x9e/0x180
>> >> >>     [<c1668bb5>] binder_thread_write+0xcf5/0x23a0
>> >> >>     [<c166b091>] binder_ioctl+0x1f1/0x530
>> >> >>     [<c130dcf6>] do_vfs_ioctl+0x86/0x5e0
>> >> >>     [<c130e282>] sys_ioctl+0x32/0x60
>> >> >>     [<c1872e01>] syscall_call+0x7/0xb
>> >> >>     [<ffffffff>] 0xffffffff
>> >> >>
>> >> >> The work item in async_todo list is not freed when binder released.
>> >> >> Also the async transaction should also be freed in binder_release_work.
>> >> >>
>> >> >> Signed-off-by: Leon Ma <xindong.ma@intel.com>
>> >> >> Signed-off-by: Di Zhang <di.zhang@intel.com>
>> >> >> ---
>> >> >>  drivers/staging/android/binder.c |    6 ++++++
>> >> >>  1 files changed, 6 insertions(+), 0 deletions(-)
>> >> >
>> >> > Nice fix, but next time can you at least use scripts/get_maintainer.pl
>> >> > to figure out who to send this to?  I'll queue it up soon, but it took
>> >> > akpm to point me at this for me to notice it.
>> >>
>> >> This patch was just pointer out to me as well. I have a similar fix
>> >> queued up at https://android-review.googlesource.com/#/c/43801/ that
>> >> is still being tested. It fixes this leak and a theoretical leak of
>> >> death notification objects.
>> >
>> > Ok, should I hold off applying this patch and wait for your patch
>> > instead?
>> >
>>
>> I just sent two patches, but they are not fully tested yet.
>
> Ok, care to test them and after that, resend them with the changes I
> noted in the changelog section of the patch?
>

All the new cleanup code has been tested now. I also included two
older patches that are not in mainline.

-- 
Arve Hjønnevåg