Merge #311

311: Fix thread pointer calculation, rework TaskTLS r=mkroening a=mkroening

Closes hermit-os/hermit-rs#170. 🎉 

Replaces #305.

Depends on:

* hermit-os/loader#46
* hermit-os/uhyve#241

The cause of the issue is the calculation of the thread pointer:

```python
# Current calculation (wrong)
thread_ptr = tls_block_ptr + round(tls_len, 32)

# Correct calculation
thread_ptr = tls_block_ptr + round(tls_len, tls_align)
# round(tls_len, tls_align) is called tls_offset
```

This is fixed in the first two commits of this PR.

The later commits completely rework the `TaskTLS` struct. I did this to better understand the problem. The old comments were partly outdated and wrong. Also the code was not very rusty. I replaced manual memory allocations with `Box`es and restructured and recommented the code.

Co-authored-by: Martin Kröning <[email protected]>

.gitlab-ci.yml

@@ -62,7 +62,7 @@ test:integration:
     - lscpu
     - kvm-ok
     - python3 --version
-    - cargo install uhyve --locked
+    - cargo +nightly install --git https://github.com/hermitcore/uhyve.git --locked uhyve
     - HERMIT_LOG_LEVEL_FILTER=Debug cargo test --test '*' --no-fail-fast -Z build-std=core,alloc 
         -Z build-std-features=compiler-builtins-mem --no-default-features --features=pci,acpi 
         --target x86_64-unknown-none-hermitkernel -- --uhyve_path=$HOME/.cargo/bin/uhyve --veryverbose
@@ -82,7 +82,7 @@ test:uhyve:
    script:
      - lscpu
      - kvm-ok
-     - cargo install uhyve --locked
+     - cargo +nightly install --git https://github.com/hermitcore/uhyve.git --locked uhyve
      - uhyve -v -c 1 rusty-hermit/target/x86_64-unknown-hermit/debug/rusty_demo
      - uhyve -v -c 2 rusty-hermit/target/x86_64-unknown-hermit/debug/rusty_demo
      - uhyve -v -c 1 rusty-hermit/target/x86_64-unknown-hermit/release/rusty_demo

src/arch/aarch64/kernel/bootinfo.rs

@@ -28,6 +28,7 @@ pub struct BootInfo {
 	pub hcip: [u8; 4],
 	pub hcgateway: [u8; 4],
 	pub hcmask: [u8; 4],
+	pub tls_align: u64,
 }
 
 impl BootInfo {
@@ -58,6 +59,7 @@ impl BootInfo {
 			hcip: [255, 255, 255, 255],
 			hcgateway: [255, 255, 255, 255],
 			hcmask: [255, 255, 255, 0],
+			tls_align: 0,
 		}
 	}
 }
@@ -71,6 +73,7 @@ impl fmt::Debug for BootInfo {
 		writeln!(f, "tls_start {:#x}", self.tls_start)?;
 		writeln!(f, "tls_filesz {:#x}", self.tls_filesz)?;
 		writeln!(f, "tls_memsz {:#x}", self.tls_memsz)?;
+		writeln!(f, "tls_align {:#x}", self.tls_align)?;
 		writeln!(f, "image_size {:#x}", self.image_size)?;
 		writeln!(f, "current_stack_address {:#x}", self.current_stack_address)?;
 		writeln!(

src/arch/aarch64/kernel/mod.rs

@@ -63,6 +63,10 @@ pub fn get_tls_memsz() -> usize {
 	0
 }
 
+pub fn get_tls_align() -> usize {
+	0
+}
+
 /// Whether HermitCore is running under the "uhyve" hypervisor.
 pub fn is_uhyve() -> bool {
 	unsafe { core::ptr::read_volatile(&BOOT_INFO.uhyve) != 0 }

src/arch/aarch64/kernel/scheduler.rs

@@ -207,16 +207,11 @@ pub struct TaskTLS {
 }
 
 impl TaskTLS {
-	pub fn new(tls_size: usize) -> Self {
+	fn from_environment() -> Self {
 		Self {
 			address: VirtAddr::zero(),
 		}
 	}
-
-	#[inline]
-	pub fn address(&self) -> VirtAddr {
-		self.address
-	}
 }
 
 impl Drop for TaskTLS {
@@ -234,7 +229,7 @@ impl Drop for TaskTLS {
 
 impl Clone for TaskTLS {
 	fn clone(&self) -> Self {
-		TaskTLS::new(environment::get_tls_memsz())
+		TaskTLS::from_environment()
 	}
 }
 
@@ -249,10 +244,10 @@ extern "C" fn task_entry(func: extern "C" fn(usize), arg: usize) {
 		// Yes, it does, so we have to allocate TLS memory.
 		// Allocate enough space for the given size and one more variable of type usize, which holds the tls_pointer.
 		let tls_allocation_size = tls_size + mem::size_of::<usize>();
-		let tls = TaskTLS::new(tls_allocation_size);
+		let tls = TaskTLS::from_environment();
 
 		// The tls_pointer is the address to the end of the TLS area requested by the task.
-		let tls_pointer = tls.address() + tls_size;
+		let tls_pointer = tls.address + tls_size;
 
 		// TODO: Implement AArch64 TLS
 
@@ -261,7 +256,7 @@ extern "C" fn task_entry(func: extern "C" fn(usize), arg: usize) {
 		debug!(
 			"Set up TLS for task {} at address {:#X}",
 			current_task_borrowed.id,
-			tls.address()
+			tls.address
 		);
 		current_task_borrowed.tls = Some(tls);
 	}*/

src/arch/x86_64/kernel/mod.rs

@@ -76,6 +76,7 @@ pub struct BootInfo {
 	hcip: [u8; 4],
 	hcgateway: [u8; 4],
 	hcmask: [u8; 4],
+	tls_align: u64,
 }
 
 impl BootInfo {
@@ -106,6 +107,7 @@ impl BootInfo {
 		hcip: [0; 4],
 		hcgateway: [0; 4],
 		hcmask: [0; 4],
+		tls_align: 0,
 	};
 
 	pub const fn current_stack_address_offset() -> isize {
@@ -239,6 +241,10 @@ pub fn get_tls_memsz() -> usize {
 	unsafe { core::ptr::read_volatile(&(*BOOT_INFO).tls_memsz) as usize }
 }
 
+pub fn get_tls_align() -> usize {
+	unsafe { core::ptr::read_volatile(&(*BOOT_INFO).tls_align) as usize }
+}
+
 pub fn get_mbinfo() -> VirtAddr {
 	unsafe { VirtAddr(core::ptr::read_volatile(&(*BOOT_INFO).mb_info)) }
 }

src/arch/x86_64/kernel/scheduler.rs

@@ -1,7 +1,7 @@
 //! Architecture dependent interface to initialize a task
 
-use alloc::alloc::{alloc, dealloc, Layout};
-use core::{mem, ptr};
+use alloc::boxed::Box;
+use core::{mem, ptr, slice};
 
 use crate::arch::x86_64::kernel::apic;
 use crate::arch::x86_64::kernel::idt;
@@ -238,89 +238,70 @@ impl Clone for TaskStacks {
 }
 
 pub struct TaskTLS {
-	address: VirtAddr,
-	fs: VirtAddr,
-	layout: Layout,
+	_block: Box<[u8]>,
+	thread_ptr: Box<*mut ()>,
 }
 
 impl TaskTLS {
-	pub fn new(tls_size: usize) -> Self {
-		// determine the size of tdata (tls without tbss)
-		let tdata_size: usize = environment::get_tls_filesz();
-		// Yes, it does, so we have to allocate TLS memory.
-		// Allocate enough space for the given size and one more variable of type usize, which holds the tls_pointer.
-		let tls_allocation_size = align_up!(tls_size, 32) + mem::size_of::<usize>();
-		// We allocate in 128 byte granularity (= cache line size) to avoid false sharing
-		let memory_size = align_up!(tls_allocation_size, 128);
-		let layout =
-			Layout::from_size_align(memory_size, 128).expect("TLS has an invalid size / alignment");
-		let ptr = VirtAddr(unsafe { alloc(layout) as u64 });
-
-		// The tls_pointer is the address to the end of the TLS area requested by the task.
-		let tls_pointer = ptr + align_up!(tls_size, 32);
+	fn from_environment() -> Self {
+		// For details on thread-local storage data structures see
+		//
+		// “ELF Handling For Thread-Local Storage” Section 3.4.6: x86-64 Specific Definitions for Run-Time Handling of TLS
+		// https://akkadia.org/drepper/tls.pdf
+
+		// Get TLS initialization image
+		let tls_init_image = {
+			let tls_init_data = environment::get_tls_start().as_ptr::<u8>();
+			let tls_init_len = environment::get_tls_filesz();
+
+			// SAFETY: We will have to trust the environment here.
+			unsafe { slice::from_raw_parts(tls_init_data, tls_init_len) }
+		};
 
-		unsafe {
-			// Copy over TLS variables with their initial values.
-			ptr::copy_nonoverlapping(
-				environment::get_tls_start().as_ptr::<u8>(),
-				ptr.as_mut_ptr::<u8>(),
-				tdata_size,
-			);
+		// Allocate TLS block
+		let mut block = {
+			let tls_len = environment::get_tls_memsz();
+			let tls_align = environment::get_tls_align();
 
-			ptr::write_bytes(
-				ptr.as_mut_ptr::<u8>()
-					.offset(tdata_size.try_into().unwrap()),
-				0,
-				align_up!(tls_size, 32) - tdata_size,
-			);
+			// As described in “ELF Handling For Thread-Local Storage”
+			let tls_offset = align_up!(tls_len, tls_align);
 
-			// The x86-64 TLS specification also requires that the tls_pointer can be accessed at fs:0.
-			// This allows TLS variable values to be accessed by "mov rax, fs:0" and a later "lea rdx, [rax+VARIABLE_OFFSET]".
-			// See "ELF Handling For Thread-Local Storage", version 0.20 by Ulrich Drepper, page 12 for details.
-			//
-			// fs:0 is where tls_pointer points to and we have reserved space for a usize value above.
-			*(tls_pointer.as_mut_ptr::<u64>()) = tls_pointer.as_u64();
-		}
+			// To access TLS blocks on x86-64, TLS offsets are *subtracted* from the thread register value.
+			// So the thread pointer needs to be `block_ptr + tls_offset`.
+			// Allocating only tls_len bytes would be enough to hold the TLS block.
+			// For the thread pointer to be sound though, we need it's value to be included in or one byte past the same allocation.
+			let block = Box::<[u8]>::new_zeroed_slice(tls_offset);
 
-		debug!(
-			"Set up TLS at {:#x}, tdata_size {:#x}, tls_size {:#x}",
-			tls_pointer, tdata_size, tls_size
-		);
+			// SAFETY: All u8s can hold the bit-pattern 0 as a valid value
+			unsafe { block.assume_init() }
+		};
 
-		Self {
-			address: ptr,
-			fs: tls_pointer,
-			layout,
-		}
-	}
+		// Initialize beginning of the TLS block with TLS initialization image
+		block[..tls_init_image.len()].copy_from_slice(tls_init_image);
 
-	#[inline]
-	pub fn address(&self) -> VirtAddr {
-		self.address
-	}
+		// The end of the TLS block was already zeroed by the allocator
 
-	#[inline]
-	pub fn get_fs(&self) -> VirtAddr {
-		self.fs
-	}
-}
+		// thread_ptr = block_ptr + tls_offset
+		// block.len() == tls_offset
+		let thread_ptr = block.as_mut_ptr_range().end.cast::<()>();
 
-impl Drop for TaskTLS {
-	fn drop(&mut self) {
-		debug!(
-			"Deallocate TLS at {:#x} (layout {:?})",
-			self.address, self.layout,
-		);
+		// Put thread pointer on heap, so it does not move and can be referenced in fs:0
+		let thread_ptr = Box::new(thread_ptr);
 
-		unsafe {
-			dealloc(self.address.as_mut_ptr::<u8>(), self.layout);
+		Self {
+			_block: block,
+			thread_ptr,
 		}
 	}
+
+	fn thread_ptr(&self) -> &*mut () {
+		&*self.thread_ptr
+	}
 }
 
 impl Clone for TaskTLS {
 	fn clone(&self) -> Self {
-		TaskTLS::new(environment::get_tls_memsz())
+		Self::from_environment()
 	}
 }
 
@@ -357,11 +338,9 @@ extern "C" fn task_entry(func: extern "C" fn(usize), arg: usize) -> ! {
 
 impl TaskFrame for Task {
 	fn create_stack_frame(&mut self, func: extern "C" fn(usize), arg: usize) {
-		// Check if the task (process or thread) uses Thread-Local-Storage.
-		let tls_size = environment::get_tls_memsz();
-		// check is TLS is already allocated
-		if self.tls.is_none() && tls_size > 0 {
-			self.tls = Some(TaskTLS::new(tls_size))
+		// Check if TLS is allocated already and if the task uses thread-local storage.
+		if self.tls.is_none() && environment::get_tls_memsz() > 0 {
+			self.tls = Some(TaskTLS::from_environment());
 		}
 
 		unsafe {
@@ -377,7 +356,7 @@ impl TaskFrame for Task {
 			ptr::write_bytes(stack.as_mut_ptr::<u8>(), 0, mem::size_of::<State>());
 
 			if let Some(tls) = &self.tls {
-				(*state).fs = tls.get_fs().as_u64();
+				(*state).fs = tls.thread_ptr() as *const _ as u64;
 			}
 			(*state).rip = task_start as usize as u64;
 			(*state).rdi = func as usize as u64;

src/environment.rs

@@ -4,14 +4,14 @@
 
 #[cfg(target_arch = "x86_64")]
 pub use crate::arch::x86_64::kernel::{
-	get_base_address, get_cmdline, get_cmdsize, get_image_size, get_tls_filesz, get_tls_memsz,
-	get_tls_start, is_single_kernel, is_uhyve,
+	get_base_address, get_cmdline, get_cmdsize, get_image_size, get_tls_align, get_tls_filesz,
+	get_tls_memsz, get_tls_start, is_single_kernel, is_uhyve,
 };
 
 #[cfg(target_arch = "aarch64")]
 pub use crate::arch::aarch64::kernel::{
-	get_base_address, get_cmdline, get_cmdsize, get_image_size, get_tls_filesz, get_tls_memsz,
-	get_tls_start, is_single_kernel, is_uhyve,
+	get_base_address, get_cmdline, get_cmdsize, get_image_size, get_tls_align, get_tls_filesz,
+	get_tls_memsz, get_tls_start, is_single_kernel, is_uhyve,
 };
 
 use crate::util;

src/lib.rs

@@ -19,6 +19,7 @@
 #![feature(linkage)]
 #![feature(linked_list_cursors)]
 #![feature(naked_functions)]
+#![feature(new_uninit)]
 #![feature(panic_info_message)]
 #![feature(specialization)]
 #![feature(nonnull_slice_from_raw_parts)]