drivers/android/binder.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-)
Identify buffer addresses using vma offsets instead of full user
addresses in debug logs.
Signed-off-by: Tiffany Y. Yang <ynaffit@google.com>
---
drivers/android/binder.c | 31 ++++++++++++++++---------------
1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index d1aa6d24450a..994ae205aa07 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3261,20 +3261,20 @@ static void binder_transaction(struct binder_proc *proc,
if (reply)
binder_debug(BINDER_DEBUG_TRANSACTION,
- "%d:%d BC_REPLY %d -> %d:%d, data %016llx-%016llx size %lld-%lld-%lld\n",
+ "%d:%d BC_REPLY %d -> %d:%d, buffer offset %lx-%lx size %lld-%lld-%lld\n",
proc->pid, thread->pid, t->debug_id,
target_proc->pid, target_thread->pid,
- (u64)tr->data.ptr.buffer,
- (u64)tr->data.ptr.offsets,
+ (unsigned long)tr->data.ptr.buffer - proc->alloc.buffer,
+ (unsigned long)tr->data.ptr.offsets - proc->alloc.buffer,
(u64)tr->data_size, (u64)tr->offsets_size,
(u64)extra_buffers_size);
else
binder_debug(BINDER_DEBUG_TRANSACTION,
- "%d:%d BC_TRANSACTION %d -> %d - node %d, data %016llx-%016llx size %lld-%lld-%lld\n",
+ "%d:%d BC_TRANSACTION %d -> %d - node %d, buffer offset %lx-%lx size %lld-%lld-%lld\n",
proc->pid, thread->pid, t->debug_id,
target_proc->pid, target_node->debug_id,
- (u64)tr->data.ptr.buffer,
- (u64)tr->data.ptr.offsets,
+ (unsigned long)tr->data.ptr.buffer - proc->alloc.buffer,
+ (unsigned long)tr->data.ptr.offsets - proc->alloc.buffer,
(u64)tr->data_size, (u64)tr->offsets_size,
(u64)extra_buffers_size);
@@ -4223,20 +4223,21 @@ static int binder_thread_write(struct binder_proc *proc,
if (IS_ERR_OR_NULL(buffer)) {
if (PTR_ERR(buffer) == -EPERM) {
binder_user_error(
- "%d:%d BC_FREE_BUFFER u%016llx matched unreturned or currently freeing buffer\n",
+ "%d:%d BC_FREE_BUFFER matched unreturned or currently freeing buffer at offset %lx\n",
proc->pid, thread->pid,
- (u64)data_ptr);
+ (unsigned long)data_ptr - proc->alloc.buffer);
} else {
binder_user_error(
- "%d:%d BC_FREE_BUFFER u%016llx no match\n",
+ "%d:%d BC_FREE_BUFFER no match for buffer at offset %lx\n",
proc->pid, thread->pid,
- (u64)data_ptr);
+ (unsigned long)data_ptr - proc->alloc.buffer);
}
break;
}
binder_debug(BINDER_DEBUG_FREE_BUFFER,
- "%d:%d BC_FREE_BUFFER u%016llx found buffer %d for %s transaction\n",
- proc->pid, thread->pid, (u64)data_ptr,
+ "%d:%d BC_FREE_BUFFER at offset %lx found buffer %d for %s transaction\n",
+ proc->pid, thread->pid,
+ (unsigned long)data_ptr - proc->alloc.buffer,
buffer->debug_id,
buffer->transaction ? "active" : "finished");
binder_free_buf(proc, thread, buffer, false);
@@ -5053,7 +5054,7 @@ static int binder_thread_read(struct binder_proc *proc,
trace_binder_transaction_received(t);
binder_stat_br(proc, thread, cmd);
binder_debug(BINDER_DEBUG_TRANSACTION,
- "%d:%d %s %d %d:%d, cmd %u size %zd-%zd ptr %016llx-%016llx\n",
+ "%d:%d %s %d %d:%d, cmd %u size %zd-%zd ptr offset %lx-%lx\n",
proc->pid, thread->pid,
(cmd == BR_TRANSACTION) ? "BR_TRANSACTION" :
(cmd == BR_TRANSACTION_SEC_CTX) ?
@@ -5061,8 +5062,8 @@ static int binder_thread_read(struct binder_proc *proc,
t->debug_id, t_from ? t_from->proc->pid : 0,
t_from ? t_from->pid : 0, cmd,
t->buffer->data_size, t->buffer->offsets_size,
- (u64)trd->data.ptr.buffer,
- (u64)trd->data.ptr.offsets);
+ (unsigned long)trd->data.ptr.buffer - proc->alloc.buffer,
+ (unsigned long)trd->data.ptr.offsets - proc->alloc.buffer);
if (t_from)
binder_thread_dec_tmpref(t_from);
--
2.49.0.395.g12beb8f557-googOn Mon, Mar 24, 2025 at 06:07:18PM +0000, Tiffany Y. Yang wrote: > Identify buffer addresses using vma offsets instead of full user > addresses in debug logs. > > Signed-off-by: Tiffany Y. Yang <ynaffit@google.com> > --- > drivers/android/binder.c | 31 ++++++++++++++++--------------- > 1 file changed, 16 insertions(+), 15 deletions(-) > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index d1aa6d24450a..994ae205aa07 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -3261,20 +3261,20 @@ static void binder_transaction(struct binder_proc *proc, > > if (reply) > binder_debug(BINDER_DEBUG_TRANSACTION, > - "%d:%d BC_REPLY %d -> %d:%d, data %016llx-%016llx size %lld-%lld-%lld\n", > + "%d:%d BC_REPLY %d -> %d:%d, buffer offset %lx-%lx size %lld-%lld-%lld\n", > proc->pid, thread->pid, t->debug_id, > target_proc->pid, target_thread->pid, > - (u64)tr->data.ptr.buffer, > - (u64)tr->data.ptr.offsets, > + (unsigned long)tr->data.ptr.buffer - proc->alloc.buffer, > + (unsigned long)tr->data.ptr.offsets - proc->alloc.buffer, These could be pointers to anywhere in user memory, not necessarily the alloc->buffer. So there will be cases where this substraction doesn't make sense. However, you are correct that we shouldn't log these addrs so maybe just don't? wdyt? > (u64)tr->data_size, (u64)tr->offsets_size, > (u64)extra_buffers_size); > else > binder_debug(BINDER_DEBUG_TRANSACTION, > - "%d:%d BC_TRANSACTION %d -> %d - node %d, data %016llx-%016llx size %lld-%lld-%lld\n", > + "%d:%d BC_TRANSACTION %d -> %d - node %d, buffer offset %lx-%lx size %lld-%lld-%lld\n", > proc->pid, thread->pid, t->debug_id, > target_proc->pid, target_node->debug_id, > - (u64)tr->data.ptr.buffer, > - (u64)tr->data.ptr.offsets, > + (unsigned long)tr->data.ptr.buffer - proc->alloc.buffer, > + (unsigned long)tr->data.ptr.offsets - proc->alloc.buffer, same here. Regards, -- Carlos Llamas
Carlos Llamas <cmllamas@google.com> writes: > On Mon, Mar 24, 2025 at 06:07:18PM +0000, Tiffany Y. Yang wrote: >> Identify buffer addresses using vma offsets instead of full user >> addresses in debug logs. >> >> Signed-off-by: Tiffany Y. Yang <ynaffit@google.com> >> --- >> drivers/android/binder.c | 31 ++++++++++++++++--------------- >> 1 file changed, 16 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/android/binder.c b/drivers/android/binder.c >> index d1aa6d24450a..994ae205aa07 100644 >> --- a/drivers/android/binder.c >> +++ b/drivers/android/binder.c >> @@ -3261,20 +3261,20 @@ static void binder_transaction(struct binder_proc *proc, >> >> if (reply) >> binder_debug(BINDER_DEBUG_TRANSACTION, >> - "%d:%d BC_REPLY %d -> %d:%d, data %016llx-%016llx size %lld-%lld-%lld\n", >> + "%d:%d BC_REPLY %d -> %d:%d, buffer offset %lx-%lx size %lld-%lld-%lld\n", >> proc->pid, thread->pid, t->debug_id, >> target_proc->pid, target_thread->pid, >> - (u64)tr->data.ptr.buffer, >> - (u64)tr->data.ptr.offsets, >> + (unsigned long)tr->data.ptr.buffer - proc->alloc.buffer, >> + (unsigned long)tr->data.ptr.offsets - proc->alloc.buffer, > > These could be pointers to anywhere in user memory, not necessarily the > alloc->buffer. So there will be cases where this substraction doesn't > make sense. However, you are correct that we shouldn't log these addrs > so maybe just don't? wdyt? > Ah, in that case I think it makes sense to remove them here. What do you think about printing the full buffer and offsets values in cases where we would print a binder_user_error or binder_transaction_error instead. Ideally, I would try to limit this to cases when the data or offsets ptr is invalid / copy would fail. Ostensibly this wouldn't reveal dangerous information about the user address space because the print statements would only happen when the data wasn't where it was supposed to be and it would help with debugging, but I'm not sure if this line of thought makes sense... >> (u64)tr->data_size, (u64)tr->offsets_size, >> (u64)extra_buffers_size); >> else >> binder_debug(BINDER_DEBUG_TRANSACTION, >> - "%d:%d BC_TRANSACTION %d -> %d - node %d, data %016llx-%016llx size %lld-%lld-%lld\n", >> + "%d:%d BC_TRANSACTION %d -> %d - node %d, buffer offset %lx-%lx size %lld-%lld-%lld\n", >> proc->pid, thread->pid, t->debug_id, >> target_proc->pid, target_node->debug_id, >> - (u64)tr->data.ptr.buffer, >> - (u64)tr->data.ptr.offsets, >> + (unsigned long)tr->data.ptr.buffer - proc->alloc.buffer, >> + (unsigned long)tr->data.ptr.offsets - proc->alloc.buffer, > same here. > > Regards,
On Tue, Mar 25, 2025 at 12:41:39AM +0000, Tiffany Y. Yang wrote: > Carlos Llamas <cmllamas@google.com> writes: > > > On Mon, Mar 24, 2025 at 06:07:18PM +0000, Tiffany Y. Yang wrote: > >> Identify buffer addresses using vma offsets instead of full user > >> addresses in debug logs. > >> > >> Signed-off-by: Tiffany Y. Yang <ynaffit@google.com> > >> --- > >> drivers/android/binder.c | 31 ++++++++++++++++--------------- > >> 1 file changed, 16 insertions(+), 15 deletions(-) > >> > >> diff --git a/drivers/android/binder.c b/drivers/android/binder.c > >> index d1aa6d24450a..994ae205aa07 100644 > >> --- a/drivers/android/binder.c > >> +++ b/drivers/android/binder.c > >> @@ -3261,20 +3261,20 @@ static void binder_transaction(struct binder_proc *proc, > >> > >> if (reply) > >> binder_debug(BINDER_DEBUG_TRANSACTION, > >> - "%d:%d BC_REPLY %d -> %d:%d, data %016llx-%016llx size %lld-%lld-%lld\n", > >> + "%d:%d BC_REPLY %d -> %d:%d, buffer offset %lx-%lx size %lld-%lld-%lld\n", > >> proc->pid, thread->pid, t->debug_id, > >> target_proc->pid, target_thread->pid, > >> - (u64)tr->data.ptr.buffer, > >> - (u64)tr->data.ptr.offsets, > >> + (unsigned long)tr->data.ptr.buffer - proc->alloc.buffer, > >> + (unsigned long)tr->data.ptr.offsets - proc->alloc.buffer, > > > > These could be pointers to anywhere in user memory, not necessarily the > > alloc->buffer. So there will be cases where this substraction doesn't > > make sense. However, you are correct that we shouldn't log these addrs > > so maybe just don't? wdyt? > > > > Ah, in that case I think it makes sense to remove them here. What > do you think about printing the full buffer and offsets values in cases > where we would print a binder_user_error or binder_transaction_error > instead. Ideally, I would try to limit this to cases when the data or > offsets ptr is invalid / copy would fail. Ostensibly this wouldn't > reveal dangerous information about the user address space because the > print statements would only happen when the data wasn't where it was > supposed to be and it would help with debugging, but I'm not sure if > this line of thought makes sense... My 2 cents... I'm sure there will be a _few_ exceptions in which having the pointers from binder_transaction_data logged would aid debugging. However, this won't be info that most users care about. In practice, logging an error with "invalid buffer/offsets pointer" message is enough. There are _other_ pointers that users do care about when debugging, such as binder_ptr_cookie but not these. So I think is better if we don't log them at all, as calculating an "offset" is not possible either. -- Carlos Llamas
Carlos Llamas <cmllamas@google.com> writes: > On Tue, Mar 25, 2025 at 12:41:39AM +0000, Tiffany Y. Yang wrote: >> Carlos Llamas <cmllamas@google.com> writes: >> >> > On Mon, Mar 24, 2025 at 06:07:18PM +0000, Tiffany Y. Yang wrote: >> >> Identify buffer addresses using vma offsets instead of full user >> >> addresses in debug logs. >> >> >> >> Signed-off-by: Tiffany Y. Yang <ynaffit@google.com> >> >> --- >> >> drivers/android/binder.c | 31 ++++++++++++++++--------------- >> >> 1 file changed, 16 insertions(+), 15 deletions(-) >> >> >> >> diff --git a/drivers/android/binder.c b/drivers/android/binder.c >> >> index d1aa6d24450a..994ae205aa07 100644 >> >> --- a/drivers/android/binder.c >> >> +++ b/drivers/android/binder.c >> >> @@ -3261,20 +3261,20 @@ static void binder_transaction(struct binder_proc *proc, >> >> >> >> if (reply) >> >> binder_debug(BINDER_DEBUG_TRANSACTION, >> >> - "%d:%d BC_REPLY %d -> %d:%d, data %016llx-%016llx size %lld-%lld-%lld\n", >> >> + "%d:%d BC_REPLY %d -> %d:%d, buffer offset %lx-%lx size %lld-%lld-%lld\n", >> >> proc->pid, thread->pid, t->debug_id, >> >> target_proc->pid, target_thread->pid, >> >> - (u64)tr->data.ptr.buffer, >> >> - (u64)tr->data.ptr.offsets, >> >> + (unsigned long)tr->data.ptr.buffer - proc->alloc.buffer, >> >> + (unsigned long)tr->data.ptr.offsets - proc->alloc.buffer, >> > >> > These could be pointers to anywhere in user memory, not necessarily the >> > alloc->buffer. So there will be cases where this substraction doesn't >> > make sense. However, you are correct that we shouldn't log these addrs >> > so maybe just don't? wdyt? >> > >> >> Ah, in that case I think it makes sense to remove them here. What >> do you think about printing the full buffer and offsets values in cases >> where we would print a binder_user_error or binder_transaction_error >> instead. Ideally, I would try to limit this to cases when the data or >> offsets ptr is invalid / copy would fail. Ostensibly this wouldn't >> reveal dangerous information about the user address space because the >> print statements would only happen when the data wasn't where it was >> supposed to be and it would help with debugging, but I'm not sure if >> this line of thought makes sense... > > My 2 cents... > > I'm sure there will be a _few_ exceptions in which having the pointers > from binder_transaction_data logged would aid debugging. However, this > won't be info that most users care about. In practice, logging an error > with "invalid buffer/offsets pointer" message is enough. > > There are _other_ pointers that users do care about when debugging, such > as binder_ptr_cookie but not these. So I think is better if we don't log > them at all, as calculating an "offset" is not possible either. This makes sense to me! I'll drop them and send out another patch.
On Mon, Mar 24, 2025 at 06:07:18PM +0000, Tiffany Y. Yang wrote:
> Identify buffer addresses using vma offsets instead of full user
> addresses in debug logs.
>
> Signed-off-by: Tiffany Y. Yang <ynaffit@google.com>
> ---
> drivers/android/binder.c | 31 ++++++++++++++++---------------
> 1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index d1aa6d24450a..994ae205aa07 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -3261,20 +3261,20 @@ static void binder_transaction(struct binder_proc *proc,
>
> if (reply)
> binder_debug(BINDER_DEBUG_TRANSACTION,
> - "%d:%d BC_REPLY %d -> %d:%d, data %016llx-%016llx size %lld-%lld-%lld\n",
> + "%d:%d BC_REPLY %d -> %d:%d, buffer offset %lx-%lx size %lld-%lld-%lld\n",
> proc->pid, thread->pid, t->debug_id,
> target_proc->pid, target_thread->pid,
> - (u64)tr->data.ptr.buffer,
> - (u64)tr->data.ptr.offsets,
> + (unsigned long)tr->data.ptr.buffer - proc->alloc.buffer,
> + (unsigned long)tr->data.ptr.offsets - proc->alloc.buffer,
> (u64)tr->data_size, (u64)tr->offsets_size,
> (u64)extra_buffers_size);
> else
> binder_debug(BINDER_DEBUG_TRANSACTION,
> - "%d:%d BC_TRANSACTION %d -> %d - node %d, data %016llx-%016llx size %lld-%lld-%lld\n",
> + "%d:%d BC_TRANSACTION %d -> %d - node %d, buffer offset %lx-%lx size %lld-%lld-%lld\n",
> proc->pid, thread->pid, t->debug_id,
> target_proc->pid, target_node->debug_id,
> - (u64)tr->data.ptr.buffer,
> - (u64)tr->data.ptr.offsets,
> + (unsigned long)tr->data.ptr.buffer - proc->alloc.buffer,
> + (unsigned long)tr->data.ptr.offsets - proc->alloc.buffer,
> (u64)tr->data_size, (u64)tr->offsets_size,
> (u64)extra_buffers_size);
>
> @@ -4223,20 +4223,21 @@ static int binder_thread_write(struct binder_proc *proc,
> if (IS_ERR_OR_NULL(buffer)) {
> if (PTR_ERR(buffer) == -EPERM) {
> binder_user_error(
> - "%d:%d BC_FREE_BUFFER u%016llx matched unreturned or currently freeing buffer\n",
> + "%d:%d BC_FREE_BUFFER matched unreturned or currently freeing buffer at offset %lx\n",
> proc->pid, thread->pid,
> - (u64)data_ptr);
> + (unsigned long)data_ptr - proc->alloc.buffer);
> } else {
> binder_user_error(
> - "%d:%d BC_FREE_BUFFER u%016llx no match\n",
> + "%d:%d BC_FREE_BUFFER no match for buffer at offset %lx\n",
> proc->pid, thread->pid,
> - (u64)data_ptr);
> + (unsigned long)data_ptr - proc->alloc.buffer);
> }
> break;
> }
> binder_debug(BINDER_DEBUG_FREE_BUFFER,
> - "%d:%d BC_FREE_BUFFER u%016llx found buffer %d for %s transaction\n",
> - proc->pid, thread->pid, (u64)data_ptr,
> + "%d:%d BC_FREE_BUFFER at offset %lx found buffer %d for %s transaction\n",
> + proc->pid, thread->pid,
> + (unsigned long)data_ptr - proc->alloc.buffer,
> buffer->debug_id,
> buffer->transaction ? "active" : "finished");
> binder_free_buf(proc, thread, buffer, false);
> @@ -5053,7 +5054,7 @@ static int binder_thread_read(struct binder_proc *proc,
> trace_binder_transaction_received(t);
> binder_stat_br(proc, thread, cmd);
> binder_debug(BINDER_DEBUG_TRANSACTION,
> - "%d:%d %s %d %d:%d, cmd %u size %zd-%zd ptr %016llx-%016llx\n",
> + "%d:%d %s %d %d:%d, cmd %u size %zd-%zd ptr offset %lx-%lx\n",
> proc->pid, thread->pid,
> (cmd == BR_TRANSACTION) ? "BR_TRANSACTION" :
> (cmd == BR_TRANSACTION_SEC_CTX) ?
> @@ -5061,8 +5062,8 @@ static int binder_thread_read(struct binder_proc *proc,
> t->debug_id, t_from ? t_from->proc->pid : 0,
> t_from ? t_from->pid : 0, cmd,
> t->buffer->data_size, t->buffer->offsets_size,
> - (u64)trd->data.ptr.buffer,
> - (u64)trd->data.ptr.offsets);
> + (unsigned long)trd->data.ptr.buffer - proc->alloc.buffer,
> + (unsigned long)trd->data.ptr.offsets - proc->alloc.buffer);
>
> if (t_from)
> binder_thread_dec_tmpref(t_from);
> --
> 2.49.0.395.g12beb8f557-goog
>
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- This looks like a new version of a previously submitted patch, but you
did not list below the --- line any changes from the previous version.
Please read the section entitled "The canonical patch format" in the
kernel file, Documentation/process/submitting-patches.rst for what
needs to be done here to properly describe this.
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.
thanks,
greg k-h's patch email bot
© 2016 - 2025 Red Hat, Inc.