Fix various deadlocks

kernel/src/driver/pci/device.rs

@@ -59,7 +59,7 @@ impl Object for PciDevice {
 		}
 
 		let index = usize::try_from(offset - 1).ok()?;
-		let pci = PCI.lock();
+		let pci = PCI.auto_lock();
 		let pci = pci.as_ref().unwrap();
 		let header = pci.get(self.bus, self.device, 0).unwrap();
 		let bar = header.base_addresses().get(index)?;

kernel/src/driver/pci/table.rs

@@ -52,7 +52,7 @@ impl Object for PciTable {
 		Ticket::new_complete(
 			path_to_bdf(path)
 				.and_then(|(bus, dev, func)| {
-					let pci = super::PCI.lock();
+					let pci = super::PCI.auto_lock();
 					pci.as_ref()
 						.unwrap()
 						.get(bus, dev, func)
@@ -99,7 +99,7 @@ impl Iterator for QueryTags {
 	type Item = Ticket<QueryResult>;
 
 	fn next(&mut self) -> Option<Self::Item> {
-		let pci = super::PCI.lock();
+		let pci = super::PCI.auto_lock();
 		let pci = pci.as_ref().unwrap();
 		while self.index < 0x100 << 8 {
 			let (bus, dev, func) = n_to_bdf(self.index.into()).unwrap();

kernel/src/driver/ps2/keyboard/table.rs

@@ -29,7 +29,7 @@ impl Object for ScancodeReader {
 	fn read(&self, length: usize) -> Ticket<Box<[u8]>> {
 		if length == 0 {
 			Ticket::new_complete(Ok([].into()))
-		} else if let Some(s) = super::EVENTS.lock().pop() {
+		} else if let Some(s) = super::EVENTS.auto_lock().pop() {
 			Ticket::new_complete(Ok(<[u8; 4]>::from(s).into()))
 		} else {
 			let (ticket, waker) = Ticket::new();

kernel/src/memory/frame/mod.rs

@@ -203,7 +203,6 @@ where
 }
 
 /// Allocate a physically contiguous range of pages.
-#[track_caller]
 pub fn allocate_contiguous(count: NonZeroUsize) -> Result<PPN, AllocateContiguousError> {
 	dumb_stack::STACK
 		.auto_lock()

kernel/src/memory/virtual/address_space.rs

@@ -75,6 +75,8 @@ impl AddressSpace {
 		object: Arc<dyn MemoryObject>,
 		rwx: RWX,
 	) -> Result<NonNull<Page>, MapError> {
+		// FIXME this will deadlock because there is now a circular dependency
+		// on the heap allocator
 		let mut objects = KERNEL_MAPPED_OBJECTS.auto_lock();
 
 		let (range, frames, index) = Self::map_object_common(
@@ -145,20 +147,22 @@ impl AddressSpace {
 		count: NonZeroUsize,
 	) -> Result<(), UnmapError> {
 		let mut objects = KERNEL_MAPPED_OBJECTS.auto_lock();
-		unsafe {
-			Self::unmap_object_common(&mut objects, base, count)?;
+		let obj = unsafe {
 			r#virtual::AddressSpace::kernel_unmap(base, count).unwrap();
-		}
+			Self::unmap_object_common(&mut objects, base, count)?
+		};
+		drop(objects); // Release now to avoid deadlock
+		drop(obj);
 		Ok(())
 	}
 
 	unsafe fn unmap_object_common(
 		objects: &mut Vec<(RangeInclusive<NonNull<Page>>, Arc<dyn MemoryObject>)>,
 		base: NonNull<Page>,
 		count: NonZeroUsize,
-	) -> Result<(), UnmapError> {
+	) -> Result<Option<Arc<dyn MemoryObject>>, UnmapError> {
 		let i = objects.iter().position(|e| e.0.contains(&base)).unwrap();
-		let (range, _obj) = &objects[i];
+		let (range, _) = &objects[i];
 		let end = base
 			.as_ptr()
 			.wrapping_add(count.get())
@@ -167,11 +171,10 @@ impl AddressSpace {
 			.cast();
 		let unmap_range = base..=NonNull::new(end).unwrap();
 		if &unmap_range == range {
-			objects.remove(i);
+			Ok(Some(objects.remove(i).1))
 		} else {
 			todo!("partial unmap");
 		}
-		Ok(())
 	}
 
 	/// Map a virtual address to a physical address.

kernel/src/object_table/job.rs

@@ -71,7 +71,7 @@ impl JobTask {
 
 impl Drop for JobTask {
 	fn drop(&mut self) {
-		match mem::replace(&mut *self.shared.lock(), JobInner::Cancelled) {
+		match mem::replace(&mut *self.shared.auto_lock(), JobInner::Cancelled) {
 			JobInner::Active { job, table, .. } => {
 				job.map(|job| Weak::upgrade(&table).map(|t| t.cancel_job(job.0)));
 			}
@@ -84,7 +84,7 @@ impl Future for JobTask {
 	type Output = Result<(JobId, JobRequest), Cancelled>;
 
 	fn poll(self: Pin<&mut Self>, cx: &mut Context) -> Poll<Self::Output> {
-		match &mut *self.shared.lock() {
+		match &mut *self.shared.auto_lock() {
 			JobInner::Active { waker, job, .. } => {
 				if let Some(s) = job.take() {
 					Poll::Ready(Ok(s))

kernel/src/object_table/root.rs

@@ -1,5 +1,5 @@
 use super::{Error, Object, Query, QueryResult, StreamingTable, Ticket};
-use crate::sync::Mutex;
+use crate::sync::SpinLock;
 use alloc::{
 	boxed::Box,
 	collections::BTreeMap,
@@ -22,28 +22,28 @@ use alloc::{
 /// process/
 /// ```
 pub struct Root {
-	objects: Mutex<BTreeMap<Box<[u8]>, Weak<dyn Object>>>,
+	objects: SpinLock<BTreeMap<Box<[u8]>, Weak<dyn Object>>>,
 }
 
 impl Root {
 	/// Create a new root
 	pub fn new() -> Self {
 		Self {
-			objects: Mutex::new(BTreeMap::new()),
+			objects: SpinLock::new(BTreeMap::new()),
 		}
 	}
 
 	/// Add a new object to the root.
 	pub fn add(&self, name: impl Into<Box<[u8]>>, object: Weak<dyn Object>) {
-		self.objects.lock_unchecked().insert(name.into(), object);
+		self.objects.auto_lock().insert(name.into(), object);
 	}
 
 	fn find<'a>(&self, path: &'a [u8]) -> Option<(Arc<dyn Object>, &'a [u8], &'a [u8])> {
 		let (object, rest) = path
 			.iter()
 			.position(|c| *c == b'/')
 			.map_or((path, &b""[..]), |i| (&path[..i], &path[i + 1..]));
-		let mut objects = self.objects.lock();
+		let mut objects = self.objects.auto_lock();
 		if let Some(obj) = objects.get(object) {
 			if let Some(obj) = Weak::upgrade(&obj) {
 				Some((obj, object, rest))
@@ -70,7 +70,7 @@ impl Object for Root {
 			impl<I: Iterator<Item = Ticket<QueryResult>>> Query for Q<I> {}
 
 			// Filter any dead objects before querying.
-			let mut objects = self.objects.lock();
+			let mut objects = self.objects.auto_lock();
 			objects.retain(|_, v| v.strong_count() > 0);
 			Ticket::new_complete(Ok(Box::new(Q(objects
 				.keys()
@@ -104,7 +104,7 @@ impl Object for Root {
 				Ticket::new_complete(if path.contains(&b'/') {
 					Err(Error::DoesNotExist)
 				} else {
-					let mut objects = self.objects.lock();
+					let mut objects = self.objects.auto_lock();
 					let tbl = StreamingTable::new() as Arc<dyn Object>;
 					let r = objects.insert(path.into(), Arc::downgrade(&tbl));
 					assert!(r.is_none());

kernel/src/object_table/streaming.rs

@@ -34,7 +34,7 @@ impl StreamingTable {
 		let job_id = self.job_id_counter.fetch_add(1, Ordering::Relaxed);
 
 		loop {
-			let j = self.job_handlers.lock().pop();
+			let j = self.job_handlers.auto_lock().pop();
 			if let Some(w) = j {
 				if let Ok(mut w) = w.lock() {
 					self.tickets
@@ -44,7 +44,7 @@ impl StreamingTable {
 					break;
 				}
 			} else {
-				let mut l = self.jobs.lock();
+				let mut l = self.jobs.auto_lock();
 				l.push((StreamJob { job_id, job }, Some(ticket_waker.into()), prefix));
 				break;
 			}
@@ -76,20 +76,20 @@ impl StreamingTable {
 
 impl Table for StreamingTable {
 	fn take_job(self: Arc<Self>, _timeout: Duration) -> JobTask {
-		let job = self.jobs.lock().pop().map(|(job, tkt, prefix)| {
+		let job = self.jobs.auto_lock().pop().map(|(job, tkt, prefix)| {
 			tkt.map(|tkt| self.tickets.auto_lock().push((job.job_id, tkt, prefix)));
 			(job.job_id, job.job)
 		});
 		let s = Arc::downgrade(&self);
 		let (job, waker) = JobTask::new(s, job);
-		self.job_handlers.lock().push(waker);
+		self.job_handlers.auto_lock().push(waker);
 		job
 	}
 
 	fn finish_job(self: Arc<Self>, job: Result<JobResult, Error>, job_id: JobId) -> Result<(), ()> {
 		let (tw, mut prefix);
 		{
-			let mut c = self.tickets.lock();
+			let mut c = self.tickets.auto_lock();
 			let mut c = c.drain_filter(|e| e.0 == job_id);
 			(_, tw, prefix) = c.next().ok_or(())?;
 			assert!(c.next().is_none());

kernel/src/object_table/ticket.rs

@@ -73,7 +73,7 @@ impl<T> Future for Ticket<T> {
 	type Output = Result<T, Error>;
 
 	fn poll(self: Pin<&mut Self>, cx: &mut Context) -> Poll<Self::Output> {
-		let mut t = self.inner.lock();
+		let mut t = self.inner.auto_lock();
 		if let Some(s) = t.status.take() {
 			return Poll::Ready(s);
 		}

kernel/src/scheduler/process/elf.rs

@@ -102,8 +102,8 @@ impl super::Process {
 			core::slice::from_raw_parts(data[0].as_ptr().cast::<u8>(), Page::SIZE * data.len())
 		};
 
-		let slf = Self::new()?;
-		*slf.objects.lock_unchecked() = objects;
+		let slf = Self::new().map_err(ElfError::AllocateError)?;
+		*slf.objects.auto_lock() = objects;
 
 		(data.len() >= 16)
 			.then(|| ())

kernel/src/scheduler/process/io.rs

@@ -111,8 +111,8 @@ impl super::Process {
 			.find(|q| q.user_ptr == base)
 			.ok_or(ProcessQueueError::InvalidAddress)?;
 
-		let mut objects = self.objects.lock();
 		let mut queries = self.queries.lock();
+		let mut objects = self.objects.lock();
 
 		// Poll tickets first as it may shrink the ticket Vec.
 		poll_tickets(queue, &mut objects, &mut queries);
@@ -382,9 +382,13 @@ impl super::Process {
 				break;
 			}
 
-			let polls = poll_tickets(queue, &mut self.objects.lock(), &mut self.queries.lock());
-			if polls > 0 {
-				break;
+			{
+				let mut queries = self.queries.lock();
+				let mut objects = self.objects.auto_lock();
+				let polls = poll_tickets(queue, &mut objects, &mut queries);
+				if polls > 0 {
+					break;
+				}
 			}
 
 			// Prevent blocking other threads.

kernel/src/scheduler/process/mod.rs

@@ -22,7 +22,7 @@ pub struct Process {
 	address_space: SpinLock<AddressSpace>,
 	hint_color: u8,
 	threads: SpinLock<Arena<Arc<Thread>, u8>>,
-	objects: Mutex<Arena<Arc<dyn Object>, u8>>,
+	objects: SpinLock<Arena<Arc<dyn Object>, u8>>,
 	queries: Mutex<Arena<Box<dyn Query>, u8>>,
 	io_queues: Mutex<Vec<io::Queue>>,
 }
@@ -52,7 +52,7 @@ impl From<JobTask> for TicketOrJob {
 }
 
 impl Process {
-	fn new() -> Result<Self, frame::AllocateContiguousError> {
+	fn new() -> Result<Self, frame::AllocateError> {
 		Ok(Self {
 			address_space: SpinLock::new(AddressSpace::new()?),
 			hint_color: 0,

kernel/src/sync/mutex.rs

@@ -24,11 +24,6 @@ impl<T> Mutex<T> {
 			crate::arch::interrupts_enabled(),
 			"interrupts are disabled. Is the mutex being locked inside an ISR?"
 		);
-		self.lock_unchecked()
-	}
-
-	#[track_caller]
-	pub fn lock_unchecked(&self) -> Guard<T> {
 		// TODO detect double locks by same thread
 		loop {
 			match self
@@ -81,6 +76,11 @@ impl<T> DerefMut for Guard<'_, T> {
 
 impl<T> Drop for Guard<'_, T> {
 	fn drop(&mut self) {
+		debug_assert_ne!(
+			self.lock.lock.load(Ordering::Relaxed),
+			0,
+			"lock was released"
+		);
 		self.lock.lock.store(0, Ordering::Release);
 	}
 }

kernel/src/sync/spinlock.rs

@@ -140,6 +140,11 @@ impl<T> DerefMut for Guard<'_, T> {
 impl<T> Drop for Guard<'_, T> {
 	fn drop(&mut self) {
 		ensure_interrupts_off();
+		debug_assert_ne!(
+			self.lock.lock.load(Ordering::Relaxed),
+			0,
+			"lock was released"
+		);
 		self.lock.lock.store(0, Ordering::Release);
 		crate::arch::enable_interrupts();
 	}
@@ -162,6 +167,11 @@ impl<T> DerefMut for IsrGuard<'_, T> {
 impl<T> Drop for IsrGuard<'_, T> {
 	fn drop(&mut self) {
 		ensure_interrupts_off();
+		debug_assert_ne!(
+			self.lock.lock.load(Ordering::Relaxed),
+			0,
+			"lock was released"
+		);
 		self.lock.lock.store(0, Ordering::Release);
 	}
 }