[Xenomai] rtdm: minimal patch for sys_rtdm_recvmsg

[Xenomai] rtdm: minimal patch for sys_rtdm_recvmsg

[Manuel Huber]

Hello!

I plan to add a minor change to RTnet which allows to get reception
timestamps through recvmsg:

SO_TIMESTAMPNS patch:
http://sourceforge.net/mailarchive/forum.php?thread_name=51BAB330.70500%40web.de&forum_name=rtnet-developers 
<http://sourceforge.net/mailarchive/forum.php?thread_name=51BAB330.70500%40web.de&forum_name=rtnet-developers>

Current implementation (of RTDM) in RTAI and Xenomai doesn't fix the
msg_control and msg_controllen field in struct msghdr. I believe, it
should be done similar to the linux kernel:

1. User specifies how much space has been allocated for control messages
    - msg_control is set to starting address of buffer
    - msg_controllen is set to the size of the buffer
2. User calls recvmsg
3. Control messages are tried to be inserted by some kernel code
4. msg_control is set to the next free byte, msg_controllen is updated to
    the actual space left in the buffer.
5. Before copying the struct to the user, msg_control has to be reset to
    the actual starting address and msg_controllen has to be set to the
    number of bytes written to the control message buffer rather than the
    space left.
6. Back to user space.

Current implementation could cause problems: Control messages are
currently not used but the msg_controllen field inidicates that control
messages have been written to the buffer; I'm not sure that this is a
violation of some standard, but at least it's different from the linux
kernel implementation!

The patch has been tested on 3.5.7 kernel and is based on SHA
69f6cb5ec287afff5ab197528d9eb1177f6de6d6
(http://git.xenomai.org/xenomai-jki.git) and should work as expected.


Best Regards

Manuel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Xenomai-0001-rtdm-Fix-msghdr-struct-cmsg-in-sys_rtdm_recvmsg.patch
Type: text/x-patch
Size: 1722 bytes
Desc: not available
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130616/18f43ac0/attachment.bin>

Re: [Xenomai] rtdm: minimal patch for sys_rtdm_recvmsg

[Jan Kiszka]

On 2013-06-16 23:04, Manuel Huber wrote:
> Hello!
> 
> I plan to add a minor change to RTnet which allows to get reception
> timestamps through recvmsg:
> 
> SO_TIMESTAMPNS patch:
> http://sourceforge.net/mailarchive/forum.php?thread_name=51BAB330.70500%40web.de&forum_name=rtnet-developers
> <http://sourceforge.net/mailarchive/forum.php?thread_name=51BAB330.70500%40web.de&forum_name=rtnet-developers>
> 
> 
> Current implementation (of RTDM) in RTAI and Xenomai doesn't fix the
> msg_control and msg_controllen field in struct msghdr. I believe, it
> should be done similar to the linux kernel:
> 
> 1. User specifies how much space has been allocated for control messages
>    - msg_control is set to starting address of buffer
>    - msg_controllen is set to the size of the buffer
> 2. User calls recvmsg
> 3. Control messages are tried to be inserted by some kernel code
> 4. msg_control is set to the next free byte, msg_controllen is updated to
>    the actual space left in the buffer.
> 5. Before copying the struct to the user, msg_control has to be reset to
>    the actual starting address and msg_controllen has to be set to the
>    number of bytes written to the control message buffer rather than the
>    space left.
> 6. Back to user space.
> 
> Current implementation could cause problems: Control messages are
> currently not used but the msg_controllen field inidicates that control
> messages have been written to the buffer; I'm not sure that this is a
> violation of some standard, but at least it's different from the linux
> kernel implementation!
> 
> The patch has been tested on 3.5.7 kernel and is based on SHA
> 69f6cb5ec287afff5ab197528d9eb1177f6de6d6
> (http://git.xenomai.org/xenomai-jki.git) and should work as expected.
> 
> 
> Best Regards
> 
> Manuel
> -------------- next part --------------
> A non-text attachment was scrubbed...
> Name: Xenomai-0001-rtdm-Fix-msghdr-struct-cmsg-in-sys_rtdm_recvmsg.patch
> Type: text/x-patch
> Size: 1722 bytes
> Desc: not available
> URL:
> <http://www.xenomai.org/pipermail/xenomai/attachments/20130616/18f43ac0/attachment.bin>
> 

Please post patches inline, makes reviewing easier. The explanatory
part, like above, can be put after the "---" so that it won't show up in
the commit log.

> From 79872fff7bf6536007e1a09c353c4194b0def57c Mon Sep 17 00:00:00 2001
> From: Manuel Huber <Manuel.h87@gmail.com>
> Date: Fri, 14 Jun 2013 10:39:50 +0200
> Subject: [PATCH] rtdm: Fix msghdr struct (cmsg) in sys_rtdm_recvmsg
> 
> 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.
> ---
>  ksrc/skins/rtdm/syscall.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/ksrc/skins/rtdm/syscall.c b/ksrc/skins/rtdm/syscall.c
> index 0ff5d40..e5d9ae0 100644
> --- a/ksrc/skins/rtdm/syscall.c
> +++ b/ksrc/skins/rtdm/syscall.c
> @@ -79,6 +79,7 @@ static int sys_rtdm_recvmsg(struct pt_regs *regs)
>  {
>  	struct task_struct *p = current;
>  	struct msghdr krnl_msg;
> +	void *cmsg_control;
>  	int ret;
>  
>  	if (unlikely(!access_wok(__xn_reg_arg2(regs),
> @@ -88,11 +89,17 @@ static int sys_rtdm_recvmsg(struct pt_regs *regs)
>  					 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;
>  
> +	krnl_msg.msg_controllen = (__kernel_size_t)(krnl_msg.msg_control -
> +						    cmsg_control);

So this creates an API between the RTDM core and the driver implementing
msg_control support. Should be documented then (rtdm_recvmsg_handler_t).

> +	krnl_msg.msg_control = cmsg_control;
> +
>  	if (unlikely(__xn_copy_to_user((void __user *)__xn_reg_arg2(regs),
>  				       &krnl_msg, sizeof(krnl_msg))))

Let's just only copy back what can change, msg_flags and msg_controllen,
just like the kernel does.

Jan

>  		return -EFAULT;
> -- 
> 1.7.0.4


-------------- 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/20130617/dd73cdff/attachment.pgp>

Re: [Xenomai] rtdm: minimal patch for sys_rtdm_recvmsg

[Manuel Huber]

> Please post patches inline, makes reviewing easier. The explanatory
> part, like above, can be put after the "---" so that it won't show up in
> the commit log.
Oh, sorry; I forgot to send it plain-text...

> Let's just only copy back what can change, msg_flags and msg_controllen,
> just like the kernel does.
I have already implemented a version that does that, but then I looked at
rt_udp_recvmsg implementation (stack/ipv4/udp/udp.c) and saw that it also
sets msg_namelen field. This member will never be used in RTnet source
code, so I think it should be copied back to the user...
I mean it seems a little useless, because the user has to know the structur
anyway; I'm not sure if code really relies on this behaviour....

So if it's necessary to write back the msg_namelen field, I would need 3
__xn_put_user calls. I just though it looks ugly and probably does worse
then a single __xn_copy_to_user call.

Do you think it's necessary to write back msg_namelen? Because otherwise
I would definitely prefer using __xn_put_user twice.



 From ddb64a6eebcfc811187df3859f2e0092c0b6cfbd Mon Sep 17 00:00:00 2001
From: Manuel Huber <Manuel.h87@gmail.com>
Date: Mon, 17 Jun 2013 09:49:25 +0200
Subject: [PATCH] rtdm: Fix msghdr struct (cmsg) in sys_rtdm_recvmsg

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
---
  ksrc/skins/rtdm/syscall.c |   19 +++++++++++++++----
  1 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/ksrc/skins/rtdm/syscall.c b/ksrc/skins/rtdm/syscall.c
index 0ff5d40..1f3bccb 100644
--- a/ksrc/skins/rtdm/syscall.c
+++ b/ksrc/skins/rtdm/syscall.c
@@ -79,22 +79,33 @@ 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(krnl_msg.msg_namelen,
+                   (void __user *)&(usr_msg->msg_namelen)) ||
+             __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.7.0.4

[Xenomai] Fwd: Re: rtdm: minimal patch for sys_rtdm_recvmsg

[Manuel Huber]

Sorry, I messed up again... Tabs have been converted to spaces...

I hope it works this time... sorry again..

Manuel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Xenomai-0001-rtdm-Fix-msghdr-struct-cmsg-in-sys_rtdm_recvmsg-P2.patch
Type: text/x-patch
Size: 2232 bytes
Desc: not available
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130617/9a7252ff/attachment.bin>

Re: [Xenomai] rtdm: minimal patch for sys_rtdm_recvmsg

[Jan Kiszka]

On 2013-06-17 11:53, Manuel Huber wrote:
>> Please post patches inline, makes reviewing easier. The explanatory
>> part, like above, can be put after the "---" so that it won't show up in
>> the commit log.
> Oh, sorry; I forgot to send it plain-text...
> 
>> Let's just only copy back what can change, msg_flags and msg_controllen,
>> just like the kernel does.
> I have already implemented a version that does that, but then I looked at
> rt_udp_recvmsg implementation (stack/ipv4/udp/udp.c) and saw that it also
> sets msg_namelen field. This member will never be used in RTnet source
> code, so I think it should be copied back to the user...
> I mean it seems a little useless, because the user has to know the structur
> anyway; I'm not sure if code really relies on this behaviour....
> 
> So if it's necessary to write back the msg_namelen field, I would need 3
> __xn_put_user calls. I just though it looks ugly and probably does worse
> then a single __xn_copy_to_user call.
> 
> Do you think it's necessary to write back msg_namelen? Because otherwise
> I would definitely prefer using __xn_put_user twice.
> 

I think you stumbled over one of the very old and broken edges of RTnet.
It really makes no sense to copy namelen back. Rather, RTnet should
check msg_namelen before blindly copying the address into the msg_name
buffer.

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/20130622/5d6c982e/attachment.pgp>

Re: [Xenomai] Problem accessing msg_name in rt_udp_recvmsg

[Manuel Huber]

> Isn't msg->msg_name a user space buffer? Why is it possible to access
> it from kernel space (Line 424 - 426)? I'm not really familiar with
> the Linux kernel that much, therefore I checked some other parts of
> RTnet (ipv4/tcp/tcp.c) and there is something strange as well:
>
>     2053     len = msg->msg_iov[0].iov_len;
>     2054     buf = msg->msg_iov[0].iov_base;
>
> So I'm really getting confused... I mean wouldn't such a bug cause
> serious problems? I'm running RTnet since months using the recvmsg
> system call (udp) all the time and never encountered a problem. Sorry
> ifthis question is somehow stupid, I really tried to figure it out
> myself...
Okay, sorry; There is a big difference between Linux and Xenomai most
probably.Is it possible to access any user-space buffer from Xenomai
in Primary Mode? What happens when a buffer is to small or invalid?
Is it a problem to use rtdm_(safe_)copy_to_user in Primary Mode or does
it only add overhead?

Sorry for not paying enough attention in first place, and for asking
so many questions...

Re: [Xenomai] Problem accessing msg_name in rt_udp_recvmsg

[Jan Kiszka]

On 2013-06-22 23:35, Manuel Huber wrote:
>> Isn't msg->msg_name a user space buffer? Why is it possible to access
>> it from kernel space (Line 424 - 426)? I'm not really familiar with
>> the Linux kernel that much, therefore I checked some other parts of
>> RTnet (ipv4/tcp/tcp.c) and there is something strange as well:
>>
>>     2053     len = msg->msg_iov[0].iov_len;
>>     2054     buf = msg->msg_iov[0].iov_base;
>>
>> So I'm really getting confused... I mean wouldn't such a bug cause
>> serious problems? I'm running RTnet since months using the recvmsg
>> system call (udp) all the time and never encountered a problem. Sorry
>> ifthis question is somehow stupid, I really tried to figure it out
>> myself...
> Okay, sorry; There is a big difference between Linux and Xenomai most
> probably.Is it possible to access any user-space buffer from Xenomai
> in Primary Mode? What happens when a buffer is to small or invalid?
> Is it a problem to use rtdm_(safe_)copy_to_user in Primary Mode or does
> it only add overhead?

It is possible to access user space memory directly from Xenomai in
primary mode. However, RTnet is known to be buggy here because it
accesses user memory at some spots without using
rtdm_*_copy_to/from_user. This will cause kernel oopses when that memory
is not mapped in or the pointer is invalid.

> 
> Sorry for not paying enough attention in first place, and for asking
> so many questions...
> 

No problem at all!

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/20130623/29f0d5ff/attachment.pgp>