Re: [LTP] [PATCH v8] shmctl03: Fix 32-bit compat mode failure

[Petr Vorel <pvorel@suse.cz>]

Hi Wei,

> On 64-bit kernels, the default shmmax and shmall values often
> exceed the range of a 32-bit unsigned long or are clipped
> differently by the kernel's compat syscall layer than they
> appear in /proc.

FYI an old comment from Arnd Bergmann [1]

[1] https://lore.kernel.org/ltp/c9ba86f6-dea8-47bd-88e1-edf49e4bf9fd@app.fastmail.com/

> Link: https://lore.kernel.org/ltp/aJm5SBOaRoe1e0PB@yuki.lan/
> Suggested-by: Cyril Hrubis <chrubis@suse.cz>
> Reviewed-by: Andrea Cervesato <andrea.cervesato@suse.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
> v7->v8:
> - Improving and Shortening the Commit Message.
> - Renaming the Assert Flag: Renamed TST_ASSERT_BITWISE to TST_ASSERT_TRUNC_32BIT.

>  include/tst_assert.h                        | 42 ++++++++++++++++++---
>  lib/tst_assert.c                            | 27 ++++++++++---

Wouldn't it make sense to split library part into separate commit?

>  testcases/kernel/syscalls/shmctl/shmctl03.c | 17 +++++++--
>  3 files changed, 72 insertions(+), 14 deletions(-)

> diff --git a/include/tst_assert.h b/include/tst_assert.h
> index dcb62dfea..045942b1b 100644
> --- a/include/tst_assert.h
> +++ b/include/tst_assert.h
> @@ -21,14 +21,46 @@ void tst_assert_int(const char *file, const int lineno,
>  #define TST_ASSERT_FILE_INT(path, prefix, val) \
>  	tst_assert_file_int(__FILE__, __LINE__, path, prefix, val)

> -/*
> - * Same as tst_assert_int() but for unsigned long.
> +/**
> + * TST_ASSERT_SATURATED_INT - Clamp the sysfs/procfs value to INT_MAX.
> + *
> + * If this flag is set and the value read from the sysfs/procfs file exceeds
> + * INT_MAX, the compared value is clamped (saturated) to INT_MAX.
> + */
> +#define TST_ASSERT_SATURATED_INT   0x01

IMHO enum (instead of plain definitions) would be more suitable for this
(see tst_cmd.h). First good benefit is that will do the check for correct
values for free.

> +
> +/**
> + * TST_ASSERT_TRUNC_32BIT - Keep only the lower 32 bits of the read value.
> + *
> + * If this flag is set, only the low 32 bits of the value read from the
> + * sysfs/procfs file are compared.
> + */
> +#define TST_ASSERT_TRUNC_32BIT     0x02
> +
> +/**
> + * tst_assert_ulong() - Assert that an unsigned long value in a file matches.
> + * @file: The source file of the assertion (usually __FILE__).
> + * @lineno: The source line number of the assertion (usually __LINE__).
> + * @path: Path to the sysfs or procfs file to read from.
> + * @val: The expected unsigned long value to compare against.
> + * @flags: Bitwise flags controlling how the read value is processed.
> + *         Supported flags:
> + *         - %TST_ASSERT_SATURATED_INT: Clamps the value at %INT_MAX if it
Unfortunately lists aren't supported in kerneldoc parameters.  Therefore this
will not work, it will be rendered as inline:

Supported flags: - TST_ASSERT_SATURATED_INT: Clamps the value at INT_MAX if it exceeds it. - TST_ASSERT_TRUNC_32BIT: Keeps only the low 32 bits of the read value, truncating any higher bits. - 0: Direct comparison without any adjustment.

You will see it if you rebase the current master (I added tst_assert.h to the
docs), it'd be great to double check how the docs actually look like.

Therefore using enum will solve it because we don't need to list supported flags
(there will be link to the enum itself) - second benefit for enum.

> + *           exceeds it.
> + *         - %TST_ASSERT_TRUNC_32BIT: Keeps only the low 32 bits of the
> + *           read value, truncating any higher bits.
> + *         - %0: Direct comparison without any adjustment.
> + *
> + * This function reads an integer value from the file specified by @path
> + * and compares it with @val. It allows handling of 32-bit compat mode
> + * truncation/clamping on 64-bit systems via @flags.
>   */
>  void tst_assert_ulong(const char *file, const int lineno,
> -                      const char *path, unsigned long val);
> +                      const char *path, unsigned long val, int flags);

> -#define TST_ASSERT_ULONG(path, val) \
> -	tst_assert_ulong(__FILE__, __LINE__, path, val)
> +#define TST_ASSERT_ULONG(path, val, ...) \
> +	tst_assert_ulong(__FILE__, __LINE__, path, val, \
> +		TST_2_(dummy, ##__VA_ARGS__, 0))
I've never noticed TST_2_(), nice way to avoid passing 0 as flag.

>  /*
>   * Asserts that integer value stored in the prefix field of file pointed by path
> diff --git a/lib/tst_assert.c b/lib/tst_assert.c
> index b68bd5d39..9a606d863 100644
> --- a/lib/tst_assert.c
> +++ b/lib/tst_assert.c
> @@ -5,6 +5,7 @@
>   * Copyright (c) 2020 Cyril Hrubis <chrubis@suse.cz>
>   */
>  #include <stdio.h>
> +#include <limits.h>
Why is this added?

>  #define TST_NO_DEFAULT_MAIN
>  #include "tst_assert.h"
>  #include "tst_test.h"
> @@ -23,18 +24,32 @@ void tst_assert_int(const char *file, const int lineno, const char *path, int va
>  	tst_res_(file, lineno, TFAIL, "%s != %d got %d", path, val, sys_val);
>  }

> -void tst_assert_ulong(const char *file, const int lineno, const char *path, unsigned long val)
> +void tst_assert_ulong(const char *file, const int lineno, const char *path,
> +		      unsigned long val, int flags)
>  {
> -	unsigned long sys_val;
> -
> -	safe_file_scanf(file, lineno, NULL, path, "%lu", &sys_val);
> +	unsigned long long sys_val_64;
> +	unsigned long expected_val;
> +
> +	safe_file_scanf(file, lineno, NULL, path, "%llu", &sys_val_64);
> +
> +	if (flags & TST_ASSERT_SATURATED_INT) {
> +		if (sys_val_64 > (unsigned long long)INT_MAX)
> +			expected_val = (unsigned long)INT_MAX;
> +		else
> +			expected_val = (unsigned long)sys_val_64;
> +	} else if (flags & TST_ASSERT_TRUNC_32BIT) {
> +		expected_val = (unsigned long)(sys_val_64 & 0xFFFFFFFFULL);
> +	} else {
> +		expected_val = (unsigned long)sys_val_64;
> +	}

> -	if (val == sys_val) {
> +	if (val == expected_val) {
>  		tst_res_(file, lineno, TPASS, "%s = %lu", path, val);
>  		return;
>  	}

> -	tst_res_(file, lineno, TFAIL, "%s != %lu got %lu", path, val, sys_val);
> +	tst_res_(file, lineno, TFAIL, "%s != %lu got %lu (raw: %llu)",
> +		path, val, expected_val, sys_val_64);
>  }

>  void tst_assert_file_int(const char *file, const int lineno, const char *path, const char *prefix, int val)
> diff --git a/testcases/kernel/syscalls/shmctl/shmctl03.c b/testcases/kernel/syscalls/shmctl/shmctl03.c
> index 9e1c2f099..0ac3f45c5 100644
> --- a/testcases/kernel/syscalls/shmctl/shmctl03.c
> +++ b/testcases/kernel/syscalls/shmctl/shmctl03.c
> @@ -30,9 +30,20 @@ static void verify_ipcinfo(void)
>  	else
>  		tst_res(TPASS, "shmmin = 1");

> -	TST_ASSERT_ULONG(PATH_KERN_SHMMAX, info.shmmax);
> -	TST_ASSERT_ULONG(PATH_KERN_SHMMNI, info.shmmni);
> -	TST_ASSERT_ULONG(PATH_KERN_SHMALL, info.shmall);
> +	if (tst_is_compat_mode()) {
> +		/*
> +		 * On 64-bit kernel, shmmax is clamped to INT_MAX for 32-bit
> +		 * compat syscall, while shmmni and shmall are truncated
> +		 * to 32-bit.
> +		 */
> +		TST_ASSERT_ULONG(PATH_KERN_SHMMAX, info.shmmax, TST_ASSERT_SATURATED_INT);
> +		TST_ASSERT_ULONG(PATH_KERN_SHMMNI, info.shmmni, TST_ASSERT_TRUNC_32BIT);
> +		TST_ASSERT_ULONG(PATH_KERN_SHMALL, info.shmall, TST_ASSERT_TRUNC_32BIT);

> +	} else {
> +		TST_ASSERT_ULONG(PATH_KERN_SHMMAX, info.shmmax);
> +		TST_ASSERT_ULONG(PATH_KERN_SHMMNI, info.shmmni);
> +		TST_ASSERT_ULONG(PATH_KERN_SHMALL, info.shmall);
> +	}

nit: I'd personally code it with ternary operator, but whatever you prefer.

bool c = tst_is_compat_mode());
/*
 * On 64-bit kernel, shmmax is clamped to INT_MAX for 32-bit
 * compat syscall, while shmmni and shmall are truncated
 * to 32-bit.
 */
TST_ASSERT_ULONG(PATH_KERN_SHMMAX, info.shmmax, c ? TST_ASSERT_SATURATED_INT : 0);
TST_ASSERT_ULONG(PATH_KERN_SHMMNI, info.shmmni, c ? TST_ASSERT_TRUNC_32BIT : 0);
TST_ASSERT_ULONG(PATH_KERN_SHMALL, info.shmall, c ? TST_ASSERT_TRUNC_32BIT: 0);

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp