Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Fri, 22 Apr 2022 17:25:07 +0200
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Carlos Llamas <cmllamas@google.com>
Cc:     Arve =?iso-8859-1?B?SGr4bm5lduVn?= <arve@android.com>,
        Todd Kjos <tkjos@android.com>,
        Martijn Coenen <maco@android.com>,
        Joel Fernandes <joel@joelfernandes.org>,
        Christian Brauner <brauner@kernel.org>,
        Hridya Valsaraju <hridya@google.com>,
        Suren Baghdasaryan <surenb@google.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Masahiro Yamada <masahiroy@kernel.org>,
        Li Li <dualli@google.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] binder: add BINDER_GET_EXTENDED_ERROR ioctl
Message-ID: <YmLI03OT6st9fcQD@kroah.com>
References: <20220421042040.759068-1-cmllamas@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220421042040.759068-1-cmllamas@google.com>
Precedence: bulk

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 <cmllamas@google.com>
> ---
>  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