From 44fe81228df0cfdfe6e43a84227d727fef00d57c Mon Sep 17 00:00:00 2001 From: Andrew Boie Date: Thu, 12 Apr 2018 17:38:12 -0700 Subject: [PATCH] kernel: pipes: add k_pipe_alloc_init() User mode can't be trusted to provide the kernel buffers for internal use. The syscall for k_pipe_init() has been removed in favor of a new API to draw the buffer memory from the calling thread's resource pool. K_PIPE_DEFINE() now properly locates the allocated buffer into kernel memory. Signed-off-by: Andrew Boie --- include/kernel.h | 44 +++++++++++--- kernel/pipes.c | 49 +++++++++++++--- kernel/userspace.c | 3 + tests/kernel/pipe/pipe_api/prj.conf | 1 + tests/kernel/pipe/pipe_api/src/main.c | 38 ++++++++++-- .../pipe/pipe_api/src/test_pipe_contexts.c | 32 ++++++++++ .../kernel/pipe/pipe_api/src/test_pipe_fail.c | 58 +++++++++++++++---- 7 files changed, 194 insertions(+), 31 deletions(-) diff --git a/include/kernel.h b/include/kernel.h index 16747f8bebb..21a0b3a33c0 100644 --- a/include/kernel.h +++ b/include/kernel.h @@ -3347,6 +3347,8 @@ extern int k_mbox_data_block_get(struct k_mbox_msg *rx_msg, * @cond INTERNAL_HIDDEN */ +#define K_PIPE_FLAG_ALLOC BIT(0) /* Buffer was allocated */ + struct k_pipe { unsigned char *buffer; /* Pipe buffer: may be NULL */ size_t size; /* Buffer size */ @@ -3360,6 +3362,7 @@ struct k_pipe { } wait_q; _OBJECT_TRACING_NEXT_PTR(k_pipe); + u8_t flags; /* Flags */ }; #define _K_PIPE_INITIALIZER(obj, pipe_buffer, pipe_buffer_size) \ @@ -3398,11 +3401,11 @@ struct k_pipe { * or zero if no ring buffer is used. * @param pipe_align Alignment of the pipe's ring buffer (power of 2). */ -#define K_PIPE_DEFINE(name, pipe_buffer_size, pipe_align) \ - static unsigned char __noinit __aligned(pipe_align) \ - _k_pipe_buf_##name[pipe_buffer_size]; \ - struct k_pipe name \ - __in_section(_k_pipe, static, name) = \ +#define K_PIPE_DEFINE(name, pipe_buffer_size, pipe_align) \ + static unsigned char __kernel_noinit __aligned(pipe_align) \ + _k_pipe_buf_##name[pipe_buffer_size]; \ + struct k_pipe name \ + __in_section(_k_pipe, static, name) = \ _K_PIPE_INITIALIZER(name, _k_pipe_buf_##name, pipe_buffer_size) /** @@ -3418,8 +3421,35 @@ struct k_pipe { * * @return N/A */ -__syscall void k_pipe_init(struct k_pipe *pipe, unsigned char *buffer, - size_t size); +void k_pipe_init(struct k_pipe *pipe, unsigned char *buffer, size_t size); + +/** + * @brief Release a pipe's allocated buffer + * + * If a pipe object was given a dynamically allocated buffer via + * k_pipe_alloc_init(), this will free it. This function does nothing + * if the buffer wasn't dynamically allocated. + * + * @param pipe Address of the pipe. + */ +void k_pipe_cleanup(struct k_pipe *pipe); + +/** + * @brief Initialize a pipe and allocate a buffer for it + * + * Storage for the buffer region will be allocated from the calling thread's + * resource pool. This memory will be released if k_pipe_cleanup() is called, + * or userspace is enabled and the pipe object loses all references to it. + * + * This function should only be called on uninitialized pipe objects. + * + * @param pipe Address of the pipe. + * @param size Size of the pipe's ring buffer (in bytes), or zero if no ring + * buffer is used. + * @retval 0 on success + * @retval -ENOMEM if memory couln't be allocated + */ +__syscall int k_pipe_alloc_init(struct k_pipe *pipe, size_t size); /** * @brief Write data to a pipe. diff --git a/kernel/pipes.c b/kernel/pipes.c index 5b86bffc5c6..66282bf2567 100644 --- a/kernel/pipes.c +++ b/kernel/pipes.c @@ -19,6 +19,7 @@ #include #include #include +#include struct k_pipe_desc { unsigned char *buffer; /* Position in src/dest buffer */ @@ -127,31 +128,63 @@ SYS_INIT(init_pipes_module, PRE_KERNEL_1, CONFIG_KERNEL_INIT_PRIORITY_OBJECTS); #endif /* CONFIG_NUM_PIPE_ASYNC_MSGS or CONFIG_OBJECT_TRACING */ -void _impl_k_pipe_init(struct k_pipe *pipe, unsigned char *buffer, size_t size) +void k_pipe_init(struct k_pipe *pipe, unsigned char *buffer, size_t size) { pipe->buffer = buffer; pipe->size = size; pipe->bytes_used = 0; pipe->read_index = 0; pipe->write_index = 0; + pipe->flags = 0; sys_dlist_init(&pipe->wait_q.writers); sys_dlist_init(&pipe->wait_q.readers); SYS_TRACING_OBJ_INIT(k_pipe, pipe); _k_object_init(pipe); } -#ifdef CONFIG_USERSPACE -_SYSCALL_HANDLER(k_pipe_init, pipe, buffer, size) +int _impl_k_pipe_alloc_init(struct k_pipe *pipe, size_t size) { - _SYSCALL_OBJ_INIT(pipe, K_OBJ_PIPE); - _SYSCALL_MEMORY_WRITE(buffer, size); + void *buffer; + int ret; - _impl_k_pipe_init((struct k_pipe *)pipe, (unsigned char *)buffer, - size); - return 0; + if (size) { + buffer = z_thread_malloc(size); + if (buffer) { + k_pipe_init(pipe, buffer, size); + pipe->flags = K_PIPE_FLAG_ALLOC; + ret = 0; + } else { + ret = -ENOMEM; + } + } else { + k_pipe_init(pipe, NULL, 0); + ret = 0; + } + + return ret; +} + +#ifdef CONFIG_USERSPACE +_SYSCALL_HANDLER(k_pipe_alloc_init, pipe, size) +{ + _SYSCALL_OBJ_NEVER_INIT(pipe, K_OBJ_PIPE); + + return _impl_k_pipe_alloc_init((struct k_pipe *)pipe, size); } #endif +void k_pipe_cleanup(struct k_pipe *pipe) +{ + __ASSERT_NO_MSG(sys_dlist_is_empty(&pipe->wait_q.readers)); + __ASSERT_NO_MSG(sys_dlist_is_empty(&pipe->wait_q.writers)); + + if (pipe->flags & K_PIPE_FLAG_ALLOC) { + k_free(pipe->buffer); + pipe->buffer = NULL; + pipe->flags &= ~K_PIPE_FLAG_ALLOC; + } +} + /** * @brief Copy bytes from @a src to @a dest * diff --git a/kernel/userspace.c b/kernel/userspace.c index bd50b21d3ff..bcd196f25a6 100644 --- a/kernel/userspace.c +++ b/kernel/userspace.c @@ -258,6 +258,9 @@ static void unref_check(struct _k_object *ko) * specifically needs to happen depends on the object type. */ switch (ko->type) { + case K_OBJ_PIPE: + k_pipe_cleanup((struct k_pipe *)ko->name); + break; default: break; } diff --git a/tests/kernel/pipe/pipe_api/prj.conf b/tests/kernel/pipe/pipe_api/prj.conf index 29f2d64944e..928ab3b4968 100644 --- a/tests/kernel/pipe/pipe_api/prj.conf +++ b/tests/kernel/pipe/pipe_api/prj.conf @@ -1,3 +1,4 @@ CONFIG_ZTEST=y CONFIG_IRQ_OFFLOAD=y CONFIG_TEST_USERSPACE=y +CONFIG_DYNAMIC_OBJECTS=y diff --git a/tests/kernel/pipe/pipe_api/src/main.c b/tests/kernel/pipe/pipe_api/src/main.c index b534c3667e3..f777319edbd 100644 --- a/tests/kernel/pipe/pipe_api/src/main.c +++ b/tests/kernel/pipe/pipe_api/src/main.c @@ -13,28 +13,56 @@ #include extern void test_pipe_thread2thread(void); + extern void test_pipe_put_fail(void); extern void test_pipe_get_fail(void); extern void test_pipe_block_put(void); extern void test_pipe_block_put_sema(void); extern void test_pipe_get_put(void); +#ifdef CONFIG_USERSPACE +extern void test_pipe_user_thread2thread(void); +extern void test_pipe_user_put_fail(void); +extern void test_pipe_user_get_fail(void); +extern void test_resource_pool_auto_free(void); +#endif /* k objects */ extern struct k_pipe pipe, kpipe, put_get_pipe; extern struct k_sem end_sema; extern struct k_stack tstack; extern struct k_thread tdata; +extern struct k_mem_pool test_pool; + +#ifndef CONFIG_USERSPACE +#define dummy_test(_name) \ + static void _name(void) \ + { \ + ztest_test_skip(); \ + } + +dummy_test(test_pipe_user_thread2thread); +dummy_test(test_pipe_user_put_fail); +dummy_test(test_pipe_user_get_fail); +dummy_test(test_resource_pool_auto_free); +#endif /* !CONFIG_USERSPACE */ + /*test case main entry*/ void test_main(void) { - k_thread_access_grant(k_current_get(), - &kpipe, &pipe, &end_sema, &tdata, &tstack, + k_thread_access_grant(k_current_get(), &pipe, + &kpipe, &end_sema, &tdata, &tstack, &put_get_pipe, NULL); + k_thread_resource_pool_assign(k_current_get(), &test_pool); + ztest_test_suite(pipe_api, - ztest_user_unit_test(test_pipe_thread2thread), - ztest_user_unit_test(test_pipe_put_fail), - ztest_user_unit_test(test_pipe_get_fail), + ztest_unit_test(test_pipe_thread2thread), + ztest_user_unit_test(test_pipe_user_thread2thread), + ztest_user_unit_test(test_pipe_user_put_fail), + ztest_user_unit_test(test_pipe_user_get_fail), + ztest_unit_test(test_resource_pool_auto_free), + ztest_unit_test(test_pipe_put_fail), + ztest_unit_test(test_pipe_get_fail), ztest_unit_test(test_pipe_block_put), ztest_unit_test(test_pipe_block_put_sema), ztest_unit_test(test_pipe_get_put)); diff --git a/tests/kernel/pipe/pipe_api/src/test_pipe_contexts.c b/tests/kernel/pipe/pipe_api/src/test_pipe_contexts.c index 8925d86a11b..5598be11112 100644 --- a/tests/kernel/pipe/pipe_api/src/test_pipe_contexts.c +++ b/tests/kernel/pipe/pipe_api/src/test_pipe_contexts.c @@ -33,6 +33,12 @@ K_THREAD_STACK_DEFINE(tstack, STACK_SIZE); __kernel struct k_thread tdata; K_SEM_DEFINE(end_sema, 0, 1); +/* By design, only two blocks. We should never need more than that, one + * to allocate the pipe object, one for its buffer. Both should be auto- + * released when the thread exits + */ +K_MEM_POOL_DEFINE(test_pool, 128, 128, 2, 4); + static void tpipe_put(struct k_pipe *ppipe) { size_t to_wt, wt_byte = 0; @@ -127,6 +133,22 @@ void test_pipe_thread2thread(void) tpipe_thread_thread(&kpipe); } +#ifdef CONFIG_USERSPACE +void test_pipe_user_thread2thread(void) +{ + /**TESTPOINT: test k_pipe_init pipe*/ + + struct k_pipe *p = k_object_alloc(K_OBJ_PIPE); + + zassert_true(p != NULL, NULL); + zassert_false(k_pipe_alloc_init(p, PIPE_LEN), NULL); + tpipe_thread_thread(&pipe); + + /**TESTPOINT: test K_PIPE_DEFINE pipe*/ + tpipe_thread_thread(&kpipe); +} +#endif + void test_pipe_block_put(void) { @@ -172,3 +194,13 @@ void test_pipe_get_put(void) k_thread_abort(tid); } +#ifdef CONFIG_USERSPACE +void test_resource_pool_auto_free(void) +{ + /* Pool has 2 blocks, both should succeed if kernel object and pipe + * buffer are auto-freed when the allocating threads exit + */ + zassert_true(k_mem_pool_malloc(&test_pool, 64) != NULL, NULL); + zassert_true(k_mem_pool_malloc(&test_pool, 64) != NULL, NULL); +} +#endif diff --git a/tests/kernel/pipe/pipe_api/src/test_pipe_fail.c b/tests/kernel/pipe/pipe_api/src/test_pipe_fail.c index a68666aee61..046c1b067d8 100644 --- a/tests/kernel/pipe/pipe_api/src/test_pipe_fail.c +++ b/tests/kernel/pipe/pipe_api/src/test_pipe_fail.c @@ -24,38 +24,74 @@ static unsigned char __aligned(4) data[] = "abcd1234"; __kernel struct k_pipe put_get_pipe; -/*test cases*/ -void test_pipe_put_fail(void *p1, void *p2, void *p3) -{ +static void put_fail(struct k_pipe *p) +{ size_t wt_byte = 0; - k_pipe_init(&put_get_pipe, data, PIPE_LEN); - zassert_false(k_pipe_put(&put_get_pipe, data, PIPE_LEN, &wt_byte, + zassert_false(k_pipe_put(p, data, PIPE_LEN, &wt_byte, 1, K_FOREVER), NULL); /**TESTPOINT: pipe put returns -EIO*/ - zassert_equal(k_pipe_put(&put_get_pipe, data, PIPE_LEN, &wt_byte, + zassert_equal(k_pipe_put(p, data, PIPE_LEN, &wt_byte, 1, K_NO_WAIT), -EIO, NULL); zassert_false(wt_byte, NULL); /**TESTPOINT: pipe put returns -EAGAIN*/ - zassert_equal(k_pipe_put(&put_get_pipe, data, PIPE_LEN, &wt_byte, + zassert_equal(k_pipe_put(p, data, PIPE_LEN, &wt_byte, 1, TIMEOUT), -EAGAIN, NULL); zassert_true(wt_byte < 1, NULL); } -void test_pipe_get_fail(void *p1, void *p2, void *p3) +/*test cases*/ +void test_pipe_put_fail(void) { + k_pipe_init(&put_get_pipe, data, PIPE_LEN); + put_fail(&put_get_pipe); +} + +#ifdef CONFIG_USERSPACE +void test_pipe_user_put_fail(void) +{ + struct k_pipe *p = k_object_alloc(K_OBJ_PIPE); + + zassert_true(p != NULL, NULL); + zassert_false(k_pipe_alloc_init(p, PIPE_LEN), NULL); + + put_fail(p); +} +#endif + +static void get_fail(struct k_pipe *p) +{ unsigned char rx_data[PIPE_LEN]; size_t rd_byte = 0; - k_pipe_init(&put_get_pipe, data, PIPE_LEN); /**TESTPOINT: pipe put returns -EIO*/ - zassert_equal(k_pipe_get(&put_get_pipe, rx_data, PIPE_LEN, &rd_byte, 1, + zassert_equal(k_pipe_get(p, rx_data, PIPE_LEN, &rd_byte, 1, K_NO_WAIT), -EIO, NULL); zassert_false(rd_byte, NULL); /**TESTPOINT: pipe put returns -EAGAIN*/ - zassert_equal(k_pipe_get(&put_get_pipe, rx_data, PIPE_LEN, &rd_byte, 1, + zassert_equal(k_pipe_get(p, rx_data, PIPE_LEN, &rd_byte, 1, TIMEOUT), -EAGAIN, NULL); zassert_true(rd_byte < 1, NULL); } + + +void test_pipe_get_fail(void) +{ + k_pipe_init(&put_get_pipe, data, PIPE_LEN); + + get_fail(&put_get_pipe); +} + +#ifdef CONFIG_USERSPACE +void test_pipe_user_get_fail(void) +{ + struct k_pipe *p = k_object_alloc(K_OBJ_PIPE); + + zassert_true(p != NULL, NULL); + zassert_false(k_pipe_alloc_init(p, PIPE_LEN), NULL); + + get_fail(p); +} +#endif