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: Tue, 06 Jan 2015 11:46:09 -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> <5488807A dot 3090501 at linux dot vnet dot ibm dot com>
Ping.
On 10-12-2014 15:18, Adhemerval Zanella wrote:
> 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;
>> }
>>