This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: BZ# 16418: Fix powerpc get_clockfreq raciness
- From: Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>
- To: libc-alpha at sourceware dot org
- Date: Wed, 10 Dec 2014 15:18:50 -0200
- Subject: Re: BZ# 16418: Fix powerpc get_clockfreq raciness
- Authentication-results: sourceware.org; auth=none
- References: <5473A6C3 dot 5020806 at linux dot vnet dot ibm dot com>
Ping.
On 24-11-2014 19:44, Adhemerval Zanella wrote:
> This patch fixes powerpc __get_clockfreq racy and cancel-safe issues by
> dropping internal static cache and by using nocancel file operations.
> The vDSO failure check is also removed, since kernel code does not
> return an error (it cleans cr0.so bit on function return) and the static
> code (to read value /proc) now uses non-cancellable calls.
>
> Since currently I don't see this code patch to be performance sensitive
> (usually the clock frequency is obtained once to transform timebase
> values), I don't see a problem to drop its internal cache. Also, if
> latency came up as being important for this one, correct approach would
> be use IFUNC to call vDSO symbols directly (which I do not aim to
> implement now).
>
> Tested on powerpc64 and powerpc32.
>
> --
>
> [BZ# 16418]
> * sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
> (__get_clockfreq): Make code racy and cancel safe.
>
> ---
>
> diff --git a/NEWS b/NEWS
> index ad170c4..833a680 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -9,10 +9,10 @@ Version 2.21
>
> * The following bugs are resolved with this release:
>
> - 6652, 12926, 14132, 14138, 14171, 14498, 15215, 15884, 16469, 17266,
> - 17344, 17363, 17370, 17371, 17411, 17460, 17475, 17485, 17501, 17506,
> - 17508, 17522, 17555, 17570, 17571, 17572, 17573, 17574, 17582, 17583,
> - 17584, 17585, 17589, 17594, 17616, 17625.
> + 6652, 12926, 14132, 14138, 14171, 14498, 15215, 15884, 16418, 16469,
> + 17266, 17344, 17363, 17370, 17371, 17411, 17460, 17475, 17485, 17501,
> + 17506, 17508, 17522, 17555, 17570, 17571, 17572, 17573, 17574, 17582,
> + 17583, 17584, 17585, 17589, 17594, 17616, 17625.
>
> * CVE-2104-7817 The wordexp function could ignore the WRDE_NOCMD flag
> under certain input conditions resulting in the execution of a shell for
> diff --git a/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c b/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
> index 62217b1..44f90b4 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
> +++ b/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
> @@ -24,95 +24,85 @@
> #include <libc-internal.h>
> #include <sysdep.h>
> #include <bits/libc-vdso.h>
> +#include <not-cancel.h>
>
> hp_timing_t
> __get_clockfreq (void)
> {
> + hp_timing_t result = 0L;
> +
> +#ifdef SHARED
> + /* The vDSO does not return an error (it clear cr0.so on returning). */
> + INTERNAL_SYSCALL_DECL (err);
> + result =
> + INTERNAL_VSYSCALL_NO_SYSCALL_FALLBACK (get_tbfreq, err, uint64_t, 0);
> +#else
> /* We read the information from the /proc filesystem. /proc/cpuinfo
> contains at least one line like:
> timebase : 33333333
> We search for this line and convert the number into an integer. */
> - static hp_timing_t timebase_freq;
> - hp_timing_t result = 0L;
> + int fd = __open_nocancel ("/proc/cpuinfo", O_RDONLY);
> + if (__glibc_likely (fd != -1))
> + return result;
>
> - /* If this function was called before, we know the result. */
> - if (timebase_freq != 0)
> - return timebase_freq;
> + /* The timebase will be in the 1st 1024 bytes for systems with up
> + to 8 processors. If the first read returns less then 1024
> + bytes read, we have the whole cpuinfo and can start the scan.
> + Otherwise we will have to read more to insure we have the
> + timebase value in the scan. */
> + char buf[1024];
> + ssize_t n;
>
> - /* If we can use the vDSO to obtain the timebase even better. */
> -#ifdef SHARED
> - INTERNAL_SYSCALL_DECL (err);
> - timebase_freq =
> - INTERNAL_VSYSCALL_NO_SYSCALL_FALLBACK (get_tbfreq, err, uint64_t, 0);
> - if (INTERNAL_SYSCALL_ERROR_P (timebase_freq, err)
> - && INTERNAL_SYSCALL_ERRNO (timebase_freq, err) == ENOSYS)
> -#endif
> + n = __read_nocancel (fd, buf, sizeof (buf));
> + if (n == sizeof (buf))
> {
> - int fd = __open ("/proc/cpuinfo", O_RDONLY);
> + /* We are here because the 1st read returned exactly sizeof
> + (buf) bytes. This implies that we are not at EOF and may
> + not have read the timebase value yet. So we need to read
> + more bytes until we know we have EOF. We copy the lower
> + half of buf to the upper half and read sizeof (buf)/2
> + bytes into the lower half of buf and repeat until we
> + reach EOF. We can assume that the timebase will be in
> + the last 512 bytes of cpuinfo, so two 512 byte half_bufs
> + will be sufficient to contain the timebase and will
> + handle the case where the timebase spans the half_buf
> + boundry. */
> + const ssize_t half_buf = sizeof (buf) / 2;
> + while (n >= half_buf)
> + {
> + memcpy (buf, buf + half_buf, half_buf);
> + n = __read_nocancel (fd, buf + half_buf, half_buf);
> + }
> + if (n >= 0)
> + n += half_buf;
> + }
> + __close_nocancel (fd);
>
> - if (__glibc_likely (fd != -1))
> + if (__glibc_likely (n > 0))
> + {
> + char *mhz = memmem (buf, n, "timebase", 7);
> +
> + if (__glibc_likely (mhz != NULL))
> {
> - /* The timebase will be in the 1st 1024 bytes for systems with up
> - to 8 processors. If the first read returns less then 1024
> - bytes read, we have the whole cpuinfo and can start the scan.
> - Otherwise we will have to read more to insure we have the
> - timebase value in the scan. */
> - char buf[1024];
> - ssize_t n;
> + char *endp = buf + n;
>
> - n = __read (fd, buf, sizeof (buf));
> - if (n == sizeof (buf))
> - {
> - /* We are here because the 1st read returned exactly sizeof
> - (buf) bytes. This implies that we are not at EOF and may
> - not have read the timebase value yet. So we need to read
> - more bytes until we know we have EOF. We copy the lower
> - half of buf to the upper half and read sizeof (buf)/2
> - bytes into the lower half of buf and repeat until we
> - reach EOF. We can assume that the timebase will be in
> - the last 512 bytes of cpuinfo, so two 512 byte half_bufs
> - will be sufficient to contain the timebase and will
> - handle the case where the timebase spans the half_buf
> - boundry. */
> - const ssize_t half_buf = sizeof (buf) / 2;
> - while (n >= half_buf)
> - {
> - memcpy (buf, buf + half_buf, half_buf);
> - n = __read (fd, buf + half_buf, half_buf);
> - }
> - if (n >= 0)
> - n += half_buf;
> - }
> + /* Search for the beginning of the string. */
> + while (mhz < endp && (*mhz < '0' || *mhz > '9') && *mhz != '\n')
> + ++mhz;
>
> - if (__builtin_expect (n, 1) > 0)
> + while (mhz < endp && *mhz != '\n')
> {
> - char *mhz = memmem (buf, n, "timebase", 7);
> -
> - if (__glibc_likely (mhz != NULL))
> + if (*mhz >= '0' && *mhz <= '9')
> {
> - char *endp = buf + n;
> -
> - /* Search for the beginning of the string. */
> - while (mhz < endp && (*mhz < '0' || *mhz > '9')
> - && *mhz != '\n')
> - ++mhz;
> -
> - while (mhz < endp && *mhz != '\n')
> - {
> - if (*mhz >= '0' && *mhz <= '9')
> - {
> - result *= 10;
> - result += *mhz - '0';
> - }
> -
> - ++mhz;
> - }
> + result *= 10;
> + result += *mhz - '0';
> }
> - timebase_freq = result;
> +
> + ++mhz;
> }
> - __close (fd);
> }
> }
> +#endif
>
> - return timebase_freq;
> + return result;
> }
>