Re: [PATCH v3 6/6] binder: encapsulate individual alloc test cases

[Tiffany Yang <ynaffit@google.com>]

Kees Cook <kees@kernel.org> writes:


> For both stringify functions, snprintf is potentially unsafe. In the
> spirit of recent string API discussions, please switch to using a
> seq_buf:


> static void stringify_free_seq(struct kunit *test, int *seq, seq_buf *buf)
> {
> 	unsigned int i;

> 	for (i = 0; i < BUFFER_NUM; i++)
> 		seq_buf_printf(buf, "[%d]", seq[i])
> 	KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(buf));
> }
> ...

> 	DECLARE_SEQ_BUF(freeseq_buf, FREESEQ_BUFLEN);
> 	...
> 	stringify_free_seq(test, tc->free_sequence, &freeseq_buf);




Thanks for calling attention to this! Will be fixed for v4!


>>   static bool check_buffer_pages_allocated(struct kunit *test,
>> @@ -124,28 +164,30 @@ static bool check_buffer_pages_allocated(struct  
>> kunit *test,
>>   	return true;
>>   }

>> -static void binder_alloc_test_alloc_buf(struct kunit *test,
>> -					struct binder_alloc *alloc,
>> -					struct binder_buffer *buffers[],
>> -					size_t *sizes, int *seq)
>> +static unsigned long binder_alloc_test_alloc_buf(struct kunit *test,
>> +						 struct binder_alloc *alloc,
>> +						 struct binder_buffer *buffers[],
>> +						 size_t *sizes, int *seq)
>>   {
>> +	unsigned long failures = 0;
>>   	int i;

>>   	for (i = 0; i < BUFFER_NUM; i++) {
>>   		buffers[i] = binder_alloc_new_buf(alloc, sizes[i], 0, 0, 0);
>>   		if (IS_ERR(buffers[i]) ||
>> -		    !check_buffer_pages_allocated(test, alloc, buffers[i], sizes[i]))  
>> {
>> -			pr_err_size_seq(test, sizes, seq);
>> -			binder_alloc_test_failures++;
>> -		}
>> +		    !check_buffer_pages_allocated(test, alloc, buffers[i], sizes[i]))
>> +			failures++;
>>   	}
>> +
>> +	return failures;
>>   }

>> -static void binder_alloc_test_free_buf(struct kunit *test,
>> -				       struct binder_alloc *alloc,
>> -				       struct binder_buffer *buffers[],
>> -				       size_t *sizes, int *seq, size_t end)
>> +static unsigned long binder_alloc_test_free_buf(struct kunit *test,
>> +						struct binder_alloc *alloc,
>> +						struct binder_buffer *buffers[],
>> +						size_t *sizes, int *seq, size_t end)
>>   {
>> +	unsigned long failures = 0;
>>   	int i;

>>   	for (i = 0; i < BUFFER_NUM; i++)
>> @@ -153,17 +195,19 @@ static void binder_alloc_test_free_buf(struct  
>> kunit *test,

>>   	for (i = 0; i <= (end - 1) / PAGE_SIZE; i++) {
>>   		if (list_empty(page_to_lru(alloc->pages[i]))) {
>> -			pr_err_size_seq(test, sizes, seq);
>>   			kunit_err(test, "expect lru but is %s at page index %d\n",
>>   				  alloc->pages[i] ? "alloc" : "free", i);
>> -			binder_alloc_test_failures++;
>> +			failures++;
>>   		}
>>   	}
>> +
>> +	return failures;
>>   }

>> -static void binder_alloc_test_free_page(struct kunit *test,
>> -					struct binder_alloc *alloc)
>> +static unsigned long binder_alloc_test_free_page(struct kunit *test,
>> +						 struct binder_alloc *alloc)
>>   {
>> +	unsigned long failures = 0;
>>   	unsigned long count;
>>   	int i;

>> @@ -177,27 +221,70 @@ static void binder_alloc_test_free_page(struct  
>> kunit *test,
>>   			kunit_err(test, "expect free but is %s at page index %d\n",
>>   				  list_empty(page_to_lru(alloc->pages[i])) ?
>>   				  "alloc" : "lru", i);
>> -			binder_alloc_test_failures++;
>> +			failures++;
>>   		}
>>   	}
>> +
>> +	return failures;
>>   }

>> -static void binder_alloc_test_alloc_free(struct kunit *test,
>> +/* Executes one full test run for the given test case. */
>> +static bool binder_alloc_test_alloc_free(struct kunit *test,
>>   					 struct binder_alloc *alloc,
>> -					 size_t *sizes, int *seq, size_t end)
>> +					 struct binder_alloc_test_case_info *tc,
>> +					 size_t end)
>>   {
>> +	unsigned long pages = PAGE_ALIGN(end) / PAGE_SIZE;
>>   	struct binder_buffer *buffers[BUFFER_NUM];
>> -
>> -	binder_alloc_test_alloc_buf(test, alloc, buffers, sizes, seq);
>> -	binder_alloc_test_free_buf(test, alloc, buffers, sizes, seq, end);
>> +	unsigned long failures;
>> +	bool failed = false;
>> +
>> +	failures = binder_alloc_test_alloc_buf(test, alloc, buffers,
>> +					       tc->buffer_sizes,
>> +					       tc->free_sequence);
>> +	failed = failed || failures;
>> +	KUNIT_EXPECT_EQ_MSG(test, failures, 0,
>> +			    "Initial allocation failed: %lu/%u buffers with errors",
>> +			    failures, BUFFER_NUM);
>> +
>> +	failures = binder_alloc_test_free_buf(test, alloc, buffers,
>> +					      tc->buffer_sizes,
>> +					      tc->free_sequence, end);
>> +	failed = failed || failures;
>> +	KUNIT_EXPECT_EQ_MSG(test, failures, 0,
>> +			    "Initial buffers not freed correctly: %lu/%lu pages not on lru  
>> list",
>> +			    failures, pages);

>>   	/* Allocate from lru. */
>> -	binder_alloc_test_alloc_buf(test, alloc, buffers, sizes, seq);
>> -	if (list_lru_count(alloc->freelist))
>> -		kunit_err(test, "lru list should be empty but is not\n");
>> -
>> -	binder_alloc_test_free_buf(test, alloc, buffers, sizes, seq, end);
>> -	binder_alloc_test_free_page(test, alloc);
>> +	failures = binder_alloc_test_alloc_buf(test, alloc, buffers,
>> +					       tc->buffer_sizes,
>> +					       tc->free_sequence);
>> +	failed = failed || failures;
>> +	KUNIT_EXPECT_EQ_MSG(test, failures, 0,
>> +			    "Reallocation failed: %lu/%u buffers with errors",
>> +			    failures, BUFFER_NUM);
>> +
>> +	failures = list_lru_count(alloc->freelist);
>> +	failed = failed || failures;
>> +	KUNIT_EXPECT_EQ_MSG(test, failures, 0,
>> +			    "lru list should be empty after reallocation but still has %lu  
>> pages",
>> +			    failures);
>> +
>> +	failures = binder_alloc_test_free_buf(test, alloc, buffers,
>> +					      tc->buffer_sizes,
>> +					      tc->free_sequence, end);
>> +	failed = failed || failures;
>> +	KUNIT_EXPECT_EQ_MSG(test, failures, 0,
>> +			    "Reallocated buffers not freed correctly: %lu/%lu pages not on  
>> lru list",
>> +			    failures, pages);
>> +
>> +	failures = binder_alloc_test_free_page(test, alloc);
>> +	failed = failed || failures;
>> +	KUNIT_EXPECT_EQ_MSG(test, failures, 0,
>> +			    "Failed to clean up allocated pages: %lu/%lu pages still  
>> installed",
>> +			    failures, (alloc->buffer_size / PAGE_SIZE));
>> +
>> +	return failed;
>>   }

>>   static bool is_dup(int *seq, int index, int val)
>> @@ -213,24 +300,44 @@ static bool is_dup(int *seq, int index, int val)

>>   /* Generate BUFFER_NUM factorial free orders. */
>>   static void permute_frees(struct kunit *test, struct binder_alloc  
>> *alloc,
>> -			  size_t *sizes, int *seq, int index, size_t end)
>> +			  struct binder_alloc_test_case_info *tc,
>> +			  unsigned long *runs, unsigned long *failures,
>> +			  int index, size_t end)
>>   {
>> +	bool case_failed;
>>   	int i;

>>   	if (index == BUFFER_NUM) {
>> -		binder_alloc_test_alloc_free(test, alloc, sizes, seq, end);
>> +		char freeseq_buf[FREESEQ_BUFLEN];
>> +
>> +		case_failed = binder_alloc_test_alloc_free(test, alloc, tc, end);
>> +		*runs += 1;
>> +		*failures += case_failed;
>> +
>> +		if (case_failed || PRINT_ALL_CASES) {
>> +			stringify_free_seq(test, tc->free_sequence, freeseq_buf,
>> +					   FREESEQ_BUFLEN);
>> +			kunit_err(test, "case %lu: [%s] | %s - %s - %s", *runs,
>> +				  case_failed ? "FAILED" : "PASSED",
>> +				  tc->front_pages ? "front" : "back ",
>> +				  tc->alignments, freeseq_buf);
>> +		}
>> +
>>   		return;
>>   	}
>>   	for (i = 0; i < BUFFER_NUM; i++) {
>> -		if (is_dup(seq, index, i))
>> +		if (is_dup(tc->free_sequence, index, i))
>>   			continue;
>> -		seq[index] = i;
>> -		permute_frees(test, alloc, sizes, seq, index + 1, end);
>> +		tc->free_sequence[index] = i;
>> +		permute_frees(test, alloc, tc, runs, failures, index + 1, end);
>>   	}
>>   }

>> -static void gen_buf_sizes(struct kunit *test, struct binder_alloc  
>> *alloc,
>> -			  size_t *end_offset)
>> +static void gen_buf_sizes(struct kunit *test,
>> +			  struct binder_alloc *alloc,
>> +			  struct binder_alloc_test_case_info *tc,
>> +			  size_t *end_offset, unsigned long *runs,
>> +			  unsigned long *failures)
>>   {
>>   	size_t last_offset, offset = 0;
>>   	size_t front_sizes[BUFFER_NUM];
>> @@ -238,31 +345,45 @@ static void gen_buf_sizes(struct kunit *test,  
>> struct binder_alloc *alloc,
>>   	int seq[BUFFER_NUM] = {0};
>>   	int i;

>> +	tc->free_sequence = seq;
>>   	for (i = 0; i < BUFFER_NUM; i++) {
>>   		last_offset = offset;
>>   		offset = end_offset[i];
>>   		front_sizes[i] = offset - last_offset;
>>   		back_sizes[BUFFER_NUM - i - 1] = front_sizes[i];
>>   	}
>> +	back_sizes[0] += alloc->buffer_size - end_offset[BUFFER_NUM - 1];
>> +
>>   	/*
>>   	 * Buffers share the first or last few pages.
>>   	 * Only BUFFER_NUM - 1 buffer sizes are adjustable since
>>   	 * we need one giant buffer before getting to the last page.
>>   	 */
>> -	back_sizes[0] += alloc->buffer_size - end_offset[BUFFER_NUM - 1];
>> -	permute_frees(test, alloc, front_sizes, seq, 0,
>> +	tc->front_pages = true;
>> +	tc->buffer_sizes = front_sizes;
>> +	permute_frees(test, alloc, tc, runs, failures, 0,
>>   		      end_offset[BUFFER_NUM - 1]);
>> -	permute_frees(test, alloc, back_sizes, seq, 0, alloc->buffer_size);
>> +
>> +	tc->front_pages = false;
>> +	tc->buffer_sizes = back_sizes;
>> +	permute_frees(test, alloc, tc, runs, failures, 0, alloc->buffer_size);
>>   }

>>   static void gen_buf_offsets(struct kunit *test, struct binder_alloc  
>> *alloc,
>> -			    size_t *end_offset, int index)
>> +			    size_t *end_offset, int *alignments,
>> +			    unsigned long *runs, unsigned long *failures,
>> +			    int index)
>>   {
>>   	size_t end, prev;
>>   	int align;

>>   	if (index == BUFFER_NUM) {
>> -		gen_buf_sizes(test, alloc, end_offset);
>> +		struct binder_alloc_test_case_info tc = {0};
>> +
>> +		stringify_alignments(test, alignments, tc.alignments,
>> +				     ALIGNMENTS_BUFLEN);
>> +
>> +		gen_buf_sizes(test, alloc, &tc, end_offset, runs, failures);
>>   		return;
>>   	}
>>   	prev = index == 0 ? 0 : end_offset[index - 1];
>> @@ -276,7 +397,9 @@ static void gen_buf_offsets(struct kunit *test,  
>> struct binder_alloc *alloc,
>>   		else
>>   			end += BUFFER_MIN_SIZE;
>>   		end_offset[index] = end;
>> -		gen_buf_offsets(test, alloc, end_offset, index + 1);
>> +		alignments[index] = align;
>> +		gen_buf_offsets(test, alloc, end_offset, alignments, runs,
>> +				failures, index + 1);
>>   	}
>>   }

>> @@ -328,10 +451,15 @@ static void binder_alloc_exhaustive_test(struct  
>> kunit *test)
>>   {
>>   	struct binder_alloc_test *priv = test->priv;
>>   	size_t end_offset[BUFFER_NUM];
>> +	int alignments[BUFFER_NUM];
>> +	unsigned long failures = 0;
>> +	unsigned long runs = 0;

>> -	gen_buf_offsets(test, &priv->alloc, end_offset, 0);
>> +	gen_buf_offsets(test, &priv->alloc, end_offset, alignments, &runs,
>> +			&failures, 0);

>> -	KUNIT_EXPECT_EQ(test, binder_alloc_test_failures, 0);
>> +	KUNIT_EXPECT_EQ(test, runs, TOTAL_EXHAUSTIVE_CASES);
>> +	KUNIT_EXPECT_EQ(test, failures, 0);
>>   }

>>   /* ===== End test cases ===== */
>> --
>> 2.50.0.727.gbf7dc18ff4-goog


> Otherwise looks good to me.

-- 
Tiffany Y. Yang