lkl: 32-bit timespec fixes #556

ddiss · 2025-01-31T05:31:36Z

The following changes since commit b357b84371983c7de658fec83265b9fe7e48c20a:

  Merge pull request #555 from ddiss/lkl_test_icmp_buf_overflow (2025-01-29 17:36:39 -0800)

are available in the Git repository at:

  https://github.com/ddiss/linux lkl_tests_parallel_build_fix

for you to fetch changes up to 0692c1e05031ac61758d22a14417b50ab81732e2:

  lkl: fix 32-bit timespec casts (2025-02-06 09:47:30 +1100)

----------------------------------------------------------------
David Disseldorp (5):
      lkl tools: use 64-bit lkl_sys_nanosleep on 32-bit builds
      lkl tools: fix some -Wincompatible-pointer-types errors
      lkl cptofs: handle stat errors earlier
      lkl: rework broken lkl_sys_select timeval conversion
      lkl: fix 32-bit timespec casts

 tools/lkl/cptofs.c         | 44 ++++++++++++++++++++++++++++----------------
 tools/lkl/include/lkl.h    | 36 +++++++++++++++++-------------------
 tools/lkl/lib/fs.c         |  5 ++---
 tools/lkl/lib/posix-host.c | 21 ++++++++++++++++++---
 tools/lkl/lklfuse.c        | 20 ++++++++------------
 tools/lkl/tests/boot.c     |  7 ++++---
 tools/lkl/tests/net-test.c |  4 ++--
 7 files changed, 79 insertions(+), 58 deletions(-)

Use the always-64-bit __lkl__kernel_timespec struct for lkl_sys_nanosleep calls and rewrite the lkl_sys_nanosleep_time32 wrapper to use __lkl__NR_clock_nanosleep_time64 on 32-bit builds. This fixes the following -Wincompatible-pointer-types errors: lib/fs.c:287:43: error: passing argument 1 of ‘lkl_sys_nanosleep_time32’ from incompatible pointer type [-Wincompatible-pointer-types] 287 | lkl_sys_nanosleep((struct __lkl__kernel_timespec *)&ts, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | struct __lkl__kernel_timespec * In file included from tools/lkl//include/lkl_host.h:9, from lib/fs.c:5: tools/lkl//include/lkl.h:68:65: note: expected ‘struct lkl_timespec *’ but argument is of type ‘struct __lkl__kernel_timespec *’ 68 | static inline int lkl_sys_nanosleep_time32(struct lkl_timespec *rqtp, | ~~~~~~~~~~~~~~~~~~~~~^~~~ make[1]: *** [tools/build/Makefile.build:98: tools/lkl/lib/fs.o] Error 1 Signed-off-by: David Disseldorp <[email protected]>

Explicitly cast between unsigned long and size_t for malloc, memcpy and memset. Signed-off-by: David Disseldorp <[email protected]>

ddiss · 2025-02-03T23:47:16Z

Regarding the checkpatch warnings: two are from unwrapped compiler output in the commit messages and one is for a /* fall through */ comment in code copied from test.c -> test.h. I'd prefer to leave them as-is but can change if required.

I have some more 32-bit time_t fixes which I'll push here soon.

tavip · 2025-01-31T19:17:59Z

tools/lkl/include/lkl.h

-static inline int lkl_sys_nanosleep_time32(struct lkl_timespec *rqtp,
-					   struct lkl_timespec *rmtp)
+static inline int lkl_sys_nanosleep(struct __lkl__kernel_timespec *rqtp,
+				    struct __lkl__kernel_timespec *rmtp)


I think we should follow what Linux does and provide timespec for LKL APIs, even if for 32 bit machines it means that we loose range. Per timespec(3) man:

SYNOPSIS #include <time.h> struct timespec { time_t tv_sec; /* Seconds */ /* ... */ tv_nsec; /* Nanoseconds [0, 999'999'999] */ }; DESCRIPTION Describes times in seconds and nanoseconds. tv_nsec is of an implementation-defined signed type capable of holding the specified range. Under glibc, this is usually long, and long long on X32. It can be safely down-cast to any concrete 32-bit integer type for processing.

So basically use a local __lkl_kernel_timespec, copy the data from lkl_timespec argument, and pass the local to the system call.

Then we can remove __lkl_kernel_timespec from LKL libs and apps which we introduced in 3d4047a. @thehajime does this sound reasonable?

Do you just want this wrapper for nanosleep, or should I also add lkl_timespec wrappers for lkl_sys_utimensat()? I was hoping to avoid going too far down the generic-libc path and just stick to the modern kernel API.

I've pushed a962b15 to this branch (with two extra changes) to show where other wrappers would be needed if we switch callers to lkl_timespec.
On the generic-libc side, I wonder whether lkl could hook into nolibc via e.g. tools/include/nolibc/arch-lkl.h. It's already in-tree and relatively exhaustive in terms of syscalls.

Then we can remove __lkl_kernel_timespec from LKL libs and apps which we introduced in 3d4047a. @thehajime does this sound reasonable?

sorry, I don't quite remember what/why my followup fixes (3d4047a) did.. my best guess is to silence warnings from compilation.

Do you just want this wrapper for nanosleep, or should I also add lkl_timespec wrappers for lkl_sys_utimensat()? I was hoping to avoid going too far down the generic-libc path and just stick to the modern kernel API.

I thought more about this and I think you are right, we should stick with modern kernel APIs.

We should be able to override struct timespec defined in include/uapi/linux/time.h with a custom one arch/lkl/include/uapi/asm/types.h. If you prefer to merge as is (use __lkl_kernel_timespec) for now we can cleanup later.

...

We should be able to override struct timespec defined in include/uapi/linux/time.h with a custom one arch/lkl/include/uapi/asm/types.h. If you prefer to merge as is (use __lkl_kernel_timespec) for now we can cleanup later.

I'm not sure I follow; would you like some sort of helper macro to ease conversion from timespec to __lkl_kernel_timespec? Thanks a lot for the review btw

Lets merge this as it is and I will followup with a proposal.

tools/lkl/tests/Build

ddiss · 2025-02-05T10:13:01Z

I've reworked the lkl_sys_select timeval conversion fix to drop the overflow detection, as discussed with @thehajime in #557

Don't bother parsing the results if the initial stat call failed. Signed-off-by: David Disseldorp <[email protected]>

max_time is incorrect for both 8-byte and 4-byte time_t sizes. It should be 0x7fffffffffffffff on 64-bit arches instead of 0x7fe. Given that we will (in the next commit) be casting from a (long x 2) lkl_timeval to a (long long x 2) __lkl__kernel_timespec, I think we can ignore overflow and match the nolibc conversion logic. Link: lkl#557 Fixes: 780bdc7 ("lkl: follow up fixes after 4.17 merge") Signed-off-by: David Disseldorp <[email protected]>

On 32-bit architectures, struct lkl_timespec is 2*sizeof(long) while __lkl__kernel_timespec is 2*sizeof(long long); casting these pointer types is unsafe. Fixes: 3d4047a ("lkl: follow up fixes after v5.1 merge (y2038)") Signed-off-by: David Disseldorp <[email protected]>

ddiss · 2025-02-05T22:54:19Z

Changes since previous version:

drop c221333 ("lkl tests: fix parallel build failures") now tracked via Parallel buld issues and missing output when building certain objects #558

tavip

LGTM, great work @ddis, thank you!

ddiss added 2 commits January 31, 2025 16:26

lkl tools: fix some -Wincompatible-pointer-types errors

4348d61

Explicitly cast between unsigned long and size_t for malloc, memcpy and memset. Signed-off-by: David Disseldorp <[email protected]>

tavip reviewed Feb 4, 2025

View reviewed changes

ddiss mentioned this pull request Feb 4, 2025

timeval -> timespec conversion broken? #557

Closed

ddiss force-pushed the lkl_tests_parallel_build_fix branch from a962b15 to 31f9880 Compare February 5, 2025 10:05

ddiss added 3 commits February 6, 2025 09:21

lkl cptofs: handle stat errors earlier

75fd3b6

Don't bother parsing the results if the initial stat call failed. Signed-off-by: David Disseldorp <[email protected]>

ddiss force-pushed the lkl_tests_parallel_build_fix branch from 31f9880 to 0692c1e Compare February 5, 2025 22:48

ddiss changed the title ~~lkl: fix parallel builds and some minor compiler warnings~~ lkl: 32-bit timespec fixes Feb 5, 2025

tavip approved these changes Feb 5, 2025

View reviewed changes

tavip merged commit 66e6ad4 into lkl:master Feb 6, 2025
12 of 14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lkl: 32-bit timespec fixes #556

lkl: 32-bit timespec fixes #556

ddiss commented Jan 31, 2025 •

edited

Loading

ddiss commented Feb 3, 2025

tavip Jan 31, 2025

ddiss Feb 4, 2025

ddiss Feb 4, 2025

thehajime Feb 4, 2025

tavip Feb 4, 2025

ddiss Feb 5, 2025

tavip Feb 5, 2025

ddiss commented Feb 5, 2025

ddiss commented Feb 5, 2025

tavip left a comment

lkl: 32-bit timespec fixes #556

lkl: 32-bit timespec fixes #556

Conversation

ddiss commented Jan 31, 2025 • edited Loading

ddiss commented Feb 3, 2025

tavip Jan 31, 2025

Choose a reason for hiding this comment

ddiss Feb 4, 2025

Choose a reason for hiding this comment

ddiss Feb 4, 2025

Choose a reason for hiding this comment

thehajime Feb 4, 2025

Choose a reason for hiding this comment

tavip Feb 4, 2025

Choose a reason for hiding this comment

ddiss Feb 5, 2025

Choose a reason for hiding this comment

tavip Feb 5, 2025

Choose a reason for hiding this comment

ddiss commented Feb 5, 2025

ddiss commented Feb 5, 2025

tavip left a comment

Choose a reason for hiding this comment

ddiss commented Jan 31, 2025 •

edited

Loading