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

[Jan Kiszka <jan.kiszka@domain.hid>]

[-- 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 --]