答复: [PATCH] ANDROID: binder: fix binder work return error is wrongly consumed
diff mbox series

Message ID 1525276537088.37613@xiaomi.com
State New, archived
Headers show
Series
  • 答复: [PATCH] ANDROID: binder: fix binder work return error is wrongly consumed
Related show

Commit Message

袁辉辉 May 2, 2018, 3:55 p.m. UTC
Hi,

I add one nesting  scenarios which is binder work return error is wrongly consumed in here: 
https://issuetracker.google.com/u/0/issues/79123724

Interpretation of the  process see attached:
(+ stands for production, - stands for consumption,BW_XXX is a shorthand for BINDE_WORK_XXX)

1) The first oneway reportOneDeath() consumed BINDER_WORK_RETURN_ERROR which is producted by bindApplication()

2) Ths second non-oneway reportOneDeath() consumed BR_TRANSACTION_COMPLETE which left by the first reportOneDeath()

3) The thread will hang in bindApplication() forever, because BINDER_WORK_RETURN_ERROR  
which is producted by bindApplication() is wrongly consumed by reportOneDeath()

Patch
diff mbox series

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 4eab5be3d00f..1ed1809b8769 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -861,6 +861,60 @@  binder_enqueue_thread_work(struct binder_thread *thread,
        binder_inner_proc_unlock(thread->proc);
 }

+/**
+ * binder_enqueue_work_head_ilocked() - Add an item to the head of work list
+ * @work:         struct binder_work to add to list
+ * @target_list:  list to add work to
+ *
+ * Adds the work to the specified list. Asserts that work
+ * is not already on a list.
+ *
+ * Requires the proc->inner_lock to be held.
+ */
+static void
+binder_enqueue_work_head_ilocked(struct binder_work *work,
+                          struct list_head *target_list)
+{
+       BUG_ON(target_list == NULL);
+       BUG_ON(work->entry.next && !list_empty(&work->entry));
+       list_add(&work->entry, target_list);
+}
+
+/**
+ * binder_enqueue_thread_work_head_ilocked() - Add an item to the head of thread work list
+ * @thread:       thread to queue work to
+ * @work:         struct binder_work to add to list
+ *
+ * Adds the work to the head of thread todo list, and enables processing
+ * of the todo queue.
+ *
+ * Requires the proc->inner_lock to be held.
+ */
+static void
+binder_enqueue_thread_work_head_ilocked(struct binder_thread *thread,
+                                  struct binder_work *work)
+{
+       binder_enqueue_work_head_ilocked(work, &thread->todo);
+       thread->process_todo = true;
+}
+
+/**
+ * binder_enqueue_thread_work_head() - Add an item to the head of thread work list
+ * @thread:       thread to queue work to
+ * @work:         struct binder_work to add to list
+ *
+ * Adds the work to the head of thread todo list, and enables processing
+ * of the todo queue.
+ */
+static void
+binder_enqueue_thread_work_head(struct binder_thread *thread,
+                          struct binder_work *work)
+{
+       binder_inner_proc_lock(thread->proc);
+       binder_enqueue_thread_work_head_ilocked(thread, work);
+       binder_inner_proc_unlock(thread->proc);
+}
+
 static void
 binder_dequeue_work_ilocked(struct binder_work *work)
 {
@@ -3287,11 +3341,11 @@  static void binder_transaction(struct binder_proc *proc,
        BUG_ON(thread->return_error.cmd != BR_OK);
        if (in_reply_to) {
                thread->return_error.cmd = BR_TRANSACTION_COMPLETE;
-               binder_enqueue_thread_work(thread, &thread->return_error.work);
+               binder_enqueue_thread_work_head(thread, &thread->return_error.work);
                binder_send_failed_reply(in_reply_to, return_error);
        } else {
                thread->return_error.cmd = return_error;
-               binder_enqueue_thread_work(thread, &thread->return_error.work);
+               binder_enqueue_thread_work_head(thread, &thread->return_error.work);
        }
 }

@@ -3929,6 +3983,7 @@  static int binder_thread_read(struct binder_proc *proc,
                        ptr += sizeof(uint32_t);

                        binder_stat_br(proc, thread, e->cmd);
+                       goto done; /* RETURN_ERROR notifications can finish transactions */
                } break;
                case BINDER_WORK_TRANSACTION_COMPLETE: {
                        binder_inner_proc_unlock(proc);