What? I think people think "rust without unsafe" eliminates certain classes of bugs. Are we really going to imply that people don't understand that "unsafe" labeled code is ... uh.. possibly unsafe? I don't believe that these mythical "naive" people exist who think code explicitly labelled unsafe is still safe.
I don't think this code needed to be unsafe. This code doesn't involve any I/O or kernel pointer/CPU register management; it's just modifying a list.
I'm sure the people who wrote this code had their reasons to write the code like this (probably simplicity or performance), but this type of kernel code could be done safely.
As pointed out by yuriks [0], it seems the patch authors are interested in looking into a safer solution [1]:
> The previous patches in this series illustrate why the List::remove
method is really dangerous. I think the real takeaway here is to replace
the linked lists with a different data structure without this unsafe
footgun, but for now we fix the bugs and add a warning to the docs.
You can’t write rust code without relying on unsafe code. Much of the standard library contains unsafe, which they have in parts taken the time to formally verify.
I would presume the ratio of safe to unsafe code leads to less unsafe code being written over time as the full ”kernel standard library” gets built out allowing all other parts to replace their hand rolled implementations with the standard one.
These mythical people are here in the comment section with us. All of them appear to be Rust detractors that are incapable of grasping that one line of unsafe code amongst nine lines of provably-safe code is somehow no better than ten lines of unsafe C.
The SAFETY comment is just a brief description of the important points the author considered when writing the block, and perhaps points you need to consider if you modify it. Do people just blindly assume that comments in an algorithm are correct and not misleading? In other languages they don't, I don't see why rust'd be any different.
A SAFETY comment is supposed to justify why the unsafe code is sound. Here it justified the wrong thing. Ownership was not the problem, concurrent mutation was. That is exactly the kind of gap a SAFETY comment can hide by giving a false sense that the hard parts were already considered.
The fact that this survived review is the worrying part. Unsafe blocks are intentionally small and localized in Rust precisely so the safety argument can be checked. If the stated safety argument is incomplete and still passes review, that suggests reviewers are relying on the comment as the proof, rather than rederiving the invariants themselves. Unless of course the wrong people are reviewing these changes. Why rewrite in Rust if we don't apply extreme scrutiny to the tiny subset (presumably) that should be scrutinized.
To be clear, I think this is a failure of process, not Rust of course.
> Ownership was not the problem, concurrent mutation was.
I think the safety comment might have been more on-point than you think. If you look at the original code, it did something like:
- Take a lock
- Swap a `Node`'s `death_list` (i.e., a list of `NodeDeath`s) with an empty one
- Release the lock
- Iterate over the taken `death_list`
While in another thread, you have a `NodeDeath`:
- Take a lock
- Get its parent's `death_list`
- Remove itself from said list.
- Release the lock
The issue is what happens when a `NodeDeath` from the original list tries to remove itself after the parent Node swapped its `death_list`. In that case, the `NodeDeath` grabs the replacement list from its parent node, and the subsequent attempt to remove itself from the replacement list violates the precondition in the safety comment.
> Why rewrite in Rust if we don't apply extreme scrutiny to the tiny subset (presumably) that should be scrutinized.
That "extreme scrutiny" was applied does not guarantee that all possible bugs will be found. Humans are only human, after all.
