mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Carlos Llamas <cmllamas@google.com>
To: "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Arve Hjønnevåg" <arve@android.com>,
	"Todd Kjos" <tkjos@android.com>,
	"Martijn Coenen" <maco@android.com>,
	"Joel Fernandes" <joel@joelfernandes.org>,
	"Christian Brauner" <brauner@kernel.org>,
	"Carlos Llamas" <cmllamas@google.com>,
	"Suren Baghdasaryan" <surenb@google.com>
Cc: linux-kernel@vger.kernel.org, kernel-team@android.com
Subject: [PATCH 20/21] binder: reverse locking order in shrinker callback
Date: Thu,  2 Nov 2023 18:59:21 +0000	[thread overview]
Message-ID: <20231102185934.773885-21-cmllamas@google.com> (raw)
In-Reply-To: <20231102185934.773885-1-cmllamas@google.com>

The locking order currently requires the alloc->mutex to be acquired
first followed by the mmap lock. However, the alloc->mutex is converted
into a spinlock in subsequent commits so the order needs to be reversed
to avoid nesting the sleeping mmap lock under the spinlock.

The shrinker's callback binder_alloc_free_page() is the only place that
needs to be reordered since other functions have been refactored and no
longer nest these locks.

Some minor cosmetic changes are also included in this patch.

Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 44 ++++++++++++++++------------------
 1 file changed, 20 insertions(+), 24 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index e739be7f2dd4..0ba9f524e0ff 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -1065,35 +1065,38 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 				       void *cb_arg)
 	__must_hold(lock)
 {
-	struct mm_struct *mm = NULL;
-	struct binder_lru_page *page = container_of(item,
-						    struct binder_lru_page,
-						    lru);
-	struct binder_alloc *alloc;
+	struct binder_lru_page *page = container_of(item, typeof(*page), lru);
+	struct binder_alloc *alloc = page->alloc;
+	struct mm_struct *mm = alloc->mm;
+	struct vm_area_struct *vma;
 	unsigned long page_addr;
 	size_t index;
-	struct vm_area_struct *vma;
 
-	alloc = page->alloc;
+	if (!mmget_not_zero(mm))
+		goto err_mmget;
+	if (!mmap_read_trylock(mm))
+		goto err_mmap_read_lock_failed;
 	if (!mutex_trylock(&alloc->mutex))
 		goto err_get_alloc_mutex_failed;
-
 	if (!page->page_ptr)
 		goto err_page_already_freed;
 
 	index = page - alloc->pages;
 	page_addr = alloc->buffer + index * PAGE_SIZE;
 
-	mm = alloc->mm;
-	if (!mmget_not_zero(mm))
-		goto err_mmget;
-	if (!mmap_read_trylock(mm))
-		goto err_mmap_read_lock_failed;
 	vma = vma_lookup(mm, page_addr);
 	if (vma && vma != binder_alloc_get_vma(alloc))
 		goto err_invalid_vma;
 
+	trace_binder_unmap_kernel_start(alloc, index);
+
+	__free_page(page->page_ptr);
+	page->page_ptr = NULL;
+
+	trace_binder_unmap_kernel_end(alloc, index);
+
 	list_lru_isolate(lru, item);
+	mutex_unlock(&alloc->mutex);
 	spin_unlock(lock);
 
 	if (vma) {
@@ -1103,28 +1106,21 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 
 		trace_binder_unmap_user_end(alloc, index);
 	}
+
 	mmap_read_unlock(mm);
 	mmput_async(mm);
 
-	trace_binder_unmap_kernel_start(alloc, index);
-
-	__free_page(page->page_ptr);
-	page->page_ptr = NULL;
-
-	trace_binder_unmap_kernel_end(alloc, index);
-
 	spin_lock(lock);
-	mutex_unlock(&alloc->mutex);
 	return LRU_REMOVED_RETRY;
 
 err_invalid_vma:
+err_page_already_freed:
+	mutex_unlock(&alloc->mutex);
+err_get_alloc_mutex_failed:
 	mmap_read_unlock(mm);
 err_mmap_read_lock_failed:
 	mmput_async(mm);
 err_mmget:
-err_page_already_freed:
-	mutex_unlock(&alloc->mutex);
-err_get_alloc_mutex_failed:
 	return LRU_SKIP;
 }
 
-- 
2.42.0.869.gea05f2083d-goog


  parent reply	other threads:[~2023-11-02 19:02 UTC|newest]

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-11-02 18:59 [PATCH 00/21] binder: convert alloc->mutex to spinlock Carlos Llamas
2023-11-02 18:59 ` [PATCH 01/21] binder: use EPOLLERR from eventpoll.h Carlos Llamas
2023-11-07  9:07   ` Alice Ryhl
2023-11-02 18:59 ` [PATCH 02/21] binder: fix use-after-free in shinker's callback Carlos Llamas
2023-11-02 19:20   ` Liam R. Howlett
2023-11-02 20:09     ` Carlos Llamas
2023-11-02 20:27       ` Liam R. Howlett
2023-11-07  9:07   ` Alice Ryhl
2023-11-02 18:59 ` [PATCH 03/21] binder: fix race between mmput() and do_exit() Carlos Llamas
2023-11-07  9:07   ` Alice Ryhl
2023-11-02 18:59 ` [PATCH 04/21] binder: fix async space check for 0-sized buffers Carlos Llamas
2023-11-07  9:08   ` Alice Ryhl
2023-11-02 18:59 ` [PATCH 05/21] binder: fix trivial typo of binder_free_buf_locked() Carlos Llamas
2023-11-07  9:08   ` Alice Ryhl
2023-12-01  6:52     ` Carlos Llamas
2023-11-02 18:59 ` [PATCH 06/21] binder: fix comment on binder_alloc_new_buf() return value Carlos Llamas
2023-11-07  9:08   ` Alice Ryhl
2023-11-02 18:59 ` [PATCH 07/21] binder: remove extern from function prototypes Carlos Llamas
2023-11-07  9:08   ` Alice Ryhl
2023-11-02 18:59 ` [PATCH 08/21] binder: keep vma addresses type as unsigned long Carlos Llamas
2023-11-07  9:08   ` Alice Ryhl
2023-12-01  7:01     ` Carlos Llamas
2023-11-02 18:59 ` [PATCH 09/21] binder: split up binder_update_page_range() Carlos Llamas
2023-11-07  9:08   ` Alice Ryhl
2023-12-01  7:03     ` Carlos Llamas
2023-11-02 18:59 ` [PATCH 10/21] binder: do unlocked work in binder_alloc_new_buf() Carlos Llamas
2023-11-07  9:08   ` Alice Ryhl
2023-12-01  7:10     ` Carlos Llamas
2023-11-02 18:59 ` [PATCH 11/21] binder: remove pid param " Carlos Llamas
2023-11-07  9:08   ` Alice Ryhl
2023-11-02 18:59 ` [PATCH 12/21] binder: separate the no-space debugging logic Carlos Llamas
2023-11-07  9:08   ` Alice Ryhl
2023-11-02 18:59 ` [PATCH 13/21] binder: relocate low space calculation Carlos Llamas
2023-11-07  9:08   ` Alice Ryhl
2023-12-01  7:12     ` Carlos Llamas
2023-11-02 18:59 ` [PATCH 14/21] binder: do not add pages to LRU in release path Carlos Llamas
2023-11-07  9:08   ` Alice Ryhl
2023-12-01  7:15     ` Carlos Llamas
2023-11-02 18:59 ` [PATCH 15/21] binder: relocate binder_alloc_clear_buf() Carlos Llamas
2023-11-07  9:08   ` Alice Ryhl
2023-11-02 18:59 ` [PATCH 16/21] binder: refactor page range allocation Carlos Llamas
2023-11-07  9:08   ` Alice Ryhl
2023-12-01  7:19     ` Carlos Llamas
2023-11-02 18:59 ` [PATCH 17/21] binder: malloc new_buffer outside of locks Carlos Llamas
2023-11-07  9:08   ` Alice Ryhl
2023-12-01  7:20     ` Carlos Llamas
2023-11-02 18:59 ` [PATCH 18/21] binder: initialize lru pages in mmap callback Carlos Llamas
2023-11-07  9:08   ` Alice Ryhl
2023-11-02 18:59 ` [PATCH 19/21] binder: perform page allocation outside of locks Carlos Llamas
2023-11-07  9:08   ` Alice Ryhl
2023-12-01  7:39     ` Carlos Llamas
2023-11-02 18:59 ` Carlos Llamas [this message]
2023-11-07  9:08   ` [PATCH 20/21] binder: reverse locking order in shrinker callback Alice Ryhl
2023-12-01  7:42     ` Carlos Llamas
2023-11-02 18:59 ` [PATCH 21/21] binder: switch alloc->mutex to spinlock_t Carlos Llamas
2023-11-07  9:08   ` Alice Ryhl
2023-12-01  7:46     ` Carlos Llamas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20231102185934.773885-21-cmllamas@google.com \
    --to=cmllamas@google.com \
    --cc=arve@android.com \
    --cc=brauner@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=joel@joelfernandes.org \
    --cc=kernel-team@android.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maco@android.com \
    --cc=surenb@google.com \
    --cc=tkjos@android.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®