netdev - Re: [PATCH 1/2] selftests: Provide local define of min() and max()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <64e22df53d1e6_3580162945b@willemb.c.googlers.com.notmuch>
Date: Sun, 20 Aug 2023 11:15:01 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Mahmoud Maatuq <mahmoudmatook.mm@...il.com>, 
 linux-kernel@...r.kernel.org, 
 linux-kselftest@...r.kernel.org, 
 kuba@...nel.org, 
 netdev@...r.kernel.org, 
 willemdebruijn.kernel@...il.com, 
 davem@...emloft.net, 
 pabeni@...hat.com, 
 edumazet@...gle.com, 
 shuah@...nel.org
Cc: linux-kernel-mentees@...ts.linuxfoundation.org, 
 Mahmoud Maatuq <mahmoudmatook.mm@...il.com>
Subject: Re: [PATCH 1/2] selftests: Provide local define of min() and max()

Mahmoud Maatuq wrote:
> to avoid manual calculation of min and max values
> and fix coccinelle warnings such WARNING opportunity for min()/max()
> adding one common definition that could be used in multiple files
> under selftests.
> there are also some defines for min/max scattered locally inside sources
> under selftests.
> this also prepares for cleaning up those redundant defines and include
> kselftest.h instead.
> 
> Signed-off-by: Mahmoud Maatuq <mahmoudmatook.mm@...il.com>
> ---
>  tools/testing/selftests/kselftest.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
> index 829be379545a..e8eb7e9afbc6 100644
> --- a/tools/testing/selftests/kselftest.h
> +++ b/tools/testing/selftests/kselftest.h
> @@ -55,6 +55,13 @@
>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
>  #endif
>  
> +#ifndef min
> +# define min(x, y) ((x) < (y) ? (x) : (y))
> +#endif
> +#ifndef max
> +# define max(x, y) ((x) < (y) ? (y) : (x))
> +#endif
> +

Should this more closely follow include/linux/minmax.h, which is a lot
more strict?

I'm fine with this simpler, more relaxed, version for testing, but
calling it out for people to speak up.

Only the first two of these comments in minmax.h apply to this
userspace code.

/*
 * min()/max()/clamp() macros must accomplish three things:
 *
 * - avoid multiple evaluations of the arguments (so side-effects like
 *   "x++" happen only once) when non-constant.
 * - perform strict type-checking (to generate warnings instead of
 *   nasty runtime surprises). See the "unnecessary" pointer comparison
 *   in __typecheck().
 * - retain result as a constant expressions when called with only
 *   constant expressions (to avoid tripping VLA warnings in stack
 *   allocation usage).
 */

Note that a more strict version that includes __typecheck would
warn on the type difference between total_len and cfg_mss. Fine
with changing the type of cfg_mss in the follow-on patch to address
that.