Repository navigation
Conversation
| /// | ||
| /// # Context | ||
| /// | ||
| /// This function might sleep, don't call in atomic contexts. |
There was a problem hiding this comment.
nit: we're trying to avoid the imperative mode in documentation, so instead of having "do this" or "don't do that", we'd rather have something like "Callers must not be in atomic context because this might sleep".
| /// [`std::thread::yield_now`]: https://doc.rust-lang.org/std/thread/fn.yield_now.html | ||
| #[doc(alias = "yield_now")] | ||
| pub fn schedule() { | ||
| // SAFETY: If we can schedule back from other thread, then this can be treated as a no-op. A |
There was a problem hiding this comment.
Can we violate safety by calling schedule?
If we call this from atomic context, we may deadlock the system, which doesn't violate memory safety. If we call it with state set to TASK_DEAD, the function never returning doesn't violate safety either, does it? Is there anything else that we could do?
There was a problem hiding this comment.
It's about the safety of thread users.
We can treat
set_special_state(TASK_DEAD);
schedule();
, as a decrease on the ref-count of the task_struct.
Normally it's OK if we maintain all references to task_struct via ref-counts. However, there are a few places in kernel C side where a task_struct is referenced with raw pointer. The assumption is that no one should call do_exit() or set TASK_DEAD unless they are writing task_struct or kthread code. If Rust code is allowed to freely do_exit() or set TASK_DEAD, then C side will have dangling pointers to task_struct.
For example, in workqueue, a worker (kernel thread) is referenced without any ref-count addition, so if a Future sets TASK_DEAD and schedule, the worker thread may get freed. And the memory will get corrupted.
There was a problem hiding this comment.
It's about the safety of thread users.
We can treat
set_special_state(TASK_DEAD); schedule();, as a decrease on the ref-count of the
task_struct.Normally it's OK if we maintain all references to
task_structvia ref-counts. However, there are a few places in kernel C side where atask_structis referenced with raw pointer. The assumption is that no one should calldo_exit()or setTASK_DEADunless they are writingtask_structorkthreadcode. If Rust code is allowed to freelydo_exit()or setTASK_DEAD, then C side will have dangling pointers totask_struct.For example, in workqueue, a worker (kernel thread) is referenced without any ref-count addition, so if a
FuturesetsTASK_DEADandschedule, the worker thread may get freed. And the memory will get corrupted.
I must admit I did think this far originally. It just looked very dangerous to me if we allow Rust code to set the current thread state to TASK_DEAD. I will update the SAFETY: comments
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
| // `TASK_DEAD` + `schedule`. We can make [`Task::schedule`] because of this, but it doesn't | ||
| // provide extra safety if we make setting `TASK_DEAD` unsafe. And given [`Task::schedule`] | ||
| // may be widely used, let's make it safe and require all `TASK_DEAD` settings to take great | ||
| // caution. |
There was a problem hiding this comment.
End up kinda saying "the safety is a design choice", I'm not happy about this, but it's the best I can get.
Signed-off-by: Boqun Feng boqun.feng@gmail.com