[PATCH i-g-t] lib/igt_core: Use suffix for casting to long to avoid overflow

[Lucas De Marchi]

On Tue, Dec 17, 2024 at 05:47:44PM +0100, Kamil Konieczny wrote:
>Hi Zbigniew,
>On 2024-12-17 at 11:58:07 +0100, Zbigniew Kempczyński wrote:
>
>please improve subject, imho better something like:
>
>Avoid overflow in micro/nano-seconds calculations
>or
>Fix overflow in micro/nano-seconds calculations
>
>or propose yourself one line description.
>
>> Multiplication without using casting to long (long) for at least
>> one argument in expression may cause overflow especially when we're
>> using this for calculating timeouts in nanoseconds precision.
>>
>> Ensure we're picking valid ul/ull suffix for base definitions for
>> all dependents.
>>
>> Reported-by: Ilia Levi <ilia.levi at intel.com>
>> Closes: https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/issues/163
>>
>
>I agree here with Lucas, remove this empty line.
>
>> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>> Cc: Ilia Levi <ilia.levi at intel.com>
>> ---
>>  lib/igt_core.h | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/igt_core.h b/lib/igt_core.h
>> index 90f57402f0..1b6e70182b 100644
>> --- a/lib/igt_core.h
>> +++ b/lib/igt_core.h
>> @@ -1517,11 +1517,17 @@ void igt_kmsg(const char *format, ...);
>>  #define le32_to_cpu(x)  (x)
>>  #endif
>>
>> -#define MSEC_PER_SEC (1000u)
>> +#if __WORDSIZE == 64
>
>I am not sure if this is defined on all platforms where igt
>is compiled, on musl and on non-Linux like FreeBSD,
>why not make #ifndef/#define/#endif for __WORDSIZE ?

because if it's not defined, how are you going to decide which side of
them to use?

from the tools/  include in kernel, one could do

#ifndef __WORDSIZE
#define __WORDSIZE (__SIZEOF_LONG__ * 8)
#endif

another possible solution is checking ULONG_MAX, like in dnf:

https://github.com/rpm-software-management/libdnf/pull/1159/files

but I think the kernel one is cleaner.

Lucas De Marchi

>
>BTW is it just sizeof(1ul) == 8 ?
>
>With this fixed and subject improved
>Reviewed-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
>
>Regards,
>Kamil
>
>> +#define MSEC_PER_SEC (1000ul)
>> +#define USEC_PER_MSEC (1000ul)
>> +#else
>> +#define MSEC_PER_SEC (1000ull)
>> +#define USEC_PER_MSEC (1000ull)
>> +#endif
>> +
>>  #define USEC_PER_SEC (1000u * MSEC_PER_SEC)
>>  #define USEC_PER_DECISEC (100u * MSEC_PER_SEC)
>>  #define NSEC_PER_SEC (1000u * USEC_PER_SEC)
>> -#define USEC_PER_MSEC (1000u)
>>  #define NSEC_PER_MSEC (1000u * USEC_PER_MSEC)
>>
>>  #define for_if(expr__) if (!(expr__)) {} else
>> --
>> 2.34.1
>>