Skip to content

Comments

Add support for MADV_GUARD_{INSTALL,REMOVE}.#3999

Merged
khuey merged 1 commit intorr-debugger:masterfrom
emilio:madv-guard
Aug 1, 2025
Merged

Add support for MADV_GUARD_{INSTALL,REMOVE}.#3999
khuey merged 1 commit intorr-debugger:masterfrom
emilio:madv-guard

Conversation

@emilio
Copy link
Contributor

@emilio emilio commented Aug 1, 2025

Fixes #3997

@emilio
Copy link
Contributor Author

emilio commented Aug 1, 2025

Seems like the failure (arm64 failure in alternate_thread_diversion-no-syscallbuf) is not related to this patch.

@khuey
Copy link
Collaborator

khuey commented Aug 1, 2025

This doesn't actually work when the tracee touches the guard page. These are not being executed during replay (see process_madvise() in replay_syscall.cc). rr expects all SEGV_MAPERR SIGSEGVs to be DETERMINISTIC_SIGs (that is, to be recreated by normal program execution as opposed to having to be injected by rr) (see is_deterministic_signal()), and not executing MADV_GUARD_{INSTALL,REMOVE} during replay breaks that.

However, executing MADV_GUARD_{INSTALL,REMOVE} during replay is a trace portability hazard, because a trace that uses them will no longer replay on a kernel without them. In particular it means these traces won't work with the hosted version of Pernosco, which is running on a pre-6.13 kernel. So instead I think we would need to track the guard regions during recording and if we see a SEGV_MAPERR in one record it as a NONDETERMINISTIC_SIG so rr knows it has to inject that signal. This also requires handling the fork, mremap, partial unmap, etc behavior described in the man page (and assuming the man page is an accurate description of the kernel's behavior).

That brings me to my real point: is this actually worth it? As far as I can tell the point of MADV_GUARD_INSTALL is just to avoid a mprotect(PROT_NONE) and the resulting vma. That seems like a relatively minor benefit, especially since any code trying to do this is going to have a fallback for older kernels. Why don't you just emulate_result(-EINVAL) for MADV_GUARD_{INSTALL,REMOVE}?

Regardless, this needs real tests. And please fix the indentation in the constant definitions to match our style.

As an aside, I tried to trigger #3997 on tip firefox on 6.15 and couldn't. Is it glibc that's using this? If so, which version?

Copy link
Collaborator

@khuey khuey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the reasons previously described this can't be merged as is.

@emilio
Copy link
Contributor Author

emilio commented Aug 1, 2025

As an aside, I tried to trigger #3997 on tip firefox on 6.15 and couldn't. Is it glibc that's using this? If so, which version?

Yes, seems to be. glibc 2.42

@emilio
Copy link
Contributor Author

emilio commented Aug 1, 2025

That brings me to my real point: is this actually worth it? As far as I can tell the point of MADV_GUARD_INSTALL is just to avoid a mprotect(PROT_NONE) and the resulting vma. That seems like a relatively minor benefit, especially since any code trying to do this is going to have a fallback for older kernels. Why don't you just emulate_result(-EINVAL) for MADV_GUARD_{INSTALL,REMOVE}?

I think that's fair, can try.

@emilio
Copy link
Contributor Author

emilio commented Aug 1, 2025

@khuey does it look better now? Any suggestions for the tests?

Copy link
Collaborator

@khuey khuey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks better. Do we really need the (uintptr_t) cast?

If we're just stubbing it out I think the DO_MADVISE test is fine.

@emilio
Copy link
Contributor Author

emilio commented Aug 1, 2025

I don't think so, no. Removed the cast.

@khuey khuey merged commit 34ff3a7 into rr-debugger:master Aug 1, 2025
5 checks passed
@emilio emilio deleted the madv-guard branch August 1, 2025 22:56
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.

Recording Firefox no longer works after kernel update

2 participants