From 12c60852f166324bb7919943cd70ef59031082b0 Mon Sep 17 00:00:00 2001
From: Chen Chengjun <chenchengjun.ccj@antgroup.com>
Date: Fri, 9 Aug 2024 16:13:05 +0800
Subject: [PATCH] Modify the logics of setting child thread tid

---
 kernel/aster-nix/src/process/clone.rs | 30 ++++++---------------------
 kernel/aster-nix/src/thread/task.rs   | 12 +++++++++++
 kernel/aster-nix/src/vm/vmar/mod.rs   |  5 +++++
 3 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/kernel/aster-nix/src/process/clone.rs b/kernel/aster-nix/src/process/clone.rs
index 62de1bf53..409eefc35 100644
--- a/kernel/aster-nix/src/process/clone.rs
+++ b/kernel/aster-nix/src/process/clone.rs
@@ -4,10 +4,8 @@
 
 use core::sync::atomic::Ordering;
 
-use aster_rights::Full;
 use ostd::{
     cpu::UserContext,
-    mm::VmIo,
     user::{UserContextApi, UserSpace},
 };
 
@@ -25,8 +23,6 @@ use crate::{
     fs::{file_table::FileTable, fs_resolver::FsResolver, utils::FileCreationMask},
     prelude::*,
     thread::{allocate_tid, thread_table, Thread, Tid},
-    util::write_val_to_user,
-    vm::vmar::Vmar,
 };
 
 bitflags! {
@@ -199,12 +195,7 @@ fn clone_child_thread(parent_context: &UserContext, clone_args: CloneArgs) -> Re
     let child_posix_thread = child_thread.as_posix_thread().unwrap();
     clone_parent_settid(child_tid, clone_args.parent_tidptr, clone_flags)?;
     clone_child_cleartid(child_posix_thread, clone_args.child_tidptr, clone_flags)?;
-    clone_child_settid(
-        child_root_vmar,
-        child_tid,
-        clone_args.child_tidptr,
-        clone_flags,
-    )?;
+    clone_child_settid(child_posix_thread, clone_args.child_tidptr, clone_flags)?;
     Ok(child_thread)
 }
 
@@ -303,14 +294,7 @@ fn clone_child_process(
     let child_posix_thread = child_thread.as_posix_thread().unwrap();
     clone_parent_settid(child_tid, clone_args.parent_tidptr, clone_flags)?;
     clone_child_cleartid(child_posix_thread, clone_args.child_tidptr, clone_flags)?;
-
-    let child_root_vmar = child.root_vmar();
-    clone_child_settid(
-        child_root_vmar,
-        child_tid,
-        clone_args.child_tidptr,
-        clone_flags,
-    )?;
+    clone_child_settid(child_posix_thread, clone_args.child_tidptr, clone_flags)?;
 
     // Sets parent process and group for child process.
     set_parent_and_group(&current, &child);
@@ -324,20 +308,18 @@ fn clone_child_cleartid(
     clone_flags: CloneFlags,
 ) -> Result<()> {
     if clone_flags.contains(CloneFlags::CLONE_CHILD_CLEARTID) {
-        let mut clear_tid = child_posix_thread.clear_child_tid().lock();
-        *clear_tid = child_tidptr;
+        *child_posix_thread.clear_child_tid().lock() = child_tidptr;
     }
     Ok(())
 }
 
 fn clone_child_settid(
-    child_root_vmar: &Vmar<Full>,
-    child_tid: Tid,
+    child_posix_thread: &PosixThread,
     child_tidptr: Vaddr,
     clone_flags: CloneFlags,
 ) -> Result<()> {
     if clone_flags.contains(CloneFlags::CLONE_CHILD_SETTID) {
-        child_root_vmar.write_val(child_tidptr, &child_tid)?;
+        *child_posix_thread.set_child_tid().lock() = child_tidptr;
     }
     Ok(())
 }
@@ -348,7 +330,7 @@ fn clone_parent_settid(
     clone_flags: CloneFlags,
 ) -> Result<()> {
     if clone_flags.contains(CloneFlags::CLONE_PARENT_SETTID) {
-        write_val_to_user(parent_tidptr, &child_tid)?;
+        CurrentUserSpace::get().write_val(parent_tidptr, &child_tid)?;
     }
     Ok(())
 }
diff --git a/kernel/aster-nix/src/thread/task.rs b/kernel/aster-nix/src/thread/task.rs
index 799d736d4..2e75f4bf4 100644
--- a/kernel/aster-nix/src/thread/task.rs
+++ b/kernel/aster-nix/src/thread/task.rs
@@ -12,6 +12,7 @@ use crate::{
     process::{posix_thread::PosixThreadExt, signal::handle_pending_signal},
     syscall::handle_syscall,
     thread::exception::handle_exception,
+    vm::vmar::is_userspace_vaddr,
 };
 
 /// create new task with userspace and parent process
@@ -37,6 +38,17 @@ pub fn create_new_user_task(user_space: Arc<UserSpace>, thread_ref: Weak<Thread>
         );
 
         let posix_thread = current_thread.as_posix_thread().unwrap();
+
+        let child_tid_ptr = *posix_thread.set_child_tid().lock();
+
+        // The `clone` syscall may require child process to write the thread pid to the specified address.
+        // Make sure the store operation completes before the clone call returns control to user space
+        // in the child process.
+        if is_userspace_vaddr(child_tid_ptr) {
+            CurrentUserSpace::get()
+                .write_val(child_tid_ptr, &current_thread.tid())
+                .unwrap();
+        }
         let has_kernel_event_fn = || posix_thread.has_pending();
         loop {
             let return_reason = user_mode.execute(has_kernel_event_fn);
diff --git a/kernel/aster-nix/src/vm/vmar/mod.rs b/kernel/aster-nix/src/vm/vmar/mod.rs
index 72e66d72f..3793d8392 100644
--- a/kernel/aster-nix/src/vm/vmar/mod.rs
+++ b/kernel/aster-nix/src/vm/vmar/mod.rs
@@ -143,6 +143,11 @@ impl VmarInner {
 pub const ROOT_VMAR_LOWEST_ADDR: Vaddr = 0x001_0000; // 64 KiB is the Linux configurable default
 const ROOT_VMAR_CAP_ADDR: Vaddr = MAX_USERSPACE_VADDR;
 
+/// Returns whether the input `vaddr` is a legal user space virtual address.
+pub fn is_userspace_vaddr(vaddr: Vaddr) -> bool {
+    (ROOT_VMAR_LOWEST_ADDR..ROOT_VMAR_CAP_ADDR).contains(&vaddr)
+}
+
 impl Interval<usize> for Arc<Vmar_> {
     fn range(&self) -> Range<usize> {
         self.base..(self.base + self.size)