* precedence bug in MAKE_PROCESS_CPUCLOCK macro?
@ 2017-12-24 3:26 Nick Desaulniers
2017-12-28 15:49 ` Thomas Gleixner
0 siblings, 1 reply; 6+ messages in thread
From: Nick Desaulniers @ 2017-12-24 3:26 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: Al Viro, Deepa Dinamani, Linux Kernel Mailing List
I'm seeing the following warning compiling with Clang:
kernel/time/posix-cpu-timers.c:1397:29: warning: shifting a negative
signed value is undefined
[-Wshift-negative-value]
return posix_cpu_clock_get(THREAD_CLOCK, tp);
^~~~~~~~~~~~
kernel/time/posix-cpu-timers.c:1367:22: note: expanded from macro 'THREAD_CLOCK'
#define THREAD_CLOCK MAKE_THREAD_CPUCLOCK(0, CPUCLOCK_SCHED)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./include/linux/posix-timers.h:48:2: note: expanded from macro
'MAKE_THREAD_CPUCLOCK'
MAKE_PROCESS_CPUCLOCK((tid), (clock) | CPUCLOCK_PERTHREAD_MASK)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./include/linux/posix-timers.h:46:23: note: expanded from macro
'MAKE_PROCESS_CPUCLOCK'
((~(clockid_t) (pid) << 3) | (clockid_t) (clock))
~~~~~~~~~~~~~~~~~~ ^
If I understand C's operator precedence rules
(http://en.cppreference.com/w/c/language/operator_precedence)
correctly, then I suspect the problem is in the sub-expression:
(~(clockid_t) (pid) << 3)
where pid (an argument to the macro) is first cast to a clockid_t (aka
[signed] int), then negated, then shifted by 3 (oops, undefined
behavior).
Should the result after negation be cast to an unsigned int, or should
the left shift happen before negation?
CPUCLOCK_PID and CLOCKID_TO_FD seem to shift then negate, but
FD_TO_CLOCKID seems to have the same issue as MAKE_PROCESS_CPUCLOCK.
Changing the sub-expression to:
(~(clockid_t) ((pid) << 3))
changes what it evaluates to. Changing it to:
(~(unsigned) (pid) << 3)
or
((unsigned) ~(clockid_t) (pid) << 3)
or
(((unsigned) ~(clockid_t) (pid)) << 3) /* ugly */
does not.
I'm happy to send a patch with your suggestion.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: precedence bug in MAKE_PROCESS_CPUCLOCK macro? 2017-12-24 3:26 precedence bug in MAKE_PROCESS_CPUCLOCK macro? Nick Desaulniers @ 2017-12-28 15:49 ` Thomas Gleixner 2017-12-29 3:01 ` [PATCH] posix-timers: prevent UB from shifting negative signed value Nick Desaulniers 0 siblings, 1 reply; 6+ messages in thread From: Thomas Gleixner @ 2017-12-28 15:49 UTC (permalink / raw) To: Nick Desaulniers; +Cc: Al Viro, Deepa Dinamani, Linux Kernel Mailing List On Sat, 23 Dec 2017, Nick Desaulniers wrote: > I'm seeing the following warning compiling with Clang: > > kernel/time/posix-cpu-timers.c:1397:29: warning: shifting a negative > signed value is undefined > [-Wshift-negative-value] > return posix_cpu_clock_get(THREAD_CLOCK, tp); > ^~~~~~~~~~~~ > kernel/time/posix-cpu-timers.c:1367:22: note: expanded from macro 'THREAD_CLOCK' > #define THREAD_CLOCK MAKE_THREAD_CPUCLOCK(0, CPUCLOCK_SCHED) > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ./include/linux/posix-timers.h:48:2: note: expanded from macro > 'MAKE_THREAD_CPUCLOCK' > MAKE_PROCESS_CPUCLOCK((tid), (clock) | CPUCLOCK_PERTHREAD_MASK) > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ./include/linux/posix-timers.h:46:23: note: expanded from macro > 'MAKE_PROCESS_CPUCLOCK' > ((~(clockid_t) (pid) << 3) | (clockid_t) (clock)) > ~~~~~~~~~~~~~~~~~~ ^ > > If I understand C's operator precedence rules > (http://en.cppreference.com/w/c/language/operator_precedence) > correctly, then I suspect the problem is in the sub-expression: > > (~(clockid_t) (pid) << 3) > > where pid (an argument to the macro) is first cast to a clockid_t (aka > [signed] int), then negated, then shifted by 3 (oops, undefined > behavior). > > Should the result after negation be cast to an unsigned int, or should > the left shift happen before negation? > > CPUCLOCK_PID and CLOCKID_TO_FD seem to shift then negate, but > FD_TO_CLOCKID seems to have the same issue as MAKE_PROCESS_CPUCLOCK. > > Changing the sub-expression to: > > (~(clockid_t) ((pid) << 3)) > > changes what it evaluates to. Changing it to: > > (~(unsigned) (pid) << 3) > > or > > ((unsigned) ~(clockid_t) (pid) << 3) > > or > > (((unsigned) ~(clockid_t) (pid)) << 3) /* ugly */ All of these are butt ugly. And the same problem exists for FD_TO_CLOCKID. My preference would be to replace all these crappy macros with simple inline functions. Thanks, tglx ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] posix-timers: prevent UB from shifting negative signed value 2017-12-28 15:49 ` Thomas Gleixner @ 2017-12-29 3:01 ` Nick Desaulniers 2017-12-29 3:08 ` Nick Desaulniers 2017-12-29 3:11 ` [PATCH v2] " Nick Desaulniers 0 siblings, 2 replies; 6+ messages in thread From: Nick Desaulniers @ 2017-12-29 3:01 UTC (permalink / raw) To: Thomas Gleixner Cc: Nick Desaulniers, Shuah Khan, Deepa Dinamani, Ingo Molnar, Al Viro, Frederic Weisbecker, Dimitri Sivanich, linux-kernel, linux-kselftest Shifting a negative signed number is undefined behavior. Looking at the macros MAKE_PROCESS_CPUCLOCK and FD_TO_CLOCKID, it seems that the subexpression: (~(clockid_t) (pid) << 3) where clockid_t resolves to a signed int, which once negated, is undefined behavior to shift the value of if the results thus far are negative. It was further suggested to make these macros into inline functions. Suggested-by: Thomas Gleixner <tglx@linutronix.de> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com> --- include/linux/posix-timers.h | 25 +++++++++++++++++++------ kernel/time/posix-clock.c | 2 +- kernel/time/posix-cpu-timers.c | 4 ++-- tools/testing/selftests/ptp/testptp.c | 4 +--- 4 files changed, 23 insertions(+), 12 deletions(-) diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h index 672c4f3..b3d87c0 100644 --- a/include/linux/posix-timers.h +++ b/include/linux/posix-timers.h @@ -42,13 +42,26 @@ struct cpu_timer_list { #define CLOCKFD CPUCLOCK_MAX #define CLOCKFD_MASK (CPUCLOCK_PERTHREAD_MASK|CPUCLOCK_CLOCK_MASK) -#define MAKE_PROCESS_CPUCLOCK(pid, clock) \ - ((~(clockid_t) (pid) << 3) | (clockid_t) (clock)) -#define MAKE_THREAD_CPUCLOCK(tid, clock) \ - MAKE_PROCESS_CPUCLOCK((tid), (clock) | CPUCLOCK_PERTHREAD_MASK) +static inline clockid_t make_process_cpuclock(const unsigned int pid, + const clockid_t clock) +{ + return ((~pid) << 3) | clock; +} +static inline clockid_t make_thread_cpuclock(const unsigned int tid, + const clockid_t clock) +{ + return make_process_cpuclock(tid, clock | CPUCLOCK_PERTHREAD_MASK); +} -#define FD_TO_CLOCKID(fd) ((~(clockid_t) (fd) << 3) | CLOCKFD) -#define CLOCKID_TO_FD(clk) ((unsigned int) ~((clk) >> 3)) +inline int fd_to_clockid(const int fd) +{ + return (int) make_process_cpuclock((unsigned int) fd, CLOCKFD); +} + +inline int clockid_to_fd(const clockid_t clk) +{ + return ~(clk >> 3); +} #define REQUEUE_PENDING 1 diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c index 17cdc55..cc91d90 100644 --- a/kernel/time/posix-clock.c +++ b/kernel/time/posix-clock.c @@ -216,7 +216,7 @@ struct posix_clock_desc { static int get_clock_desc(const clockid_t id, struct posix_clock_desc *cd) { - struct file *fp = fget(CLOCKID_TO_FD(id)); + struct file *fp = fget(clockid_to_fd(id)); int err = -EINVAL; if (!fp) diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c index 1f27887a..cef79ca 100644 --- a/kernel/time/posix-cpu-timers.c +++ b/kernel/time/posix-cpu-timers.c @@ -1363,8 +1363,8 @@ static long posix_cpu_nsleep_restart(struct restart_block *restart_block) return do_cpu_nanosleep(which_clock, TIMER_ABSTIME, &t); } -#define PROCESS_CLOCK MAKE_PROCESS_CPUCLOCK(0, CPUCLOCK_SCHED) -#define THREAD_CLOCK MAKE_THREAD_CPUCLOCK(0, CPUCLOCK_SCHED) +#define PROCESS_CLOCK make_process_cpuclock(0, CPUCLOCK_SCHED) +#define THREAD_CLOCK make_thread_cpuclock(0, CPUCLOCK_SCHED) static int process_cpu_clock_getres(const clockid_t which_clock, struct timespec64 *tp) diff --git a/tools/testing/selftests/ptp/testptp.c b/tools/testing/selftests/ptp/testptp.c index 5d2eae1..a5d8f0a 100644 --- a/tools/testing/selftests/ptp/testptp.c +++ b/tools/testing/selftests/ptp/testptp.c @@ -60,9 +60,7 @@ static int clock_adjtime(clockid_t id, struct timex *tx) static clockid_t get_clockid(int fd) { #define CLOCKFD 3 -#define FD_TO_CLOCKID(fd) ((~(clockid_t) (fd) << 3) | CLOCKFD) - - return FD_TO_CLOCKID(fd); + return (((unsigned int) ~fd) << 3) | CLOCKFD; } static void handle_alarm(int s) -- 2.7.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] posix-timers: prevent UB from shifting negative signed value 2017-12-29 3:01 ` [PATCH] posix-timers: prevent UB from shifting negative signed value Nick Desaulniers @ 2017-12-29 3:08 ` Nick Desaulniers 2017-12-29 3:11 ` [PATCH v2] " Nick Desaulniers 1 sibling, 0 replies; 6+ messages in thread From: Nick Desaulniers @ 2017-12-29 3:08 UTC (permalink / raw) To: Thomas Gleixner Cc: Nick Desaulniers, Shuah Khan, Deepa Dinamani, Ingo Molnar, Al Viro, Frederic Weisbecker, Dimitri Sivanich, Linux Kernel Mailing List, linux-kselftest sorry, fd_to_clockid() should probably return a clockid_t, let me fix that. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] posix-timers: prevent UB from shifting negative signed value 2017-12-29 3:01 ` [PATCH] posix-timers: prevent UB from shifting negative signed value Nick Desaulniers 2017-12-29 3:08 ` Nick Desaulniers @ 2017-12-29 3:11 ` Nick Desaulniers 2018-01-04 14:01 ` [tip:timers/core] posix-timers: Prevent " tip-bot for Nick Desaulniers 1 sibling, 1 reply; 6+ messages in thread From: Nick Desaulniers @ 2017-12-29 3:11 UTC (permalink / raw) To: Thomas Gleixner Cc: Nick Desaulniers, Shuah Khan, Deepa Dinamani, Al Viro, Frederic Weisbecker, Ingo Molnar, Dimitri Sivanich, linux-kernel, linux-kselftest Shifting a negative signed number is undefined behavior. Looking at the macros MAKE_PROCESS_CPUCLOCK and FD_TO_CLOCKID, it seems that the subexpression: (~(clockid_t) (pid) << 3) where clockid_t resolves to a signed int, which once negated, is undefined behavior to shift the value of if the results thus far are negative. It was further suggested to make these macros into inline functions. Suggested-by: Thomas Gleixner <tglx@linutronix.de> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com> --- Changes in v2: * change return type of fd_to_clockid() from int to clockid_t * mark fd_to_clockid() and clockid_to_fd() as static inline, not just inline. include/linux/posix-timers.h | 25 +++++++++++++++++++------ kernel/time/posix-clock.c | 2 +- kernel/time/posix-cpu-timers.c | 4 ++-- tools/testing/selftests/ptp/testptp.c | 4 +--- 4 files changed, 23 insertions(+), 12 deletions(-) diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h index 672c4f3..c85704f 100644 --- a/include/linux/posix-timers.h +++ b/include/linux/posix-timers.h @@ -42,13 +42,26 @@ struct cpu_timer_list { #define CLOCKFD CPUCLOCK_MAX #define CLOCKFD_MASK (CPUCLOCK_PERTHREAD_MASK|CPUCLOCK_CLOCK_MASK) -#define MAKE_PROCESS_CPUCLOCK(pid, clock) \ - ((~(clockid_t) (pid) << 3) | (clockid_t) (clock)) -#define MAKE_THREAD_CPUCLOCK(tid, clock) \ - MAKE_PROCESS_CPUCLOCK((tid), (clock) | CPUCLOCK_PERTHREAD_MASK) +static inline clockid_t make_process_cpuclock(const unsigned int pid, + const clockid_t clock) +{ + return ((~pid) << 3) | clock; +} +static inline clockid_t make_thread_cpuclock(const unsigned int tid, + const clockid_t clock) +{ + return make_process_cpuclock(tid, clock | CPUCLOCK_PERTHREAD_MASK); +} -#define FD_TO_CLOCKID(fd) ((~(clockid_t) (fd) << 3) | CLOCKFD) -#define CLOCKID_TO_FD(clk) ((unsigned int) ~((clk) >> 3)) +static inline clockid_t fd_to_clockid(const int fd) +{ + return make_process_cpuclock((unsigned int) fd, CLOCKFD); +} + +static inline int clockid_to_fd(const clockid_t clk) +{ + return ~(clk >> 3); +} #define REQUEUE_PENDING 1 diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c index 17cdc55..cc91d90 100644 --- a/kernel/time/posix-clock.c +++ b/kernel/time/posix-clock.c @@ -216,7 +216,7 @@ struct posix_clock_desc { static int get_clock_desc(const clockid_t id, struct posix_clock_desc *cd) { - struct file *fp = fget(CLOCKID_TO_FD(id)); + struct file *fp = fget(clockid_to_fd(id)); int err = -EINVAL; if (!fp) diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c index 1f27887a..cef79ca 100644 --- a/kernel/time/posix-cpu-timers.c +++ b/kernel/time/posix-cpu-timers.c @@ -1363,8 +1363,8 @@ static long posix_cpu_nsleep_restart(struct restart_block *restart_block) return do_cpu_nanosleep(which_clock, TIMER_ABSTIME, &t); } -#define PROCESS_CLOCK MAKE_PROCESS_CPUCLOCK(0, CPUCLOCK_SCHED) -#define THREAD_CLOCK MAKE_THREAD_CPUCLOCK(0, CPUCLOCK_SCHED) +#define PROCESS_CLOCK make_process_cpuclock(0, CPUCLOCK_SCHED) +#define THREAD_CLOCK make_thread_cpuclock(0, CPUCLOCK_SCHED) static int process_cpu_clock_getres(const clockid_t which_clock, struct timespec64 *tp) diff --git a/tools/testing/selftests/ptp/testptp.c b/tools/testing/selftests/ptp/testptp.c index 5d2eae1..a5d8f0a 100644 --- a/tools/testing/selftests/ptp/testptp.c +++ b/tools/testing/selftests/ptp/testptp.c @@ -60,9 +60,7 @@ static int clock_adjtime(clockid_t id, struct timex *tx) static clockid_t get_clockid(int fd) { #define CLOCKFD 3 -#define FD_TO_CLOCKID(fd) ((~(clockid_t) (fd) << 3) | CLOCKFD) - - return FD_TO_CLOCKID(fd); + return (((unsigned int) ~fd) << 3) | CLOCKFD; } static void handle_alarm(int s) -- 2.7.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip:timers/core] posix-timers: Prevent UB from shifting negative signed value 2017-12-29 3:11 ` [PATCH v2] " Nick Desaulniers @ 2018-01-04 14:01 ` tip-bot for Nick Desaulniers 0 siblings, 0 replies; 6+ messages in thread From: tip-bot for Nick Desaulniers @ 2018-01-04 14:01 UTC (permalink / raw) To: linux-tip-commits Cc: hpa, sivanich, mingo, viro, shuah, tglx, fweisbec, nick.desaulniers, linux-kernel, deepa.kernel Commit-ID: 29f1b2b0fecfae69e31833836f1da3136696eee5 Gitweb: https://git.kernel.org/tip/29f1b2b0fecfae69e31833836f1da3136696eee5 Author: Nick Desaulniers <nick.desaulniers@gmail.com> AuthorDate: Thu, 28 Dec 2017 22:11:36 -0500 Committer: Thomas Gleixner <tglx@linutronix.de> CommitDate: Thu, 4 Jan 2018 14:57:10 +0100 posix-timers: Prevent UB from shifting negative signed value Shifting a negative signed number is undefined behavior. Looking at the macros MAKE_PROCESS_CPUCLOCK and FD_TO_CLOCKID, it seems that the subexpression: (~(clockid_t) (pid) << 3) where clockid_t resolves to a signed int, which once negated, is undefined behavior to shift the value of if the results thus far are negative. It was further suggested to make these macros into inline functions. Suggested-by: Thomas Gleixner <tglx@linutronix.de> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Cc: Dimitri Sivanich <sivanich@hpe.com> Cc: Frederic Weisbecker <fweisbec@gmail.com> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: linux-kselftest@vger.kernel.org Cc: Shuah Khan <shuah@kernel.org> Cc: Deepa Dinamani <deepa.kernel@gmail.com> Link: https://lkml.kernel.org/r/1514517100-18051-1-git-send-email-nick.desaulniers@gmail.com --- include/linux/posix-timers.h | 25 +++++++++++++++++++------ kernel/time/posix-clock.c | 2 +- kernel/time/posix-cpu-timers.c | 4 ++-- tools/testing/selftests/ptp/testptp.c | 4 +--- 4 files changed, 23 insertions(+), 12 deletions(-) diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h index 672c4f3..c85704f 100644 --- a/include/linux/posix-timers.h +++ b/include/linux/posix-timers.h @@ -42,13 +42,26 @@ struct cpu_timer_list { #define CLOCKFD CPUCLOCK_MAX #define CLOCKFD_MASK (CPUCLOCK_PERTHREAD_MASK|CPUCLOCK_CLOCK_MASK) -#define MAKE_PROCESS_CPUCLOCK(pid, clock) \ - ((~(clockid_t) (pid) << 3) | (clockid_t) (clock)) -#define MAKE_THREAD_CPUCLOCK(tid, clock) \ - MAKE_PROCESS_CPUCLOCK((tid), (clock) | CPUCLOCK_PERTHREAD_MASK) +static inline clockid_t make_process_cpuclock(const unsigned int pid, + const clockid_t clock) +{ + return ((~pid) << 3) | clock; +} +static inline clockid_t make_thread_cpuclock(const unsigned int tid, + const clockid_t clock) +{ + return make_process_cpuclock(tid, clock | CPUCLOCK_PERTHREAD_MASK); +} -#define FD_TO_CLOCKID(fd) ((~(clockid_t) (fd) << 3) | CLOCKFD) -#define CLOCKID_TO_FD(clk) ((unsigned int) ~((clk) >> 3)) +static inline clockid_t fd_to_clockid(const int fd) +{ + return make_process_cpuclock((unsigned int) fd, CLOCKFD); +} + +static inline int clockid_to_fd(const clockid_t clk) +{ + return ~(clk >> 3); +} #define REQUEUE_PENDING 1 diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c index 17cdc55..cc91d90 100644 --- a/kernel/time/posix-clock.c +++ b/kernel/time/posix-clock.c @@ -216,7 +216,7 @@ struct posix_clock_desc { static int get_clock_desc(const clockid_t id, struct posix_clock_desc *cd) { - struct file *fp = fget(CLOCKID_TO_FD(id)); + struct file *fp = fget(clockid_to_fd(id)); int err = -EINVAL; if (!fp) diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c index 1f27887a..cef79ca 100644 --- a/kernel/time/posix-cpu-timers.c +++ b/kernel/time/posix-cpu-timers.c @@ -1363,8 +1363,8 @@ static long posix_cpu_nsleep_restart(struct restart_block *restart_block) return do_cpu_nanosleep(which_clock, TIMER_ABSTIME, &t); } -#define PROCESS_CLOCK MAKE_PROCESS_CPUCLOCK(0, CPUCLOCK_SCHED) -#define THREAD_CLOCK MAKE_THREAD_CPUCLOCK(0, CPUCLOCK_SCHED) +#define PROCESS_CLOCK make_process_cpuclock(0, CPUCLOCK_SCHED) +#define THREAD_CLOCK make_thread_cpuclock(0, CPUCLOCK_SCHED) static int process_cpu_clock_getres(const clockid_t which_clock, struct timespec64 *tp) diff --git a/tools/testing/selftests/ptp/testptp.c b/tools/testing/selftests/ptp/testptp.c index 5d2eae1..a5d8f0a 100644 --- a/tools/testing/selftests/ptp/testptp.c +++ b/tools/testing/selftests/ptp/testptp.c @@ -60,9 +60,7 @@ static int clock_adjtime(clockid_t id, struct timex *tx) static clockid_t get_clockid(int fd) { #define CLOCKFD 3 -#define FD_TO_CLOCKID(fd) ((~(clockid_t) (fd) << 3) | CLOCKFD) - - return FD_TO_CLOCKID(fd); + return (((unsigned int) ~fd) << 3) | CLOCKFD; } static void handle_alarm(int s) ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-01-04 14:04 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-12-24 3:26 precedence bug in MAKE_PROCESS_CPUCLOCK macro? Nick Desaulniers 2017-12-28 15:49 ` Thomas Gleixner 2017-12-29 3:01 ` [PATCH] posix-timers: prevent UB from shifting negative signed value Nick Desaulniers 2017-12-29 3:08 ` Nick Desaulniers 2017-12-29 3:11 ` [PATCH v2] " Nick Desaulniers 2018-01-04 14:01 ` [tip:timers/core] posix-timers: Prevent " tip-bot for Nick Desaulniers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®