Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an IS_SCRATCH flag to mark scratch memory with. #3792

Closed
wants to merge 1 commit into from

Conversation

khuey
Copy link
Collaborator

@khuey khuey commented Jul 28, 2024

Fixes #3779

@khuey khuey requested a review from rocallahan July 28, 2024 19:38
@khuey
Copy link
Collaborator Author

khuey commented Jul 28, 2024

I'm not entirely convinced this is sufficient. In #3779 it was reported that two scratch memory segments ended up being adjacent to each other. The reporter claims this patch fixes his issue. But it's not clear to me that this actually prevents coalescing adjacent scratch maps in all circumstances. is_coalescable() checks that the two flags are equal to each other (

return mleft.flags == mright.flags;
) but other comments suggest that mappings with flags should never be coalesced (
ASSERT(t, !mapping.flags);
). Is there something I'm missing that prevents maps from being merged when is_coalescable() is true? Or is the reporter just getting lucky with their test case.

@rocallahan
Copy link
Collaborator

Is there something I'm missing that prevents maps from being merged when is_coalescable() is true?

I don't think you're missing anything.

What if we fix this a different way? In inject_handled_signal() we could grab the current contents of the memory where the sigframe is going to appear, and then we could diff it against the memory after the singlestep has caused the kernel to write the sigframe. Then we can record only the changed range.

I think that would solve our replay issues, but ... what if the kernel actually uses the scratch mapping to store part of the sigframe? I don't think there's anything preventing that from happening, right? Which suggests we actually need a real guard page to stop that.

@khuey khuey closed this Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assertion `nwritten == buf_size' failed to hold (Again)
2 participants