Incorrect value for SIGRTMAX
Hello,
Few months ago, Corey Minyard corrected handling of incorrect values of signals. cf
http://linux.bkbits.net:8080/linux-2.5/[email protected]?nav=index.html%7Csrc/%7Csrc/kernel%7Crelated/kernel/posix-timers.c
Working on the High-Resolution Timers project, I noticed there is an error in good_sigevent() to catch the case when sigev_signo is 0. In this function, we want to return NULL when sigev_signo is 0. The one liner attached (used for a long time in the HRT patch) should do the trick, it's against vanilla 2.6.1 .
Eric
kernel/posix-timers.c renamed from kernel/posix-timers.c.orig +1 -2
@@ -344,8 +344,7 @@return NULL ;if ((event->sigev_notify & ~SIGEV_NONE &MIPS_SIGEV ) && - event->sigev_signo && - ((unsigned ) (event->sigev_signo >SIGRTMAX ))) + ((unsigned ) (event->sigev_signo >SIGRTMAX ) || !event->sigev_signo))return NULL ;return rtn;
Quoted 12 lines · [email protected] wrote:
Hello,
Few months ago, Corey Minyard corrected handling of incorrect values of signals. cf
http://linux.bkbits.net:8080/linux-2.5/[email protected]?nav=index.html%7Csrc/%7Csrc/kernel%7Crelated/kernel/posix-timers.cWorking on the High-Resolution Timers project, I noticed there is an error in
good_sigevent()to catch the case whensigev_signois 0. In this function, we want to return NULL whensigev_signois 0. The one liner attached (used for a long time in the HRT patch) should do the trick, it's against vanilla 2.6.1 .
OK, the current code is obviously junk:
if ((event->sigev_notify & ~SIGEV_NONE & MIPS_SIGEV) && event->sigev_signo && ((unsigned) (event->sigev_signo > SIGRTMAX))) return NULL;
a) it's doing
if (foo && foo > N)
which is redundant.
b) it's casting the result of (foo > N) to unsigned which is nonsensical.
Your patch doesn't address b).
I don't thik there's a case in which sigev_signo can be negative anyway. But if there is, the cast should be done like the below, yes?
kernel/posix-timers.c | 3 +-- 1 files changed, 1 insertion(+), 2 deletions(-)
kernel/posix-timers.c renamed from kernel/posix-timers.c~SIGRTMAX-fix +1 -2
@@ -344,8 +344,7 @@ static inline struct task_struct * good_return NULL ;if ((event->sigev_notify & ~SIGEV_NONE &MIPS_SIGEV ) && - event->sigev_signo && - ((unsigned ) (event->sigev_signo >SIGRTMAX ))) + (((unsigned )event->sigev_signo >SIGRTMAX ) || !event->sigev_signo))return NULL ;return rtn;
_
Quoting Andrew Morton <[email protected]>:
Quoted 6 lines
b) it's casting the result of (foo > N) to unsigned which is nonsensical.
Your patch doesn't address b).
I don't thik there's a case in which
sigev_signocan be negative anyway. But if there is, the cast should be done like the below, yes?
God! I hadn't catch this one :-) Actually, the cast is needed because sigev_signo is an int, this catches the (all fobidden) negative values.
Your patch is the right one :-)
Eric
Quoted 19 lines
kernel/posix-timers.c | 3 +-- 1 files changed, 1 insertion(+), 2 deletions(-)diff -puN
kernel/posix-timers.c~SIGRTMAX-fixkernel/posix-timers.c
--- 25/kernel/posix-timers.c~SIGRTMAX-fix 2004-01-24 14:27:13.000000000
-0800
+++25-akpm/kernel/posix-timers.c2004-01-24 14:28:21.000000000 -0800kernel/posix-timers.c renamed from kernel/posix-timers.c~SIGRTMAX-fix +1
-2@@ -344,8 +344,7 @@ static inline struct task_struct * good_return NULL ;if ((event->sigev_notify & ~SIGEV_NONE &MIPS_SIGEV ) && - event->sigev_signo && - ((unsigned ) (event->sigev_signo >SIGRTMAX ))) + (((unsigned )event->sigev_signo >SIGRTMAX ) || !event->sigev_signo))return NULL ;return rtn;
If we are going to open this, I would like to eliminate the "MIGS_SIGEV" stuff.
If you can wait till Monday...
Another issue is that this is the only place in the kernel where SIGRTMAX is used (or it was a few months ago). If memory serves, it also seems that it is the wrong value in at least some archs.
George
Quoted 43 lines · [email protected] wrote:
Quoting Andrew Morton <[email protected]>:
Quoted 5 lines
b) it's casting the result of (foo > N) to unsigned which is nonsensical.
Your patch doesn't address b).
I don't thik there's a case in which
sigev_signocan be negative anyway. But if there is, the cast should be done like the below, yes?God! I hadn't catch this one :-) Actually, the cast is needed because
sigev_signois an int, this catches the (all fobidden) negative values.Your patch is the right one :-)
EricQuoted 9 lines
kernel/posix-timers.c| 3 +--1 files changed, 1 insertion(+), 2 deletions(-)diff -puN
kernel/posix-timers.c~SIGRTMAX-fixkernel/posix-timers.c
--- 25/kernel/posix-timers.c~SIGRTMAX-fix 2004-01-24 14:27:13.000000000
-0800
+++25-akpm/kernel/posix-timers.c2004-01-24 14:28:21.000000000 -0800kernel/posix-timers.c static inline struct task_struct * good_ · line 350
Quoted 9 lines
kernel/posix-timers.c renamed from kernel/posix-timers.c~SIGRTMAX-fix +1
-2@@ -344,8 +344,7 @@ static inline struct task_struct * good_return NULL ;if ((event->sigev_notify & ~SIGEV_NONE &MIPS_SIGEV ) && - event->sigev_signo && - ((unsigned ) (event->sigev_signo >SIGRTMAX ))) + (((unsigned )event->sigev_signo >SIGRTMAX ) || !event->sigev_signo))return NULL ;return rtn;-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 3 lines
George Anzinger [email protected] High-res-timers: http://sourceforge.net/projects/high-res-timers/ Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml
Quoting George Anzinger <[email protected]>:
Quoted 2 lines
If we are going to open this, I would like to eliminate the "
MIGS_SIGEV" stuff. If you can wait till Monday...
Eliminating the special code for MIGS_SIGEV would be a great idea!
Quoted 3 lines
Another issue is that this is the only place in the kernel where SIGRTMAX is used (or it was a few months ago). If memory serves, it also seems that it is the wrong value in at least some archs.
Actually, it's not. That's exactly what the patch of Corey Minyard addressed. Half a dozen arch were corrected and now SIGRTMAX means SIGRTMAX :-)
Eric
PATCH 1/1 Incorrect value for SIGRTMAX, MIPS nonsense removed, timer_gettime fix
In reply to [email protected]The attached patch does the following:
Removes C++ comment in favor of C style.
Removes the special treatment for MIPS SIGEV values. We only require (and error if this fails) that the SIGEV_THREAD_ID value not share bits with the other SIGEV values. Note that mips has yet to define this value so when they do...
Corrects the check for the signal range to be from 1 to SIGRTMAX inclusive.
Adds a check to verify that kmem_cache_alloc() actually returned a timer, error if not.
Fixes a bug in timer_gettime() where the incorrect value was returned if a signal was pending on the timer OR the timer was a SIGEV_NONE timer.
Signature · 3 lines
George Anzinger [email protected] High-res-timers: http://sourceforge.net/projects/high-res-timers/ Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml
kernel/posix-timers.c +45 -26
@@ -2,8 +2,26 @@* linux/kernel/posix_timers.c * * -* 2002-10-15 Posix Clocks & timers by George Anzinger -* Copyright (C) 2002 by MontaVista Software. +* 2002-10-15 Posix Clocks & timers +* by George Anzinger [email protected] +* +* Copyright (C) 2002 2003 by MontaVista Software. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or (at +* your option) any later version. +* +* This program is distributed in the hope that it will be useful, but +* WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +* General Public License for more details. + +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. +* +* MontaVista Software | 1237 East Arques Avenue | Sunnyvale | CA 94085 | USA */ /* These are all the functions necessary to implement
@@ -33,7 +51,7 @@ result; })#endif -#define CLOCK_REALTIME_RES TICK_NSEC // In nano seconds. +#define CLOCK_REALTIME_RES TICK_NSEC/* In nano seconds. */ static inline u64 mpy_l_X_l_ll (unsigned long mpy1,unsigned long mpy2) {
@@ -82,17 +100,15 @@# define timer_active (tmr) BARFY // error to use outside of SMP# define set_timer_inactive (tmr) do { } while (0)#endif -/* -* For some reason mips/mips64 define the SIGEV constants plus 128. -* Here we define a mask to get rid of the common bits. The -* optimizer should make this costless to all but mips. -* Note that no common bits (the non-mips case) will give 0xffffffff. -*/ -#define MIPS_SIGEV ~(SIGEV_NONE & \ - SIGEV_SIGNAL & \ - SIGEV_THREAD & \ - SIGEV_THREAD_ID) +* we assume that the new SIGEV_THREAD_ID shares no bits with the other +* SIGEV values. Here we put out an error if this assumption fails. +*/ +#if SIGEV_THREAD_ID != (SIGEV_THREAD_ID & \ + ~(SIGEV_SIGNAL |SIGEV_NONE |SIGEV_THREAD )) +#error "SIGEV_THREAD_ID must not share bit with other SIGEV values!" +#endif +#define REQUEUE_PENDING 1/*
@@ -301,7 +317,7 @@if (timr->it_incr) timr->sigq->info.si_sys_private = ++timr->it_requeue_pending; -if (timr->it_sigev_notify &SIGEV_THREAD_ID &MIPS_SIGEV ) +if (timr->it_sigev_notify &SIGEV_THREAD_ID ) ret =send_sigqueue (timr->it_sigev_signo, timr->sigq, timr->it_process);else
@@ -338,14 +354,14 @@ {struct task_struct *rtn = current; -if ((event->sigev_notify &SIGEV_THREAD_ID &MIPS_SIGEV ) && +if ((event->sigev_notify &SIGEV_THREAD_ID ) && (!(rtn =find_task_by_pid (event->sigev_notify_thread_id)) || - rtn->tgid != current->tgid)) + rtn->tgid != current->tgid || + (event->sigev_notify & ~SIGEV_THREAD_ID ) !=SIGEV_SIGNAL ))return NULL ; -if ((event->sigev_notify & ~SIGEV_NONE &MIPS_SIGEV ) && - event->sigev_signo && - ((unsigned ) (event->sigev_signo >SIGRTMAX ))) +if (((event->sigev_notify & ~SIGEV_THREAD_ID ) !=SIGEV_NONE ) && + ((unsigned int ) (event->sigev_signo -1 ) >=SIGRTMAX ))return NULL ;return rtn;
@@ -365,6 +381,8 @@ {struct k_itimer *tmr; tmr =kmem_cache_alloc (posix_timers_cache,GFP_KERNEL ); +if (!tmr) +return tmr;memset (tmr,0 ,sizeof (struct k_itimer )); tmr->it_id = (timer_t)-1 ;if (unlikely (!(tmr->sigq =sigqueue_alloc ()))) {
@@ -586,14 +604,18 @@posix_get_now (&now); -if (expires && (timr->it_sigev_notify &SIGEV_NONE ) && !timr->it_incr && -posix_time_before (&timr->it_timer, &now)) +if (expires && + ((timr->it_sigev_notify & ~SIGEV_THREAD_ID ) ==SIGEV_NONE ) && + !timr->it_incr && +posix_time_before (&timr->it_timer, &now)) timr->it_timer.expires = expires =0 ;if (expires) {if (timr->it_requeue_pending &REQUEUE_PENDING || - (timr->it_sigev_notify &SIGEV_NONE )) + (timr->it_sigev_notify & ~SIGEV_THREAD_ID ) ==SIGEV_NONE ) {while (posix_time_before (&timr->it_timer, &now))posix_bump_timer (timr); + expires = timr->it_timer.expires; + }else if (!timer_pending (&timr->it_timer)) expires =0 ;
@@ -804,7 +826,7 @@* equal to jiffies, so the timer notify function is called directly. * We do not even queue SIGEV_NONE timers! */ -if (!(timr->it_sigev_notify &SIGEV_NONE )) { +if (!((timr->it_sigev_notify & ~SIGEV_THREAD_ID ) ==SIGEV_NONE )) {if (timr->it_timer.expires == jiffies)timer_notify_task (timr);else
@@ -967,9 +989,6 @@* if we are interrupted since we don't take lock that will stall us or * any other cpu. Voila, no irq lock is needed. * -* Note also that the while loop assures that the sub_jiff_offset -* will be less than a jiffie, thus no need to normalize the result. -* Well, not really, if called with ints off :( */ static u64 do_posix_clock_monotonic_gettime_parts(
Re: [PATCH] Incorrect value for SIGRTMAX, MIPS nonsense removed, timer_gettime fix
Quoted 17 lines · George Anzinger
The attached patch does the following:
Removes C++ comment in favor of C style.
Removes the special treatment for MIPS SIGEV values. We only require (and error if this fails) that the
SIGEV_THREAD_IDvalue not share bits with the other SIGEV values. Note that mips has yet to define this value so when they do...Corrects the check for the signal range to be from 1 to SIGRTMAX inclusive.
Adds a check to verify that
kmem_cache_alloc()actually returned a timer, error if not.Fixes a bug in
timer_gettime()where the incorrect value was returned if a signal was pending on the timer OR the timer was aSIGEV_NONEtimer.
kernel/posix-timers.c line 365
Quoted 6 lines
kernel/posix-timers.c +2
-3-if ((event->sigev_notify & ~SIGEV_NONE &MIPS_SIGEV ) && - event->sigev_signo && - ((unsigned ) (event->sigev_signo >SIGRTMAX ))) +if (((event->sigev_notify & ~SIGEV_THREAD_ID ) !=SIGEV_NONE ) && + ((unsigned int ) (event->sigev_signo -1 ) >=SIGRTMAX ))return NULL ;
I was wondering if someone would try this one :( Really, this is just over the top. Take pity upon your readers, and do:
if (((event->sigev_notify & ~SIGEV_THREAD_ID) != SIGEV_NONE) && (event->sigev_signo <= 0 || event->sigev_signo > SIGRTMAX))
kernel/posix-timers.c line 832
Quoted 9 lines
kernel/posix-timers.c +1
-1@@ -804,7 +826,7 @@* equal to jiffies, so the timer notify function is called directly. * We do not even queue SIGEV_NONE timers! */ -if (!(timr->it_sigev_notify &SIGEV_NONE )) { +if (!((timr->it_sigev_notify & ~SIGEV_THREAD_ID ) ==SIGEV_NONE )) {if (timr->it_timer.expires == jiffies)timer_notify_task (timr);else
Are you sure this is correct? If so, using != would be clearer.
Re: [PATCH] Incorrect value for SIGRTMAX, MIPS nonsense removed, timer_gettime fix
Quoted 15 lines · Andrew Morton
Quoted 11 lines · George Anzinger
The attached patch does the following:
Removes C++ comment in favor of C style.
Removes the special treatment for MIPS SIGEV values. We only require (and error if this fails) that the
SIGEV_THREAD_IDvalue not share bits with the other SIGEV values. Note that mips has yet to define this value so when they do...Corrects the check for the signal range to be from 1 to SIGRTMAX inclusive.
Adds a check to verify that
kmem_cache_alloc()actually returned a timer, error if not.Fixes a bug in
timer_gettime()where the incorrect value was returned if a signal was pending on the timer OR the timer was aSIGEV_NONEtimer.
Quoted 10 lines
Quoted 6 lines
kernel/posix-timers.c +2
-3-if ((event->sigev_notify & ~SIGEV_NONE &MIPS_SIGEV ) && - event->sigev_signo && - ((unsigned ) (event->sigev_signo >SIGRTMAX ))) +if (((event->sigev_notify & ~SIGEV_THREAD_ID ) !=SIGEV_NONE ) && + ((unsigned int ) (event->sigev_signo -1 ) >=SIGRTMAX ))return NULL;I was wondering if someone would try this one :( Really, this is just over the top. Take pity upon your readers, and do:
I was rather thinking of educating them :) It does produce better code...
Quoted 5 lines
if (((event->sigev_notify & ~SIGEV_THREAD_ID) != SIGEV_NONE) && (event->sigev_signo <= 0 || event->sigev_signo > SIGRTMAX))
Quoted 12 lines
kernel/posix-timers.c line 832
Quoted 9 lines
kernel/posix-timers.c +1
-1@@ -804,7 +826,7 @@* equal to jiffies, so the timer notify function is called directly. * We do not even queue SIGEV_NONE timers! */ -if (!(timr->it_sigev_notify &SIGEV_NONE )) { +if (!((timr->it_sigev_notify & ~SIGEV_THREAD_ID ) ==SIGEV_NONE )) {if (timr->it_timer.expires == jiffies)timer_notify_task (timr);else Are you sure this is correct? If so, using != would be clearer.
Yes, if he said SIGEV_NONE we don't want to deliver a signal. The restatement is OK with me.
Shall I resubmit?
Quoted 7 lines
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 3 lines
George Anzinger [email protected] High-res-timers: http://sourceforge.net/projects/high-res-timers/ Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml
PATCH 1/1 Fine tune the time conversion to eliminate conversion errors.
In reply to [email protected]Andrew,
The time conversion code is erroring on the side of a bit too small. The attached patch forces any error to be on the high side. The current code will convert 1 nanosecond to zero jiffies (standard says that should be 1). It also is around 1 nanosecond late on each roll to the next jiffie.
I have done some error checks with this patch applied and get the following errors in PPB ( Parts Per Billion):
HZ nano sec conversion microsecond conversion
1000 315 45 1024 227 40 100 28 317
In all cases the error is on the high side, which means that the final shift will, most likely, eliminate the error bits.
Signature · 3 lines
George Anzinger [email protected] High-res-timers: http://sourceforge.net/projects/high-res-timers/ Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml
include/linux/time.h +6 -6
@@ -148,14 +148,14 @@#endif #define NSEC_JIFFIE_SC (SEC_JIFFIE_SC + 29)#define USEC_JIFFIE_SC (SEC_JIFFIE_SC + 19) -#define SEC_CONVERSION ((unsigned long)((((u64)NSEC_PER_SEC << SEC_JIFFIE_SC))\ - / (u64)TICK_NSEC)) +#define SEC_CONVERSION ((unsigned long)((((u64)NSEC_PER_SEC << SEC_JIFFIE_SC) +\ + TICK_NSEC -1) / (u64)TICK_NSEC)) -#define NSEC_CONVERSION ((unsigned long)((((u64)1 << NSEC_JIFFIE_SC))\ - / (u64)TICK_NSEC)) +#define NSEC_CONVERSION ((unsigned long)((((u64)1 << NSEC_JIFFIE_SC) +\ + TICK_NSEC -1) / (u64)TICK_NSEC))#define USEC_CONVERSION \ - ((unsigned long)((((u64)NSEC_PER_USEC << USEC_JIFFIE_SC)) \ - / (u64)TICK_NSEC)) + ((unsigned long)((((u64)NSEC_PER_USEC << USEC_JIFFIE_SC) +\ + TICK_NSEC -1) / (u64)TICK_NSEC))/* * USEC_ROUND is used in the timeval to jiffie conversion. See there * for more details. It is the scaled resolution rounding value. Note