On Thu, Apr 21, 2022 at 04:20:36AM +0000, Carlos Llamas wrote:
> Provide a userspace mechanism to pull precise error information upon
> failed operations. Extending the current error codes returned by the
> interfaces allows userspace to better determine the course of action.
> This could be for instance, retrying a failed transaction at a later
> point and thus offloading the error handling from the driver.
>
> Some of the elements for logging failed transactions and similar are
> folded into this new logic to avoid duplication. Such is the case for
> error line numbers, which become irrelevant after assigning individual
> error messages instead.
>
> This patch also adds BINDER_GET_EXTENDED_ERROR to the binderfs feature
> list, to help userspace determine if the new ioctl is supported by the
> driver.
Hint, when you say "also" in a changelog text, that's a hint that this
should be more than one patch. The last thing should be a separate
change, right?
>
> Signed-off-by: Carlos Llamas <[email protected]>
> ---
> drivers/android/binder.c | 355 +++++++++++++++-------------
> drivers/android/binder_internal.h | 9 +-
> drivers/android/binderfs.c | 8 +
> include/uapi/linux/android/binder.h | 16 ++
> 4 files changed, 219 insertions(+), 169 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 8351c5638880..42a324634f25 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2697,6 +2697,54 @@ static struct binder_node *binder_get_node_refs_for_txn(
> return target_node;
> }
>
> +#define binder_txn_error(x...) binder_transaction_error(0, x)
> +#define binder_user_txn_error(x...) binder_transaction_error(1, x)
> +static __printf(6, 7) void binder_transaction_error(int user,
> + struct binder_transaction_log_entry *e,
> + struct binder_extended_error *ee,
> + uint32_t command, int32_t param,
> + const char *format, ...)
> +{
> + struct va_format vaf;
> + va_list args;
> +
> + /* don't override previous error */
> + if (command != BR_OK && ee->command != BR_OK)
> + return;
> +
> + ee->command = command;
> + ee->param = param;
> +
> + va_start(args, format);
> + vsnprintf(e->strerr, sizeof(e->strerr), format, args);
> +
> + vaf.va = &args;
> + vaf.fmt = format;
> + if (user)
> + binder_user_error("%d:%d %pV\n",
> + e->from_proc, e->from_thread, &vaf);
> + else
> + binder_debug(BINDER_DEBUG_FAILED_TRANSACTION, "%d:%d %pV\n",
> + e->from_proc, e->from_thread, &vaf);
> + va_end(args);
> +}
> +
> +static void binder_set_txn_from_error(struct binder_transaction *txn,
> + struct binder_extended_error *ee)
> +{
> + struct binder_thread *from = binder_get_txn_from_and_acq_inner(txn);
> +
> + if (!from)
> + return;
> +
> + /* don't override previous error */
> + if (ee->command != BR_OK && from->ee.command == BR_OK)
> + from->ee = *ee;
> +
> + binder_inner_proc_unlock(from->proc);
> + binder_thread_dec_tmpref(from);
> +}
> +
> static void binder_transaction(struct binder_proc *proc,
> struct binder_thread *thread,
> struct binder_transaction_data *tr, int reply,
> @@ -2716,9 +2764,8 @@ static void binder_transaction(struct binder_proc *proc,
> struct binder_node *target_node = NULL;
> struct binder_transaction *in_reply_to = NULL;
> struct binder_transaction_log_entry *e;
> + struct binder_extended_error ee;
> uint32_t return_error = 0;
> - uint32_t return_error_param = 0;
> - uint32_t return_error_line = 0;
> binder_size_t last_fixup_obj_off = 0;
> binder_size_t last_fixup_min_off = 0;
> struct binder_context *context = proc->context;
> @@ -2741,32 +2788,30 @@ static void binder_transaction(struct binder_proc *proc,
> e->data_size = tr->data_size;
> e->offsets_size = tr->offsets_size;
> strscpy(e->context_name, proc->context->name, BINDERFS_MAX_NAME);
> + ee.id = t_debug_id;
> + ee.command = BR_OK;
> + ee.param = 0;
>
> if (reply) {
> binder_inner_proc_lock(proc);
> in_reply_to = thread->transaction_stack;
> if (in_reply_to == NULL) {
> binder_inner_proc_unlock(proc);
> - binder_user_error("%d:%d got reply transaction with no transaction stack\n",
> - proc->pid, thread->pid);
> - return_error = BR_FAILED_REPLY;
> - return_error_param = -EPROTO;
> - return_error_line = __LINE__;
> + binder_user_txn_error(e, &ee, BR_FAILED_REPLY, -EPROTO,
> + "reply with no transaction stack");
> goto err_empty_call_stack;
> }
> if (in_reply_to->to_thread != thread) {
> spin_lock(&in_reply_to->lock);
> - binder_user_error("%d:%d got reply transaction with bad transaction stack, transaction %d has target %d:%d\n",
> - proc->pid, thread->pid, in_reply_to->debug_id,
> + binder_user_txn_error(e, &ee, BR_FAILED_REPLY, -EPROTO,
> + "bad transaction stack in reply to %d %d:%d",
> + in_reply_to->debug_id,
> in_reply_to->to_proc ?
> in_reply_to->to_proc->pid : 0,
> in_reply_to->to_thread ?
> in_reply_to->to_thread->pid : 0);
> spin_unlock(&in_reply_to->lock);
> binder_inner_proc_unlock(proc);
> - return_error = BR_FAILED_REPLY;
> - return_error_param = -EPROTO;
> - return_error_line = __LINE__;
> in_reply_to = NULL;
> goto err_bad_call_stack;
> }
> @@ -2777,20 +2822,17 @@ static void binder_transaction(struct binder_proc *proc,
> if (target_thread == NULL) {
> /* annotation for sparse */
> __release(&target_thread->proc->inner_lock);
> - return_error = BR_DEAD_REPLY;
> - return_error_line = __LINE__;
> + binder_txn_error(e, &ee, BR_DEAD_REPLY, 0,
> + "reply target not found");
> goto err_dead_binder;
> }
> if (target_thread->transaction_stack != in_reply_to) {
> - binder_user_error("%d:%d got reply transaction with bad target transaction stack %d, expected %d\n",
> - proc->pid, thread->pid,
> + binder_user_txn_error(e, &ee, BR_FAILED_REPLY, -EPROTO,
> + "bad target transaction stack %d vs %d",
> target_thread->transaction_stack ?
> target_thread->transaction_stack->debug_id : 0,
> in_reply_to->debug_id);
> binder_inner_proc_unlock(target_thread->proc);
> - return_error = BR_FAILED_REPLY;
> - return_error_param = -EPROTO;
> - return_error_line = __LINE__;
> in_reply_to = NULL;
> target_thread = NULL;
> goto err_dead_binder;
> @@ -2812,15 +2854,15 @@ static void binder_transaction(struct binder_proc *proc,
> binder_proc_lock(proc);
> ref = binder_get_ref_olocked(proc, tr->target.handle,
> true);
> - if (ref) {
> + if (ref)
> target_node = binder_get_node_refs_for_txn(
> ref->node, &target_proc,
> &return_error);
> - } else {
> - binder_user_error("%d:%d got transaction to invalid handle, %u\n",
> - proc->pid, thread->pid, tr->target.handle);
> - return_error = BR_FAILED_REPLY;
> - }
> + else
> + binder_user_txn_error(e, &ee, BR_FAILED_REPLY,
> + -EINVAL,
> + "invalid transaction handle %u",
> + tr->target.handle);
> binder_proc_unlock(proc);
> } else {
> mutex_lock(&context->context_mgr_node_lock);
> @@ -2833,11 +2875,9 @@ static void binder_transaction(struct binder_proc *proc,
> return_error = BR_DEAD_REPLY;
> mutex_unlock(&context->context_mgr_node_lock);
> if (target_node && target_proc->pid == proc->pid) {
> - binder_user_error("%d:%d got transaction to context manager from process owning it\n",
> - proc->pid, thread->pid);
> - return_error = BR_FAILED_REPLY;
> - return_error_param = -EINVAL;
> - return_error_line = __LINE__;
> + binder_user_txn_error(e, &ee, BR_FAILED_REPLY,
> + -EINVAL,
> + "forbidden self transaction by context manager");
> goto err_invalid_target_handle;
> }
> }
> @@ -2845,22 +2885,20 @@ static void binder_transaction(struct binder_proc *proc,
> /*
> * return_error is set above
> */
> - return_error_param = -EINVAL;
> - return_error_line = __LINE__;
> + binder_txn_error(e, &ee, return_error, -EINVAL,
> + "cannot find target node");
You do this a lot, how about making this one commit (first one), and
then adding the new "back end" to the error stuff in a second commit.
That would make it much easier to review, first commit does nothing new,
second one adds the new functionality, and third adds the feature flag.
thanks,
greg k-h
On Fri, Apr 22, 2022 at 05:25:07PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Apr 21, 2022 at 04:20:36AM +0000, Carlos Llamas wrote:
> > Provide a userspace mechanism to pull precise error information upon
> > failed operations. Extending the current error codes returned by the
> > interfaces allows userspace to better determine the course of action.
> > This could be for instance, retrying a failed transaction at a later
> > point and thus offloading the error handling from the driver.
> >
> > Some of the elements for logging failed transactions and similar are
> > folded into this new logic to avoid duplication. Such is the case for
> > error line numbers, which become irrelevant after assigning individual
> > error messages instead.
> >
> > This patch also adds BINDER_GET_EXTENDED_ERROR to the binderfs feature
> > list, to help userspace determine if the new ioctl is supported by the
> > driver.
>
> Hint, when you say "also" in a changelog text, that's a hint that this
> should be more than one patch. The last thing should be a separate
> change, right?
Yes it should. I now notice I forgot to add the binderfs feature to the
kselftests too, so I'll include that in v2.
> > @@ -2845,22 +2885,20 @@ static void binder_transaction(struct binder_proc *proc,
> > /*
> > * return_error is set above
> > */
> > - return_error_param = -EINVAL;
> > - return_error_line = __LINE__;
> > + binder_txn_error(e, &ee, return_error, -EINVAL,
> > + "cannot find target node");
>
> You do this a lot, how about making this one commit (first one), and
> then adding the new "back end" to the error stuff in a second commit.
> That would make it much easier to review, first commit does nothing new,
> second one adds the new functionality, and third adds the feature flag.
Yeah, that sounds like the appropiate split. Thanks!
>
> thanks,
>
> greg k-h
--
Carlos Llamas