[PATCH 1/2] cobalt/vfile: seq_file seek index must progress

[PATCH 1/2] cobalt/vfile: seq_file seek index must progress

[Philippe Gerum]

From: Philippe Gerum <rpm@xenomai.org>

The offset field we receive from the kernel in a vfile next() handler
must progress in order for the loop to stop properly, independently
from our own tracking of the end-of-list condition.

Bug is reproducible by running two loops in parallel:

- one continuously spawning an application which creates a few tenths
of threads (10-20 would suffice) before exiting shortly after.

- another one continuously reading from /proc/xenomai/sched/{threads,
  stat, acct}.

At some point, the vfile handler should cause a kernel crash.

Signed-off-by: Philippe Gerum <rpm@xenomai.org>
---
 kernel/cobalt/vfile.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/kernel/cobalt/vfile.c b/kernel/cobalt/vfile.c
index c7b81a704..fae0cc377 100644
--- a/kernel/cobalt/vfile.c
+++ b/kernel/cobalt/vfile.c
@@ -109,11 +109,11 @@ static void *vfile_snapshot_next(struct seq_file *seq, void *v, loff_t *offp)
 	struct xnvfile_snapshot_iterator *it = seq->private;
 	loff_t pos = *offp;
 
+	++*offp;
+
 	if (pos >= it->nrdata)
 		return NULL;
 
-	++*offp;
-
 	return it->databuf + pos * it->vfile->datasz;
 }
 
@@ -452,17 +452,15 @@ static void *vfile_regular_next(struct seq_file *seq, void *v, loff_t *offp)
 	struct xnvfile_regular *vfile = it->vfile;
 	void *data;
 
+	it->pos = ++(*offp);
+
 	if (vfile->ops->next == NULL)
 		return NULL;
 
-	it->pos = *offp + 1;
-
 	data = vfile->ops->next(it);
 	if (data == NULL)
 		return NULL;
 
-	*offp = it->pos;
-
 	return data;
 }
 
-- 
2.31.1

[PATCH 2/2] cobalt/arm: document the syscall convention

[Philippe Gerum]

From: Philippe Gerum <rpm@xenomai.org>

Signed-off-by: Philippe Gerum <rpm@xenomai.org>
---
 .../arm/include/asm/xenomai/uapi/syscall.h    | 31 ++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/kernel/cobalt/arch/arm/include/asm/xenomai/uapi/syscall.h b/kernel/cobalt/arch/arm/include/asm/xenomai/uapi/syscall.h
index c079a358c..9146b732a 100644
--- a/kernel/cobalt/arch/arm/include/asm/xenomai/uapi/syscall.h
+++ b/kernel/cobalt/arch/arm/include/asm/xenomai/uapi/syscall.h
@@ -21,9 +21,38 @@
 #ifndef _COBALT_ARM_ASM_UAPI_SYSCALL_H
 #define _COBALT_ARM_ASM_UAPI_SYSCALL_H
 
+/*
+ * Argument marshalling for Cobalt syscalls:
+ *
+ * r7 := XENO_ARM_SYSCALL
+ * r0 := Cobalt syscall code (sc_cobalt_xx | __COBALT_SYSCALL_BIT)
+ * r1 := first argument
+ * r2 := second argument
+ * ...
+ * r5 := fifth argument
+ *
+ * r0 := <return value>
+ *
+ * This marshalling is common to the I-pipe and Dovetail-based
+ * configurations so that we have a single ABI convention for both,
+ * enabling us to have a single user build which does not depend on
+ * the pipeline type.  With Dovetail, CONFIG_IPIPE_COMPAT is enabled
+ * to allow this.
+ *
+ * __COBALT_SYSCALL_BIT is a generic marker for Cobalt system calls we
+ * use with all CPU architectures in common code, to distinguish them
+ * from native Linux syscalls. For this reason, it must be present in
+ * the Cobalt syscall code register (r0) as well.
+ */
+
 #define __xn_syscode(__nr)	(__COBALT_SYSCALL_BIT | (__nr))
 
-#define XENO_ARM_SYSCALL        0x000F0042	/* carefully chosen... */
+/*
+ * A carefully choosen ARM-specific local syscall code denoting a
+ * Cobalt syscall, which the interrupt pipeline looks for in order to
+ * route the request to the real-time core via a dedicated handler.
+ */
+#define XENO_ARM_SYSCALL        0x000F0042
 
 #define XENOMAI_SYSARCH_TSCINFO      4
 
-- 
2.31.1

Re: [PATCH 2/2] cobalt/arm: document the syscall convention

[Jan Kiszka]

On 24.05.21 09:32, Philippe Gerum wrote:
> From: Philippe Gerum <rpm@xenomai.org>
> 
> Signed-off-by: Philippe Gerum <rpm@xenomai.org>
> ---
>  .../arm/include/asm/xenomai/uapi/syscall.h    | 31 ++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/cobalt/arch/arm/include/asm/xenomai/uapi/syscall.h b/kernel/cobalt/arch/arm/include/asm/xenomai/uapi/syscall.h
> index c079a358c..9146b732a 100644
> --- a/kernel/cobalt/arch/arm/include/asm/xenomai/uapi/syscall.h
> +++ b/kernel/cobalt/arch/arm/include/asm/xenomai/uapi/syscall.h
> @@ -21,9 +21,38 @@
>  #ifndef _COBALT_ARM_ASM_UAPI_SYSCALL_H
>  #define _COBALT_ARM_ASM_UAPI_SYSCALL_H
>  
> +/*
> + * Argument marshalling for Cobalt syscalls:
> + *
> + * r7 := XENO_ARM_SYSCALL
> + * r0 := Cobalt syscall code (sc_cobalt_xx | __COBALT_SYSCALL_BIT)
> + * r1 := first argument
> + * r2 := second argument
> + * ...
> + * r5 := fifth argument
> + *
> + * r0 := <return value>
> + *
> + * This marshalling is common to the I-pipe and Dovetail-based
> + * configurations so that we have a single ABI convention for both,
> + * enabling us to have a single user build which does not depend on
> + * the pipeline type.  With Dovetail, CONFIG_IPIPE_COMPAT is enabled
> + * to allow this.
> + *
> + * __COBALT_SYSCALL_BIT is a generic marker for Cobalt system calls we
> + * use with all CPU architectures in common code, to distinguish them
> + * from native Linux syscalls. For this reason, it must be present in
> + * the Cobalt syscall code register (r0) as well.

Could we please add the reason for why we have this deviation from the
kernel's ABI on ARM? One sentence is likely enough ("it makes things
much faster" or "needed because of limited free bits" or whatever), and
it would be a perfect place to drop it here.

Thanks,
Jan

> + */
> +
>  #define __xn_syscode(__nr)	(__COBALT_SYSCALL_BIT | (__nr))
>  
> -#define XENO_ARM_SYSCALL        0x000F0042	/* carefully chosen... */
> +/*
> + * A carefully choosen ARM-specific local syscall code denoting a
> + * Cobalt syscall, which the interrupt pipeline looks for in order to
> + * route the request to the real-time core via a dedicated handler.
> + */
> +#define XENO_ARM_SYSCALL        0x000F0042
>  
>  #define XENOMAI_SYSARCH_TSCINFO      4
>  
> 

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH 1/2] cobalt/vfile: seq_file seek index must progress

[Jan Kiszka]

On 24.05.21 09:32, Philippe Gerum wrote:
> From: Philippe Gerum <rpm@xenomai.org>
> 
> The offset field we receive from the kernel in a vfile next() handler
> must progress in order for the loop to stop properly, independently
> from our own tracking of the end-of-list condition.
> 
> Bug is reproducible by running two loops in parallel:
> 
> - one continuously spawning an application which creates a few tenths
> of threads (10-20 would suffice) before exiting shortly after.
> 
> - another one continuously reading from /proc/xenomai/sched/{threads,
>   stat, acct}.
> 
> At some point, the vfile handler should cause a kernel crash.
> 
> Signed-off-by: Philippe Gerum <rpm@xenomai.org>
> ---
>  kernel/cobalt/vfile.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/cobalt/vfile.c b/kernel/cobalt/vfile.c
> index c7b81a704..fae0cc377 100644
> --- a/kernel/cobalt/vfile.c
> +++ b/kernel/cobalt/vfile.c
> @@ -109,11 +109,11 @@ static void *vfile_snapshot_next(struct seq_file *seq, void *v, loff_t *offp)
>  	struct xnvfile_snapshot_iterator *it = seq->private;
>  	loff_t pos = *offp;
>  
> +	++*offp;
> +
>  	if (pos >= it->nrdata)
>  		return NULL;
>  
> -	++*offp;
> -
>  	return it->databuf + pos * it->vfile->datasz;
>  }
>  
> @@ -452,17 +452,15 @@ static void *vfile_regular_next(struct seq_file *seq, void *v, loff_t *offp)
>  	struct xnvfile_regular *vfile = it->vfile;
>  	void *data;
>  
> +	it->pos = ++(*offp);
> +
>  	if (vfile->ops->next == NULL)
>  		return NULL;
>  
> -	it->pos = *offp + 1;
> -
>  	data = vfile->ops->next(it);
>  	if (data == NULL)
>  		return NULL;
>  
> -	*offp = it->pos;
> -
>  	return data;
>  }
>  
> 

Thanks, applied to next and all stable branches.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH 2/2] cobalt/arm: document the syscall convention

[Philippe Gerum]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> On 24.05.21 09:32, Philippe Gerum wrote:
>> From: Philippe Gerum <rpm@xenomai.org>
>> 
>> Signed-off-by: Philippe Gerum <rpm@xenomai.org>
>> ---
>>  .../arm/include/asm/xenomai/uapi/syscall.h    | 31 ++++++++++++++++++-
>>  1 file changed, 30 insertions(+), 1 deletion(-)
>> 
>> diff --git a/kernel/cobalt/arch/arm/include/asm/xenomai/uapi/syscall.h b/kernel/cobalt/arch/arm/include/asm/xenomai/uapi/syscall.h
>> index c079a358c..9146b732a 100644
>> --- a/kernel/cobalt/arch/arm/include/asm/xenomai/uapi/syscall.h
>> +++ b/kernel/cobalt/arch/arm/include/asm/xenomai/uapi/syscall.h
>> @@ -21,9 +21,38 @@
>>  #ifndef _COBALT_ARM_ASM_UAPI_SYSCALL_H
>>  #define _COBALT_ARM_ASM_UAPI_SYSCALL_H
>>  
>> +/*
>> + * Argument marshalling for Cobalt syscalls:
>> + *
>> + * r7 := XENO_ARM_SYSCALL
>> + * r0 := Cobalt syscall code (sc_cobalt_xx | __COBALT_SYSCALL_BIT)
>> + * r1 := first argument
>> + * r2 := second argument
>> + * ...
>> + * r5 := fifth argument
>> + *
>> + * r0 := <return value>
>> + *
>> + * This marshalling is common to the I-pipe and Dovetail-based
>> + * configurations so that we have a single ABI convention for both,
>> + * enabling us to have a single user build which does not depend on
>> + * the pipeline type.  With Dovetail, CONFIG_IPIPE_COMPAT is enabled
>> + * to allow this.
>> + *
>> + * __COBALT_SYSCALL_BIT is a generic marker for Cobalt system calls we
>> + * use with all CPU architectures in common code, to distinguish them
>> + * from native Linux syscalls. For this reason, it must be present in
>> + * the Cobalt syscall code register (r0) as well.
>
> Could we please add the reason for why we have this deviation from the
> kernel's ABI on ARM? One sentence is likely enough ("it makes things
> much faster" or "needed because of limited free bits" or whatever), and
> it would be a perfect place to drop it here.
>

There is no reason. Just a fact of history.

> Thanks,
> Jan
>
>> + */
>> +
>>  #define __xn_syscode(__nr)	(__COBALT_SYSCALL_BIT | (__nr))
>>  
>> -#define XENO_ARM_SYSCALL        0x000F0042	/* carefully chosen... */
>> +/*
>> + * A carefully choosen ARM-specific local syscall code denoting a
>> + * Cobalt syscall, which the interrupt pipeline looks for in order to
>> + * route the request to the real-time core via a dedicated handler.
>> + */
>> +#define XENO_ARM_SYSCALL        0x000F0042
>>  
>>  #define XENOMAI_SYSARCH_TSCINFO      4
>>  
>> 


-- 
Philippe.

Re: [PATCH 2/2] cobalt/arm: document the syscall convention

[Jan Kiszka]

On 25.05.21 09:30, Philippe Gerum wrote:
> 
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> On 24.05.21 09:32, Philippe Gerum wrote:
>>> From: Philippe Gerum <rpm@xenomai.org>
>>>
>>> Signed-off-by: Philippe Gerum <rpm@xenomai.org>
>>> ---
>>>  .../arm/include/asm/xenomai/uapi/syscall.h    | 31 ++++++++++++++++++-
>>>  1 file changed, 30 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/cobalt/arch/arm/include/asm/xenomai/uapi/syscall.h b/kernel/cobalt/arch/arm/include/asm/xenomai/uapi/syscall.h
>>> index c079a358c..9146b732a 100644
>>> --- a/kernel/cobalt/arch/arm/include/asm/xenomai/uapi/syscall.h
>>> +++ b/kernel/cobalt/arch/arm/include/asm/xenomai/uapi/syscall.h
>>> @@ -21,9 +21,38 @@
>>>  #ifndef _COBALT_ARM_ASM_UAPI_SYSCALL_H
>>>  #define _COBALT_ARM_ASM_UAPI_SYSCALL_H
>>>  
>>> +/*
>>> + * Argument marshalling for Cobalt syscalls:
>>> + *
>>> + * r7 := XENO_ARM_SYSCALL
>>> + * r0 := Cobalt syscall code (sc_cobalt_xx | __COBALT_SYSCALL_BIT)
>>> + * r1 := first argument
>>> + * r2 := second argument
>>> + * ...
>>> + * r5 := fifth argument
>>> + *
>>> + * r0 := <return value>
>>> + *
>>> + * This marshalling is common to the I-pipe and Dovetail-based
>>> + * configurations so that we have a single ABI convention for both,
>>> + * enabling us to have a single user build which does not depend on
>>> + * the pipeline type.  With Dovetail, CONFIG_IPIPE_COMPAT is enabled
>>> + * to allow this.
>>> + *
>>> + * __COBALT_SYSCALL_BIT is a generic marker for Cobalt system calls we
>>> + * use with all CPU architectures in common code, to distinguish them
>>> + * from native Linux syscalls. For this reason, it must be present in
>>> + * the Cobalt syscall code register (r0) as well.
>>
>> Could we please add the reason for why we have this deviation from the
>> kernel's ABI on ARM? One sentence is likely enough ("it makes things
>> much faster" or "needed because of limited free bits" or whatever), and
>> it would be a perfect place to drop it here.
>>
> 
> There is no reason. Just a fact of history.

That's good to hear: Time to clean up for 3.2. We are fine break ABIs on
major updates.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux