Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Work around for error code corruption in rt_cond_wait[ _until]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Work around for error code corruption in rt_cond_wait[ _until]

[Gilles Chanteperdrix]

GIT version control wrote:
> Module: xenomai-jki
> Branch: for-upstream
> Commit: 4b5370dce6f3a655ac104b93b032f3b639206fd5
> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=4b5370dce6f3a655ac104b93b032f3b639206fd5
> 
> Author: Jan Kiszka <jan.kiszka@domain.hid>
> Date:   Tue Mar  2 16:24:59 2010 +0100
> 
> Native: Work around for error code corruption in rt_cond_wait[_until]
> 
> User space suffers from losing the error code of the cond wait prologue.
> We actually had to return the prologue error to user space and use that
> one after the epilogue succeeded, but the existing kernel ABI does not
> allow this, nor is existing user space prepared for this.
> 
> As a work around, keep the prologue error code in xnthread's errcode and
> restore it from there when user space re-enters the kernel for the
> epilogue. This requires no ABI changes, but suffers from potential
> overwriting if the same or some other errcode touching service is called
> from a user space signal handler in between the calls. Moreover, current
> user space also prevents that we can return -EINTR on interruption while
> blocked on the condition variable, which was possible before.
> 
> A true fix will require a new syscall and updated user space libs.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> 
> ---
> 
>  ksrc/skins/native/syscall.c |   19 +++++++++++++++----
>  1 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/ksrc/skins/native/syscall.c b/ksrc/skins/native/syscall.c
> index 462ee7e..930a28d 100644
> --- a/ksrc/skins/native/syscall.c
> +++ b/ksrc/skins/native/syscall.c
> @@ -1841,7 +1841,7 @@ static int __rt_cond_wait_prologue(struct pt_regs *regs)
>  	RT_MUTEX *mutex;
>  	RT_COND *cond;
>  	RTIME timeout;
> -	int err;
> +	int err, epilogue_err;
>  
>  	if (__xn_safe_copy_from_user(&cph, (void __user *)__xn_reg_arg1(regs),
>  				     sizeof(cph)))
> @@ -1868,11 +1868,15 @@ static int __rt_cond_wait_prologue(struct pt_regs *regs)
>  		return -EFAULT;
>  
>  	err = rt_cond_wait_prologue(cond, mutex, &lockcnt, timeout_mode, timeout);
> +	xnpod_current_thread()->errcode = err;
>  
>  	/* Reacquire the mutex if it was unlocked in the prologue and we were
>  	   not interrupted. */
> -	if (lockcnt && err != -EINTR)
> -		err = rt_cond_wait_epilogue(mutex, lockcnt);
> +	if (lockcnt && err != -EINTR) {
> +		epilogue_err = rt_cond_wait_epilogue(mutex, lockcnt);
> +		if (!err)
> +			err = epilogue_err;

IMO, Here should be:
		if (epilogue_err < 0)
			err = epilogue_err;

-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Work around for error code corruption in rt_cond_wait[ _until]

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> GIT version control wrote:
>> Module: xenomai-jki
>> Branch: for-upstream
>> Commit: 4b5370dce6f3a655ac104b93b032f3b639206fd5
>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=4b5370dce6f3a655ac104b93b032f3b639206fd5
>>
>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>> Date:   Tue Mar  2 16:24:59 2010 +0100
>>
>> Native: Work around for error code corruption in rt_cond_wait[_until]
>>
>> User space suffers from losing the error code of the cond wait prologue.
>> We actually had to return the prologue error to user space and use that
>> one after the epilogue succeeded, but the existing kernel ABI does not
>> allow this, nor is existing user space prepared for this.
>>
>> As a work around, keep the prologue error code in xnthread's errcode and
>> restore it from there when user space re-enters the kernel for the
>> epilogue. This requires no ABI changes, but suffers from potential
>> overwriting if the same or some other errcode touching service is called
>> from a user space signal handler in between the calls. Moreover, current
>> user space also prevents that we can return -EINTR on interruption while
>> blocked on the condition variable, which was possible before.
>>
>> A true fix will require a new syscall and updated user space libs.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>
>> ---
>>
>>  ksrc/skins/native/syscall.c |   19 +++++++++++++++----
>>  1 files changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/ksrc/skins/native/syscall.c b/ksrc/skins/native/syscall.c
>> index 462ee7e..930a28d 100644
>> --- a/ksrc/skins/native/syscall.c
>> +++ b/ksrc/skins/native/syscall.c
>> @@ -1841,7 +1841,7 @@ static int __rt_cond_wait_prologue(struct pt_regs *regs)
>>  	RT_MUTEX *mutex;
>>  	RT_COND *cond;
>>  	RTIME timeout;
>> -	int err;
>> +	int err, epilogue_err;
>>  
>>  	if (__xn_safe_copy_from_user(&cph, (void __user *)__xn_reg_arg1(regs),
>>  				     sizeof(cph)))
>> @@ -1868,11 +1868,15 @@ static int __rt_cond_wait_prologue(struct pt_regs *regs)
>>  		return -EFAULT;
>>  
>>  	err = rt_cond_wait_prologue(cond, mutex, &lockcnt, timeout_mode, timeout);
>> +	xnpod_current_thread()->errcode = err;
>>  
>>  	/* Reacquire the mutex if it was unlocked in the prologue and we were
>>  	   not interrupted. */
>> -	if (lockcnt && err != -EINTR)
>> -		err = rt_cond_wait_epilogue(mutex, lockcnt);
>> +	if (lockcnt && err != -EINTR) {
>> +		epilogue_err = rt_cond_wait_epilogue(mutex, lockcnt);
>> +		if (!err)
>> +			err = epilogue_err;
> 
> IMO, Here should be:
> 		if (epilogue_err < 0)
> 			err = epilogue_err;
> 

That would be wrong (if it equals -EINTR), but I get your point: What
should take precedence, prologue or epilogue error? So far the decision
was simple (always prologue before 2.5), since we now also consider the
epilogue. So we could

 - always pick the epilogue (except for -EINTR) or
 - only do so if the prologue reported a "less fatal" error than the
   epilogue

E.g. if the prologue failed due to -EIDRM, I would prefer to always
propagate this instead of whatever the epilogue reports.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Work around for error code corruption in rt_cond_wait[ _until]

[Jan Kiszka]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> GIT version control wrote:
>>> Module: xenomai-jki
>>> Branch: for-upstream
>>> Commit: 4b5370dce6f3a655ac104b93b032f3b639206fd5
>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=4b5370dce6f3a655ac104b93b032f3b639206fd5
>>>
>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>> Date:   Tue Mar  2 16:24:59 2010 +0100
>>>
>>> Native: Work around for error code corruption in rt_cond_wait[_until]
>>>
>>> User space suffers from losing the error code of the cond wait prologue.
>>> We actually had to return the prologue error to user space and use that
>>> one after the epilogue succeeded, but the existing kernel ABI does not
>>> allow this, nor is existing user space prepared for this.
>>>
>>> As a work around, keep the prologue error code in xnthread's errcode and
>>> restore it from there when user space re-enters the kernel for the
>>> epilogue. This requires no ABI changes, but suffers from potential
>>> overwriting if the same or some other errcode touching service is called
>>> from a user space signal handler in between the calls. Moreover, current
>>> user space also prevents that we can return -EINTR on interruption while
>>> blocked on the condition variable, which was possible before.
>>>
>>> A true fix will require a new syscall and updated user space libs.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>
>>> ---
>>>
>>>  ksrc/skins/native/syscall.c |   19 +++++++++++++++----
>>>  1 files changed, 15 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/ksrc/skins/native/syscall.c b/ksrc/skins/native/syscall.c
>>> index 462ee7e..930a28d 100644
>>> --- a/ksrc/skins/native/syscall.c
>>> +++ b/ksrc/skins/native/syscall.c
>>> @@ -1841,7 +1841,7 @@ static int __rt_cond_wait_prologue(struct pt_regs *regs)
>>>  	RT_MUTEX *mutex;
>>>  	RT_COND *cond;
>>>  	RTIME timeout;
>>> -	int err;
>>> +	int err, epilogue_err;
>>>  
>>>  	if (__xn_safe_copy_from_user(&cph, (void __user *)__xn_reg_arg1(regs),
>>>  				     sizeof(cph)))
>>> @@ -1868,11 +1868,15 @@ static int __rt_cond_wait_prologue(struct pt_regs *regs)
>>>  		return -EFAULT;
>>>  
>>>  	err = rt_cond_wait_prologue(cond, mutex, &lockcnt, timeout_mode, timeout);
>>> +	xnpod_current_thread()->errcode = err;
>>>  
>>>  	/* Reacquire the mutex if it was unlocked in the prologue and we were
>>>  	   not interrupted. */
>>> -	if (lockcnt && err != -EINTR)
>>> -		err = rt_cond_wait_epilogue(mutex, lockcnt);
>>> +	if (lockcnt && err != -EINTR) {
>>> +		epilogue_err = rt_cond_wait_epilogue(mutex, lockcnt);
>>> +		if (!err)
>>> +			err = epilogue_err;
>> IMO, Here should be:
>> 		if (epilogue_err < 0)
>> 			err = epilogue_err;
>>
> 
> That would be wrong (if it equals -EINTR), but I get your point: What

In fact, my current version is wrong /wrt EINTR. Back to the drawing board.

> should take precedence, prologue or epilogue error? So far the decision
> was simple (always prologue before 2.5), since we now also consider the
> epilogue. So we could
> 
>  - always pick the epilogue (except for -EINTR) or
>  - only do so if the prologue reported a "less fatal" error than the
>    epilogue
> 
> E.g. if the prologue failed due to -EIDRM, I would prefer to always
> propagate this instead of whatever the epilogue reports.
> 
> Jan
> 

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux