Re: [PATCH v2 3/6] kselftest/arm64: mte: user_mem: check different offsets and sizes

[Shuah Khan <skhan@linuxfoundation.org>]

On 2/3/22 7:36 AM, Joey Gouly wrote:
> Iterate over different file offsets, pointer offsets and buffer sizes to exercise
> and test different behaviour.

Please add more information here.

> 
> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Shuah Khan <shuah@kernel.org>
> ---
>   .../selftests/arm64/mte/check_user_mem.c      | 49 +++++++++++--------
>   1 file changed, 29 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/testing/selftests/arm64/mte/check_user_mem.c b/tools/testing/selftests/arm64/mte/check_user_mem.c
> index e5bfeaeb64bb..35061ae23ed5 100644
> --- a/tools/testing/selftests/arm64/mte/check_user_mem.c
> +++ b/tools/testing/selftests/arm64/mte/check_user_mem.c
> @@ -26,6 +26,8 @@ static int check_usermem_access_fault(int mem_type, int mode, int mapping,
>   	char val = 'A';
>   	size_t len, read_len;
>   	void *ptr, *ptr_next;
> +	int fileoff, ptroff, size;
> +	int sizes[] = {1, 2, 3, 8, 16, 32, 4096, page_sz};
>   
>   	err = KSFT_PASS;
>   	len = 2 * page_sz;
> @@ -66,26 +68,33 @@ static int check_usermem_access_fault(int mem_type, int mode, int mapping,
>   	ptr_next = mte_insert_new_tag(ptr_next);
>   	mte_set_tag_address_range(ptr_next, tag_len);
>   
> -	lseek(fd, 0, 0);
> -	/* Copy from file into buffer with invalid tag */
> -	read_len = read(fd, ptr, len);
> -	mte_wait_after_trig();
> -	/*
> -	 * Accessing user memory in kernel with invalid tag should fail in sync
> -	 * mode without fault but may not fail in async mode as per the
> -	 * implemented MTE userspace support in Arm64 kernel.
> -	 */
> -	if (cur_mte_cxt.fault_valid) {
> -		err = KSFT_FAIL;


> -		goto usermem_acc_err;
> -	}
> -	if (mode == MTE_SYNC_ERR && read_len < len) {
> -		/* test passed */
> -	} else if (mode == MTE_ASYNC_ERR && read_len == len) {
> -		/* test passed */
> -	} else {
> -		err = KSFT_FAIL;

Why not set err in usermem_acc_err - this way you can make the error
paths common and simpler.

> -		goto usermem_acc_err;
> +	for (fileoff = 0; fileoff < 16; fileoff++) {
> +		for (ptroff = 0; ptroff < 16; ptroff++) {
> +			for (i = 0; i < ARRAY_SIZE(sizes); i++) {
> +				size = sizes[i];
> +				lseek(fd, 0, 0);
> +				/* Copy from file into buffer with invalid tag */
> +				read_len = read(fd, ptr + ptroff, size);
> +				mte_wait_after_trig();
> +				/*
> +				 * Accessing user memory in kernel with invalid tag should fail in sync
> +				 * mode without fault but may not fail in async mode as per the
> +				 * implemented MTE userspace support in Arm64 kernel.
> +				 */
> +				if (cur_mte_cxt.fault_valid) {
> +					err = KSFT_FAIL;

The above assignment can go way if you add this to usermem_acc_err
path

> +					goto usermem_acc_err;
> +				}
> +				if (mode == MTE_SYNC_ERR && read_len < len) {
> +					/* test passed */
> +				} else if (mode == MTE_ASYNC_ERR && read_len == size) {
> +					/* test passed */
> +				} else {
> +					err = KSFT_FAIL;

Same here

> +					goto usermem_acc_err;
> +				}
> +			}
> +		}
>   	}
>   usermem_acc_err:
>   	mte_free_memory((void *)ptr, len, mem_type, true);
> 

With the above requested changes -

Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel