[PATCH v2 2/6] kselftest/arm64: mte: user_mem: rework error handling

[Shuah Khan]

On 2/3/22 7:36 AM, Joey Gouly wrote:
> This makes it easier to have multiple iterations in the test.

Please add more information here. Based on the code instead of pagesz, this
test allows other sizes. This is a good change and bit more info. helps
describe the value of this change.

> 
> Signed-off-by: Joey Gouly <joey.gouly at arm.com>
> Cc: Catalin Marinas <catalin.marinas at arm.com>
> Cc: Will Deacon <will at kernel.org>
> Cc: Mark Brown <broonie at kernel.org>
> Cc: Shuah Khan <shuah at kernel.org>
> ---
>   .../selftests/arm64/mte/check_user_mem.c      | 27 ++++++++++++-------
>   1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/testing/selftests/arm64/mte/check_user_mem.c b/tools/testing/selftests/arm64/mte/check_user_mem.c
> index 5a5a7e1f5789..e5bfeaeb64bb 100644
> --- a/tools/testing/selftests/arm64/mte/check_user_mem.c
> +++ b/tools/testing/selftests/arm64/mte/check_user_mem.c
> @@ -27,7 +27,7 @@ static int check_usermem_access_fault(int mem_type, int mode, int mapping,
>   	size_t len, read_len;
>   	void *ptr, *ptr_next;
>   
> -	err = KSFT_FAIL;
> +	err = KSFT_PASS;
>   	len = 2 * page_sz;
>   	mte_switch_mode(mode, MTE_ALLOW_NON_ZERO_TAG);
>   	fd = create_temp_file();
> @@ -46,14 +46,18 @@ static int check_usermem_access_fault(int mem_type, int mode, int mapping,
>   	/* Copy from file into buffer with valid tag */
>   	read_len = read(fd, ptr, len);
>   	mte_wait_after_trig();
> -	if (cur_mte_cxt.fault_valid || read_len < len)
> +	if (cur_mte_cxt.fault_valid || read_len < len) {
> +		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;
> +	}
>   	/* Verify same pattern is read */
>   	for (i = 0; i < len; i++)
>   		if (*(char *)(ptr + i) != val)
>   			break;
> -	if (i < len)
> +	if (i < len) {
> +		err = KSFT_FAIL;

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

>   		goto usermem_acc_err;
> +	}
>   
>   	if (!tag_len)
>   		tag_len = len - tag_offset;
> @@ -71,12 +75,17 @@ static int check_usermem_access_fault(int mem_type, int mode, int mapping,
>   	 * mode without fault but may not fail in async mode as per the
>   	 * implemented MTE userspace support in Arm64 kernel.
>   	 */
> -	if (mode == MTE_SYNC_ERR &&
> -	    !cur_mte_cxt.fault_valid && read_len < len) {
> -		err = KSFT_PASS;
> -	} else if (mode == MTE_ASYNC_ERR &&
> -		   !cur_mte_cxt.fault_valid && read_len == len) {
> -		err = KSFT_PASS;
> +	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 == len) {
> +		/* test passed */
> +	} else {
> +		err = KSFT_FAIL;

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

> +		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 at linuxfoundation.org>

thanks,
-- Shuah