Re: [PATCH] Linux: Use 32-bit vDSO for clock_gettime, gettimeofday, time (bug 28071)

[Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org>]

On 12/07/2021 11:29, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>>> I'm a bit surprised that we still see the extra syscalls with your
>>> patch, but I suppose that's just the way the INTERNAL_VSYSCALL_CALL
>>> macro works.
>>
>> The INTERNAL_VSYSCALL_CALL issues the syscall if the vDSO is not present
>> as a fallback mechanism.  It should not be really necessary on most 
>> implementation currently, but there are some architectures and kernel
>> version where the vDSO does not actually does it (I think mips on kernel
>> 4.x version).
> 
> But we check the function pointer before that, so we should never hit
> the fallback path.  That's what confuses me.
> 
> Here's what I see on s390-linux-gnu (7.9.z).  System glibc
> (glibc-2.17-324.el7_9.s390):
> 
> munmap(0x7d4e3000, 35579)               = 0
> fstat64(1, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 1), ...}) = 0
> mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7d4eb000
> write(1, "errno = Success (0)\n", 20errno = Success (0)
> )   = 20
> write(1, "errno = Success (0)\n", 20errno = Success (0)
> )   = 20
> exit_group(1)                           = ?
> 
> Current development glibc with your patch applied on top:
> 
> ugetrlimit(RLIMIT_STACK, {rlim_cur=8192*1024, rlim_max=RLIM_INFINITY}) = 0
> syscall_0x193(0, 0x7f916310, 0x7f91652c, 0x7f9163fc, 0x400638, 0x7f916518) = -1 ENOSYS (Function not implemented)
> clock_gettime(CLOCK_REALTIME, {tv_sec=1626099916, tv_nsec=791410856}) = 0
> statx(1, "", AT_STATX_SYNC_AS_STAT|AT_NO_AUTOMOUNT|AT_EMPTY_PATH, STATX_BASIC_STATS, 0x7f915a90) = -1 ENOSYS (Function not implemented)
> fstatat64(1, "", {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 1), ...}, AT_EMPTY_PATH) = 0
> getrandom("\x16\x60\xeb\x47", 4, GRND_NONBLOCK) = 4
> brk(NULL)                               = 0x7d9e9000
> brk(0x7da0a000)                         = 0x7da0a000
> write(1, "errno = Success (0)\n", 20errno = Success (0)
> )   = 20
> syscall_0x193(0x1, 0x7f916310, 0x14, 0xfd46054a, 0x400638, 0x7f916518) = -1 ENOSYS (Function not implemented)
> clock_gettime(CLOCK_MONOTONIC, {tv_sec=29888, tv_nsec=604781810}) = 0
> write(1, "errno = Success (0)\n", 20errno = Success (0)
> )   = 20
> 
> The vdso has this (according to
> /lib/modules/3.10.0-1160.31.1.el7.s390x/vdso/vdso32.so, have not checked
> at run time):
> 
>     2: 00000300     84 FUNC    GLOBAL DEFAULT        7 __kernel_clock_getres@@LINUX_2.6.29
>     3: 00000230    208 FUNC    GLOBAL DEFAULT        7 __kernel_gettimeofday@@LINUX_2.6.29
>     5: 00000354    410 FUNC    GLOBAL DEFAULT        7 __kernel_clock_gettime@@LINUX_2.6.29

I am almost sure it is a kernel issue specific to s390, where kernel does 
not map the vDSO is there is no interpreter (it happens for static case
and for the loader directly). I think it was fixed upstream, although
s390 vDSO is also now gone on 5.5:

s390> uname -a
Linux 4.12.14-197.78-default #1 SMP Tue Jan 5 17:10:44 UTC 2021 (a30d048) s390x s390x s390x GNU/Linux
s390> cat dump_vdso.c
#include <stdio.h>
#include <string.h>
#include <assert.h>

int main (int argc, char *argv[])
{
  FILE *maps = fopen ("/proc/self/maps", "r");
  assert (maps != NULL);

  char *line = NULL;
  size_t s = 0;

  while (getline (&line, &s, maps) != -1)
    {
      if (strstr (line, "[vdso]") == NULL)
        continue;

      size_t start, end;
      sscanf (line, "%zx-%zx", &start, &end);

      fwrite ((void *) start, end - start, 1, stdout);
    }

  fclose (maps);

  return 0;
}
s390> gcc-8 -m31 dump_vdso.c -o dump_vdso
s390> ./dump_vdso > vdso.so
s390> readelf -Ws vdso.so | grep __kernel_clock_gettime
     5: 000003e4   496 FUNC    GLOBAL DEFAULT    8 __kernel_clock_gettime@@LINUX_2.6.29
s390> ./ld.so.1 --library-path . ./dump_vdso > vdso.so
s390> readelf -Ws vdso.so | grep __kernel_clock_gettime
readelf: Error: vdso.so: Failed to read file's magic number
s390> file vdso.so 
vdso.so: empty

With --enable-hardcoded-path-in-tests you can actually it does work as intended:

s390> ./testrun.sh --tool=strace ./time/tst-time-clobber --direct
[...]
read(3, "\177ELF\1\2\1\3\0\0\0\0\0\0\0\0\0\3\0\26\0\0\0\1\0\2eH\0\0\0004"..., 512) = 512
statx(3, "", AT_STATX_SYNC_AS_STAT|AT_NO_AUTOMOUNT|AT_EMPTY_PATH, STATX_BASIC_STATS, {stx_mask=STATX_BASIC_STATS, stx_attributes=0, stx_mode=S_IFREG|0755, stx_size=17013776, ...}) = 0
mmap2(NULL, 1773140, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0x7d0c8000
mmap2(0x7d26c000, 16384, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x1a3000) = 0x7d26c000
mmap2(0x7d270000, 36436, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x7d270000
close(3)                                = 0
mmap2(NULL, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7d0c6000
set_tid_address(0x7d0c6ce8)             = 2030
set_robust_list(0x7d0c6cf0, 12)         = 0
mprotect(0x7d26c000, 8192, PROT_READ)   = 0
mprotect(0x404000, 4096, PROT_READ)     = 0
mprotect(0x7d2ab000, 4096, PROT_READ)   = 0
ugetrlimit(RLIMIT_STACK, {rlim_cur=8192*1024, rlim_max=RLIM_INFINITY}) = 0
mmap2(NULL, 8, PROT_READ|PROT_WRITE, MAP_SHARED|MAP_ANONYMOUS, -1, 0) = 0x7d2aa000
getrandom("\xa7\x99\xbb\xee", 4, GRND_NONBLOCK) = 4
exit_group(0)                           = ?
+++ exited with 0 +++

> 
>>> Regarding the actual patch, there are a few missing spaces before
>>> parenthesis:
>>>
>>> +  int (*vdso_time64)(clockid_t clock_id, struct __timespec64 *tp)
>>> +    = GLRO(dl_vdso_clock_gettime64);
>>> +  int (*vdso_time)(clockid_t clock_id, struct timespec *tp)
>>> +    = GLRO(dl_vdso_clock_gettime);
>>
>> My understanding is for GLRO the space is not really required because the
>> macro is not used a function call.  I followed the same idea for the
>> function pointer definition.
> 
> I think the the ( in the pointer type still qualifies for the space
> because it is related to function application.  We use the space in all
> prototypes, after all.

Ack.

> 
>> Regardless of the missing space, are you ok with my patch then? 
> 
> It gets rid of the ENOSYS error, so it is a step forward