Re: [PATCH] Incorrect value for SIGRTMAX

Re: [PATCH] Incorrect value for SIGRTMAX

[eric.piel]

[-- Attachment #1: Type: text/plain, Size: 536 bytes --]

Hello,

Few months ago, Corey Minyard corrected handling of incorrect values of signals.
cf
http://linux.bkbits.net:8080/linux-2.5/cset@1.1267.56.40?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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: good-sigevent-not-handling-null.patch --]
[-- Type: text/x-patch; name="good-sigevent-not-handling-null.patch", Size: 432 bytes --]

--- linux-2.6.1/kernel/posix-timers.c.orig	2004-01-24 15:17:31.645060248 +0100
+++ linux-2.6.1/kernel/posix-timers.c	2004-01-24 15:20:56.977844920 +0100
@@ -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;

Re: [PATCH] Incorrect value for SIGRTMAX

[Andrew Morton]

eric.piel@tremplin-utc.net wrote:
>
> Hello,
> 
> Few months ago, Corey Minyard corrected handling of incorrect values of signals.
> cf
> http://linux.bkbits.net:8080/linux-2.5/cset@1.1267.56.40?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 .
> 

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(-)

diff -puN kernel/posix-timers.c~SIGRTMAX-fix kernel/posix-timers.c
--- 25/kernel/posix-timers.c~SIGRTMAX-fix	2004-01-24 14:27:13.000000000 -0800
+++ 25-akpm/kernel/posix-timers.c	2004-01-24 14:28:21.000000000 -0800
@@ -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;

_

Re: [PATCH] Incorrect value for SIGRTMAX

[eric.piel]

Quoting Andrew Morton <akpm@osdl.org>:


> 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?
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
 
> 
>  kernel/posix-timers.c |    3 +--
>  1 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff -puN kernel/posix-timers.c~SIGRTMAX-fix kernel/posix-timers.c
> --- 25/kernel/posix-timers.c~SIGRTMAX-fix	2004-01-24 14:27:13.000000000
> -0800
> +++ 25-akpm/kernel/posix-timers.c	2004-01-24 14:28:21.000000000 -0800
> @@ -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;
> 

Re: [PATCH] Incorrect value for SIGRTMAX

[George Anzinger]

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


eric.piel@tremplin-utc.net wrote:
> Quoting Andrew Morton <akpm@osdl.org>:
> 
> 
> 
>>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?
> 
> 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
>  
> 
>> kernel/posix-timers.c |    3 +--
>> 1 files changed, 1 insertion(+), 2 deletions(-)
>>
>>diff -puN kernel/posix-timers.c~SIGRTMAX-fix kernel/posix-timers.c
>>--- 25/kernel/posix-timers.c~SIGRTMAX-fix	2004-01-24 14:27:13.000000000
>>-0800
>>+++ 25-akpm/kernel/posix-timers.c	2004-01-24 14:28:21.000000000 -0800
>>@@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

Re: [PATCH] Incorrect value for SIGRTMAX

[eric.piel]

Quoting George Anzinger <george@mvista.com>:

> 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!


> 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] Incorrect value for SIGRTMAX, MIPS nonsense removed, timer_gettime fix

[George Anzinger]

[-- Attachment #1: Type: text/plain, Size: 807 bytes --]

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.
-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

[-- Attachment #2: posix-timers-2.6.1-1.0.patch --]
[-- Type: text/plain, Size: 5206 bytes --]

--- linux-2.6.1-org/kernel/posix-timers.c	2003-12-10 17:10:42.000000000 -0800
+++ linux/kernel/posix-timers.c	2004-01-27 01:10:58.000000000 -0800
@@ -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 george@mvista.com
+ *
+ *			     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(

[PATCH] Fine tune the time conversion to eliminate conversion errors.

[George Anzinger]

[-- Attachment #1: Type: text/plain, Size: 884 bytes --]

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.

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

[-- Attachment #2: time.h-2.6.1-1.0.patch --]
[-- Type: text/plain, Size: 1309 bytes --]

--- linux-2.6.1-org/include/linux/time.h	2003-08-11 13:48:55.000000000 -0700
+++ linux/include/linux/time.h	2004-01-27 00:29:12.000000000 -0800
@@ -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

Re: [PATCH] Incorrect value for SIGRTMAX, MIPS nonsense removed, timer_gettime fix

[Andrew Morton]

George Anzinger <george@mvista.com> wrote:
>
> 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.

> -	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))

> @@ -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

[George Anzinger]

Andrew Morton wrote:
> George Anzinger <george@mvista.com> wrote:
> 
>>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.
> 
> 
>>-	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...
> 
> 	if (((event->sigev_notify & ~SIGEV_THREAD_ID) != SIGEV_NONE) &&
> 		(event->sigev_signo <= 0 || event->sigev_signo > SIGRTMAX))
> 
> 
>>@@ -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?
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml