net/socket: Fix sendmsg/recvmsg user buffers for BUILD_KERNEL (fixes CAN corruption)#18712
net/socket: Fix sendmsg/recvmsg user buffers for BUILD_KERNEL (fixes CAN corruption)#18712arjav1528 wants to merge 2 commits intoapache:masterfrom
Conversation
|
@xiaoxiang781216 @jerpelea coul you please review it |
michallenc
left a comment
There was a problem hiding this comment.
I don't like this much specific code for CONFIG_BUILD_KERNEL. Maybe better approach would be to implement something like copy_to_user and copy_from_user, similar to what Linux uses?
When CONFIG_BUILD_KERNEL is enabled, user-space iovec bases and ancillary pointers must not be dereferenced from the network stack. Mirror the existing sendto() bounce-buffer approach: copy the msghdr fields, iovec array, and payload into kernel memory before calling psock_sendmsg(). This fixes corrupted CAN frames (and similar protocols) where data was copied via memcpy from user VAs during devif_send/iob_trycopyin. Signed-off-by: Arjav Patel <arjav1528@gmail.com>
When CONFIG_BUILD_KERNEL is enabled, copy the msghdr iovec snapshot, receive into kernel heap buffers via psock_recvmsg(), then copy payload, sockaddr, and ancillary data back to the user msghdr. This matches recvfrom() and keeps the network stack from writing to user virtual addresses from kernel context. Signed-off-by: Arjav Patel <arjav1528@gmail.com>
6a41f91 to
8563706
Compare
@michallenc I agree that a generic copy_to_user/copy_from_user style could be useful longer term. but the repo does not appear to have that abstraction, and sendto/recvfrom fns already handle CONFIG_BUILD_KERNEL by bouncing in the kernel buffers. it seems consistent with those existing socket wrappers and avoids expanding the scope of this PR |
|
A quick idea / opinion: I believe what is being done here is basically right, and analysis of the issue looks correct. There is, imho, an issue with the blocking socket functions. A wrong addrenv might be used by the underlying driver while copying the data. This is just sometimes saved by iob copying with readahead etc.... The copying between kernel and user buffers is anyhow specific to struct msghdr , but perhaps the CONFIG_BUILD_KERNEL specific stuff should be isolated to a separate file and just have minimal calls in recvmsg/sendmsg, before and after "psock_sendmsg / psock_recvmsg" surrounded by "ifdef CONFIG_BUILD_KERNEL". Also, maybe we should try to use IOBs for kernel side buffering instead of allocating/freeing from heap every time? |
|
Could you please add the logs from your tests? |
…r CONFIG_BUILD_KERNEL This adds copying to/from kerenel memory when doing sendmsg/recvmsg to avoid corrupting messages or other process' address environment, when the actual device drivers do copy while the calling process is blocked. This patch allocates one IOB for storing the metadata and the payload. If the payload doesn't fit in the IOB, the payload is allocated from the kernel heap. In other words, to optimize number of heap allocations, one can increase the IOB size so that all the packets fit in one IOB. This needs to be reverted when there is similar fix in upstream; the work is in progress in apache#18712 Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
…r CONFIG_BUILD_KERNEL This adds copying to/from kerenel memory when doing sendmsg/recvmsg to avoid corrupting messages or other process' address environment, when the actual device drivers do copy while the calling process is blocked. This patch allocates one IOB for storing the metadata and the payload. If the payload doesn't fit in the IOB, the payload is allocated from the kernel heap. In other words, to optimize number of heap allocations, one can increase the IOB size so that all the packets fit in one IOB. This needs to be reverted when there is similar fix in upstream; the work is in progress in apache#18712 Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
This adds copying to/from kerenel memory when doing sendmsg/recvmsg to avoid corrupting messages or other process' address environment, when the actual device drivers do copy while the calling process is blocked. This patch allocates one IOB for storing the metadata and the payload. If the payload doesn't fit in the IOB, the payload is allocated from the kernel heap. In other words, to optimize number of heap allocations, one can increase the IOB size so that all the packets fit in one IOB. This needs to be reverted when there is similar fix in upstream; the work is in progress in apache#18712 Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
This adds copying to/from kerenel memory when doing sendmsg/recvmsg to avoid corrupting messages or other process' address environment, when the actual device drivers do copy while the calling process is blocked. This patch allocates one IOB for storing the metadata and the payload. If the payload doesn't fit in the IOB, the payload is allocated from the kernel heap. In other words, to optimize number of heap allocations, one can increase the IOB size so that all the packets fit in one IOB. This needs to be reverted when there is similar fix in upstream; the work is in progress in apache#18712 Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
This adds copying to/from kerenel memory when doing sendmsg/recvmsg to avoid corrupting messages or other process' address environment, when the actual device drivers do copy while the calling process is blocked. This patch allocates one IOB for storing the metadata and the payload. If the payload doesn't fit in the IOB, the payload is allocated from the kernel heap. In other words, to optimize number of heap allocations, one can increase the IOB size so that all the packets fit in one IOB. This needs to be reverted when there is similar fix in upstream; the work is in progress in apache#18712 Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
|
Sorry for the noise, I didn't mean to link the "tiiuae" PR to this one, github does that automatically. That PR was some WIP prototyping for the same issue on top of older nuttx release, please don't let that disturb discussion in here. |
This adds copying to/from kerenel memory when doing sendmsg/recvmsg to avoid corrupting messages or other process' address environment, when the actual device drivers do copy while the calling process is blocked. This patch allocates one IOB for storing the metadata and the payload. If the payload doesn't fit in the IOB, the payload is allocated from the kernel heap. In other words, to optimize number of heap allocations, one can increase the IOB size so that all the packets fit in one IOB. This needs to be reverted when there is similar fix in upstream; the work is in progress in apache#18712 Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
This adds copying to/from kerenel memory when doing sendmsg/recvmsg to avoid corrupting messages or other process' address environment, when the actual device drivers do copy while the calling process is blocked. This patch allocates one IOB for storing the metadata and the payload. If the payload doesn't fit in the IOB, the payload is allocated from the kernel heap. In other words, to optimize number of heap allocations, one can increase the IOB size so that all the packets fit in one IOB. This needs to be reverted when there is similar fix in upstream; the work is in progress in apache#18712 Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
This adds copying to/from kerenel memory when doing sendmsg/recvmsg to avoid corrupting messages or other process' address environment, when the actual device drivers do copy while the calling process is blocked. If a large enough IOB is available for storing the metadata and the payload, it will be reserevd and used. If there is no IOB available, the data is allocated from kernel heap. In other words, to optimize number of heap allocations, one can increase the IOB size so that all the packets fit in one IOB, to avoid heap allocations. This needs to be reverted when there is similar fix in upstream; the work is in progress in apache#18712 Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
This adds copying to/from kerenel memory when doing sendmsg/recvmsg to avoid corrupting messages or other process' address environment, when the actual device drivers do copy while the calling process is blocked. If a large enough IOB is available for storing the metadata and the payload, it will be reserevd and used. If there is no IOB available, the data is allocated from kernel heap. In other words, to optimize number of heap allocations, one can increase the IOB size so that all the packets fit in one IOB, to avoid heap allocations. This needs to be reverted when there is similar fix in upstream; the work is in progress in apache#18712 Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
This adds copying to/from kerenel memory when doing sendmsg/recvmsg to avoid corrupting messages or other process' address environment, when the actual device drivers do copy while the calling process is blocked. If a large enough IOB is available for storing the metadata and the payload, it will be reserevd and used. If there is no IOB available, the data is allocated from kernel heap. In other words, to optimize number of heap allocations, one can increase the IOB size so that all the packets fit in one IOB, to avoid heap allocations. This needs to be reverted when there is similar fix in upstream; the work is in progress in apache#18712 Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
This adds copying to/from kerenel memory when doing sendmsg/recvmsg to avoid corrupting messages or other process' address environment, when the actual device drivers do copy while the calling process is blocked. If a large enough IOB is available for storing the metadata and the payload, it will be reserevd and used. If there is no IOB available, the data is allocated from kernel heap. In other words, to optimize number of heap allocations, one can increase the IOB size so that all the packets fit in one IOB, to avoid heap allocations. This needs to be reverted when there is similar fix in upstream; the work is in progress in apache#18712 Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
This adds copying to/from kerenel memory when doing sendmsg/recvmsg to avoid corrupting messages or another process' address environment when device drivers perform copying while the calling process is blocked. If a large enough IOB is available for storing metadata and the payload, it will be reserved and used. If no IOB is available, data is allocated from th kernel heap. In other words, to optimize number of heap allocations, one can increase the IOB size so that all the packets fit in one IOB, to avoid heap allocations. This needs to be reverted when a similar fix lands upstream; the work is in progress in apache#18712 Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
This adds copying to/from kerenel memory when doing sendmsg/recvmsg to avoid corrupting messages or another process' address environment when device drivers perform copying while the calling process is blocked. If a large enough IOB is available for storing metadata and the payload, it will be reserved and used. If no IOB is available, data is allocated from th kernel heap. In other words, to optimize number of heap allocations, one can increase the IOB size so that all the packets fit in one IOB, to avoid heap allocations. This needs to be reverted when a similar fix lands upstream; the work is in progress in apache#18712 Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
This adds copying to/from kerenel memory when doing sendmsg/recvmsg to avoid corrupting messages or another process' address environment when device drivers perform copying while the calling process is blocked. If a large enough IOB is available for storing metadata and the payload, it will be reserved and used. If no IOB is available, data is allocated from th kernel heap. In other words, to optimize number of heap allocations, one can increase the IOB size so that all the packets fit in one IOB, to avoid heap allocations. This needs to be reverted when a similar fix lands upstream; the work is in progress in apache#18712 Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
This adds copying to/from kerenel memory when doing sendmsg/recvmsg to avoid corrupting messages or another process' address environment when device drivers perform copying while the calling process is blocked. If a large enough IOB is available for storing metadata and the payload, it will be reserved and used. If no IOB is available, data is allocated from th kernel heap. In other words, to optimize number of heap allocations, one can increase the IOB size so that all the packets fit in one IOB, to avoid heap allocations. This needs to be reverted when a similar fix lands upstream; the work is in progress in apache#18712 Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
This adds copying to/from kerenel memory when doing sendmsg/recvmsg to avoid corrupting messages or another process' address environment when device drivers perform copying while the calling process is blocked. If a large enough IOB is available for storing metadata and the payload, it will be reserved and used. If no IOB is available, data is allocated from th kernel heap. In other words, to optimize number of heap allocations, one can increase the IOB size so that all the packets fit in one IOB, to avoid heap allocations. This needs to be reverted when a similar fix lands upstream; the work is in progress in apache#18712 Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
|
I don't understand why my work in tiiuae repository automatically causes the spam in here; does anyone know a way to get rid of the spamming? I tried to close the original PRs there and remove the references, and work in another branch but github just keeps doing this. Is there a way to clean up the spam from this PR? I don't intend to make any competing implementation with this PR, we just needed a quick "own" fix for the issue for an older nuttx branch, which I made for TII (now finished, only last tests ongoing so I don't expect more spam coming because of this..). |
This adds copying to/from kerenel memory when doing sendmsg/recvmsg to avoid corrupting messages or another process' address environment when device drivers perform copying while the calling process is blocked. If a large enough IOB is available for storing metadata and the payload, it will be reserved and used. If no IOB is available, data is allocated from th kernel heap. In other words, to optimize number of heap allocations, one can increase the IOB size so that all the packets fit in one IOB, to avoid heap allocations. This needs to be reverted when a similar fix lands upstream; the work is in progress in apache#18712 Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
Summary
Bounce
sendmsg()andrecvmsg()through kernel heap buffers whenCONFIG_BUILD_KERNELis set, matchingsendto()/recvfrom(). The network stack must notmemcpyfrom user iovec bases while running in kernel context.Related
Testing
./tools/checkpatch.sh -f net/socket/sendmsg.c net/socket/recvmsg.cpasses.cansend can0 123#deadbeefrepeatedly; confirm CAN ID/DLC stay correct.