Use `MAP_PRIVATE` (not unsound-prone `MAP_SHARED`) #122309

g-yziquel · 2024-03-10T20:36:19Z

rustbot · 2024-03-10T20:36:27Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

workingjubilee · 2024-03-10T20:55:22Z

Commands to the bots go in the PR description, not the title. Please prefer concise and descriptive PR titles.

r? @saethlin

saethlin · 2024-03-10T23:35:46Z

compiler/rustc_data_structures/src/memmap.rs

@@ -19,7 +19,7 @@ impl Mmap {
    #[inline]
    pub unsafe fn map(file: File) -> io::Result<Self> {
        // Safety: the caller must ensure that this is safe.
-        unsafe { memmap2::Mmap::map(&file).map(Mmap) }
+        unsafe { memmap2::MmapOptions::new().map_copy_read_only(&file).map(Mmap) }


Can you add a comment that explains that the default MmapOptions will pass though updates to the file, wheras this setting might pass through updates, but we really prefer the updates-are-optional version because it is more widely supported.

@saethlin You may now check whether or not the modifications to the PR made (git commit message and comment) are suitable.

In my opinion, the comment is too long, but I'm a bit at a loss to make it shorter. Please advise.

@rustbot review

This comment should be in the function body as a // comment, not as a doc comment with /// outside, because it describes an implementation detail not the API.

Your comment is long because it's too specific. memmap2 supports Windows, so talking about flags to the mmap syscall is speaking to the wrong layer of abstraction here. Instead, say something like:

// The default behavior of `memmap2` is to map such that updates to the file // are visible in the mapping. We don't want updates, so all we're doing // here is turning off an unused feature.

For the second paragraph I'd just say something like

// Most importantly, this `memmap2` call is more widely-supported: https://github.com/rust-lang/rust/issues/122262

Your explanation in the issue is quite thorough, and I wouldn't bother hurting yourself by trying to cram too much of it into a comment. We're turning off an unused feature to work on more systems. That's good enough for a casual reader.

Rather than slowly going back and forth here, I'm going to reword the comment in a way that I think we'd both be happy with and approve this.

@saethlin Fine with me. Thank you. Sorry for the delay.

However, given the backlash that raising this issue has raised (i.e. more or less complaints on #irc about being kind of an asshole), I do feel obliged to state that I regret having made this PR, and I do not feel inclined to participate any more to the rust community.

I did not come here for that, and am not willing to tolerate some behaviours.

Wow. Today's developments are even more impressive in terms of intolerance on #irc. This is definitely a community to stay away from.

What IRC are you on? If you have trouble with any of the official Rust spaces, please contact the moderation team: https://www.rust-lang.org/governance/teams/moderation

I'm not going through official complaint procedures. These people banned me. They are free to behave like abusive -------- for petty reasons. I'm just making it known that I also have limits to my tolerance. I hope this is my last message on anything related officially to rust.

Please let that be the last message on this thread: it's already polluted enough, and has been as soon as I mentioned in the upstream issue that people on irc were opposed to the change I proposed.

In two weeks involved with rust, I have met only opposition and scorn wrapped in fake netiquette. I'm done.

The language is really cool and something I'd recommend. The people, however, are not. At times, being nonjudgemental is nothing else than leniency, and this, for me, would be such a time.

Let's all drop it. Final message.

And feel free to revert that commit if any heat comes that way.

the8472 · 2024-03-10T23:44:30Z

The PR description is misleading. It does not really fix the soundness issue. And it's clearly motivated by compatibility with a buggy or incomplete filesystem implementation. I'm not a fan, but at least the PR should be honest about what's doing.

workingjubilee · 2024-03-11T00:17:14Z

@the8472 My choice in direction to update the title was based on my interpretation of @saethlin's remarks here that solicited the PR:

Virtualised cargo fails with a "No such device" error n°19 likely because of MAP_SHARED mmap call on qemu + virtiofsd with --cache=never. #122262 (comment)
Virtualised cargo fails with a "No such device" error n°19 likely because of MAP_SHARED mmap call on qemu + virtiofsd with --cache=never. #122262 (comment)

Perhaps I have misinterpreted @saethlin's remarks, but in any case, I only sought to choose something marginally preferable to the first draft when I edited it. I will leave further edits to the contributor, but please be more precise than "doesn't really fix".

the8472 · 2024-03-11T00:33:53Z

As saethlin said:

So if anything, MAP_PRIVATE is better because it's possible that a modification to the file wouldn't update the mapping and cause UB.

The distinction between MAP_PRIVATE and MAP_SHARED only comes into play when the private mapping is touched with write-intent, only then the CoW mapping becomes unshared. If other processes write to it through write syscalls or through a shared mapping then the CoW will not be broken. To my knowledge rustc never does touch those pages with write intent.
So while in the broad abstract private mappings can reduce the possibility of writes from other processes becoming visible that is not relevant to the way rust is using it since we only take immutable borrows from those mappings.
Which in turn means if there hypothetically were another process corrupting the files we'd still see immutable data change under our noses. So the UB is still there as it ever was, but this has already been deemed outside-rustc-robustness-scope in previous discussions.

Based on the linked issue the PR aims to do is to change the syscall flag to accommodate a filesystem that doesn't offer full posix support and this is not reflected in the description or the code comments. (Edit) The commit message seems more appropriate, though a bit terse.

saethlin · 2024-03-11T01:02:00Z

The PR description is misleading. It does not really fix the soundness issue. And it's clearly motivated by compatibility with a buggy or incomplete filesystem implementation. I'm not a fan, but at least the PR should be honest about what's doing.

@the8472 If you take issue with the PR title (I don't see how a reasonable person could take issue with the description) please suggest an alternative instead of being vague with a first-time contributor who is still learning about the Rust concept of soundness.

the8472 · 2024-03-11T01:10:26Z

Yes, I mean the title. And it was modified by jubilee. The previous content was better, apart from the bot command.

workingjubilee · 2024-03-11T01:46:44Z

@the8472 Given the breadth of what "virtualization" includes, including many subtly and not-so-subtly varying goals and methods to achieve it, that data point seemed pointlessly vague to me. It's like opening an LLVM bump PR and saying it's "for multiplication". I suppose that is technically true? In practice it is misleadingly vague, so it remains surprising to me that you would say anything was "clearly" anything.

technetos · 2024-03-11T02:51:30Z

Hey folks, please focus on the PR contents at hand and be constructive and helpful when commenting.

…tems. Adding support of quirky filesystems occuring in virtualised settings not having full POSIX support for memory mapped files. Example: current virtiofs with cache disabled, occuring in Incus/LXD or Kata Containers. Has been hitting various virtualised filesystems since 2016, depending on their levels of maturity at the time. The situation will perhaps improve when virtiofs DAX support patches will have made it into the qemu mainline. On a reliability level, using the MAP_PRIVATE sycall flag instead of the MAP_SHARED syscall flag for the mmap() system call does have some undefined behaviour when the caller update the memory mapping of the mmap()ed file, but MAP_SHARED does allow not only the calling process but other processes to modify the memory mapping. Thus, in the current context, using MAP_PRIVATE copy-on-write is marginally more reliable than MAP_SHARED. This discussion of reliability is orthogonal to the type system enforced safety policy of rust, which does not claim to handle memory modification of memory mapped files triggered through the operating system and not the running rust process.

saethlin · 2024-03-15T22:38:08Z

@bors r+

bors · 2024-03-15T22:38:11Z

📌 Commit 3fc5ed8 has been approved by saethlin

It is now in the queue for this repository.

bors · 2024-03-16T09:19:11Z

⌛ Testing commit 3fc5ed8 with merge 774ae59...

bors · 2024-03-16T11:17:30Z

☀️ Test successful - checks-actions
Approved by: saethlin
Pushing 774ae59 to master...

rust-timer · 2024-03-16T13:30:05Z

Finished benchmarking commit (774ae59): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	4.9%	[4.9%, 4.9%]	1
Regressions ❌ (secondary)	2.8%	[2.8%, 2.8%]	1
Improvements ✅ (primary)	-4.0%	[-4.9%, -3.1%]	2
Improvements ✅ (secondary)	-2.3%	[-3.5%, -0.5%]	9
All ❌✅ (primary)	-1.0%	[-4.9%, 4.9%]	3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.2%	[1.2%, 1.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 671.724s -> 671.684s (-0.01%)
Artifact size: 311.58 MiB -> 311.57 MiB (-0.00%)

rustbot assigned petrochenkov Mar 10, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 10, 2024

rustbot assigned saethlin and unassigned petrochenkov Mar 10, 2024

workingjubilee changed the title ~~Issue 12262: r? saethlin - Using MAP_PRIVATE (not MAP_SHARED) for virtualisation.~~ Use MAP_PRIVATE (not unsound-prone MAP_SHARED) Mar 10, 2024

saethlin reviewed Mar 10, 2024

View reviewed changes

g-yziquel force-pushed the issue-122262 branch 3 times, most recently from 559ac5d to a182920 Compare March 13, 2024 08:51

saethlin force-pushed the issue-122262 branch from a182920 to 3fc5ed8 Compare March 15, 2024 22:36

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 15, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 16, 2024

bors merged commit 774ae59 into rust-lang:master Mar 16, 2024
12 checks passed

rustbot added this to the 1.78.0 milestone Mar 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `MAP_PRIVATE` (not unsound-prone `MAP_SHARED`) #122309

Use `MAP_PRIVATE` (not unsound-prone `MAP_SHARED`) #122309

g-yziquel commented Mar 10, 2024

rustbot commented Mar 10, 2024

workingjubilee commented Mar 10, 2024

saethlin Mar 10, 2024

gl-yziquel Mar 11, 2024

gl-yziquel Mar 11, 2024 •

edited

Loading

gl-yziquel Mar 11, 2024

saethlin Mar 11, 2024

saethlin Mar 15, 2024

g-yziquel Mar 21, 2024

gl-yziquel Mar 22, 2024

saethlin Mar 22, 2024

gl-yziquel Mar 22, 2024

the8472 commented Mar 10, 2024

workingjubilee commented Mar 11, 2024

the8472 commented Mar 11, 2024 •

edited

Loading

saethlin commented Mar 11, 2024

the8472 commented Mar 11, 2024

workingjubilee commented Mar 11, 2024

technetos commented Mar 11, 2024

saethlin commented Mar 15, 2024

bors commented Mar 15, 2024

bors commented Mar 16, 2024

bors commented Mar 16, 2024

rust-timer commented Mar 16, 2024

Use MAP_PRIVATE (not unsound-prone MAP_SHARED) #122309

Use MAP_PRIVATE (not unsound-prone MAP_SHARED) #122309

Conversation

g-yziquel commented Mar 10, 2024

rustbot commented Mar 10, 2024

workingjubilee commented Mar 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gl-yziquel Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

the8472 commented Mar 10, 2024

workingjubilee commented Mar 11, 2024

the8472 commented Mar 11, 2024 • edited Loading

saethlin commented Mar 11, 2024

the8472 commented Mar 11, 2024

workingjubilee commented Mar 11, 2024

technetos commented Mar 11, 2024

saethlin commented Mar 15, 2024

bors commented Mar 15, 2024

bors commented Mar 16, 2024

bors commented Mar 16, 2024

rust-timer commented Mar 16, 2024

Overall result: no relevant changes - no action needed

Use `MAP_PRIVATE` (not unsound-prone `MAP_SHARED`) #122309

Use `MAP_PRIVATE` (not unsound-prone `MAP_SHARED`) #122309

gl-yziquel Mar 11, 2024 •

edited

Loading

the8472 commented Mar 11, 2024 •

edited

Loading