[Xenomai-core] [PATCH] Buffer over flow in /proc/xenomai/stat

[Xenomai-core] [PATCH] Buffer over flow in /proc/xenomai/stat

[Atsushi Katagiri]

Hello all.

This is a small patch that fixes a serious bug.

When we open /proc/xenomai/stat, function stat_seq_open kmalloc the area, write the data and increment iter->nentries.
The last increment of this value reaches "count",
and at the next iteration "stat_info->cpu = cpu;"  overwrites zero on illegal address!

Here is my proposal of the fix..

=====patch start=====>
diff -Nur xenomai-2.4.4-org/ksrc/nucleus/module.c xenomai-2.4.4/ksrc/nucleus/module.c
--- xenomai-2.4.4-org/ksrc/nucleus/module.c 2008-06-02 00:44:48.000000000 +0900
+++ xenomai-2.4.4/ksrc/nucleus/module.c 2008-07-29 09:46:45.000000000 +0900
@@ -443,6 +443,9 @@
   int cpu = 0;
   int err;
 
+  if (iter->nentries >= count)
+   break;
+
   /* ...over all shared IRQs on all CPUs */
   while (1) {
    stat_info = &iter->stat_info[iter->nentries];
@@ -464,7 +467,9 @@
    stat_info->pf = 0;
 
    iter->nentries++;
-  };
+   if (iter->nentries >= count)
+    break;
+  }
  }
 
  seq = file->private_data;
<=====patch end=====

I hope someone who knows this function well will solve the problem.

Regards,

Atsushi KATAGIRI
Software Engineer
A&D Company, Limited
Tokyo, Japan

Re: [Xenomai-core] [PATCH] Buffer over flow in /proc/xenomai/stat

[Philippe Gerum]

Atsushi Katagiri wrote:
> Hello all.
> 
> This is a small patch that fixes a serious bug.
> 
> When we open /proc/xenomai/stat, function stat_seq_open kmalloc the area, write the data and increment iter->nentries.
> The last increment of this value reaches "count",
> and at the next iteration "stat_info->cpu = cpu;"  overwrites zero on illegal address!
>

Did you actually see this bug happen?

This code takes a snapshot of the IRQ and thread lists, that are identified by
two fingerprint values (intr_rev, thrq_rev). The only way to have iter->nentries
greater than count at some point, would be to see more IRQ/thread descriptors
being linked to their respective lists on-the-fly while we scan them. But in
that case, the current fingerprint value would stop matching the snapshot value
as well, causing the loops to restart, re-allocating sufficient space to hold
all the data.

If you did see this bug happen, please tell us a bit more about your setup.

> Here is my proposal of the fix..
> 
> =====patch start=====>
> diff -Nur xenomai-2.4.4-org/ksrc/nucleus/module.c xenomai-2.4.4/ksrc/nucleus/module.c
> --- xenomai-2.4.4-org/ksrc/nucleus/module.c 2008-06-02 00:44:48.000000000 +0900
> +++ xenomai-2.4.4/ksrc/nucleus/module.c 2008-07-29 09:46:45.000000000 +0900
> @@ -443,6 +443,9 @@
>    int cpu = 0;
>    int err;
>  
> +  if (iter->nentries >= count)
> +   break;
> +
>    /* ...over all shared IRQs on all CPUs */
>    while (1) {
>     stat_info = &iter->stat_info[iter->nentries];
> @@ -464,7 +467,9 @@
>     stat_info->pf = 0;
>  
>     iter->nentries++;
> -  };
> +   if (iter->nentries >= count)
> +    break;
> +  }
>   }
>  
>   seq = file->private_data;
> <=====patch end=====
> 
> I hope someone who knows this function well will solve the problem.
> 
> Regards,
> 
> Atsushi KATAGIRI
> Software Engineer
> A&D Company, Limited
> Tokyo, Japan
> 
> 
> _______________________________________________
> Xenomai-core mailing list
> Xenomai-core@domain.hid
> https://mail.gna.org/listinfo/xenomai-core
> 


-- 
Philippe.

Re: [Xenomai-core] [PATCH] Buffer over flow in /proc/xenomai/stat

[Atsushi Katagiri]

Yes, I actually encountered this bug and my Linux was crashed by NULL pointer dereference.

I think this is a very simple bug.
It happens "everytime" we open /proc/xenomai/stat,
because the last iter->nentries++; (line 466) surely reaches the value of the count,
and the next iteration, line 449, surely overwrites zero on out of the kmalloced area.

If you doubt of it, please try the next debug code.

Regards,
Atsushi KATAGIRI

--- xenomai-2.4.4-org/ksrc/nucleus/module.c 2008-06-02 00:44:48.000000000 +0900
+++ xenomai-2.4.4/ksrc/nucleus/module.c 2008-08-04 17:54:12.000000000 +0900
@@ -382,6 +382,8 @@
  iter = kmalloc(sizeof(*iter)
          + (count - 1) * sizeof(struct stat_seq_info),
          GFP_KERNEL);
+ printk("iter=%x size=%d count=%d\n",iter,(int)(sizeof(*iter)
+         + (count - 1) * sizeof(struct stat_seq_info)),count); //+++++debug
  if (!iter)
   return -ENOMEM;
 
@@ -437,17 +439,29 @@
   xnlock_put_irqrestore(&nklock, s);
  }
 
+ int nentries_old = -1; //+++++debug
+
  /* Iterate over all IRQ numbers, ... */
  for (irq = 0; irq < XNARCH_NR_IRQS; irq++) {
   xnintr_t *prev = NULL;
   int cpu = 0;
   int err;
 
+//  if (iter->nentries >= count)
+//   break;
+
   /* ...over all shared IRQs on all CPUs */
   while (1) {
    stat_info = &iter->stat_info[iter->nentries];
    stat_info->cpu = cpu;
 
+//+++++debug----->
+   if(nentries_old != iter->nentries){
+    printk("stat_info=%x nentries=%d\n",stat_info,(int)(iter->nentries));
+    nentries_old = iter->nentries;
+   }
+//+++++debug<-----
+
    err = xnintr_query(irq, &cpu, &prev, intr_rev,
         stat_info->name,
         &stat_info->csw,
@@ -464,7 +478,9 @@
    stat_info->pf = 0;
 
    iter->nentries++;
-  };
+//   if (iter->nentries >= count)
+//    break;
+  }
  }
 
  seq = file->private_data;

----- Original Message ----- 
From: "Philippe Gerum" <rpm@xenomai.org>
To: "Atsushi Katagiri" <a-katagiri@domain.hid>
Cc: <xenomai@xenomai.org>
Sent: Monday, August 04, 2008 4:49 PM
Subject: Re: [Xenomai-core] [PATCH] Buffer over flow in /proc/xenomai/stat


Atsushi Katagiri wrote:
> Hello all.
> 
> This is a small patch that fixes a serious bug.
> 
> When we open /proc/xenomai/stat, function stat_seq_open kmalloc the area, write the data and increment iter->nentries.
> The last increment of this value reaches "count",
> and at the next iteration "stat_info->cpu = cpu;"  overwrites zero on illegal address!
>

Did you actually see this bug happen?

This code takes a snapshot of the IRQ and thread lists, that are identified by
two fingerprint values (intr_rev, thrq_rev). The only way to have iter->nentries
greater than count at some point, would be to see more IRQ/thread descriptors
being linked to their respective lists on-the-fly while we scan them. But in
that case, the current fingerprint value would stop matching the snapshot value
as well, causing the loops to restart, re-allocating sufficient space to hold
all the data.

If you did see this bug happen, please tell us a bit more about your setup.

> Here is my proposal of the fix..
> 
> =====patch start=====>
> diff -Nur xenomai-2.4.4-org/ksrc/nucleus/module.c xenomai-2.4.4/ksrc/nucleus/module.c
> --- xenomai-2.4.4-org/ksrc/nucleus/module.c 2008-06-02 00:44:48.000000000 +0900
> +++ xenomai-2.4.4/ksrc/nucleus/module.c 2008-07-29 09:46:45.000000000 +0900
> @@ -443,6 +443,9 @@
>    int cpu = 0;
>    int err;
>  
> +  if (iter->nentries >= count)
> +   break;
> +
>    /* ...over all shared IRQs on all CPUs */
>    while (1) {
>     stat_info = &iter->stat_info[iter->nentries];
> @@ -464,7 +467,9 @@
>     stat_info->pf = 0;
>  
>     iter->nentries++;
> -  };
> +   if (iter->nentries >= count)
> +    break;
> +  }
>   }
>  
>   seq = file->private_data;
> <=====patch end=====
> 
> I hope someone who knows this function well will solve the problem.
> 
> Regards,
> 
> Atsushi KATAGIRI
> Software Engineer
> A&D Company, Limited
> Tokyo, Japan
> 
> 
> _______________________________________________
> Xenomai-core mailing list
> Xenomai-core@domain.hid
> https://mail.gna.org/listinfo/xenomai-core
> 


-- 
Philippe.

Re: [Xenomai-core] [PATCH] Buffer over flow in /proc/xenomai/stat

[Philippe Gerum]

Atsushi-san,

Atsushi Katagiri wrote:
> Yes, I actually encountered this bug and my Linux was crashed by NULL pointer dereference.
> 
> I think this is a very simple bug.
> It happens "everytime" we open /proc/xenomai/stat,
> because the last iter->nentries++; (line 466) surely reaches the value of the count,
> and the next iteration, line 449, surely overwrites zero on out of the kmalloced area.
>

Please try this fix instead:

--- ksrc/nucleus/module.c	(revision 4074)
+++ ksrc/nucleus/module.c	(working copy)
@@ -446,7 +446,6 @@
 		/* ...over all shared IRQs on all CPUs */
 		while (1) {
 			stat_info = &iter->stat_info[iter->nentries];
-			stat_info->cpu = cpu;

 			err = xnintr_query(irq, &cpu, &prev, intr_rev,
 					   stat_info->name,
@@ -458,6 +457,7 @@
 			if (err)
 				break; /* line unused or end of chain */

+			stat_info->cpu = cpu;
 			stat_info->pid = 0;
 			stat_info->state =  0;
 			stat_info->ssw = 0;


> If you doubt of it, please try the next debug code.
> 
> Regards,
> Atsushi KATAGIRI
> 
> --- xenomai-2.4.4-org/ksrc/nucleus/module.c 2008-06-02 00:44:48.000000000 +0900
> +++ xenomai-2.4.4/ksrc/nucleus/module.c 2008-08-04 17:54:12.000000000 +0900
> @@ -382,6 +382,8 @@
>   iter = kmalloc(sizeof(*iter)
>           + (count - 1) * sizeof(struct stat_seq_info),
>           GFP_KERNEL);
> + printk("iter=%x size=%d count=%d\n",iter,(int)(sizeof(*iter)
> +         + (count - 1) * sizeof(struct stat_seq_info)),count); //+++++debug
>   if (!iter)
>    return -ENOMEM;
>  
> @@ -437,17 +439,29 @@
>    xnlock_put_irqrestore(&nklock, s);
>   }
>  
> + int nentries_old = -1; //+++++debug
> +
>   /* Iterate over all IRQ numbers, ... */
>   for (irq = 0; irq < XNARCH_NR_IRQS; irq++) {
>    xnintr_t *prev = NULL;
>    int cpu = 0;
>    int err;
>  
> +//  if (iter->nentries >= count)
> +//   break;
> +
>    /* ...over all shared IRQs on all CPUs */
>    while (1) {
>     stat_info = &iter->stat_info[iter->nentries];
>     stat_info->cpu = cpu;
>  
> +//+++++debug----->
> +   if(nentries_old != iter->nentries){
> +    printk("stat_info=%x nentries=%d\n",stat_info,(int)(iter->nentries));
> +    nentries_old = iter->nentries;
> +   }
> +//+++++debug<-----
> +
>     err = xnintr_query(irq, &cpu, &prev, intr_rev,
>          stat_info->name,
>          &stat_info->csw,
> @@ -464,7 +478,9 @@
>     stat_info->pf = 0;
>  
>     iter->nentries++;
> -  };
> +//   if (iter->nentries >= count)
> +//    break;
> +  }
>   }
>  
>   seq = file->private_data;
> 
> ----- Original Message ----- 
> From: "Philippe Gerum" <rpm@xenomai.org>
> To: "Atsushi Katagiri" <a-katagiri@domain.hid>
> Cc: <xenomai@xenomai.org>
> Sent: Monday, August 04, 2008 4:49 PM
> Subject: Re: [Xenomai-core] [PATCH] Buffer over flow in /proc/xenomai/stat
> 
> 
> Atsushi Katagiri wrote:
>> Hello all.
>>
>> This is a small patch that fixes a serious bug.
>>
>> When we open /proc/xenomai/stat, function stat_seq_open kmalloc the area, write the data and increment iter->nentries.
>> The last increment of this value reaches "count",
>> and at the next iteration "stat_info->cpu = cpu;"  overwrites zero on illegal address!
>>
> 
> Did you actually see this bug happen?
> 
> This code takes a snapshot of the IRQ and thread lists, that are identified by
> two fingerprint values (intr_rev, thrq_rev). The only way to have iter->nentries
> greater than count at some point, would be to see more IRQ/thread descriptors
> being linked to their respective lists on-the-fly while we scan them. But in
> that case, the current fingerprint value would stop matching the snapshot value
> as well, causing the loops to restart, re-allocating sufficient space to hold
> all the data.
> 
> If you did see this bug happen, please tell us a bit more about your setup.
> 
>> Here is my proposal of the fix..
>>
>> =====patch start=====>
>> diff -Nur xenomai-2.4.4-org/ksrc/nucleus/module.c xenomai-2.4.4/ksrc/nucleus/module.c
>> --- xenomai-2.4.4-org/ksrc/nucleus/module.c 2008-06-02 00:44:48.000000000 +0900
>> +++ xenomai-2.4.4/ksrc/nucleus/module.c 2008-07-29 09:46:45.000000000 +0900
>> @@ -443,6 +443,9 @@
>>    int cpu = 0;
>>    int err;
>>  
>> +  if (iter->nentries >= count)
>> +   break;
>> +
>>    /* ...over all shared IRQs on all CPUs */
>>    while (1) {
>>     stat_info = &iter->stat_info[iter->nentries];
>> @@ -464,7 +467,9 @@
>>     stat_info->pf = 0;
>>  
>>     iter->nentries++;
>> -  };
>> +   if (iter->nentries >= count)
>> +    break;
>> +  }
>>   }
>>  
>>   seq = file->private_data;
>> <=====patch end=====
>>
>> I hope someone who knows this function well will solve the problem.
>>
>> Regards,
>>
>> Atsushi KATAGIRI
>> Software Engineer
>> A&D Company, Limited
>> Tokyo, Japan
>>
>>
>> _______________________________________________
>> Xenomai-core mailing list
>> Xenomai-core@domain.hid
>> https://mail.gna.org/listinfo/xenomai-core
>>
> 
> 


-- 
Philippe.

Re: [Xenomai-core] [PATCH] Buffer over flow in /proc/xenomai/stat

[Philippe Gerum]

Philippe Gerum wrote:
> Atsushi-san,
> 
> Atsushi Katagiri wrote:
>> Yes, I actually encountered this bug and my Linux was crashed by NULL pointer dereference.
>>
>> I think this is a very simple bug.
>> It happens "everytime" we open /proc/xenomai/stat,
>> because the last iter->nentries++; (line 466) surely reaches the value of the count,
>> and the next iteration, line 449, surely overwrites zero on out of the kmalloced area.
>>
> 
> Please try this fix instead:
>

Actually, this one is better:

--- ksrc/nucleus/module.c	(revision 4074)
+++ ksrc/nucleus/module.c	(working copy)
@@ -440,13 +440,13 @@
 	/* Iterate over all IRQ numbers, ... */
 	for (irq = 0; irq < XNARCH_NR_IRQS; irq++) {
 		xnintr_t *prev = NULL;
-		int cpu = 0;
+		int cpu = 0, _cpu;
 		int err;

 		/* ...over all shared IRQs on all CPUs */
 		while (1) {
 			stat_info = &iter->stat_info[iter->nentries];
-			stat_info->cpu = cpu;
+			_cpu = cpu;

 			err = xnintr_query(irq, &cpu, &prev, intr_rev,
 					   stat_info->name,
@@ -458,6 +458,7 @@
 			if (err)
 				break; /* line unused or end of chain */

+			stat_info->cpu = _cpu;
 			stat_info->pid = 0;
 			stat_info->state =  0;
 			stat_info->ssw = 0;

-- 
Philippe.

Re: [Xenomai-core] [PATCH] Buffer over flow in /proc/xenomai/stat

[Atsushi Katagiri]

Philippe-san,

It seems to function correctly.
Thanks.

Atsushi KATAGIRI

----- Original Message ----- 
From: "Philippe Gerum" <rpm@xenomai.org>
To: "Atsushi Katagiri" <a-katagiri@domain.hid>
Cc: <xenomai@xenomai.org>
Sent: Monday, August 04, 2008 7:22 PM
Subject: Re: [Xenomai-core] [PATCH] Buffer over flow in /proc/xenomai/stat


Philippe Gerum wrote:
> Atsushi-san,
> 
> Atsushi Katagiri wrote:
>> Yes, I actually encountered this bug and my Linux was crashed by NULL pointer dereference.
>>
>> I think this is a very simple bug.
>> It happens "everytime" we open /proc/xenomai/stat,
>> because the last iter->nentries++; (line 466) surely reaches the value of the count,
>> and the next iteration, line 449, surely overwrites zero on out of the kmalloced area.
>>
> 
> Please try this fix instead:
>

Actually, this one is better:

--- ksrc/nucleus/module.c (revision 4074)
+++ ksrc/nucleus/module.c (working copy)
@@ -440,13 +440,13 @@
  /* Iterate over all IRQ numbers, ... */
  for (irq = 0; irq < XNARCH_NR_IRQS; irq++) {
  xnintr_t *prev = NULL;
- int cpu = 0;
+ int cpu = 0, _cpu;
  int err;

  /* ...over all shared IRQs on all CPUs */
  while (1) {
  stat_info = &iter->stat_info[iter->nentries];
- stat_info->cpu = cpu;
+ _cpu = cpu;

  err = xnintr_query(irq, &cpu, &prev, intr_rev,
     stat_info->name,
@@ -458,6 +458,7 @@
  if (err)
  break; /* line unused or end of chain */

+ stat_info->cpu = _cpu;
  stat_info->pid = 0;
  stat_info->state =  0;
  stat_info->ssw = 0;

-- 
Philippe.

Re: [Xenomai-core] [PATCH] Buffer over flow in /proc/xenomai/stat

[Jan Kiszka]

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

Philippe Gerum wrote:
> Philippe Gerum wrote:
>> Atsushi-san,
>>
>> Atsushi Katagiri wrote:
>>> Yes, I actually encountered this bug and my Linux was crashed by NULL pointer dereference.
>>>
>>> I think this is a very simple bug.
>>> It happens "everytime" we open /proc/xenomai/stat,
>>> because the last iter->nentries++; (line 466) surely reaches the value of the count,
>>> and the next iteration, line 449, surely overwrites zero on out of the kmalloced area.
>>>
>> Please try this fix instead:
>>
> 
> Actually, this one is better:
> 
> --- ksrc/nucleus/module.c	(revision 4074)
> +++ ksrc/nucleus/module.c	(working copy)
> @@ -440,13 +440,13 @@
>  	/* Iterate over all IRQ numbers, ... */
>  	for (irq = 0; irq < XNARCH_NR_IRQS; irq++) {
>  		xnintr_t *prev = NULL;
> -		int cpu = 0;
> +		int cpu = 0, _cpu;
>  		int err;
> 
>  		/* ...over all shared IRQs on all CPUs */
>  		while (1) {
>  			stat_info = &iter->stat_info[iter->nentries];
> -			stat_info->cpu = cpu;
> +			_cpu = cpu;
> 
>  			err = xnintr_query(irq, &cpu, &prev, intr_rev,
>  					   stat_info->name,
> @@ -458,6 +458,7 @@
>  			if (err)
>  				break; /* line unused or end of chain */
> 
> +			stat_info->cpu = _cpu;
>  			stat_info->pid = 0;
>  			stat_info->state =  0;
>  			stat_info->ssw = 0;
> 

Good catch. But for the sake of telling variable names I would suggest:

Index: ksrc/nucleus/module.c
===================================================================
--- ksrc/nucleus/module.c	(Revision 4094)
+++ ksrc/nucleus/module.c	(Arbeitskopie)
@@ -444,15 +444,15 @@ static int stat_seq_open(struct inode *i
 	/* Iterate over all IRQ numbers, ... */
 	for (irq = 0; irq < XNARCH_NR_IRQS; irq++) {
 		xnintr_t *prev = NULL;
-		int cpu = 0, _cpu;
-		int err;
+		int next_cpu = 0;
+		int cpu, err;
 
 		/* ...over all shared IRQs on all CPUs */
 		while (1) {
 			stat_info = &iter->stat_info[iter->nentries];
-			_cpu = cpu;
+			cpu = next_cpu;
 
-			err = xnintr_query(irq, &cpu, &prev, intr_rev,
+			err = xnintr_query(irq, &next_cpu, &prev, intr_rev,
 					   stat_info->name,
 					   &stat_info->csw,
 					   &stat_info->exectime,
@@ -462,7 +462,7 @@ static int stat_seq_open(struct inode *i
 			if (err)
 				break; /* line unused or end of chain */
 
-			stat_info->cpu = _cpu;
+			stat_info->cpu = cpu;
 			stat_info->pid = 0;
 			stat_info->state =  0;
 			stat_info->ssw = 0;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Xenomai-core] [PATCH] Buffer over flow in /proc/xenomai/stat

[Philippe Gerum]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> Philippe Gerum wrote:
>>> Atsushi-san,
>>>
>>> Atsushi Katagiri wrote:
>>>> Yes, I actually encountered this bug and my Linux was crashed by NULL pointer dereference.
>>>>
>>>> I think this is a very simple bug.
>>>> It happens "everytime" we open /proc/xenomai/stat,
>>>> because the last iter->nentries++; (line 466) surely reaches the value of the count,
>>>> and the next iteration, line 449, surely overwrites zero on out of the kmalloced area.
>>>>
>>> Please try this fix instead:
>>>
>> Actually, this one is better:
>>
>> --- ksrc/nucleus/module.c	(revision 4074)
>> +++ ksrc/nucleus/module.c	(working copy)
>> @@ -440,13 +440,13 @@
>>  	/* Iterate over all IRQ numbers, ... */
>>  	for (irq = 0; irq < XNARCH_NR_IRQS; irq++) {
>>  		xnintr_t *prev = NULL;
>> -		int cpu = 0;
>> +		int cpu = 0, _cpu;
>>  		int err;
>>
>>  		/* ...over all shared IRQs on all CPUs */
>>  		while (1) {
>>  			stat_info = &iter->stat_info[iter->nentries];
>> -			stat_info->cpu = cpu;
>> +			_cpu = cpu;
>>
>>  			err = xnintr_query(irq, &cpu, &prev, intr_rev,
>>  					   stat_info->name,
>> @@ -458,6 +458,7 @@
>>  			if (err)
>>  				break; /* line unused or end of chain */
>>
>> +			stat_info->cpu = _cpu;
>>  			stat_info->pid = 0;
>>  			stat_info->state =  0;
>>  			stat_info->ssw = 0;
>>
> 
> Good catch. But for the sake of telling variable names I would suggest:
> 
> Index: ksrc/nucleus/module.c
> ===================================================================
> --- ksrc/nucleus/module.c	(Revision 4094)
> +++ ksrc/nucleus/module.c	(Arbeitskopie)
> @@ -444,15 +444,15 @@ static int stat_seq_open(struct inode *i
>  	/* Iterate over all IRQ numbers, ... */
>  	for (irq = 0; irq < XNARCH_NR_IRQS; irq++) {
>  		xnintr_t *prev = NULL;
> -		int cpu = 0, _cpu;
> -		int err;
> +		int next_cpu = 0;
> +		int cpu, err;
>  
>  		/* ...over all shared IRQs on all CPUs */
>  		while (1) {
>  			stat_info = &iter->stat_info[iter->nentries];
> -			_cpu = cpu;
> +			cpu = next_cpu;
>  
> -			err = xnintr_query(irq, &cpu, &prev, intr_rev,
> +			err = xnintr_query(irq, &next_cpu, &prev, intr_rev,
>  					   stat_info->name,
>  					   &stat_info->csw,
>  					   &stat_info->exectime,
> @@ -462,7 +462,7 @@ static int stat_seq_open(struct inode *i
>  			if (err)
>  				break; /* line unused or end of chain */
>  
> -			stat_info->cpu = _cpu;
> +			stat_info->cpu = cpu;
>  			stat_info->pid = 0;
>  			stat_info->state =  0;
>  			stat_info->ssw = 0;
> 

This would be even better with a proper iterator construct.

-- 
Philippe.

[Xenomai-core] [PATCH] rework xnintr_query (was: [PATCH] Buffer over flow in /proc/xenomai/stat)

[Jan Kiszka]

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

Philippe Gerum wrote:
> ...
> This would be even better with a proper iterator construct.
> 

Something like this? I think I tested most cases successfully (e.g. 4
CPUs with a shared IRQ), but the code should also be better readable,
thus better reviewable. Comments/feedback welcome.

Jan

---
 include/nucleus/intr.h |   25 +++++++++++-----
 ksrc/nucleus/intr.c    |   76 ++++++++++++++++++++++++++++++++-----------------
 ksrc/nucleus/module.c  |   46 ++++++++++-------------------
 3 files changed, 84 insertions(+), 63 deletions(-)

Index: b/include/nucleus/intr.h
===================================================================
--- a/include/nucleus/intr.h
+++ b/include/nucleus/intr.h
@@ -69,11 +69,23 @@ typedef struct xnintr {
 
 } xnintr_t;
 
+typedef struct xnintr_iterator {
+
+    int cpu;		/* !< Current CPU in iteration. */
+
+    unsigned long hits;	/* !< Current hit counter. */
+
+    xnticks_t exectime;	/* !< Used CPU time in current accounting period. */
+
+    xnticks_t account_period; /* !< Length of accounting period. */
+
+    int list_rev;	/* !< System-wide xnintr list revision (internal use). */
+
+    xnintr_t *prev;	/* !< Previously visited xnintr object (internal use). */
+
+} xnintr_iterator_t;
+
 extern xnintr_t nkclock;
-#ifdef CONFIG_XENO_OPT_STATS
-extern int xnintr_count;
-extern int xnintr_list_rev;
-#endif
 
 #ifdef __cplusplus
 extern "C" {
@@ -108,9 +120,8 @@ int xnintr_disable(xnintr_t *intr);
 xnarch_cpumask_t xnintr_affinity(xnintr_t *intr,
                                  xnarch_cpumask_t cpumask);
 
-int xnintr_query(int irq, int *cpu, xnintr_t **prev, int revision, char *name,
-		 unsigned long *hits, xnticks_t *exectime,
-		 xnticks_t *account_period);
+int xnintr_init_query(xnintr_iterator_t *iterator);
+int xnintr_query(int irq, xnintr_iterator_t *iterator, char *name_buf);
 
 #ifdef __cplusplus
 }
Index: b/ksrc/nucleus/intr.c
===================================================================
--- a/ksrc/nucleus/intr.c
+++ b/ksrc/nucleus/intr.c
@@ -42,9 +42,9 @@
 DEFINE_PRIVATE_XNLOCK(intrlock);
 
 #ifdef CONFIG_XENO_OPT_STATS
-xnintr_t nkclock;	/* Only for statistics */
-int xnintr_count = 1;	/* Number of attached xnintr objects + nkclock */
-int xnintr_list_rev;	/* Modification counter of xnintr list */
+xnintr_t nkclock;	     /* Only for statistics */
+static int xnintr_count = 1; /* Number of attached xnintr objects + nkclock */
+static int xnintr_list_rev;  /* Modification counter of xnintr list */
 
 /* Both functions update xnintr_list_rev at the very end.
  * This guarantees that module.c::stat_seq_open() won't get
@@ -899,55 +899,79 @@ int xnintr_irq_proc(unsigned int irq, ch
 #endif /* CONFIG_PROC_FS */
 
 #ifdef CONFIG_XENO_OPT_STATS
-int xnintr_query(int irq, int *cpu, xnintr_t **prev, int revision, char *name,
-		 unsigned long *hits, xnticks_t *exectime,
-		 xnticks_t *account_period)
+int xnintr_init_query(xnintr_iterator_t *iterator)
+{
+	iterator->cpu = -1;
+	iterator->prev = NULL;
+
+	/* The order is important here: first xnintr_list_rev then
+	 * xnintr_count.  On the other hand, xnintr_attach/detach()
+	 * update xnintr_count first and then xnintr_list_rev.  This
+	 * should guarantee that we can't get an up-to-date
+	 * xnintr_list_rev and old xnintr_count here. The other way
+	 * around is not a problem as xnintr_query() will notice this
+	 * fact later.  Should xnintr_list_rev change later,
+	 * xnintr_query() will trigger an appropriate error below. */
+
+	iterator->list_rev = xnintr_list_rev;
+	xnarch_memory_barrier();
+
+	return xnintr_count;
+}
+
+int xnintr_query(int irq, xnintr_iterator_t *iterator, char *name_buf)
 {
 	xnintr_t *intr;
 	xnticks_t last_switch;
 	int head;
-	int cpu_no = *cpu;
+	int cpu_no = iterator->cpu + 1;
 	int err = 0;
 	spl_t s;
 
+	if (cpu_no == xnarch_num_online_cpus())
+		cpu_no = 0;
+	iterator->cpu = cpu_no;
+
 	xnlock_get_irqsave(&intrlock, s);
 
-	if (revision != xnintr_list_rev) {
+	if (iterator->list_rev != xnintr_list_rev) {
 		err = -EAGAIN;
 		goto unlock_and_exit;
 	}
 
-	if (*prev)
-		intr = xnintr_shirq_next(*prev);
-	else if (irq == XNARCH_TIMER_IRQ)
-		intr = &nkclock;
-	else
-		intr = xnintr_shirq_first(irq);
+	if (!iterator->prev) {
+		if (irq == XNARCH_TIMER_IRQ)
+			intr = &nkclock;
+		else
+			intr = xnintr_shirq_first(irq);
+	} else
+		intr = xnintr_shirq_next(iterator->prev);
 
 	if (!intr) {
+		cpu_no = -1;
+		iterator->prev = NULL;
 		err = -ENODEV;
 		goto unlock_and_exit;
 	}
 
-	head = snprintf(name, XNOBJECT_NAME_LEN, "IRQ%d: ", irq);
-	name += head;
-	strncpy(name, intr->name, XNOBJECT_NAME_LEN-head);
+	head = snprintf(name_buf, XNOBJECT_NAME_LEN, "IRQ%d: ", irq);
+	strncpy(name_buf + head, intr->name, XNOBJECT_NAME_LEN-head);
 
-	*hits = xnstat_counter_get(&intr->stat[cpu_no].hits);
+	iterator->hits = xnstat_counter_get(&intr->stat[cpu_no].hits);
 
 	last_switch = xnpod_sched_slot(cpu_no)->last_account_switch;
 
-	*exectime       = intr->stat[cpu_no].account.total;
-	*account_period = last_switch - intr->stat[cpu_no].account.start;
+	iterator->exectime = intr->stat[cpu_no].account.total;
+	iterator->account_period =
+		last_switch - intr->stat[cpu_no].account.start;
 
-	intr->stat[cpu_no].account.total  = 0;
+	intr->stat[cpu_no].account.total = 0;
 	intr->stat[cpu_no].account.start = last_switch;
 
-	if (++cpu_no == xnarch_num_online_cpus()) {
-		cpu_no = 0;
-		*prev  = intr;
-	}
-	*cpu = cpu_no;
+	/* Proceed to next entry in shared IRQ chain when all CPUs
+	 * have been visited for this one. */
+	if (cpu_no + 1 == xnarch_num_online_cpus())
+		iterator->prev = intr;
 
      unlock_and_exit:
 	xnlock_put_irqrestore(&intrlock, s);
Index: b/ksrc/nucleus/module.c
===================================================================
--- a/ksrc/nucleus/module.c
+++ b/ksrc/nucleus/module.c
@@ -352,7 +352,9 @@ static int stat_seq_open(struct inode *i
 	struct seq_file *seq;
 	xnholder_t *holder;
 	struct stat_seq_info *stat_info;
-	int err, count, thrq_rev, intr_rev, irq;
+	xnintr_iterator_t intr_iter;
+	int err, count, thrq_rev, irq;
+	int intr_count;
 	spl_t s;
 
 	if (!xnpod_active_p())
@@ -368,18 +370,8 @@ static int stat_seq_open(struct inode *i
 
 	xnlock_put_irqrestore(&nklock, s);
 
-	/* The order is important here: first xnintr_list_rev then
-	 * xnintr_count.  On the other hand, xnintr_attach/detach()
-	 * update xnintr_count first and then xnintr_list_rev.  This
-	 * should guarantee that we can't get an up-to-date
-	 * xnintr_list_rev and old xnintr_count here. The other way
-	 * around is not a problem as xnintr_query() will notice this
-	 * fact later.  Should xnintr_list_rev change later,
-	 * xnintr_query() will trigger an appropriate error below. */
-
-	intr_rev = xnintr_list_rev;
-	xnarch_memory_barrier();
-	count += xnintr_count * RTHAL_NR_CPUS;
+	intr_count = xnintr_init_query(&intr_iter);
+	count += intr_count * RTHAL_NR_CPUS;
 
 	if (iter)
 		kfree(iter);
@@ -442,35 +434,29 @@ static int stat_seq_open(struct inode *i
 	}
 
 	/* Iterate over all IRQ numbers, ... */
-	for (irq = 0; irq < XNARCH_NR_IRQS; irq++) {
-		xnintr_t *prev = NULL;
-		int cpu = 0, _cpu;
-		int err;
-
+	for (irq = 0; irq < XNARCH_NR_IRQS; irq++)
 		/* ...over all shared IRQs on all CPUs */
 		while (1) {
 			stat_info = &iter->stat_info[iter->nentries];
-			_cpu = cpu;
 
-			err = xnintr_query(irq, &cpu, &prev, intr_rev,
-					   stat_info->name,
-					   &stat_info->csw,
-					   &stat_info->exectime,
-					   &stat_info->account_period);
-			if (err == -EAGAIN)
-				goto restart_unlocked;
-			if (err)
+			err = xnintr_query(irq, &intr_iter, stat_info->name);
+			if (err) {
+				if (err == -EAGAIN)
+					goto restart_unlocked;
 				break; /* line unused or end of chain */
+			}
 
-			stat_info->cpu = _cpu;
+			stat_info->cpu = intr_iter.cpu;
+			stat_info->csw = intr_iter.hits;
+			stat_info->exectime = intr_iter.exectime;
+			stat_info->account_period = intr_iter.account_period;
 			stat_info->pid = 0;
 			stat_info->state =  0;
 			stat_info->ssw = 0;
 			stat_info->pf = 0;
 
 			iter->nentries++;
-		};
-	}
+		}
 
 	seq = file->private_data;
 	seq->private = iter;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Xenomai-core] [PATCH] rework xnintr_query

[Philippe Gerum]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> ...
>> This would be even better with a proper iterator construct.
>>
> 
> Something like this? I think I tested most cases successfully (e.g. 4
> CPUs with a shared IRQ), but the code should also be better readable,
> thus better reviewable. Comments/feedback welcome.
>

Fine with me; this patch now gives strong hints about when and how long the
collected per-IRQ data is available.

> Jan
> 
> ---
>  include/nucleus/intr.h |   25 +++++++++++-----
>  ksrc/nucleus/intr.c    |   76 ++++++++++++++++++++++++++++++++-----------------
>  ksrc/nucleus/module.c  |   46 ++++++++++-------------------
>  3 files changed, 84 insertions(+), 63 deletions(-)
> 
> Index: b/include/nucleus/intr.h
> ===================================================================
> --- a/include/nucleus/intr.h
> +++ b/include/nucleus/intr.h
> @@ -69,11 +69,23 @@ typedef struct xnintr {
>  
>  } xnintr_t;
>  
> +typedef struct xnintr_iterator {
> +
> +    int cpu;		/* !< Current CPU in iteration. */
> +
> +    unsigned long hits;	/* !< Current hit counter. */
> +
> +    xnticks_t exectime;	/* !< Used CPU time in current accounting period. */
> +
> +    xnticks_t account_period; /* !< Length of accounting period. */
> +
> +    int list_rev;	/* !< System-wide xnintr list revision (internal use). */
> +
> +    xnintr_t *prev;	/* !< Previously visited xnintr object (internal use). */
> +
> +} xnintr_iterator_t;
> +
>  extern xnintr_t nkclock;
> -#ifdef CONFIG_XENO_OPT_STATS
> -extern int xnintr_count;
> -extern int xnintr_list_rev;
> -#endif
>  
>  #ifdef __cplusplus
>  extern "C" {
> @@ -108,9 +120,8 @@ int xnintr_disable(xnintr_t *intr);
>  xnarch_cpumask_t xnintr_affinity(xnintr_t *intr,
>                                   xnarch_cpumask_t cpumask);
>  
> -int xnintr_query(int irq, int *cpu, xnintr_t **prev, int revision, char *name,
> -		 unsigned long *hits, xnticks_t *exectime,
> -		 xnticks_t *account_period);
> +int xnintr_init_query(xnintr_iterator_t *iterator);
> +int xnintr_query(int irq, xnintr_iterator_t *iterator, char *name_buf);
>  

s,xnintr_query,xnintr_next_query,

so that the naming mandates calling xnintr_init_query first.

>  #ifdef __cplusplus
>  }
> Index: b/ksrc/nucleus/intr.c
> ===================================================================
> --- a/ksrc/nucleus/intr.c
> +++ b/ksrc/nucleus/intr.c
> @@ -42,9 +42,9 @@
>  DEFINE_PRIVATE_XNLOCK(intrlock);
>  
>  #ifdef CONFIG_XENO_OPT_STATS
> -xnintr_t nkclock;	/* Only for statistics */
> -int xnintr_count = 1;	/* Number of attached xnintr objects + nkclock */
> -int xnintr_list_rev;	/* Modification counter of xnintr list */
> +xnintr_t nkclock;	     /* Only for statistics */
> +static int xnintr_count = 1; /* Number of attached xnintr objects + nkclock */
> +static int xnintr_list_rev;  /* Modification counter of xnintr list */
>  
>  /* Both functions update xnintr_list_rev at the very end.
>   * This guarantees that module.c::stat_seq_open() won't get
> @@ -899,55 +899,79 @@ int xnintr_irq_proc(unsigned int irq, ch
>  #endif /* CONFIG_PROC_FS */
>  
>  #ifdef CONFIG_XENO_OPT_STATS
> -int xnintr_query(int irq, int *cpu, xnintr_t **prev, int revision, char *name,
> -		 unsigned long *hits, xnticks_t *exectime,
> -		 xnticks_t *account_period)
> +int xnintr_init_query(xnintr_iterator_t *iterator)
> +{
> +	iterator->cpu = -1;
> +	iterator->prev = NULL;
> +
> +	/* The order is important here: first xnintr_list_rev then
> +	 * xnintr_count.  On the other hand, xnintr_attach/detach()
> +	 * update xnintr_count first and then xnintr_list_rev.  This
> +	 * should guarantee that we can't get an up-to-date
> +	 * xnintr_list_rev and old xnintr_count here. The other way
> +	 * around is not a problem as xnintr_query() will notice this
> +	 * fact later.  Should xnintr_list_rev change later,
> +	 * xnintr_query() will trigger an appropriate error below. */
> +
> +	iterator->list_rev = xnintr_list_rev;
> +	xnarch_memory_barrier();
> +
> +	return xnintr_count;
> +}
> +
> +int xnintr_query(int irq, xnintr_iterator_t *iterator, char *name_buf)
>  {
>  	xnintr_t *intr;
>  	xnticks_t last_switch;
>  	int head;
> -	int cpu_no = *cpu;
> +	int cpu_no = iterator->cpu + 1;
>  	int err = 0;
>  	spl_t s;
>  
> +	if (cpu_no == xnarch_num_online_cpus())
> +		cpu_no = 0;
> +	iterator->cpu = cpu_no;
> +
>  	xnlock_get_irqsave(&intrlock, s);
>  
> -	if (revision != xnintr_list_rev) {
> +	if (iterator->list_rev != xnintr_list_rev) {
>  		err = -EAGAIN;
>  		goto unlock_and_exit;
>  	}
>  
> -	if (*prev)
> -		intr = xnintr_shirq_next(*prev);
> -	else if (irq == XNARCH_TIMER_IRQ)
> -		intr = &nkclock;
> -	else
> -		intr = xnintr_shirq_first(irq);
> +	if (!iterator->prev) {
> +		if (irq == XNARCH_TIMER_IRQ)
> +			intr = &nkclock;
> +		else
> +			intr = xnintr_shirq_first(irq);
> +	} else
> +		intr = xnintr_shirq_next(iterator->prev);
>  
>  	if (!intr) {
> +		cpu_no = -1;
> +		iterator->prev = NULL;
>  		err = -ENODEV;
>  		goto unlock_and_exit;
>  	}
>  
> -	head = snprintf(name, XNOBJECT_NAME_LEN, "IRQ%d: ", irq);
> -	name += head;
> -	strncpy(name, intr->name, XNOBJECT_NAME_LEN-head);
> +	head = snprintf(name_buf, XNOBJECT_NAME_LEN, "IRQ%d: ", irq);
> +	strncpy(name_buf + head, intr->name, XNOBJECT_NAME_LEN-head);

-	strncpy(name_buf + head, intr->name, XNOBJECT_NAME_LEN-head);
+	strncpy(name_buf + head, intr->name, XNOBJECT_NAME_LEN-1-head);
+	name_buf[XNOBJECT_NAME_LEN-1] = '\0';

Object names should always be null-terminated.

> -	*hits = xnstat_counter_get(&intr->stat[cpu_no].hits);
> +	iterator->hits = xnstat_counter_get(&intr->stat[cpu_no].hits);
>  
>  	last_switch = xnpod_sched_slot(cpu_no)->last_account_switch;
>  
> -	*exectime       = intr->stat[cpu_no].account.total;
> -	*account_period = last_switch - intr->stat[cpu_no].account.start;
> +	iterator->exectime = intr->stat[cpu_no].account.total;
> +	iterator->account_period =
> +		last_switch - intr->stat[cpu_no].account.start;
>  
> -	intr->stat[cpu_no].account.total  = 0;
> +	intr->stat[cpu_no].account.total = 0;
>  	intr->stat[cpu_no].account.start = last_switch;
>  
> -	if (++cpu_no == xnarch_num_online_cpus()) {
> -		cpu_no = 0;
> -		*prev  = intr;
> -	}
> -	*cpu = cpu_no;
> +	/* Proceed to next entry in shared IRQ chain when all CPUs
> +	 * have been visited for this one. */
> +	if (cpu_no + 1 == xnarch_num_online_cpus())
> +		iterator->prev = intr;
>  
>       unlock_and_exit:
>  	xnlock_put_irqrestore(&intrlock, s);
> Index: b/ksrc/nucleus/module.c
> ===================================================================
> --- a/ksrc/nucleus/module.c
> +++ b/ksrc/nucleus/module.c
> @@ -352,7 +352,9 @@ static int stat_seq_open(struct inode *i
>  	struct seq_file *seq;
>  	xnholder_t *holder;
>  	struct stat_seq_info *stat_info;
> -	int err, count, thrq_rev, intr_rev, irq;
> +	xnintr_iterator_t intr_iter;
> +	int err, count, thrq_rev, irq;
> +	int intr_count;
>  	spl_t s;
>  
>  	if (!xnpod_active_p())
> @@ -368,18 +370,8 @@ static int stat_seq_open(struct inode *i
>  
>  	xnlock_put_irqrestore(&nklock, s);
>  
> -	/* The order is important here: first xnintr_list_rev then
> -	 * xnintr_count.  On the other hand, xnintr_attach/detach()
> -	 * update xnintr_count first and then xnintr_list_rev.  This
> -	 * should guarantee that we can't get an up-to-date
> -	 * xnintr_list_rev and old xnintr_count here. The other way
> -	 * around is not a problem as xnintr_query() will notice this
> -	 * fact later.  Should xnintr_list_rev change later,
> -	 * xnintr_query() will trigger an appropriate error below. */
> -
> -	intr_rev = xnintr_list_rev;
> -	xnarch_memory_barrier();
> -	count += xnintr_count * RTHAL_NR_CPUS;
> +	intr_count = xnintr_init_query(&intr_iter);
> +	count += intr_count * RTHAL_NR_CPUS;
>  
>  	if (iter)
>  		kfree(iter);
> @@ -442,35 +434,29 @@ static int stat_seq_open(struct inode *i
>  	}
>  
>  	/* Iterate over all IRQ numbers, ... */
> -	for (irq = 0; irq < XNARCH_NR_IRQS; irq++) {
> -		xnintr_t *prev = NULL;
> -		int cpu = 0, _cpu;
> -		int err;
> -
> +	for (irq = 0; irq < XNARCH_NR_IRQS; irq++)
>  		/* ...over all shared IRQs on all CPUs */
>  		while (1) {
>  			stat_info = &iter->stat_info[iter->nentries];
> -			_cpu = cpu;
>  
> -			err = xnintr_query(irq, &cpu, &prev, intr_rev,
> -					   stat_info->name,
> -					   &stat_info->csw,
> -					   &stat_info->exectime,
> -					   &stat_info->account_period);
> -			if (err == -EAGAIN)
> -				goto restart_unlocked;
> -			if (err)
> +			err = xnintr_query(irq, &intr_iter, stat_info->name);
> +			if (err) {
> +				if (err == -EAGAIN)
> +					goto restart_unlocked;
>  				break; /* line unused or end of chain */
> +			}
>  
> -			stat_info->cpu = _cpu;
> +			stat_info->cpu = intr_iter.cpu;
> +			stat_info->csw = intr_iter.hits;
> +			stat_info->exectime = intr_iter.exectime;
> +			stat_info->account_period = intr_iter.account_period;
>  			stat_info->pid = 0;
>  			stat_info->state =  0;
>  			stat_info->ssw = 0;
>  			stat_info->pf = 0;
>  
>  			iter->nentries++;
> -		};
> -	}
> +		}
>  
>  	seq = file->private_data;
>  	seq->private = iter;
> 


-- 
Philippe.

Re: [Xenomai-core] [PATCH] rework xnintr_query

[Gilles Chanteperdrix]

Philippe Gerum wrote:
>> -	head = snprintf(name, XNOBJECT_NAME_LEN, "IRQ%d: ", irq);
>> -	name += head;
>> -	strncpy(name, intr->name, XNOBJECT_NAME_LEN-head);
>> +	head = snprintf(name_buf, XNOBJECT_NAME_LEN, "IRQ%d: ", irq);
>> +	strncpy(name_buf + head, intr->name, XNOBJECT_NAME_LEN-head);
> 
> -	strncpy(name_buf + head, intr->name, XNOBJECT_NAME_LEN-head);
> +	strncpy(name_buf + head, intr->name, XNOBJECT_NAME_LEN-1-head);
> +	name_buf[XNOBJECT_NAME_LEN-1] = '\0';
> 
> Object names should always be null-terminated.

Yes, and what is the point of not doing everything with only one
snprintf ? strncpy uselessly burns CPU cycles by filling the destination
buffers with zeros if source is smaller than destination.

-- 
                                                 Gilles.

Re: [Xenomai-core] [PATCH] rework xnintr_query

[Jan Kiszka]

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

Gilles Chanteperdrix wrote:
> Philippe Gerum wrote:
>>> -	head = snprintf(name, XNOBJECT_NAME_LEN, "IRQ%d: ", irq);
>>> -	name += head;
>>> -	strncpy(name, intr->name, XNOBJECT_NAME_LEN-head);
>>> +	head = snprintf(name_buf, XNOBJECT_NAME_LEN, "IRQ%d: ", irq);
>>> +	strncpy(name_buf + head, intr->name, XNOBJECT_NAME_LEN-head);
>> -	strncpy(name_buf + head, intr->name, XNOBJECT_NAME_LEN-head);
>> +	strncpy(name_buf + head, intr->name, XNOBJECT_NAME_LEN-1-head);
>> +	name_buf[XNOBJECT_NAME_LEN-1] = '\0';
>>
>> Object names should always be null-terminated.
> 
> Yes, and what is the point of not doing everything with only one
> snprintf ? strncpy uselessly burns CPU cycles by filling the destination
> buffers with zeros if source is smaller than destination.
> 

All valid remarks, so here is a second revision of the patch.

Jan

---
 include/nucleus/intr.h |   25 +++++++++++-----
 ksrc/nucleus/intr.c    |   76 +++++++++++++++++++++++++++++++------------------
 ksrc/nucleus/module.c  |   47 ++++++++++--------------------
 3 files changed, 84 insertions(+), 64 deletions(-)

Index: b/include/nucleus/intr.h
===================================================================
--- a/include/nucleus/intr.h
+++ b/include/nucleus/intr.h
@@ -69,11 +69,23 @@ typedef struct xnintr {
 
 } xnintr_t;
 
+typedef struct xnintr_iterator {
+
+    int cpu;		/* !< Current CPU in iteration. */
+
+    unsigned long hits;	/* !< Current hit counter. */
+
+    xnticks_t exectime;	/* !< Used CPU time in current accounting period. */
+
+    xnticks_t account_period; /* !< Length of accounting period. */
+
+    int list_rev;	/* !< System-wide xnintr list revision (internal use). */
+
+    xnintr_t *prev;	/* !< Previously visited xnintr object (internal use). */
+
+} xnintr_iterator_t;
+
 extern xnintr_t nkclock;
-#ifdef CONFIG_XENO_OPT_STATS
-extern int xnintr_count;
-extern int xnintr_list_rev;
-#endif
 
 #ifdef __cplusplus
 extern "C" {
@@ -108,9 +120,8 @@ int xnintr_disable(xnintr_t *intr);
 xnarch_cpumask_t xnintr_affinity(xnintr_t *intr,
                                  xnarch_cpumask_t cpumask);
 
-int xnintr_query(int irq, int *cpu, xnintr_t **prev, int revision, char *name,
-		 unsigned long *hits, xnticks_t *exectime,
-		 xnticks_t *account_period);
+int xnintr_query_init(xnintr_iterator_t *iterator);
+int xnintr_query_next(int irq, xnintr_iterator_t *iterator, char *name_buf);
 
 #ifdef __cplusplus
 }
Index: b/ksrc/nucleus/intr.c
===================================================================
--- a/ksrc/nucleus/intr.c
+++ b/ksrc/nucleus/intr.c
@@ -42,9 +42,9 @@
 DEFINE_PRIVATE_XNLOCK(intrlock);
 
 #ifdef CONFIG_XENO_OPT_STATS
-xnintr_t nkclock;	/* Only for statistics */
-int xnintr_count = 1;	/* Number of attached xnintr objects + nkclock */
-int xnintr_list_rev;	/* Modification counter of xnintr list */
+xnintr_t nkclock;	     /* Only for statistics */
+static int xnintr_count = 1; /* Number of attached xnintr objects + nkclock */
+static int xnintr_list_rev;  /* Modification counter of xnintr list */
 
 /* Both functions update xnintr_list_rev at the very end.
  * This guarantees that module.c::stat_seq_open() won't get
@@ -899,55 +899,77 @@ int xnintr_irq_proc(unsigned int irq, ch
 #endif /* CONFIG_PROC_FS */
 
 #ifdef CONFIG_XENO_OPT_STATS
-int xnintr_query(int irq, int *cpu, xnintr_t **prev, int revision, char *name,
-		 unsigned long *hits, xnticks_t *exectime,
-		 xnticks_t *account_period)
+int xnintr_query_init(xnintr_iterator_t *iterator)
+{
+	iterator->cpu = -1;
+	iterator->prev = NULL;
+
+	/* The order is important here: first xnintr_list_rev then
+	 * xnintr_count.  On the other hand, xnintr_attach/detach()
+	 * update xnintr_count first and then xnintr_list_rev.  This
+	 * should guarantee that we can't get an up-to-date
+	 * xnintr_list_rev and old xnintr_count here. The other way
+	 * around is not a problem as xnintr_query() will notice this
+	 * fact later.  Should xnintr_list_rev change later,
+	 * xnintr_query() will trigger an appropriate error below. */
+
+	iterator->list_rev = xnintr_list_rev;
+	xnarch_memory_barrier();
+
+	return xnintr_count;
+}
+
+int xnintr_query_next(int irq, xnintr_iterator_t *iterator, char *name_buf)
 {
 	xnintr_t *intr;
 	xnticks_t last_switch;
-	int head;
-	int cpu_no = *cpu;
+	int cpu_no = iterator->cpu + 1;
 	int err = 0;
 	spl_t s;
 
+	if (cpu_no == xnarch_num_online_cpus())
+		cpu_no = 0;
+	iterator->cpu = cpu_no;
+
 	xnlock_get_irqsave(&intrlock, s);
 
-	if (revision != xnintr_list_rev) {
+	if (iterator->list_rev != xnintr_list_rev) {
 		err = -EAGAIN;
 		goto unlock_and_exit;
 	}
 
-	if (*prev)
-		intr = xnintr_shirq_next(*prev);
-	else if (irq == XNARCH_TIMER_IRQ)
-		intr = &nkclock;
-	else
-		intr = xnintr_shirq_first(irq);
+	if (!iterator->prev) {
+		if (irq == XNARCH_TIMER_IRQ)
+			intr = &nkclock;
+		else
+			intr = xnintr_shirq_first(irq);
+	} else
+		intr = xnintr_shirq_next(iterator->prev);
 
 	if (!intr) {
+		cpu_no = -1;
+		iterator->prev = NULL;
 		err = -ENODEV;
 		goto unlock_and_exit;
 	}
 
-	head = snprintf(name, XNOBJECT_NAME_LEN, "IRQ%d: ", irq);
-	name += head;
-	strncpy(name, intr->name, XNOBJECT_NAME_LEN-head);
+	snprintf(name_buf, XNOBJECT_NAME_LEN, "IRQ%d: %s", irq, intr->name);
 
-	*hits = xnstat_counter_get(&intr->stat[cpu_no].hits);
+	iterator->hits = xnstat_counter_get(&intr->stat[cpu_no].hits);
 
 	last_switch = xnpod_sched_slot(cpu_no)->last_account_switch;
 
-	*exectime       = intr->stat[cpu_no].account.total;
-	*account_period = last_switch - intr->stat[cpu_no].account.start;
+	iterator->exectime = intr->stat[cpu_no].account.total;
+	iterator->account_period =
+		last_switch - intr->stat[cpu_no].account.start;
 
-	intr->stat[cpu_no].account.total  = 0;
+	intr->stat[cpu_no].account.total = 0;
 	intr->stat[cpu_no].account.start = last_switch;
 
-	if (++cpu_no == xnarch_num_online_cpus()) {
-		cpu_no = 0;
-		*prev  = intr;
-	}
-	*cpu = cpu_no;
+	/* Proceed to next entry in shared IRQ chain when all CPUs
+	 * have been visited for this one. */
+	if (cpu_no + 1 == xnarch_num_online_cpus())
+		iterator->prev = intr;
 
      unlock_and_exit:
 	xnlock_put_irqrestore(&intrlock, s);
Index: b/ksrc/nucleus/module.c
===================================================================
--- a/ksrc/nucleus/module.c
+++ b/ksrc/nucleus/module.c
@@ -352,7 +352,9 @@ static int stat_seq_open(struct inode *i
 	struct seq_file *seq;
 	xnholder_t *holder;
 	struct stat_seq_info *stat_info;
-	int err, count, thrq_rev, intr_rev, irq;
+	xnintr_iterator_t intr_iter;
+	int err, count, thrq_rev, irq;
+	int intr_count;
 	spl_t s;
 
 	if (!xnpod_active_p())
@@ -368,18 +370,8 @@ static int stat_seq_open(struct inode *i
 
 	xnlock_put_irqrestore(&nklock, s);
 
-	/* The order is important here: first xnintr_list_rev then
-	 * xnintr_count.  On the other hand, xnintr_attach/detach()
-	 * update xnintr_count first and then xnintr_list_rev.  This
-	 * should guarantee that we can't get an up-to-date
-	 * xnintr_list_rev and old xnintr_count here. The other way
-	 * around is not a problem as xnintr_query() will notice this
-	 * fact later.  Should xnintr_list_rev change later,
-	 * xnintr_query() will trigger an appropriate error below. */
-
-	intr_rev = xnintr_list_rev;
-	xnarch_memory_barrier();
-	count += xnintr_count * RTHAL_NR_CPUS;
+	intr_count = xnintr_query_init(&intr_iter);
+	count += intr_count * RTHAL_NR_CPUS;
 
 	if (iter)
 		kfree(iter);
@@ -442,35 +434,30 @@ static int stat_seq_open(struct inode *i
 	}
 
 	/* Iterate over all IRQ numbers, ... */
-	for (irq = 0; irq < XNARCH_NR_IRQS; irq++) {
-		xnintr_t *prev = NULL;
-		int cpu = 0, _cpu;
-		int err;
-
+	for (irq = 0; irq < XNARCH_NR_IRQS; irq++)
 		/* ...over all shared IRQs on all CPUs */
 		while (1) {
 			stat_info = &iter->stat_info[iter->nentries];
-			_cpu = cpu;
 
-			err = xnintr_query(irq, &cpu, &prev, intr_rev,
-					   stat_info->name,
-					   &stat_info->csw,
-					   &stat_info->exectime,
-					   &stat_info->account_period);
-			if (err == -EAGAIN)
-				goto restart_unlocked;
-			if (err)
+			err = xnintr_query_next(irq, &intr_iter,
+						stat_info->name);
+			if (err) {
+				if (err == -EAGAIN)
+					goto restart_unlocked;
 				break; /* line unused or end of chain */
+			}
 
-			stat_info->cpu = _cpu;
+			stat_info->cpu = intr_iter.cpu;
+			stat_info->csw = intr_iter.hits;
+			stat_info->exectime = intr_iter.exectime;
+			stat_info->account_period = intr_iter.account_period;
 			stat_info->pid = 0;
 			stat_info->state =  0;
 			stat_info->ssw = 0;
 			stat_info->pf = 0;
 
 			iter->nentries++;
-		};
-	}
+		}
 
 	seq = file->private_data;
 	seq->private = iter;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]