[Xenomai-core] iovec overwriting by CAN stack

[Xenomai-core] iovec overwriting by CAN stack

[Jan Kiszka]

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

Hi Wolfgang,

it's late, so I may have misread somecode, but don't you "update" the
iovec descriptors a user passes on send/recvmsg on return (namely
iovec_len)? I received some complaints about this /wrt to in-kernel CAN
stack usage.

I always considered the same well-know behaviour of RTnet a bug, but now
I found your code is doing this systematically, also for user space
callers. Is this behaviour undefined or even required according POSIX or
whatever?

Jan


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

[Xenomai-core] Re: iovec overwriting by CAN stack

[Wolfgang Grandegger]

Jan Kiszka wrote:
> Hi Wolfgang,
> 
> it's late, so I may have misread somecode, but don't you "update" the
> iovec descriptors a user passes on send/recvmsg on return (namely
> iovec_len)? I received some complaints about this /wrt to in-kernel CAN
> stack usage.

It's done here:

http://www.rts.uni-hannover.de/xenomai/lxr/source/ksrc/drivers/can/rtcan_raw.c?v=SVN-trunk#881
http://www.rts.uni-hannover.de/xenomai/lxr/source/ksrc/drivers/can/rtcan_raw.c?v=SVN-trunk#734

I may have missed something. What are the real complaints? Is there a 
test program?

> I always considered the same well-know behaviour of RTnet a bug, but now
> I found your code is doing this systematically, also for user space
> callers. Is this behaviour undefined or even required according POSIX or
> whatever?

I don't know, it's Sebastian's Kode.

Wolfgang.

[Xenomai-core] Re: iovec overwriting by CAN stack

[Jan Kiszka]

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

Wolfgang Grandegger wrote:
> Jan Kiszka wrote:
>> Hi Wolfgang,
>>
>> it's late, so I may have misread somecode, but don't you "update" the
>> iovec descriptors a user passes on send/recvmsg on return (namely
>> iovec_len)? I received some complaints about this /wrt to in-kernel CAN
>> stack usage.
> 
> It's done here:
> 
> http://www.rts.uni-hannover.de/xenomai/lxr/source/ksrc/drivers/can/rtcan_raw.c?v=SVN-trunk#881
> 
> http://www.rts.uni-hannover.de/xenomai/lxr/source/ksrc/drivers/can/rtcan_raw.c?v=SVN-trunk#734
> 
> 
> I may have missed something. What are the real complaints? Is there a
> test program?

Not yet (will commit the related patch to our RACK likely later today).
It's simply sending frames while re-using the msg+iovec structs in a loop.

> 
>> I always considered the same well-know behaviour of RTnet a bug, but now
>> I found your code is doing this systematically, also for user space
>> callers. Is this behaviour undefined or even required according POSIX or
>> whatever?
> 
> I don't know, it's Sebastian's Kode.
> 

Hmm, hope he will not say that he imitated RTnet...

Jan


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

[Xenomai-core] Re: iovec overwriting by CAN stack

[Sebastian Smolorz]

Jan Kiszka wrote:
> Wolfgang Grandegger wrote:
> > Jan Kiszka wrote:
> >> Hi Wolfgang,
> >>
> >> it's late, so I may have misread somecode, but don't you "update" the
> >> iovec descriptors a user passes on send/recvmsg on return (namely
> >> iovec_len)? I received some complaints about this /wrt to in-kernel CAN
> >> stack usage.
> >
> > It's done here:
> >
> > http://www.rts.uni-hannover.de/xenomai/lxr/source/ksrc/drivers/can/rtcan_
> >raw.c?v=SVN-trunk#881
> >
> > http://www.rts.uni-hannover.de/xenomai/lxr/source/ksrc/drivers/can/rtcan_
> >raw.c?v=SVN-trunk#734
> >
> >
> > I may have missed something. What are the real complaints? Is there a
> > test program?
>
> Not yet (will commit the related patch to our RACK likely later today).
> It's simply sending frames while re-using the msg+iovec structs in a loop.
>
> >> I always considered the same well-know behaviour of RTnet a bug, but now
> >> I found your code is doing this systematically, also for user space
> >> callers. Is this behaviour undefined or even required according POSIX or
> >> whatever?
> >
> > I don't know, it's Sebastian's Kode.
>
> Hmm, hope he will not say that he imitated RTnet...

Rather an imitation of the Linux kernel's behaviour. The memcpy_toiovec() and 
memcpy_fromiovec() functions [1] also modify the original iovec. 


[1] http://lxr.free-electrons.com/source/net/core/iovec.c

-- 
Sebastian

[Xenomai-core] Re: iovec overwriting by CAN stack

[Jan Kiszka]

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

Sebastian Smolorz wrote:
> Jan Kiszka wrote:
>> Wolfgang Grandegger wrote:
>>> Jan Kiszka wrote:
>>>> Hi Wolfgang,
>>>>
>>>> it's late, so I may have misread somecode, but don't you "update" the
>>>> iovec descriptors a user passes on send/recvmsg on return (namely
>>>> iovec_len)? I received some complaints about this /wrt to in-kernel CAN
>>>> stack usage.
>>> It's done here:
>>>
>>> http://www.rts.uni-hannover.de/xenomai/lxr/source/ksrc/drivers/can/rtcan_
>>> raw.c?v=SVN-trunk#881
>>>
>>> http://www.rts.uni-hannover.de/xenomai/lxr/source/ksrc/drivers/can/rtcan_
>>> raw.c?v=SVN-trunk#734
>>>
>>>
>>> I may have missed something. What are the real complaints? Is there a
>>> test program?
>> Not yet (will commit the related patch to our RACK likely later today).
>> It's simply sending frames while re-using the msg+iovec structs in a loop.
>>
>>>> I always considered the same well-know behaviour of RTnet a bug, but now
>>>> I found your code is doing this systematically, also for user space
>>>> callers. Is this behaviour undefined or even required according POSIX or
>>>> whatever?
>>> I don't know, it's Sebastian's Kode.
>> Hmm, hope he will not say that he imitated RTnet...
> 
> Rather an imitation of the Linux kernel's behaviour. The memcpy_toiovec() and 
> memcpy_fromiovec() functions [1] also modify the original iovec. 
> 
> 
> [1] http://lxr.free-electrons.com/source/net/core/iovec.c
> 

But that's the kernel's private (and user-safe) copy. I failed to find
it writing that back to user space.


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

[Xenomai-core] Re: iovec overwriting by CAN stack

[Sebastian Smolorz]

Jan Kiszka wrote:
> Sebastian Smolorz wrote:
> > Jan Kiszka wrote:
> >> Wolfgang Grandegger wrote:
> >>> Jan Kiszka wrote:
> >>>> Hi Wolfgang,
> >>>>
> >>>> it's late, so I may have misread somecode, but don't you "update" the
> >>>> iovec descriptors a user passes on send/recvmsg on return (namely
> >>>> iovec_len)? I received some complaints about this /wrt to in-kernel
> >>>> CAN stack usage.
> >>>
> >>> It's done here:
> >>>
> >>> http://www.rts.uni-hannover.de/xenomai/lxr/source/ksrc/drivers/can/rtca
> >>>n_ raw.c?v=SVN-trunk#881
> >>>
> >>> http://www.rts.uni-hannover.de/xenomai/lxr/source/ksrc/drivers/can/rtca
> >>>n_ raw.c?v=SVN-trunk#734
> >>>
> >>>
> >>> I may have missed something. What are the real complaints? Is there a
> >>> test program?
> >>
> >> Not yet (will commit the related patch to our RACK likely later today).
> >> It's simply sending frames while re-using the msg+iovec structs in a
> >> loop.
> >>
> >>>> I always considered the same well-know behaviour of RTnet a bug, but
> >>>> now I found your code is doing this systematically, also for user
> >>>> space callers. Is this behaviour undefined or even required according
> >>>> POSIX or whatever?
> >>>
> >>> I don't know, it's Sebastian's Kode.
> >>
> >> Hmm, hope he will not say that he imitated RTnet...
> >
> > Rather an imitation of the Linux kernel's behaviour. The memcpy_toiovec()
> > and memcpy_fromiovec() functions [1] also modify the original iovec.
> >
> >
> > [1] http://lxr.free-electrons.com/source/net/core/iovec.c
>
> But that's the kernel's private (and user-safe) copy. I failed to find
> it writing that back to user space.

Hm. Ad hoc, I don't recall why the userspace's iovec is also updated. Perhaps 
my intention was to make the use of rtcan_raw_sendmsg consistent for kernel 
and userspace RT-Tasks so that both can rely on finding modified iovecs after 
calling sendmsg/recvmsg. But I'm not absolutely sure if there isn't a better 
explanation for this. I will try to find the reason in my backup memory. ;-)

-- 
Sebastian

[Xenomai-core] Re: iovec overwriting by CAN stack

[Jan Kiszka]

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

Sebastian Smolorz wrote:
> Jan Kiszka wrote:
>> Sebastian Smolorz wrote:
>>> Jan Kiszka wrote:
>>>> Wolfgang Grandegger wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Hi Wolfgang,
>>>>>>
>>>>>> it's late, so I may have misread somecode, but don't you "update" the
>>>>>> iovec descriptors a user passes on send/recvmsg on return (namely
>>>>>> iovec_len)? I received some complaints about this /wrt to in-kernel
>>>>>> CAN stack usage.
>>>>> It's done here:
>>>>>
>>>>> http://www.rts.uni-hannover.de/xenomai/lxr/source/ksrc/drivers/can/rtca
>>>>> n_ raw.c?v=SVN-trunk#881
>>>>>
>>>>> http://www.rts.uni-hannover.de/xenomai/lxr/source/ksrc/drivers/can/rtca
>>>>> n_ raw.c?v=SVN-trunk#734
>>>>>
>>>>>
>>>>> I may have missed something. What are the real complaints? Is there a
>>>>> test program?
>>>> Not yet (will commit the related patch to our RACK likely later today).
>>>> It's simply sending frames while re-using the msg+iovec structs in a
>>>> loop.
>>>>
>>>>>> I always considered the same well-know behaviour of RTnet a bug, but
>>>>>> now I found your code is doing this systematically, also for user
>>>>>> space callers. Is this behaviour undefined or even required according
>>>>>> POSIX or whatever?
>>>>> I don't know, it's Sebastian's Kode.
>>>> Hmm, hope he will not say that he imitated RTnet...
>>> Rather an imitation of the Linux kernel's behaviour. The memcpy_toiovec()
>>> and memcpy_fromiovec() functions [1] also modify the original iovec.
>>>
>>>
>>> [1] http://lxr.free-electrons.com/source/net/core/iovec.c
>> But that's the kernel's private (and user-safe) copy. I failed to find
>> it writing that back to user space.
> 
> Hm. Ad hoc, I don't recall why the userspace's iovec is also updated. Perhaps 
> my intention was to make the use of rtcan_raw_sendmsg consistent for kernel 
> and userspace RT-Tasks so that both can rely on finding modified iovecs after 
> calling sendmsg/recvmsg. But I'm not absolutely sure if there isn't a better 
> explanation for this. I will try to find the reason in my backup memory. ;-)
> 

So, unless we find some reason why we _must_ modify user's iovec, either
for send or receive, I would say: kill the user copy-back and work in
both case (kernel / user space caller) on the local copy of iovec (if we
still need to modify it all then).

Jan


PS: I just committed this fix (only a problem for in-kernel usage)

--- ksrc/drivers/can/rtcan_raw.c        (Revision 2340)
+++ ksrc/drivers/can/rtcan_raw.c        (Arbeitskopie)
@@ -334,7 +334,7 @@ static int rtcan_raw_setsockopt(struct r
                    return -EFAULT;
                }
            } else
-               memcpy(flist, so->optval, so->optlen);
+               memcpy(flist->flist, so->optval, so->optlen);
            flist->flistlen = flistlen;
        }


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

[Xenomai-core] Re: iovec overwriting by CAN stack

[Sebastian Smolorz]

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

Jan Kiszka wrote:
> Sebastian Smolorz wrote:
> > Jan Kiszka wrote:
> >> Sebastian Smolorz wrote:
> >>> Jan Kiszka wrote:
> >>>> Wolfgang Grandegger wrote:
> >>>>> Jan Kiszka wrote:
> >>>>>> Hi Wolfgang,
> >>>>>>
> >>>>>> it's late, so I may have misread somecode, but don't you "update"
> >>>>>> the iovec descriptors a user passes on send/recvmsg on return
> >>>>>> (namely iovec_len)? I received some complaints about this /wrt to
> >>>>>> in-kernel CAN stack usage.
> >>>>>
> >>>>> It's done here:
> >>>>>
> >>>>> http://www.rts.uni-hannover.de/xenomai/lxr/source/ksrc/drivers/can/rt
> >>>>>ca n_ raw.c?v=SVN-trunk#881
> >>>>>
> >>>>> http://www.rts.uni-hannover.de/xenomai/lxr/source/ksrc/drivers/can/rt
> >>>>>ca n_ raw.c?v=SVN-trunk#734
> >>>>>
> >>>>>
> >>>>> I may have missed something. What are the real complaints? Is there a
> >>>>> test program?
> >>>>
> >>>> Not yet (will commit the related patch to our RACK likely later
> >>>> today). It's simply sending frames while re-using the msg+iovec
> >>>> structs in a loop.
> >>>>
> >>>>>> I always considered the same well-know behaviour of RTnet a bug, but
> >>>>>> now I found your code is doing this systematically, also for user
> >>>>>> space callers. Is this behaviour undefined or even required
> >>>>>> according POSIX or whatever?
> >>>>>
> >>>>> I don't know, it's Sebastian's Kode.
> >>>>
> >>>> Hmm, hope he will not say that he imitated RTnet...
> >>>
> >>> Rather an imitation of the Linux kernel's behaviour. The
> >>> memcpy_toiovec() and memcpy_fromiovec() functions [1] also modify the
> >>> original iovec.
> >>>
> >>>
> >>> [1] http://lxr.free-electrons.com/source/net/core/iovec.c
> >>
> >> But that's the kernel's private (and user-safe) copy. I failed to find
> >> it writing that back to user space.
> >
> > Hm. Ad hoc, I don't recall why the userspace's iovec is also updated.
> > Perhaps my intention was to make the use of rtcan_raw_sendmsg consistent
> > for kernel and userspace RT-Tasks so that both can rely on finding
> > modified iovecs after calling sendmsg/recvmsg. But I'm not absolutely
> > sure if there isn't a better explanation for this. I will try to find the
> > reason in my backup memory. ;-)
>
> So, unless we find some reason why we _must_ modify user's iovec, either
> for send or receive, I would say: kill the user copy-back and work in
> both case (kernel / user space caller) on the local copy of iovec (if we
> still need to modify it all then).

Attached is a patch which leaves the caller's iovec unmodified. It compiles 
successfully (not tested further).

-- 
Sebastian

[-- Attachment #2: rtcan_iovec.patch --]
[-- Type: text/x-diff, Size: 2135 bytes --]

Index: ChangeLog
===================================================================
--- ChangeLog	(Revision 2348)
+++ ChangeLog	(Arbeitskopie)
@@ -1,3 +1,8 @@
+2007-04-02  Sebastian Smolorz <ssm@domain.hid>
+
+	* ksrc/drivers/can/rtcan_raw.c (rtcan_raw_recvmsg, rtcan_raw_sendmsg):
+	Remove modification of caller's original iovec.
+
 2007-03-31  Philippe Gerum  <rpm@xenomai.org>
 
 	* include/nucleus/queue.h (moveq): Fix source tail pointer.
Index: ksrc/drivers/can/rtcan_raw.c
===================================================================
--- ksrc/drivers/can/rtcan_raw.c	(Revision 2348)
+++ ksrc/drivers/can/rtcan_raw.c	(Arbeitskopie)
@@ -731,13 +731,6 @@ ssize_t rtcan_raw_recvmsg(struct rtdm_de
         if (rtdm_copy_to_user(user_info, iov->iov_base, &frame,
 			      sizeof(can_frame_t)))
             return -EFAULT;
-        /* Adjust iovec in the common way */
-        iov->iov_base += sizeof(can_frame_t);
-        iov->iov_len -= sizeof(can_frame_t);
-        /* ... and copy it, too. */
-        if (rtdm_copy_to_user(user_info, msg->msg_iov, iov,
-			      sizeof(struct iovec)))
-            return -EFAULT;
 
         /* Copy timestamp if existent and wanted */
         if (msg->msg_controllen) {
@@ -762,9 +755,6 @@ ssize_t rtcan_raw_recvmsg(struct rtdm_de
 
         /* Copy CAN frame */
         memcpy(iov->iov_base, &frame, sizeof(can_frame_t));
-        /* Adjust iovec in the common way */
-        iov->iov_base += sizeof(can_frame_t);
-        iov->iov_len -= sizeof(can_frame_t);
 
         /* Copy timestamp if existent and wanted */
         if (msg->msg_controllen) {
@@ -878,16 +868,6 @@ ssize_t rtcan_raw_sendmsg(struct rtdm_de
         frame = &frame_buf;
     }
 
-    /* Adjust iovec in the common way */
-    iov->iov_base += sizeof(can_frame_t);
-    iov->iov_len -= sizeof(can_frame_t);
-    /* ... and copy it back to userspace if necessary */
-    if (user_info) {
-        if (rtdm_copy_to_user(user_info, msg->msg_iov, iov,
-			      sizeof(struct iovec)))
-            return -EFAULT;
-    }
-
     /* At last, we've got the frame ... */
 
     /* Check if DLC between 0 and 15 */

[Xenomai-core] Re: iovec overwriting by CAN stack

[Jan Kiszka]

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

Sebastian Smolorz wrote:
> Jan Kiszka wrote:
>> Sebastian Smolorz wrote:
>>> Jan Kiszka wrote:
>>>> Sebastian Smolorz wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Wolfgang Grandegger wrote:
>>>>>>> Jan Kiszka wrote:
>>>>>>>> Hi Wolfgang,
>>>>>>>>
>>>>>>>> it's late, so I may have misread somecode, but don't you "update"
>>>>>>>> the iovec descriptors a user passes on send/recvmsg on return
>>>>>>>> (namely iovec_len)? I received some complaints about this /wrt to
>>>>>>>> in-kernel CAN stack usage.
>>>>>>> It's done here:
>>>>>>>
>>>>>>> http://www.rts.uni-hannover.de/xenomai/lxr/source/ksrc/drivers/can/rt
>>>>>>> ca n_ raw.c?v=SVN-trunk#881
>>>>>>>
>>>>>>> http://www.rts.uni-hannover.de/xenomai/lxr/source/ksrc/drivers/can/rt
>>>>>>> ca n_ raw.c?v=SVN-trunk#734
>>>>>>>
>>>>>>>
>>>>>>> I may have missed something. What are the real complaints? Is there a
>>>>>>> test program?
>>>>>> Not yet (will commit the related patch to our RACK likely later
>>>>>> today). It's simply sending frames while re-using the msg+iovec
>>>>>> structs in a loop.
>>>>>>
>>>>>>>> I always considered the same well-know behaviour of RTnet a bug, but
>>>>>>>> now I found your code is doing this systematically, also for user
>>>>>>>> space callers. Is this behaviour undefined or even required
>>>>>>>> according POSIX or whatever?
>>>>>>> I don't know, it's Sebastian's Kode.
>>>>>> Hmm, hope he will not say that he imitated RTnet...
>>>>> Rather an imitation of the Linux kernel's behaviour. The
>>>>> memcpy_toiovec() and memcpy_fromiovec() functions [1] also modify the
>>>>> original iovec.
>>>>>
>>>>>
>>>>> [1] http://lxr.free-electrons.com/source/net/core/iovec.c
>>>> But that's the kernel's private (and user-safe) copy. I failed to find
>>>> it writing that back to user space.
>>> Hm. Ad hoc, I don't recall why the userspace's iovec is also updated.
>>> Perhaps my intention was to make the use of rtcan_raw_sendmsg consistent
>>> for kernel and userspace RT-Tasks so that both can rely on finding
>>> modified iovecs after calling sendmsg/recvmsg. But I'm not absolutely
>>> sure if there isn't a better explanation for this. I will try to find the
>>> reason in my backup memory. ;-)
>> So, unless we find some reason why we _must_ modify user's iovec, either
>> for send or receive, I would say: kill the user copy-back and work in
>> both case (kernel / user space caller) on the local copy of iovec (if we
>> still need to modify it all then).
> 
> Attached is a patch which leaves the caller's iovec unmodified. It compiles 
> successfully (not tested further).
> 
> 
> 
> ------------------------------------------------------------------------
> 
> Index: ChangeLog
> ===================================================================
> --- ChangeLog	(Revision 2348)
> +++ ChangeLog	(Arbeitskopie)
> @@ -1,3 +1,8 @@
> +2007-04-02  Sebastian Smolorz <ssm@domain.hid>
> +
> +	* ksrc/drivers/can/rtcan_raw.c (rtcan_raw_recvmsg, rtcan_raw_sendmsg):
> +	Remove modification of caller's original iovec.
> +
>  2007-03-31  Philippe Gerum  <rpm@xenomai.org>
>  
>  	* include/nucleus/queue.h (moveq): Fix source tail pointer.
> Index: ksrc/drivers/can/rtcan_raw.c
> ===================================================================
> --- ksrc/drivers/can/rtcan_raw.c	(Revision 2348)
> +++ ksrc/drivers/can/rtcan_raw.c	(Arbeitskopie)
> @@ -731,13 +731,6 @@ ssize_t rtcan_raw_recvmsg(struct rtdm_de
>          if (rtdm_copy_to_user(user_info, iov->iov_base, &frame,
>  			      sizeof(can_frame_t)))
>              return -EFAULT;
> -        /* Adjust iovec in the common way */
> -        iov->iov_base += sizeof(can_frame_t);
> -        iov->iov_len -= sizeof(can_frame_t);
> -        /* ... and copy it, too. */
> -        if (rtdm_copy_to_user(user_info, msg->msg_iov, iov,
> -			      sizeof(struct iovec)))
> -            return -EFAULT;
>  
>          /* Copy timestamp if existent and wanted */
>          if (msg->msg_controllen) {
> @@ -762,9 +755,6 @@ ssize_t rtcan_raw_recvmsg(struct rtdm_de
>  
>          /* Copy CAN frame */
>          memcpy(iov->iov_base, &frame, sizeof(can_frame_t));
> -        /* Adjust iovec in the common way */
> -        iov->iov_base += sizeof(can_frame_t);
> -        iov->iov_len -= sizeof(can_frame_t);
>  
>          /* Copy timestamp if existent and wanted */
>          if (msg->msg_controllen) {
> @@ -878,16 +868,6 @@ ssize_t rtcan_raw_sendmsg(struct rtdm_de
>          frame = &frame_buf;
>      }
>  
> -    /* Adjust iovec in the common way */
> -    iov->iov_base += sizeof(can_frame_t);
> -    iov->iov_len -= sizeof(can_frame_t);
> -    /* ... and copy it back to userspace if necessary */
> -    if (user_info) {
> -        if (rtdm_copy_to_user(user_info, msg->msg_iov, iov,
> -			      sizeof(struct iovec)))
> -            return -EFAULT;
> -    }
> -
>      /* At last, we've got the frame ... */
>  
>      /* Check if DLC between 0 and 15 */


Basic ACK (as still no one came up remarking we actually need the update
of iovec).

I would just always use struct iovec iov_buf for the internal handling,
means always copy the original (also from in-kernel users) and simply
kill the struct iovec *iov indirection.

Jan


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

Re: [Xenomai-core] iovec overwriting by CAN stack

[Sebastian Smolorz]

Jan Kiszka wrote:
> Sebastian Smolorz wrote:
> > Jan Kiszka wrote:
> >>
> >> So, unless we find some reason why we _must_ modify user's iovec, either
> >> for send or receive, I would say: kill the user copy-back and work in
> >> both case (kernel / user space caller) on the local copy of iovec (if we
> >> still need to modify it all then).
> >
> > Attached is a patch which leaves the caller's iovec unmodified. It
> > compiles successfully (not tested further).
> >
> >
> >
> > ------------------------------------------------------------------------
> >
> > Index: ChangeLog
> > ===================================================================
> > --- ChangeLog	(Revision 2348)
> > +++ ChangeLog	(Arbeitskopie)
> > @@ -1,3 +1,8 @@
> > +2007-04-02  Sebastian Smolorz <ssm@domain.hid>
> > +
> > +	* ksrc/drivers/can/rtcan_raw.c (rtcan_raw_recvmsg, rtcan_raw_sendmsg):
> > +	Remove modification of caller's original iovec.
> > +
> >  2007-03-31  Philippe Gerum  <rpm@xenomai.org>
> >
> >  	* include/nucleus/queue.h (moveq): Fix source tail pointer.
> > Index: ksrc/drivers/can/rtcan_raw.c
> > ===================================================================
> > --- ksrc/drivers/can/rtcan_raw.c	(Revision 2348)
> > +++ ksrc/drivers/can/rtcan_raw.c	(Arbeitskopie)
> > @@ -731,13 +731,6 @@ ssize_t rtcan_raw_recvmsg(struct rtdm_de
> >          if (rtdm_copy_to_user(user_info, iov->iov_base, &frame,
> >  			      sizeof(can_frame_t)))
> >              return -EFAULT;
> > -        /* Adjust iovec in the common way */
> > -        iov->iov_base += sizeof(can_frame_t);
> > -        iov->iov_len -= sizeof(can_frame_t);
> > -        /* ... and copy it, too. */
> > -        if (rtdm_copy_to_user(user_info, msg->msg_iov, iov,
> > -			      sizeof(struct iovec)))
> > -            return -EFAULT;
> >
> >          /* Copy timestamp if existent and wanted */
> >          if (msg->msg_controllen) {
> > @@ -762,9 +755,6 @@ ssize_t rtcan_raw_recvmsg(struct rtdm_de
> >
> >          /* Copy CAN frame */
> >          memcpy(iov->iov_base, &frame, sizeof(can_frame_t));
> > -        /* Adjust iovec in the common way */
> > -        iov->iov_base += sizeof(can_frame_t);
> > -        iov->iov_len -= sizeof(can_frame_t);
> >
> >          /* Copy timestamp if existent and wanted */
> >          if (msg->msg_controllen) {
> > @@ -878,16 +868,6 @@ ssize_t rtcan_raw_sendmsg(struct rtdm_de
> >          frame = &frame_buf;
> >      }
> >
> > -    /* Adjust iovec in the common way */
> > -    iov->iov_base += sizeof(can_frame_t);
> > -    iov->iov_len -= sizeof(can_frame_t);
> > -    /* ... and copy it back to userspace if necessary */
> > -    if (user_info) {
> > -        if (rtdm_copy_to_user(user_info, msg->msg_iov, iov,
> > -			      sizeof(struct iovec)))
> > -            return -EFAULT;
> > -    }
> > -
> >      /* At last, we've got the frame ... */
> >
> >      /* Check if DLC between 0 and 15 */
>
> Basic ACK (as still no one came up remarking we actually need the update
> of iovec).
>
> I would just always use struct iovec iov_buf for the internal handling,
> means always copy the original (also from in-kernel users) and simply
> kill the struct iovec *iov indirection.

IMHO this is not necessary because the original iovec is not modified 
unexpectedly even if it resides in kernel space.

-- 
Sebastian

Re: [Xenomai-core] iovec overwriting by CAN stack

[Jan Kiszka]

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

Sebastian Smolorz wrote:
> Jan Kiszka wrote:
>> Sebastian Smolorz wrote:
>>> Jan Kiszka wrote:
>>>> So, unless we find some reason why we _must_ modify user's iovec, either
>>>> for send or receive, I would say: kill the user copy-back and work in
>>>> both case (kernel / user space caller) on the local copy of iovec (if we
>>>> still need to modify it all then).
>>> Attached is a patch which leaves the caller's iovec unmodified. It
>>> compiles successfully (not tested further).
>>>
>>>
>>>
>>> ------------------------------------------------------------------------
>>>
>>> Index: ChangeLog
>>> ===================================================================
>>> --- ChangeLog	(Revision 2348)
>>> +++ ChangeLog	(Arbeitskopie)
>>> @@ -1,3 +1,8 @@
>>> +2007-04-02  Sebastian Smolorz <ssm@domain.hid>
>>> +
>>> +	* ksrc/drivers/can/rtcan_raw.c (rtcan_raw_recvmsg, rtcan_raw_sendmsg):
>>> +	Remove modification of caller's original iovec.
>>> +
>>>  2007-03-31  Philippe Gerum  <rpm@xenomai.org>
>>>
>>>  	* include/nucleus/queue.h (moveq): Fix source tail pointer.
>>> Index: ksrc/drivers/can/rtcan_raw.c
>>> ===================================================================
>>> --- ksrc/drivers/can/rtcan_raw.c	(Revision 2348)
>>> +++ ksrc/drivers/can/rtcan_raw.c	(Arbeitskopie)
>>> @@ -731,13 +731,6 @@ ssize_t rtcan_raw_recvmsg(struct rtdm_de
>>>          if (rtdm_copy_to_user(user_info, iov->iov_base, &frame,
>>>  			      sizeof(can_frame_t)))
>>>              return -EFAULT;
>>> -        /* Adjust iovec in the common way */
>>> -        iov->iov_base += sizeof(can_frame_t);
>>> -        iov->iov_len -= sizeof(can_frame_t);
>>> -        /* ... and copy it, too. */
>>> -        if (rtdm_copy_to_user(user_info, msg->msg_iov, iov,
>>> -			      sizeof(struct iovec)))
>>> -            return -EFAULT;
>>>
>>>          /* Copy timestamp if existent and wanted */
>>>          if (msg->msg_controllen) {
>>> @@ -762,9 +755,6 @@ ssize_t rtcan_raw_recvmsg(struct rtdm_de
>>>
>>>          /* Copy CAN frame */
>>>          memcpy(iov->iov_base, &frame, sizeof(can_frame_t));
>>> -        /* Adjust iovec in the common way */
>>> -        iov->iov_base += sizeof(can_frame_t);
>>> -        iov->iov_len -= sizeof(can_frame_t);
>>>
>>>          /* Copy timestamp if existent and wanted */
>>>          if (msg->msg_controllen) {
>>> @@ -878,16 +868,6 @@ ssize_t rtcan_raw_sendmsg(struct rtdm_de
>>>          frame = &frame_buf;
>>>      }
>>>
>>> -    /* Adjust iovec in the common way */
>>> -    iov->iov_base += sizeof(can_frame_t);
>>> -    iov->iov_len -= sizeof(can_frame_t);
>>> -    /* ... and copy it back to userspace if necessary */
>>> -    if (user_info) {
>>> -        if (rtdm_copy_to_user(user_info, msg->msg_iov, iov,
>>> -			      sizeof(struct iovec)))
>>> -            return -EFAULT;
>>> -    }
>>> -
>>>      /* At last, we've got the frame ... */
>>>
>>>      /* Check if DLC between 0 and 15 */
>> Basic ACK (as still no one came up remarking we actually need the update
>> of iovec).
>>
>> I would just always use struct iovec iov_buf for the internal handling,
>> means always copy the original (also from in-kernel users) and simply
>> kill the struct iovec *iov indirection.
> 
> IMHO this is not necessary because the original iovec is not modified 
> unexpectedly even if it resides in kernel space.
> 

It as almost noise, my idea was just to save the additional indirections
(iov->xxx vs. iov_buf.xxx) for the succeeding accesses.


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