xiaoxiang781216 commented on a change in pull request #5498: URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r810169659
########## File path: sched/task/task_tls_alloc.c ########## @@ -0,0 +1,113 @@ +/**************************************************************************** + * sched/task/task_tls_alloc.c + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + ****************************************************************************/ + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include <sys/types.h> +#include <unistd.h> +#include <sched.h> +#include <errno.h> + +#include <nuttx/tls.h> +#include <nuttx/semaphore.h> + +/**************************************************************************** + * Private Data + ****************************************************************************/ + +#if CONFIG_TLS_TASK_NELEM > 0 + +static int g_tlsset = 0; +static sem_t g_tlssem = SEM_INITIALIZER(1); +static tls_dtor_t g_tlsdtor[CONFIG_TLS_TASK_NELEM]; + +/**************************************************************************** + * Public Functions + ****************************************************************************/ + +int task_tls_alloc(tls_dtor_t dtor) +{ + int ret; + int candidate; + + ret = nxsem_wait(&g_tlssem); + + if (ret < 0) + { + return ret; + } + + ret = -EAGAIN; + + for (candidate = 0; candidate < CONFIG_TLS_TASK_NELEM; candidate++) + { + int mask = 1 << candidate; + if ((g_tlsset & mask) == 0) + { + g_tlsset |= mask; + g_tlsdtor[candidate] = dtor; + ret = candidate; + break; + } + } + + nxsem_post(&g_tlssem); + return ret; +} + +/**************************************************************************** + * Name: task_tls_destruct + * + * Description: + * Destruct all TLS data element associated with allocated key + * + * Input Parameters: + * None + * + * Returned Value: + * A set of allocated TLS index + * + ****************************************************************************/ + +void task_tls_destruct(void) Review comment: > I will explain what I mean and also make some comments based on your answers. > > > TLS here mean Task local storage, collide with thread local storage.:( > > Let's use `task_ls_` and not `task_tls_` so the confusion will be removed. Do you agree? > > > From the concept, task local storage is very similar to thread local storage, but at the task level. > > Because I do not have visibility about conditions (when and how) of `task_tls_alloc` call, I can't get a full picture. Based on your example let's assume that it is a time when the library is instantiated for the first time. Then the allocated ID will be shared across the library instances while global data will be stored in Task LS. For example I have 2 different libraries that rely on top of Task LS. The library A attach `dtorA` at index 0 and library B attach `dtorB` at index 1. When one of tasks that use library A exits then `task_tls_destruct` is called and we will call `dtorB` from library A user context point of view. Yes, the `0` will be passed as a parameter, but I'm not sure if such situation is valid from isolation point of view. Yes, it's possible. Another possibility is that the task use both libraries and then dtorA/dtorB get the different non-NULL pointer. > Even more, if there is a but in library A code and it somehow at some point pass a wrong index to `task_tls_set_value` then `dtorB` will be called with non-zero argument and that value can match with the same value that is allocated by library B users. This potentially can lead to security issues. > Yes, the wrong usage will crash the system, but it doesn't increase the security risk, because the task local storage is used only in FLAT/PROTECTED build, not KERNEL build(The static/dynamic library get a new copy for each task in this case). Task in FLAT/PROTECTED mode already can access other tasks and do any evil thing. > > > We can have 0 .. 64 as a limit for CONFIG_TLS_TASK_NELEM and use up to uint64_t for index storage. Or add save task pid together with each allocated index so that can be reused in task_tls_destruct to filter destructors to call > > > > > > could you explanation more? The code is almost same as the thread local storage, just lift up one level. > > There are actually two issues here: > > 1. `CONFIG_TLS_TASK_NELEM` is not limited in Kconfig and there is next code: > > ``` > int task_tls_alloc(tls_dtor_t dtor) > { > int ret; > int candidate; > ... > for (candidate = 0; candidate < CONFIG_TLS_TASK_NELEM; candidate++) > { > int mask = 1 << candidate; > if ((g_tlsset & mask) == 0) > { > g_tlsset |= mask; > g_tlsdtor[candidate] = dtor; > ret = candidate; > break; > } > } > ... > return ret; > } > ``` > > so if `CONFIG_TLS_TASK_NELEM` is more then number of bits that `int` can carry `if ((g_tlsset & mask) == 0)` will start be `true` always and out of boundary array access will occur. Hopefully this will be caught by compiler. @no1wudi please add range 0 64 to Kconfig for both CONFIG_TLS_NELEM and CONFIG_TLS_TASK_NELEM. > 2. Because of lack of use case description I was thinking that each library A user will call `task_tls_alloc()` at the start and will use that allocated index, but maybe it is not a use case (but that is a more like a traditional TLS). Yes, you are right, each library need allocate the index when it need access the global variables at the first time. > So I was thinking that we can store a bit mask to keep tracking of indexes allocated by a task. Again even now I'm not aware about this API usage point of view assuming that it can be called by any task in the system. The API usage it's simple like the traditional TLS, @no1wudi will provide an usage in libuv like this: ``` static void uv__global_free(void* global) { if (global) { uv_library_shutdown(); uv__free(global); } } uv__global_t* uv__global_get(void) { static int index = -1; uv__global_t* global = NULL; if (index < 0) { index = task_tls_alloc(uv__global_free); } if (index >= 0) { global = (uv__global_t*)task_tls_get_value(index); if (global == NULL) { global = (uv__global_t*)uv__calloc(1, sizeof(uv__global_t)); if (global) { global->once = UV_ONCE_INIT; global->uv__signal_global_init_guard = UV_ONCE_INIT; global->uv__signal_lock_pipefd[0] = -1; global->uv__signal_lock_pipefd[1] = -1; task_tls_set_value(index, (uintptr_t)global); } } } ASSERT(global != NULL); return global; } ``` Basically, libuv define several global variables, but we have many services call libuv functions in the background. The above code support the multiple libuv's instances with minimal change. 1. uv__global_get allocate index and uv_global_t at the first call of the first task 2. uv__global_get return the same global in the subsequent calls of the first task 3. uv__global_get allocate a new global when the second task call libuv's function at the fist time 4. uv__global_get return the second global in the subsequent calls of the second task I hope this case help you understand the usage. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
