[Xenomai] [PATCH] Xenomai recvmsg cmsg patch

[Xenomai] [PATCH] Xenomai recvmsg cmsg patch

[Manuel Huber]

Current implementation of Xenomai's recvmsg syscall doesn't
handle msghdr fields (msg_control, msg_controllen) correctly. It's
necessary to change msg_controllen to the number of bytes written
to the buffer and fix msg_control if it has been used to point to
the actual starting address of the buffer. This would be necessary
to apply another feature to RTnet that allows to pass timestamps
from the driver to userspace applications easily 
(like SO_TIMESTAMP...). Please apply ;)

Manuel


Manuel Huber (1):
  rtdm: Fix msghdr struct (cmsg) in sys_rtdm_recvmsg

 ksrc/skins/rtdm/syscall.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

-- 
1.8.3

[Xenomai] [PATCH] rtdm: Fix msghdr struct (cmsg) in sys_rtdm_recvmsg

[Manuel Huber]

From: Manuel Huber <Manuel.h87@gmail.com>

Whenever a new control message is put into msg_control buffer
the actual address and the space left is saved to msg_control
and msg_controllen. This allows adding messages as long as
there is enough space left in the user-supplied buffer. Both
fields have to be fixed again before passing them to the user
by copying the original starting address of the buffer to
msg_control and saving the actual amount of bytes written to
the buffer to msg_controllen.

* Explicit use of __xn_put_user rather then __xn_copy_to_user
* Don't write back msg->msg_namelen
---
 ksrc/skins/rtdm/syscall.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/ksrc/skins/rtdm/syscall.c b/ksrc/skins/rtdm/syscall.c
index 0ff5d40..7dd20e3 100644
--- a/ksrc/skins/rtdm/syscall.c
+++ b/ksrc/skins/rtdm/syscall.c
@@ -79,22 +79,31 @@ static int sys_rtdm_recvmsg(struct pt_regs *regs)
 {
 	struct task_struct *p = current;
 	struct msghdr krnl_msg;
+	void *cmsg_control;
+	struct msghdr __user *usr_msg;
 	int ret;
 
-	if (unlikely(!access_wok(__xn_reg_arg2(regs),
+	usr_msg = (void __user *)__xn_reg_arg2(regs);
+
+	if (unlikely(!access_wok((void __user *)usr_msg,
 				 sizeof(krnl_msg)) ||
 		     __xn_copy_from_user(&krnl_msg,
-					 (void __user *)__xn_reg_arg2(regs),
+					 (void __user *)usr_msg,
 					 sizeof(krnl_msg))))
 		return -EFAULT;
 
+	cmsg_control = krnl_msg.msg_control;
+
 	ret = __rt_dev_recvmsg(p, __xn_reg_arg1(regs), &krnl_msg,
 			       __xn_reg_arg3(regs));
 	if (unlikely(ret < 0))
 		return ret;
 
-	if (unlikely(__xn_copy_to_user((void __user *)__xn_reg_arg2(regs),
-				       &krnl_msg, sizeof(krnl_msg))))
+	if (unlikely(__xn_put_user((typeof(krnl_msg.msg_controllen))(
+					   krnl_msg.msg_control - cmsg_control),
+				   (void __user *)&usr_msg->msg_controllen) ||
+		     __xn_put_user(krnl_msg.msg_flags,
+				   (void __user *)&(usr_msg->msg_flags))))
 		return -EFAULT;
 
 	return ret;
-- 
1.8.3

Re: [Xenomai] [PATCH] rtdm: Fix msghdr struct (cmsg) in sys_rtdm_recvmsg

[Jan Kiszka]

On 2013-08-13 22:24, Manuel Huber wrote:
> From: Manuel Huber <Manuel.h87@gmail.com>
> 
> Whenever a new control message is put into msg_control buffer
> the actual address and the space left is saved to msg_control
> and msg_controllen. This allows adding messages as long as
> there is enough space left in the user-supplied buffer. Both
> fields have to be fixed again before passing them to the user
> by copying the original starting address of the buffer to
> msg_control and saving the actual amount of bytes written to
> the buffer to msg_controllen.
> 
> * Explicit use of __xn_put_user rather then __xn_copy_to_user
> * Don't write back msg->msg_namelen
> ---
>  ksrc/skins/rtdm/syscall.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/ksrc/skins/rtdm/syscall.c b/ksrc/skins/rtdm/syscall.c
> index 0ff5d40..7dd20e3 100644
> --- a/ksrc/skins/rtdm/syscall.c
> +++ b/ksrc/skins/rtdm/syscall.c
> @@ -79,22 +79,31 @@ static int sys_rtdm_recvmsg(struct pt_regs *regs)
>  {
>  	struct task_struct *p = current;
>  	struct msghdr krnl_msg;
> +	void *cmsg_control;
> +	struct msghdr __user *usr_msg;
>  	int ret;
>  
> -	if (unlikely(!access_wok(__xn_reg_arg2(regs),
> +	usr_msg = (void __user *)__xn_reg_arg2(regs);
> +
> +	if (unlikely(!access_wok((void __user *)usr_msg,
>  				 sizeof(krnl_msg)) ||
>  		     __xn_copy_from_user(&krnl_msg,
> -					 (void __user *)__xn_reg_arg2(regs),
> +					 (void __user *)usr_msg,
>  					 sizeof(krnl_msg))))
>  		return -EFAULT;
>  
> +	cmsg_control = krnl_msg.msg_control;
> +
>  	ret = __rt_dev_recvmsg(p, __xn_reg_arg1(regs), &krnl_msg,
>  			       __xn_reg_arg3(regs));
>  	if (unlikely(ret < 0))
>  		return ret;
>  
> -	if (unlikely(__xn_copy_to_user((void __user *)__xn_reg_arg2(regs),
> -				       &krnl_msg, sizeof(krnl_msg))))
> +	if (unlikely(__xn_put_user((typeof(krnl_msg.msg_controllen))(
> +					   krnl_msg.msg_control - cmsg_control),

This still lacks documentation in rtdm_recvmsg_handler_t that
msg_control is supposed to be moved forward by the driver when control
data is written to user space.

Also, you should update existing users of this interface in Xenomai,
i.e. the CAN layer.

> +				   (void __user *)&usr_msg->msg_controllen) ||
> +		     __xn_put_user(krnl_msg.msg_flags,
> +				   (void __user *)&(usr_msg->msg_flags))))
>  		return -EFAULT;
>  
>  	return ret;
> 

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] [PATCH] rtdm: Fix msghdr struct (cmsg) in sys_rtdm_recvmsg

[Manuel Huber]

> This still lacks documentation in rtdm_recvmsg_handler_t that
> msg_control is supposed to be moved forward by the driver when control
> data is written to user space.
Oh, sorry; I forgot to add this....
Is this description okay:?

  @param[in,out] msg Message descriptor as passed by the user, automatically
  mirrored to safe kernel memory in case of user mode call. If msg_control
  points to a valid buffer (not NULL) and msg_controllen is > 0, cmsghdr
  structures can be put into msg_control buffer by driver code. The
  msg_controllen field has to be decremented accordingly to mirror the
  actual space left and msg_control shall be incremented to always point
  to the next free byte.
  If there is insufficient space left, MSG_CTRUNC will be set in msg_flags,
  but the call shall succeed.
  Before returning, the actual number of bytes written to the msg_control
  buffer is written to msg_controllen and msg_control is reset to the 
original
  starting address.

> Also, you should update existing users
> of this interface in Xenomai, i.e. the CAN layer.
Oh, sorry; I just assumed that msg_control has not been used, sorry...
I checked the drivers and I believe it's only rtcan. There are two
ways to fix rtcan:

1) I can break existing user code by using rt_put_cmsg from my RTnet
patch (which does the same as put_cmsg from linux kernel) because
that's how you are supposed to use msg_control buffer (to allow extra
messages from every layer) and rtcan simply writes the timestamp to
the buffer (no cmsghdr structure).

2) I can just fix current rtcan implementation to work with the
proposed patch (fixing msg_controllen and msg_control according to the
protocol in the driver) and not break existing user code but keep
violating the original protocol (by not using cmsghr).

3) 1 + 2: Keep original interface to not break existing user code but
add a warning when activating RTCAN_RTIOC_TAKE_TIMESTAMP (deprecated
interface) and implement another interface that complies to the
protocol and can be used instead (but adds overhead to code compared
to the current interface...).

In case 1, 3 I would also like to move rt_put_cmsg to xenomai and
maybe define SCM_TIMESTAMPNS64 in xenomai and use it in rtcan and
RTnet to simply copy nanosecs_abs_t (by using rt_put_cmsg). As a
legacy option I could implement SO_TIMESTAMPNS interface which
converts nanosecs_abs_t to a timespec struct (as in my RTnet patch
rt_nanosecs_to_ts ()).

I would add SCM_TIMESTAMPNS64 (we can rename that...) to rtdm
header files as a standard option to get timestamps through recvmsg...

But I can't test rtcan (I think...); Is there someone who can? Change
2 is rather simple and should work (without testing) if I'm very
careful. 1 and especially 3 are bigger changes (maybe 20-30 lines),
this must be tested;

I personally like number 3 the most; I would start to implement number
2 send the patch and then start developing number 3.

What do you think?

Re: [Xenomai] [PATCH] rtdm: Fix msghdr struct (cmsg) in sys_rtdm_recvmsg

[Jan Kiszka]

On 2013-08-17 12:41, Manuel Huber wrote:
>> This still lacks documentation in rtdm_recvmsg_handler_t that
>> msg_control is supposed to be moved forward by the driver when control
>> data is written to user space.
> Oh, sorry; I forgot to add this....
> Is this description okay:?
> 
>  @param[in,out] msg Message descriptor as passed by the user, automatically
>  mirrored to safe kernel memory in case of user mode call. If msg_control
>  points to a valid buffer (not NULL) and msg_controllen is > 0, cmsghdr
>  structures can be put into msg_control buffer by driver code. The
>  msg_controllen field has to be decremented accordingly to mirror the
>  actual space left and msg_control shall be incremented to always point
>  to the next free byte.

Decrementing msg_controllen is not part of an interface between core and
driver, is it? Let's keep it straight.

>  If there is insufficient space left, MSG_CTRUNC will be set in msg_flags,

Who will set it, core layer or driver?

>  but the call shall succeed.
>  Before returning, the actual number of bytes written to the msg_control
>  buffer is written to msg_controllen and msg_control is reset to the
> original
>  starting address.

That's confusing. Better state that the core layer will update the
user's msg_controllen field according to the number of bytes the driver
reported to have written (via moving msg_control). This implies that the
user's msg_control field is not updated.

> 
>> Also, you should update existing users
>> of this interface in Xenomai, i.e. the CAN layer.
> Oh, sorry; I just assumed that msg_control has not been used, sorry...
> I checked the drivers and I believe it's only rtcan. There are two
> ways to fix rtcan:
> 
> 1) I can break existing user code by using rt_put_cmsg from my RTnet
> patch (which does the same as put_cmsg from linux kernel) because
> that's how you are supposed to use msg_control buffer (to allow extra
> messages from every layer) and rtcan simply writes the timestamp to
> the buffer (no cmsghdr structure).
> 
> 2) I can just fix current rtcan implementation to work with the
> proposed patch (fixing msg_controllen and msg_control according to the
> protocol in the driver) and not break existing user code but keep
> violating the original protocol (by not using cmsghr).
> 
> 3) 1 + 2: Keep original interface to not break existing user code but
> add a warning when activating RTCAN_RTIOC_TAKE_TIMESTAMP (deprecated
> interface) and implement another interface that complies to the
> protocol and can be used instead (but adds overhead to code compared
> to the current interface...).
> 
> In case 1, 3 I would also like to move rt_put_cmsg to xenomai and
> maybe define SCM_TIMESTAMPNS64 in xenomai and use it in rtcan and
> RTnet to simply copy nanosecs_abs_t (by using rt_put_cmsg). As a
> legacy option I could implement SO_TIMESTAMPNS interface which
> converts nanosecs_abs_t to a timespec struct (as in my RTnet patch
> rt_nanosecs_to_ts ()).
> 
> I would add SCM_TIMESTAMPNS64 (we can rename that...) to rtdm
> header files as a standard option to get timestamps through recvmsg...
> 
> But I can't test rtcan (I think...); Is there someone who can? Change
> 2 is rather simple and should work (without testing) if I'm very
> careful. 1 and especially 3 are bigger changes (maybe 20-30 lines),
> this must be tested;
> 
> I personally like number 3 the most; I would start to implement number
> 2 send the patch and then start developing number 3.
> 
> What do you think?

Starting with option 2 sounds reasonable. We can then discuss
introducing a generic timestamp feature that all RTDM protocol drivers
can implement in the same way while keeping the existing ones of RTCAN
and RTnet for backward compatibility.

Regarding CAN testing: You should be able to work with virtual CAN
adapters, they should also produce timestamps. CC'ing Wolfgang to
confirm this.

Jan


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130818/5e51b835/attachment.pgp>