[PATCH v3 2/6] binder: Store lru freelist in binder_alloc

Tiffany Yang posted 6 patches 2 months, 3 weeks ago
There is a newer version of this series
[PATCH v3 2/6] binder: Store lru freelist in binder_alloc
Posted by Tiffany Yang 2 months, 3 weeks ago
Store a pointer to the free pages list that the binder allocator should
use for a process inside of struct binder_alloc. This change allows
binder allocator code to be tested and debugged deterministically while
a system is using binder; i.e., without interfering with other binder
processes and independently of the shrinker. This is necessary to
convert the current binder_alloc_selftest into a kunit test that does
not rely on hijacking an existing binder_proc to run.

A binder process's binder_alloc->freelist should not be changed after
it is initialized. A sole exception is the process that runs the
existing binder_alloc selftest. Its freelist can be temporarily replaced
for the duration of the test because it runs as a single thread before
any pages can be added to the global binder freelist, and the test frees
every page it allocates before dropping the binder_selftest_lock. This
exception allows the existing selftest to be used to check for
regressions, but it will be dropped when the binder_alloc tests are
converted to kunit in a subsequent patch in this series.

Signed-off-by: Tiffany Yang <ynaffit@google.com>
---
 drivers/android/binder_alloc.c          | 25 +++++++----
 drivers/android/binder_alloc.h          |  3 +-
 drivers/android/binder_alloc_selftest.c | 59 ++++++++++++++++++++-----
 3 files changed, 67 insertions(+), 20 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index fcfaf1b899c8..2e89f9127883 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -26,7 +26,7 @@
 #include "binder_alloc.h"
 #include "binder_trace.h"
 
-struct list_lru binder_freelist;
+static struct list_lru binder_freelist;
 
 static DEFINE_MUTEX(binder_alloc_mmap_lock);
 
@@ -210,7 +210,7 @@ static void binder_lru_freelist_add(struct binder_alloc *alloc,
 
 		trace_binder_free_lru_start(alloc, index);
 
-		ret = list_lru_add(&binder_freelist,
+		ret = list_lru_add(alloc->freelist,
 				   page_to_lru(page),
 				   page_to_nid(page),
 				   NULL);
@@ -409,7 +409,7 @@ static void binder_lru_freelist_del(struct binder_alloc *alloc,
 		if (page) {
 			trace_binder_alloc_lru_start(alloc, index);
 
-			on_lru = list_lru_del(&binder_freelist,
+			on_lru = list_lru_del(alloc->freelist,
 					      page_to_lru(page),
 					      page_to_nid(page),
 					      NULL);
@@ -1007,7 +1007,7 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc)
 			if (!page)
 				continue;
 
-			on_lru = list_lru_del(&binder_freelist,
+			on_lru = list_lru_del(alloc->freelist,
 					      page_to_lru(page),
 					      page_to_nid(page),
 					      NULL);
@@ -1229,6 +1229,17 @@ binder_shrink_scan(struct shrinker *shrink, struct shrink_control *sc)
 
 static struct shrinker *binder_shrinker;
 
+static void __binder_alloc_init(struct binder_alloc *alloc,
+				struct list_lru *freelist)
+{
+	alloc->pid = current->group_leader->pid;
+	alloc->mm = current->mm;
+	mmgrab(alloc->mm);
+	mutex_init(&alloc->mutex);
+	INIT_LIST_HEAD(&alloc->buffers);
+	alloc->freelist = freelist;
+}
+
 /**
  * binder_alloc_init() - called by binder_open() for per-proc initialization
  * @alloc: binder_alloc for this proc
@@ -1238,11 +1249,7 @@ static struct shrinker *binder_shrinker;
  */
 void binder_alloc_init(struct binder_alloc *alloc)
 {
-	alloc->pid = current->group_leader->pid;
-	alloc->mm = current->mm;
-	mmgrab(alloc->mm);
-	mutex_init(&alloc->mutex);
-	INIT_LIST_HEAD(&alloc->buffers);
+	__binder_alloc_init(alloc, &binder_freelist);
 }
 
 int binder_alloc_shrinker_init(void)
diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
index feecd7414241..aa05a9df1360 100644
--- a/drivers/android/binder_alloc.h
+++ b/drivers/android/binder_alloc.h
@@ -15,7 +15,6 @@
 #include <linux/list_lru.h>
 #include <uapi/linux/android/binder.h>
 
-extern struct list_lru binder_freelist;
 struct binder_transaction;
 
 /**
@@ -91,6 +90,7 @@ static inline struct list_head *page_to_lru(struct page *p)
  * @free_async_space:   VA space available for async buffers. This is
  *                      initialized at mmap time to 1/2 the full VA space
  * @pages:              array of struct page *
+ * @freelist:           lru list to use for free pages (invariant after init)
  * @buffer_size:        size of address space specified via mmap
  * @pid:                pid for associated binder_proc (invariant after init)
  * @pages_high:         high watermark of offset in @pages
@@ -113,6 +113,7 @@ struct binder_alloc {
 	struct rb_root allocated_buffers;
 	size_t free_async_space;
 	struct page **pages;
+	struct list_lru *freelist;
 	size_t buffer_size;
 	int pid;
 	size_t pages_high;
diff --git a/drivers/android/binder_alloc_selftest.c b/drivers/android/binder_alloc_selftest.c
index 486af3ec3c02..8b18b22aa3de 100644
--- a/drivers/android/binder_alloc_selftest.c
+++ b/drivers/android/binder_alloc_selftest.c
@@ -8,8 +8,9 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
-#include <linux/mm_types.h>
 #include <linux/err.h>
+#include <linux/list_lru.h>
+#include <linux/mm_types.h>
 #include "binder_alloc.h"
 
 #define BUFFER_NUM 5
@@ -18,6 +19,7 @@
 static bool binder_selftest_run = true;
 static int binder_selftest_failures;
 static DEFINE_MUTEX(binder_selftest_lock);
+static struct list_lru binder_selftest_freelist;
 
 /**
  * enum buf_end_align_type - Page alignment of a buffer
@@ -142,11 +144,6 @@ static void binder_selftest_free_buf(struct binder_alloc *alloc,
 	for (i = 0; i < BUFFER_NUM; i++)
 		binder_alloc_free_buf(alloc, buffers[seq[i]]);
 
-		/**
-		 * Error message on a free page can be false positive
-		 * if binder shrinker ran during binder_alloc_free_buf
-		 * calls above.
-		 */
 	for (i = 0; i <= (end - 1) / PAGE_SIZE; i++) {
 		if (list_empty(page_to_lru(alloc->pages[i]))) {
 			pr_err_size_seq(sizes, seq);
@@ -162,8 +159,8 @@ static void binder_selftest_free_page(struct binder_alloc *alloc)
 	int i;
 	unsigned long count;
 
-	while ((count = list_lru_count(&binder_freelist))) {
-		list_lru_walk(&binder_freelist, binder_alloc_free_page,
+	while ((count = list_lru_count(&binder_selftest_freelist))) {
+		list_lru_walk(&binder_selftest_freelist, binder_alloc_free_page,
 			      NULL, count);
 	}
 
@@ -187,7 +184,7 @@ static void binder_selftest_alloc_free(struct binder_alloc *alloc,
 
 	/* Allocate from lru. */
 	binder_selftest_alloc_buf(alloc, buffers, sizes, seq);
-	if (list_lru_count(&binder_freelist))
+	if (list_lru_count(&binder_selftest_freelist))
 		pr_err("lru list should be empty but is not\n");
 
 	binder_selftest_free_buf(alloc, buffers, sizes, seq, end);
@@ -275,6 +272,20 @@ static void binder_selftest_alloc_offset(struct binder_alloc *alloc,
 	}
 }
 
+int binder_selftest_alloc_get_page_count(struct binder_alloc *alloc)
+{
+	struct page *page;
+	int allocated = 0;
+	int i;
+
+	for (i = 0; i < alloc->buffer_size / PAGE_SIZE; i++) {
+		page = alloc->pages[i];
+		if (page)
+			allocated++;
+	}
+	return allocated;
+}
+
 /**
  * binder_selftest_alloc() - Test alloc and free of buffer pages.
  * @alloc: Pointer to alloc struct.
@@ -286,6 +297,7 @@ static void binder_selftest_alloc_offset(struct binder_alloc *alloc,
  */
 void binder_selftest_alloc(struct binder_alloc *alloc)
 {
+	struct list_lru *prev_freelist;
 	size_t end_offset[BUFFER_NUM];
 
 	if (!binder_selftest_run)
@@ -293,14 +305,41 @@ void binder_selftest_alloc(struct binder_alloc *alloc)
 	mutex_lock(&binder_selftest_lock);
 	if (!binder_selftest_run || !alloc->mapped)
 		goto done;
+
+	prev_freelist = alloc->freelist;
+
+	/*
+	 * It is not safe to modify this process's alloc->freelist if it has any
+	 * pages on a freelist. Since the test runs before any binder ioctls can
+	 * be dealt with, none of its pages should be allocated yet.
+	 */
+	if (binder_selftest_alloc_get_page_count(alloc)) {
+		pr_err("process has existing alloc state\n");
+		goto cleanup;
+	}
+
+	if (list_lru_init(&binder_selftest_freelist)) {
+		pr_err("failed to init test freelist\n");
+		goto cleanup;
+	}
+
+	alloc->freelist = &binder_selftest_freelist;
+
 	pr_info("STARTED\n");
 	binder_selftest_alloc_offset(alloc, end_offset, 0);
-	binder_selftest_run = false;
 	if (binder_selftest_failures > 0)
 		pr_info("%d tests FAILED\n", binder_selftest_failures);
 	else
 		pr_info("PASSED\n");
 
+	if (list_lru_count(&binder_selftest_freelist))
+		pr_err("expect test freelist to be empty\n");
+
+cleanup:
+	/* Even if we didn't run the test, it's no longer thread-safe. */
+	binder_selftest_run = false;
+	alloc->freelist = prev_freelist;
+	list_lru_destroy(&binder_selftest_freelist);
 done:
 	mutex_unlock(&binder_selftest_lock);
 }
-- 
2.50.0.727.gbf7dc18ff4-goog
Re: [PATCH v3 2/6] binder: Store lru freelist in binder_alloc
Posted by Kees Cook 2 months, 3 weeks ago
On Mon, Jul 14, 2025 at 11:53:15AM -0700, Tiffany Yang wrote:
> Store a pointer to the free pages list that the binder allocator should
> use for a process inside of struct binder_alloc. This change allows
> binder allocator code to be tested and debugged deterministically while
> a system is using binder; i.e., without interfering with other binder
> processes and independently of the shrinker. This is necessary to
> convert the current binder_alloc_selftest into a kunit test that does
> not rely on hijacking an existing binder_proc to run.
> 
> A binder process's binder_alloc->freelist should not be changed after
> it is initialized. A sole exception is the process that runs the
> existing binder_alloc selftest. Its freelist can be temporarily replaced
> for the duration of the test because it runs as a single thread before
> any pages can be added to the global binder freelist, and the test frees
> every page it allocates before dropping the binder_selftest_lock. This
> exception allows the existing selftest to be used to check for
> regressions, but it will be dropped when the binder_alloc tests are
> converted to kunit in a subsequent patch in this series.
> 
> Signed-off-by: Tiffany Yang <ynaffit@google.com>
> ---
>  drivers/android/binder_alloc.c          | 25 +++++++----
>  drivers/android/binder_alloc.h          |  3 +-
>  drivers/android/binder_alloc_selftest.c | 59 ++++++++++++++++++++-----
>  3 files changed, 67 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
> index fcfaf1b899c8..2e89f9127883 100644
> --- a/drivers/android/binder_alloc.c
> +++ b/drivers/android/binder_alloc.c
> @@ -26,7 +26,7 @@
>  #include "binder_alloc.h"
>  #include "binder_trace.h"
>  
> -struct list_lru binder_freelist;
> +static struct list_lru binder_freelist;
>  
>  static DEFINE_MUTEX(binder_alloc_mmap_lock);
>  
> @@ -210,7 +210,7 @@ static void binder_lru_freelist_add(struct binder_alloc *alloc,
>  
>  		trace_binder_free_lru_start(alloc, index);
>  
> -		ret = list_lru_add(&binder_freelist,
> +		ret = list_lru_add(alloc->freelist,
>  				   page_to_lru(page),
>  				   page_to_nid(page),
>  				   NULL);
> @@ -409,7 +409,7 @@ static void binder_lru_freelist_del(struct binder_alloc *alloc,
>  		if (page) {
>  			trace_binder_alloc_lru_start(alloc, index);
>  
> -			on_lru = list_lru_del(&binder_freelist,
> +			on_lru = list_lru_del(alloc->freelist,
>  					      page_to_lru(page),
>  					      page_to_nid(page),
>  					      NULL);
> @@ -1007,7 +1007,7 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc)
>  			if (!page)
>  				continue;
>  
> -			on_lru = list_lru_del(&binder_freelist,
> +			on_lru = list_lru_del(alloc->freelist,
>  					      page_to_lru(page),
>  					      page_to_nid(page),
>  					      NULL);
> @@ -1229,6 +1229,17 @@ binder_shrink_scan(struct shrinker *shrink, struct shrink_control *sc)
>  
>  static struct shrinker *binder_shrinker;
>  
> +static void __binder_alloc_init(struct binder_alloc *alloc,
> +				struct list_lru *freelist)
> +{
> +	alloc->pid = current->group_leader->pid;
> +	alloc->mm = current->mm;
> +	mmgrab(alloc->mm);
> +	mutex_init(&alloc->mutex);
> +	INIT_LIST_HEAD(&alloc->buffers);
> +	alloc->freelist = freelist;
> +}
> +
>  /**
>   * binder_alloc_init() - called by binder_open() for per-proc initialization
>   * @alloc: binder_alloc for this proc
> @@ -1238,11 +1249,7 @@ static struct shrinker *binder_shrinker;
>   */
>  void binder_alloc_init(struct binder_alloc *alloc)
>  {
> -	alloc->pid = current->group_leader->pid;
> -	alloc->mm = current->mm;
> -	mmgrab(alloc->mm);
> -	mutex_init(&alloc->mutex);
> -	INIT_LIST_HEAD(&alloc->buffers);
> +	__binder_alloc_init(alloc, &binder_freelist);
>  }
>  
>  int binder_alloc_shrinker_init(void)
> diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
> index feecd7414241..aa05a9df1360 100644
> --- a/drivers/android/binder_alloc.h
> +++ b/drivers/android/binder_alloc.h
> @@ -15,7 +15,6 @@
>  #include <linux/list_lru.h>
>  #include <uapi/linux/android/binder.h>
>  
> -extern struct list_lru binder_freelist;
>  struct binder_transaction;
>  
>  /**
> @@ -91,6 +90,7 @@ static inline struct list_head *page_to_lru(struct page *p)
>   * @free_async_space:   VA space available for async buffers. This is
>   *                      initialized at mmap time to 1/2 the full VA space
>   * @pages:              array of struct page *
> + * @freelist:           lru list to use for free pages (invariant after init)
>   * @buffer_size:        size of address space specified via mmap
>   * @pid:                pid for associated binder_proc (invariant after init)
>   * @pages_high:         high watermark of offset in @pages
> @@ -113,6 +113,7 @@ struct binder_alloc {
>  	struct rb_root allocated_buffers;
>  	size_t free_async_space;
>  	struct page **pages;
> +	struct list_lru *freelist;
>  	size_t buffer_size;
>  	int pid;
>  	size_t pages_high;
> diff --git a/drivers/android/binder_alloc_selftest.c b/drivers/android/binder_alloc_selftest.c
> index 486af3ec3c02..8b18b22aa3de 100644
> --- a/drivers/android/binder_alloc_selftest.c
> +++ b/drivers/android/binder_alloc_selftest.c
> @@ -8,8 +8,9 @@
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
> -#include <linux/mm_types.h>
>  #include <linux/err.h>
> +#include <linux/list_lru.h>
> +#include <linux/mm_types.h>
>  #include "binder_alloc.h"
>  
>  #define BUFFER_NUM 5
> @@ -18,6 +19,7 @@
>  static bool binder_selftest_run = true;
>  static int binder_selftest_failures;
>  static DEFINE_MUTEX(binder_selftest_lock);
> +static struct list_lru binder_selftest_freelist;
>  
>  /**
>   * enum buf_end_align_type - Page alignment of a buffer
> @@ -142,11 +144,6 @@ static void binder_selftest_free_buf(struct binder_alloc *alloc,
>  	for (i = 0; i < BUFFER_NUM; i++)
>  		binder_alloc_free_buf(alloc, buffers[seq[i]]);
>  
> -		/**
> -		 * Error message on a free page can be false positive
> -		 * if binder shrinker ran during binder_alloc_free_buf
> -		 * calls above.
> -		 */
>  	for (i = 0; i <= (end - 1) / PAGE_SIZE; i++) {
>  		if (list_empty(page_to_lru(alloc->pages[i]))) {
>  			pr_err_size_seq(sizes, seq);

Ah, I see the comment is removed entirely here. Better to move this
hunk to patch 1, then.

> @@ -162,8 +159,8 @@ static void binder_selftest_free_page(struct binder_alloc *alloc)
>  	int i;
>  	unsigned long count;
>  
> -	while ((count = list_lru_count(&binder_freelist))) {
> -		list_lru_walk(&binder_freelist, binder_alloc_free_page,
> +	while ((count = list_lru_count(&binder_selftest_freelist))) {
> +		list_lru_walk(&binder_selftest_freelist, binder_alloc_free_page,
>  			      NULL, count);
>  	}
>  
> @@ -187,7 +184,7 @@ static void binder_selftest_alloc_free(struct binder_alloc *alloc,
>  
>  	/* Allocate from lru. */
>  	binder_selftest_alloc_buf(alloc, buffers, sizes, seq);
> -	if (list_lru_count(&binder_freelist))
> +	if (list_lru_count(&binder_selftest_freelist))
>  		pr_err("lru list should be empty but is not\n");
>  
>  	binder_selftest_free_buf(alloc, buffers, sizes, seq, end);
> @@ -275,6 +272,20 @@ static void binder_selftest_alloc_offset(struct binder_alloc *alloc,
>  	}
>  }
>  
> +int binder_selftest_alloc_get_page_count(struct binder_alloc *alloc)
> +{
> +	struct page *page;
> +	int allocated = 0;
> +	int i;
> +
> +	for (i = 0; i < alloc->buffer_size / PAGE_SIZE; i++) {
> +		page = alloc->pages[i];
> +		if (page)
> +			allocated++;
> +	}
> +	return allocated;
> +}
> +
>  /**
>   * binder_selftest_alloc() - Test alloc and free of buffer pages.
>   * @alloc: Pointer to alloc struct.
> @@ -286,6 +297,7 @@ static void binder_selftest_alloc_offset(struct binder_alloc *alloc,
>   */
>  void binder_selftest_alloc(struct binder_alloc *alloc)
>  {
> +	struct list_lru *prev_freelist;
>  	size_t end_offset[BUFFER_NUM];
>  
>  	if (!binder_selftest_run)
> @@ -293,14 +305,41 @@ void binder_selftest_alloc(struct binder_alloc *alloc)
>  	mutex_lock(&binder_selftest_lock);
>  	if (!binder_selftest_run || !alloc->mapped)
>  		goto done;
> +
> +	prev_freelist = alloc->freelist;
> +
> +	/*
> +	 * It is not safe to modify this process's alloc->freelist if it has any
> +	 * pages on a freelist. Since the test runs before any binder ioctls can
> +	 * be dealt with, none of its pages should be allocated yet.
> +	 */
> +	if (binder_selftest_alloc_get_page_count(alloc)) {
> +		pr_err("process has existing alloc state\n");
> +		goto cleanup;
> +	}
> +
> +	if (list_lru_init(&binder_selftest_freelist)) {
> +		pr_err("failed to init test freelist\n");
> +		goto cleanup;
> +	}
> +
> +	alloc->freelist = &binder_selftest_freelist;
> +
>  	pr_info("STARTED\n");
>  	binder_selftest_alloc_offset(alloc, end_offset, 0);
> -	binder_selftest_run = false;
>  	if (binder_selftest_failures > 0)
>  		pr_info("%d tests FAILED\n", binder_selftest_failures);
>  	else
>  		pr_info("PASSED\n");
>  
> +	if (list_lru_count(&binder_selftest_freelist))
> +		pr_err("expect test freelist to be empty\n");
> +
> +cleanup:
> +	/* Even if we didn't run the test, it's no longer thread-safe. */
> +	binder_selftest_run = false;
> +	alloc->freelist = prev_freelist;
> +	list_lru_destroy(&binder_selftest_freelist);
>  done:
>  	mutex_unlock(&binder_selftest_lock);
>  }
> -- 
> 2.50.0.727.gbf7dc18ff4-goog

Otherwise looks good.

-- 
Kees Cook
Re: [PATCH v3 2/6] binder: Store lru freelist in binder_alloc
Posted by Tiffany Yang 2 months, 3 weeks ago
Kees Cook <kees@kernel.org> writes:
>> -		/**
>> -		 * Error message on a free page can be false positive
>> -		 * if binder shrinker ran during binder_alloc_free_buf
>> -		 * calls above.
>> -		 */
>>   	for (i = 0; i <= (end - 1) / PAGE_SIZE; i++) {
>>   		if (list_empty(page_to_lru(alloc->pages[i]))) {
>>   			pr_err_size_seq(sizes, seq);

> Ah, I see the comment is removed entirely here. Better to move this
> hunk to patch 1, then.


This comment is actually "true" until we allow a test-specific freelist
in this change, but I've moved the "for" line back to where it was in
the previous change. Thanks for noting this!

>> @@ -162,8 +159,8 @@ static void binder_selftest_free_page(struct  
>> binder_alloc *alloc)
>>   	int i;
>>   	unsigned long count;

>> -	while ((count = list_lru_count(&binder_freelist))) {
>> -		list_lru_walk(&binder_freelist, binder_alloc_free_page,
>> +	while ((count = list_lru_count(&binder_selftest_freelist))) {
>> +		list_lru_walk(&binder_selftest_freelist, binder_alloc_free_page,
>>   			      NULL, count);
>>   	}

>> @@ -187,7 +184,7 @@ static void binder_selftest_alloc_free(struct  
>> binder_alloc *alloc,

>>   	/* Allocate from lru. */
>>   	binder_selftest_alloc_buf(alloc, buffers, sizes, seq);
>> -	if (list_lru_count(&binder_freelist))
>> +	if (list_lru_count(&binder_selftest_freelist))
>>   		pr_err("lru list should be empty but is not\n");

>>   	binder_selftest_free_buf(alloc, buffers, sizes, seq, end);
>> @@ -275,6 +272,20 @@ static void binder_selftest_alloc_offset(struct  
>> binder_alloc *alloc,
>>   	}
>>   }

>> +int binder_selftest_alloc_get_page_count(struct binder_alloc *alloc)
>> +{
>> +	struct page *page;
>> +	int allocated = 0;
>> +	int i;
>> +
>> +	for (i = 0; i < alloc->buffer_size / PAGE_SIZE; i++) {
>> +		page = alloc->pages[i];
>> +		if (page)
>> +			allocated++;
>> +	}
>> +	return allocated;
>> +}
>> +
>>   /**
>>    * binder_selftest_alloc() - Test alloc and free of buffer pages.
>>    * @alloc: Pointer to alloc struct.
>> @@ -286,6 +297,7 @@ static void binder_selftest_alloc_offset(struct  
>> binder_alloc *alloc,
>>    */
>>   void binder_selftest_alloc(struct binder_alloc *alloc)
>>   {
>> +	struct list_lru *prev_freelist;
>>   	size_t end_offset[BUFFER_NUM];

>>   	if (!binder_selftest_run)
>> @@ -293,14 +305,41 @@ void binder_selftest_alloc(struct binder_alloc  
>> *alloc)
>>   	mutex_lock(&binder_selftest_lock);
>>   	if (!binder_selftest_run || !alloc->mapped)
>>   		goto done;
>> +
>> +	prev_freelist = alloc->freelist;
>> +
>> +	/*
>> +	 * It is not safe to modify this process's alloc->freelist if it has  
>> any
>> +	 * pages on a freelist. Since the test runs before any binder ioctls  
>> can
>> +	 * be dealt with, none of its pages should be allocated yet.
>> +	 */
>> +	if (binder_selftest_alloc_get_page_count(alloc)) {
>> +		pr_err("process has existing alloc state\n");
>> +		goto cleanup;
>> +	}
>> +
>> +	if (list_lru_init(&binder_selftest_freelist)) {
>> +		pr_err("failed to init test freelist\n");
>> +		goto cleanup;
>> +	}
>> +
>> +	alloc->freelist = &binder_selftest_freelist;
>> +
>>   	pr_info("STARTED\n");
>>   	binder_selftest_alloc_offset(alloc, end_offset, 0);
>> -	binder_selftest_run = false;
>>   	if (binder_selftest_failures > 0)
>>   		pr_info("%d tests FAILED\n", binder_selftest_failures);
>>   	else
>>   		pr_info("PASSED\n");

>> +	if (list_lru_count(&binder_selftest_freelist))
>> +		pr_err("expect test freelist to be empty\n");
>> +
>> +cleanup:
>> +	/* Even if we didn't run the test, it's no longer thread-safe. */
>> +	binder_selftest_run = false;
>> +	alloc->freelist = prev_freelist;
>> +	list_lru_destroy(&binder_selftest_freelist);
>>   done:
>>   	mutex_unlock(&binder_selftest_lock);
>>   }
>> --
>> 2.50.0.727.gbf7dc18ff4-goog

> Otherwise looks good.

-- 
Tiffany Y. Yang
Re: [PATCH v3 2/6] binder: Store lru freelist in binder_alloc
Posted by Carlos Llamas 2 months, 3 weeks ago
On Mon, Jul 14, 2025 at 11:53:15AM -0700, Tiffany Yang wrote:
> Store a pointer to the free pages list that the binder allocator should
> use for a process inside of struct binder_alloc. This change allows
> binder allocator code to be tested and debugged deterministically while
> a system is using binder; i.e., without interfering with other binder
> processes and independently of the shrinker. This is necessary to
> convert the current binder_alloc_selftest into a kunit test that does
> not rely on hijacking an existing binder_proc to run.
>
> A binder process's binder_alloc->freelist should not be changed after
> it is initialized. A sole exception is the process that runs the
> existing binder_alloc selftest. Its freelist can be temporarily replaced
> for the duration of the test because it runs as a single thread before
> any pages can be added to the global binder freelist, and the test frees
> every page it allocates before dropping the binder_selftest_lock. This
> exception allows the existing selftest to be used to check for
> regressions, but it will be dropped when the binder_alloc tests are
> converted to kunit in a subsequent patch in this series.
>
> Signed-off-by: Tiffany Yang <ynaffit@google.com>
> ---

LGTM!

Acked-by: Carlos Llamas <cmllamas@google.com>