This is an automated email from the ASF dual-hosted git repository.
xiaoxiang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/nuttx.git
The following commit(s) were added to refs/heads/master by this push:
new 64d8249895 sched/addrenv, binfmt: Always allocate address environment
from heap
64d8249895 is described below
commit 64d8249895260a67d5406ead123f04b456c1bd47
Author: Ville Juven <[email protected]>
AuthorDate: Thu Apr 13 11:48:06 2023 +0300
sched/addrenv, binfmt: Always allocate address environment from heap
Instead of using a volatile storage for the address environment in the
binfmt / loadinfo structures, always allocate the address environment
from kheap.
This serves two purposes:
- If the task creation fails, any kernel thread that depends on the
address environment created during task creation will not lose their
mappings (because they hold a reference to it)
- The current address environment variable (g_addrenv) will NEVER contain
a stale / incorrect value
- Releasing the address environment is simplified as any pointer given
to addrenv_drop() can be assumed to be heap memory
- Makes the kludge function addrenv_clear_current irrelevant, as the
system will NEVER have invalid mappings any more
---
binfmt/binfmt_execmodule.c | 6 +--
binfmt/elf.c | 6 +--
binfmt/libelf/libelf_addrenv.c | 40 ++++++++-------
include/nuttx/addrenv.h | 30 ++----------
include/nuttx/binfmt/binfmt.h | 4 +-
include/nuttx/binfmt/elf.h | 4 +-
sched/addrenv/addrenv.c | 107 +++++------------------------------------
sched/task/task_init.c | 13 ++---
8 files changed, 50 insertions(+), 160 deletions(-)
diff --git a/binfmt/binfmt_execmodule.c b/binfmt/binfmt_execmodule.c
index cb38e56d6d..bf6fbdc37f 100644
--- a/binfmt/binfmt_execmodule.c
+++ b/binfmt/binfmt_execmodule.c
@@ -117,7 +117,7 @@ int exec_module(FAR struct binary_s *binp,
{
FAR struct task_tcb_s *tcb;
#if defined(CONFIG_ARCH_ADDRENV) && defined(CONFIG_BUILD_KERNEL)
- FAR struct arch_addrenv_s *addrenv = &binp->addrenv.addrenv;
+ FAR struct arch_addrenv_s *addrenv = &binp->addrenv->addrenv;
FAR void *vheap;
#endif
FAR void *stackaddr = NULL;
@@ -165,7 +165,7 @@ int exec_module(FAR struct binary_s *binp,
#if defined(CONFIG_ARCH_ADDRENV) && defined(CONFIG_BUILD_KERNEL)
/* Instantiate the address environment containing the user heap */
- ret = addrenv_select(&binp->addrenv);
+ ret = addrenv_select(binp->addrenv);
if (ret < 0)
{
berr("ERROR: addrenv_select() failed: %d\n", ret);
@@ -242,7 +242,7 @@ int exec_module(FAR struct binary_s *binp,
#ifdef CONFIG_ARCH_ADDRENV
/* Attach the address environment to the new task */
- ret = addrenv_attach((FAR struct tcb_s *)tcb, &binp->addrenv);
+ ret = addrenv_attach((FAR struct tcb_s *)tcb, binp->addrenv);
if (ret < 0)
{
berr("ERROR: addrenv_attach() failed: %d\n", ret);
diff --git a/binfmt/elf.c b/binfmt/elf.c
index 1b20eaef35..08f778632c 100644
--- a/binfmt/elf.c
+++ b/binfmt/elf.c
@@ -271,11 +271,7 @@ static int elf_loadbinary(FAR struct binary_s *binp,
* needed when the module is executed.
*/
- up_addrenv_clone(&loadinfo.addrenv.addrenv, &binp->addrenv.addrenv);
-
- /* Take a reference to the address environment, so it won't get freed */
-
- addrenv_take(&binp->addrenv);
+ binp->addrenv = loadinfo.addrenv;
#else
binp->alloc[0] = (FAR void *)loadinfo.textalloc;
diff --git a/binfmt/libelf/libelf_addrenv.c b/binfmt/libelf/libelf_addrenv.c
index e3ff9dede0..b437ca3c2b 100644
--- a/binfmt/libelf/libelf_addrenv.c
+++ b/binfmt/libelf/libelf_addrenv.c
@@ -84,24 +84,30 @@ int elf_addrenv_alloc(FAR struct elf_loadinfo_s *loadinfo,
size_t textsize,
size_t datasize, size_t heapsize)
{
#ifdef CONFIG_ARCH_ADDRENV
- FAR struct arch_addrenv_s *addrenv = &loadinfo->addrenv.addrenv;
+ FAR struct arch_addrenv_s *addrenv;
FAR void *vtext;
FAR void *vdata;
int ret;
/* Create an address environment for the new ELF task */
+ loadinfo->addrenv = addrenv_allocate();
+ if (!loadinfo->addrenv)
+ {
+ return -ENOMEM;
+ }
+
+ /* Start creating the address environment sections */
+
+ addrenv = &loadinfo->addrenv->addrenv;
+
ret = up_addrenv_create(textsize, datasize, heapsize, addrenv);
if (ret < 0)
{
berr("ERROR: up_addrenv_create failed: %d\n", ret);
- return ret;
+ goto errout_with_addrenv;
}
- /* Take a reference to the address environment, so it won't get freed */
-
- addrenv_take(&loadinfo->addrenv);
-
/* Get the virtual address associated with the start of the address
* environment. This is the base address that we will need to use to
* access the ELF image (but only if the address environment has been
@@ -112,19 +118,24 @@ int elf_addrenv_alloc(FAR struct elf_loadinfo_s
*loadinfo, size_t textsize,
if (ret < 0)
{
berr("ERROR: up_addrenv_vtext failed: %d\n", ret);
- return ret;
+ goto errout_with_addrenv;
}
ret = up_addrenv_vdata(addrenv, textsize, &vdata);
if (ret < 0)
{
berr("ERROR: up_addrenv_vdata failed: %d\n", ret);
- return ret;
+ goto errout_with_addrenv;
}
loadinfo->textalloc = (uintptr_t)vtext;
loadinfo->dataalloc = (uintptr_t)vdata;
+
return OK;
+
+errout_with_addrenv:
+ addrenv_drop(loadinfo->addrenv, false);
+ return ret;
#else
/* Allocate memory to hold the ELF image */
@@ -177,7 +188,7 @@ int elf_addrenv_select(FAR struct elf_loadinfo_s *loadinfo)
/* Instantiate the new address environment */
- ret = addrenv_select(&loadinfo->addrenv);
+ ret = addrenv_select(loadinfo->addrenv);
if (ret < 0)
{
berr("ERROR: addrenv_select failed: %d\n", ret);
@@ -186,7 +197,7 @@ int elf_addrenv_select(FAR struct elf_loadinfo_s *loadinfo)
/* Allow write access to .text */
- ret = up_addrenv_mprot(&loadinfo->addrenv.addrenv, loadinfo->textalloc,
+ ret = up_addrenv_mprot(&loadinfo->addrenv->addrenv, loadinfo->textalloc,
loadinfo->textsize, ELF_TEXT_WRE);
if (ret < 0)
{
@@ -219,7 +230,7 @@ int elf_addrenv_restore(FAR struct elf_loadinfo_s *loadinfo)
/* Remove write access to .text */
- ret = up_addrenv_mprot(&loadinfo->addrenv.addrenv, loadinfo->textalloc,
+ ret = up_addrenv_mprot(&loadinfo->addrenv->addrenv, loadinfo->textalloc,
loadinfo->textsize, ELF_TEXT_WRD);
if (ret < 0)
{
@@ -261,15 +272,10 @@ int elf_addrenv_restore(FAR struct elf_loadinfo_s
*loadinfo)
void elf_addrenv_free(FAR struct elf_loadinfo_s *loadinfo)
{
#ifdef CONFIG_ARCH_ADDRENV
- int ret;
/* Free the address environment */
- ret = up_addrenv_destroy(&loadinfo->addrenv.addrenv);
- if (ret < 0)
- {
- berr("ERROR: up_addrenv_destroy failed: %d\n", ret);
- }
+ addrenv_drop(loadinfo->addrenv, false);
#else
if (loadinfo->textalloc != 0)
diff --git a/include/nuttx/addrenv.h b/include/nuttx/addrenv.h
index c2416ff548..afac66accc 100644
--- a/include/nuttx/addrenv.h
+++ b/include/nuttx/addrenv.h
@@ -297,35 +297,14 @@ struct addrenv_reserve_s
* Allocate an address environment for a new process.
*
* Input Parameters:
- * tcb - The tcb of the newly created task.
- * ttype - The type of the task.
- *
- * Returned Value:
- * This is a NuttX internal function so it follows the convention that
- * 0 (OK) is returned on success and a negated errno is returned on
- * failure.
- *
- ****************************************************************************/
-
-int addrenv_allocate(FAR struct tcb_s *tcb, uint8_t ttype);
-
-/****************************************************************************
- * Name: addrenv_free
- *
- * Description:
- * Free an address environment for a process.
- *
- * Input Parameters:
- * tcb - The tcb of the task.
+ * None
*
* Returned Value:
- * This is a NuttX internal function so it follows the convention that
- * 0 (OK) is returned on success and a negated errno is returned on
- * failure.
+ * Pointer to the new address environment, or NULL if out of memory.
*
****************************************************************************/
-int addrenv_free(FAR struct tcb_s *tcb);
+FAR struct addrenv_s *addrenv_allocate(void);
/****************************************************************************
* Name: addrenv_switch
@@ -364,8 +343,7 @@ int addrenv_switch(FAR struct tcb_s *tcb);
*
****************************************************************************/
-int addrenv_attach(FAR struct tcb_s *tcb,
- FAR const struct addrenv_s *addrenv);
+int addrenv_attach(FAR struct tcb_s *tcb, FAR struct addrenv_s *addrenv);
/****************************************************************************
* Name: addrenv_join
diff --git a/include/nuttx/binfmt/binfmt.h b/include/nuttx/binfmt/binfmt.h
index 80f80c8e62..6a1cac2589 100644
--- a/include/nuttx/binfmt/binfmt.h
+++ b/include/nuttx/binfmt/binfmt.h
@@ -82,11 +82,11 @@ struct binary_s
#ifdef CONFIG_ARCH_ADDRENV
/* Address environment.
*
- * addrenv - This is the handle created by up_addrenv_create() that can be
+ * addrenv - This is the handle created by addrenv_allocate() that can be
* used to manage the tasks address space.
*/
- addrenv_t addrenv; /* Address environment */
+ FAR addrenv_t *addrenv; /* Address environment */
#endif
size_t mapsize; /* Size of the mapped address region
(needed for munmap) */
diff --git a/include/nuttx/binfmt/elf.h b/include/nuttx/binfmt/elf.h
index 8370ec8832..4fa595c106 100644
--- a/include/nuttx/binfmt/elf.h
+++ b/include/nuttx/binfmt/elf.h
@@ -117,12 +117,12 @@ struct elf_loadinfo_s
/* Address environment.
*
- * addrenv - This is the handle created by up_addrenv_create() that can be
+ * addrenv - This is the handle created by addrenv_allocate() that can be
* used to manage the tasks address space.
*/
#ifdef CONFIG_ARCH_ADDRENV
- addrenv_t addrenv; /* Address environment */
+ FAR addrenv_t *addrenv; /* Address environment */
#endif
uint16_t symtabidx; /* Symbol table section index */
diff --git a/sched/addrenv/addrenv.c b/sched/addrenv/addrenv.c
index cfbcca5506..286512ab03 100644
--- a/sched/addrenv/addrenv.c
+++ b/sched/addrenv/addrenv.c
@@ -86,35 +86,6 @@ static void addrenv_destroy(FAR void *arg)
kmm_free(addrenv);
}
-/****************************************************************************
- * Name: addrenv_clear_current
- *
- * Description:
- * Clear the current addrenv from g_addrenv, if it matches the input.
- *
- * Input Parameters:
- * addrenv - Pointer to the addrenv to free.
- *
- * Returned Value:
- * None.
- *
- ****************************************************************************/
-
-static void addrenv_clear_current(FAR const addrenv_t *addrenv)
-{
- int i;
-
- /* Mark no address environment */
-
- for (i = 0; i < CONFIG_SMP_NCPUS; i++)
- {
- if (addrenv == g_addrenv[i])
- {
- g_addrenv[i] = NULL;
- }
- }
-}
-
/****************************************************************************
* Public Functions
****************************************************************************/
@@ -237,62 +208,26 @@ int addrenv_switch(FAR struct tcb_s *tcb)
* Allocate an address environment for a new process.
*
* Input Parameters:
- * tcb - The tcb of the newly created task.
- * ttype - The type of the task.
+ * None
*
* Returned Value:
- * This is a NuttX internal function so it follows the convention that
- * 0 (OK) is returned on success and a negated errno is returned on
- * failure.
+ * Pointer to the new address environment, or NULL if out of memory.
*
****************************************************************************/
-int addrenv_allocate(FAR struct tcb_s *tcb, uint8_t ttype)
+FAR struct addrenv_s *addrenv_allocate(void)
{
- int ret = OK;
+ FAR struct addrenv_s *addrenv;
- if ((ttype & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_KERNEL)
+ addrenv = (FAR struct addrenv_s *)kmm_zalloc(sizeof(struct addrenv_s));
+ if (addrenv)
{
- tcb->addrenv_own = NULL;
- }
- else
- {
- tcb->addrenv_own = (FAR struct addrenv_s *)
- kmm_zalloc(sizeof(struct addrenv_s));
- if (tcb->addrenv_own == NULL)
- {
- ret = -ENOMEM;
- }
- }
+ /* Take reference so this won't get freed */
- return ret;
-}
-
-/****************************************************************************
- * Name: addrenv_free
- *
- * Description:
- * Free an address environment for a process.
- *
- * Input Parameters:
- * tcb - The tcb of the task.
- *
- * Returned Value:
- * This is a NuttX internal function so it follows the convention that
- * 0 (OK) is returned on success and a negated errno is returned on
- * failure.
- *
- ****************************************************************************/
-
-int addrenv_free(FAR struct tcb_s *tcb)
-{
- if (tcb->addrenv_own != NULL)
- {
- kmm_free(tcb->addrenv_own);
- tcb->addrenv_own = NULL;
+ addrenv->refs = 1;
}
- return OK;
+ return addrenv;
}
/****************************************************************************
@@ -313,24 +248,12 @@ int addrenv_free(FAR struct tcb_s *tcb)
*
****************************************************************************/
-int addrenv_attach(FAR struct tcb_s *tcb,
- FAR const struct addrenv_s *addrenv)
+int addrenv_attach(FAR struct tcb_s *tcb, FAR struct addrenv_s *addrenv)
{
- int ret;
-
- /* Clone the address environment for us */
-
- ret = up_addrenv_clone(&addrenv->addrenv, &tcb->addrenv_own->addrenv);
- if (ret < 0)
- {
- berr("ERROR: up_addrenv_clone failed: %d\n", ret);
- return ret;
- }
-
/* Attach the address environment */
+ tcb->addrenv_own = addrenv;
tcb->addrenv_curr = tcb->addrenv_own;
- tcb->addrenv_own->refs = 1;
return OK;
}
@@ -452,14 +375,6 @@ int addrenv_restore(void)
{
FAR struct tcb_s *tcb = this_task();
addrenv_give(tcb->addrenv_curr);
-
- if (tcb->addrenv_own == NULL)
- {
- /* Kernel thread, clear g_addrenv, as it is not valid any more */
-
- addrenv_clear_current(tcb->addrenv_curr);
- }
-
tcb->addrenv_curr = tcb->addrenv_own;
return addrenv_switch(tcb);
}
diff --git a/sched/task/task_init.c b/sched/task/task_init.c
index 95b0927cc0..06ef5691b5 100644
--- a/sched/task/task_init.c
+++ b/sched/task/task_init.c
@@ -97,12 +97,11 @@ int nxtask_init(FAR struct task_tcb_s *tcb, const char
*name, int priority,
#endif
#ifdef CONFIG_ARCH_ADDRENV
- /* Allocate address environment for the task */
+ /* Kernel threads do not own any address environment */
- ret = addrenv_allocate(&tcb->cmn, tcb->cmn.flags);
- if (ret < 0)
+ if ((ttype & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_KERNEL)
{
- return ret;
+ tcb->cmn.addrenv_own = NULL;
}
#endif
@@ -111,7 +110,7 @@ int nxtask_init(FAR struct task_tcb_s *tcb, const char
*name, int priority,
ret = group_allocate(tcb, tcb->cmn.flags);
if (ret < 0)
{
- goto errout_with_addrenv;
+ return ret;
}
/* Duplicate the parent tasks environment */
@@ -201,10 +200,6 @@ errout_with_group:
group_leave(&tcb->cmn);
-errout_with_addrenv:
-#ifdef CONFIG_ARCH_ADDRENV
- addrenv_free(&tcb->cmn);
-#endif
return ret;
}
