reconsider k_queue APIs

by Andrew Boie:
(1:38:29 PM) andrew.j.ross: Dunno, can't we just say that k_queue (which is just what we used to call fifo/lifo) is only usable from a non-protected context?  If we have a "queue" of stuff that we have to copy in and out instead of linking directly, we should be using something like a "k_pipe".
(1:54:28 PM) apboie: andrew.j.ross: that is entirely reasonable, we could just say k_queue is a supervisor-only API
(1:55:23 PM) apboie: but consider this use-case: the two application threads can both read/write toplevel globals, they don't need to copy the data back and forth, the issue is just to protect the private kernel data that is currently attached to the buffers themselves
(1:55:43 PM) apboie: pipe is a heavyweight alternative if I understand correctly as the data itself is entirely copied
(1:56:15 PM) apboie: in other words it's OK for the app threads to have these shared buffers, just get the kernel housekeeping located somewhere else
(1:56:46 PM) apboie: we are gonna need allocators for kernel objects anyway: GH-3624
(1:57:01 PM) andrew.j.ross: In a protected context everything needs to be copied by definition though.  If you're enqueuing stuff that is in shared memory, then surely k_queue would work.
(1:57:14 PM) andrew.j.ross: Or am I misunderstanding your use case?
(1:57:15 PM) apboie: i don't think i am explaining myself well
(1:57:35 PM) apboie: the buffers are in memory shared by all the app threads, no issue there
(1:58:05 PM) apboie: the problem is that the kernel objects are currently inside these buffers and they need to be walled off from userspace since corruption or malice to them could crash the kernel
(1:58:23 PM) apboie: we can keep the buffers where they are if the app wants to use them to exchange data between threads
(1:58:40 PM) apboie: but the embedded kernel objects need to go somewhere else
(1:59:34 PM) apboie: i.e. not embedded within the buffers
(1:59:59 PM) andrew.j.ross: OK, so it's the "the first four bytes of an enqueued object are a kernel-only pointer" that you're trying to fix?
(2:00:16 PM) apboie: its not a pointer is the struct k_queue itself..
(2:00:55 PM) apboie: unless I'm looking at the code wrong
(2:01:11 PM) apboie: if the buffers just have a pointer value in them its no problem we will have a mechanism to validate those anyway
(2:01:12 PM) andrew.j.ross: But that can stay in kernel memory, though.  All the user thread needs to see is a handle to it.  Or maybe I'm still not understanding.  The contents of struct k_queue is just a wait_q and the list head.
(2:01:58 PM) andrew.j.ross: Or you want a hybrid where you can enumerate a list without context switch but still hit the kernel for blocking? 
(2:02:08 PM) andrew.j.ross: Sorry, "syscall", not "context switch"
(2:02:33 PM) apboie: andrew.j.ross: it may be that i'm confused and overthinking this, if the buffer just has a pointer to the kobject and not the kobject itself we are totally fine
(2:04:46 PM) andrew.j.ross: I guess the test I would use is "can all the k_queue_*() APIs be implemented by a syscall into kernel space?"  and AFAICT it meets it.  Maybe not as fast as you'd like for the case of e.g. removing the head of a non-empty FIFO.  But it would work.
(2:06:28 PM) apboie: andrew.j.ross: the space used by the kernel at the beginning of the data buffer is a sys_snode_t and not a pointer to the kernel object
(2:06:41 PM) apboie: basically every data buffer is acting as a linked list node
(2:07:06 PM) apboie: we need to either be able to validate the integrity of these nodes or put them somewhere else
(2:07:29 PM) apboie: a malicious thread could corrupt the slist in the buffer and cause the kernel to explode when it walks the linked list
(2:08:27 PM) apboie: what i'm trying to enforce here is that userland can't make the kernel blow up by messing with kernel's bookkeeping data and with k_queue I think we have a gap here
(2:08:28 PM) andrew.j.ross: Ah, OK.  Gotcha.  Yes, if you want to protect the pointers then the whole thing needs to be in kernel space, which means that you need a separately-allocated "list node" that lives external to the struct.  Which means either using a heap/mem_pool or equivalent for everything we're currently using fifo's for.  Which sounds bad.
(2:08:47 PM) ***andrew.j.ross thinks this comes straight back to "don't use fifo/lifo's from user space"
(2:08:57 PM) apboie: andrew.j.ross: 2 things
(2:09:30 PM) apboie: 1. forbidding fifo/lifos from userspace is bad because then you will have to do a full buffer copy to exchange data when really we just need to protect the kernel lists
(2:10:26 PM) apboie: 2. having a heap/mem-pool for kernel objects is something we already have to have anyway. userland can't create their own kernel objects anymore, they will either need to use special toplevel macros to instantiate them, or for every kernel object type have alloc/free APIs to create them from some kernel slab heap
(2:10:55 PM) ***andrew.j.ross was indeed hoping we could get away with static macro-assisted allocation, actually.
(2:11:39 PM) andrew.j.ross: I'm not saying we don't want a kernel heap, but we don't want to tie core APIs to a heap either.
(2:11:48 PM) apboie: these objects have to get created somehow
(2:12:09 PM) apboie: we can try just static allocation but I suspect it will be an inconvenience to users
(2:15:40 PM) apboie: we don't need to modify the core APIs, they take kernel object pointer as parameter, its just a matter of, where do we get the kernel object from? a toplevel macro is one way to do it, and an API to alloc off a special heap for kernel objects would be an alternative.
(2:16:51 PM) apboie: the k_queue thing is a special case because in addition to the k_queue itself, we have these linked list nodes currently in the buffers. i can see your point that this would tie the k_queue API to an allocator.
(2:17:02 PM) apboie: so yeah it's gonna require some extra thought. :(
zephyriot / zephyr-issues

reconsider k_queue APIs #2073
