kern: Make stack zap failure less catastrophic #1947
Conversation
There was a problem hiding this comment.
I agree that a failure to scribble over the stack should probably not take down the whole system, so this looks good to me! It might be nice if there were some comments explaining why we allow this to fail in the source, for future readers? Not a huge deal though.
| @@ -309,15 +309,14 @@ pub fn reinitialize(task: &mut task::Task) { | |||
| .iter() | |||
| .find(|region| region.contains(initial_stack.saturating_sub(4))) | |||
| { | |||
| let mut uslice: USlice<u32> = USlice::from_raw( | |||
| if let Ok(mut uslice) = USlice::<u32>::from_raw( | |||
There was a problem hiding this comment.
mmmaybe worth a comment here to the effect of "if this fails, don't crash the whole system, as it's just for diagnostic purposes"? also, perhaps we should mention why it might fail here?
| for word in zap.iter_mut() { | ||
| *word = 0xbaddcafe; | ||
| ) { | ||
| let zap = task.try_write(&mut uslice).unwrap_lite(); |
There was a problem hiding this comment.
AFAICT, the try_write call failing here would be Bad News --- if a task's stack region isn't actual memory or that memory isn't writable, something has gone seriously wrong, so I think unwrapping this is reasonable. but, that might not be immediately obvious to a reader, who would see that we allow the Uslice::from_raw call to fail, but if we can't write to it, we throw up our hands and kill the whole system, which might look weird at a glance. Maybe a comment describing the difference between the two failure modes would be worth including here?
There was a problem hiding this comment.
Yeah, that was approximately my thinking with leaving the unwrap, but you're right that a comment would improve things.
|
Comments added, PTAL. |
cbiffle
force-pushed
the
cbiffle/tolerate-stack-zap-failure
branch
from
c2951ae to
bbe264a
Compare
The kernel really wants to scribble task memory. Like, really really. Like, contains
unwrap()s about it.This is silly. It's a diagnostic tool. The kernel doesn't care if it works or not, and the application should not fail to start because of it.
So, this commit makes it optional and tolerates failure.
Is this me papering over a kernel bug? Yes, #1943 to be precise. But it's reasonable paper.