1011350 - Distinguish anonymous mappings in SystemMemoryReporter with prctl(PR_SET_VMA, PR_SET_VMA_ANON_NAME, ...)

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

In bug 947072 I tried to use a file-based scheme to tag anonymous memory mappings as coming from different places (e.g. jemalloc, JS heap, etc). It ended up being a bit too invasive and complex.

But jld discovered that there's a kernel patch that implements a variant of prctl() that lets you annotate anonymous mappings in exactly this fashion. The description is here: https://lkml.org/lkml/2013/10/14/789.

As for which devices have or could have this patch, jld said:

> On hamachi/buri you're out of luck, unless someone manages to
> reverse-engineer how to rebuild the kernel and boot image such that it won't
> be rejected by the bootloader on images newer than FxOS 1.1 or so.
>
> Devices that already have the patch: nexus-4-kk, nexus-5, dolphin.
>
> Devices where I know we can rebuild the kernels and the merge conflicts don't
> look too bad: emulator-kk/emulator-x86-kk, flame (pending bug 1004195), keon,
> peak.

This can easily be implemented in such a way that kernels lacking the prctl() support won't be affected -- in that case the prctl() calls will just harmlessly fail. (Or perhaps support can be determined at build time?)

Comment 1

[Mike Hommey [:glandium]]

> (Or perhaps support can be determined at build time?)

Not a good idea, generally.

Comment 2

[Thinker Li [:sinker]]

(In reply to Mike Hommey [:glandium] from comment #1)
> Not a good idea, generally.

I am curious why people favor run-time detection over build time setting for debug features?
They have been rarely used and introduced overhead.  How do we judge a thing to favor run-time detection?

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

I adapted the patch from bug 947072 in order to test the feature.  As a side effect, I have most of a patch for this bug.

Discovery: the patch also adds a "Name: " line to /proc/N/smaps for named anonymous memory which breaks the current (somewhat delicate) smaps parser.  In particular, this means it's probably already broken for Fennec on some currently shipping Android devices.

Comment 4

[Nicholas Nethercote [inactive]]

> Discovery: the patch also adds a "Name: " line to /proc/N/smaps for named
> anonymous memory which breaks the current (somewhat delicate) smaps parser. 
> In particular, this means it's probably already broken for Fennec on some
> currently shipping Android devices.

The smaps parser is only used by the SystemMemoryReporter, which is only on by default for B2G. So any brokenness is (a) not that surprising, and (b) not that bad.

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: Part 1: Add mfbt support for PR_SET_VMA_ANON_NAME.

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: Part 2: Use TaggedAnonymousMemory to distinguish our various mmap call sites.

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: Part 3: Collect TaggedAnonymousMemory info in SystemMemoryReporter.

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 8436222 [details] [diff] [review]
Part 1: Add mfbt support for PR_SET_VMA_ANON_NAME.

Review of attachment 8436222 [details] [diff] [review]:
-----------------------------------------------------------------

MFBT is moving to Gecko style (e.g. bug 1014377). Sorry that this isn't obvious... but please put all the '*'s on the left, and use |aFoo| for all the parameter names. (I think they were the only style violations of note.)

Other than that, it looks good. But I'm not an MFBT peer, so I'm r?ing froydnj for general MFBT-ness.

::: mfbt/TaggedAnonymousMemory.cpp
@@ +8,5 @@
> +
> +#include "mozilla/TaggedAnonymousMemory.h"
> +
> +#include "mozilla/Atomics.h"
> +#include "mozilla/Assertions.h"

Nit: might as well do them in alphabetical order.

@@ +14,5 @@
> +#include <sys/types.h>
> +#include <sys/mman.h>
> +#include <sys/prctl.h>
> +#include <sys/syscall.h>
> +#include <unistd.h>

Ditto.

@@ +17,5 @@
> +#include <sys/syscall.h>
> +#include <unistd.h>
> +
> +#ifndef PR_SET_VMA
> +#define PR_SET_VMA		0x53564d41

Does this deserve a comment?

@@ +21,5 @@
> +#define PR_SET_VMA		0x53564d41
> +#endif
> +
> +#ifndef PR_SET_VMA_ANON_NAME
> +#define PR_SET_VMA_ANON_NAME		0

Ditto?

@@ +31,5 @@
> +// LD_PRELOAD interceptions, because such interceptions can perform
> +// reentrant calls to malloc and thus result in mozjemalloc deadlocks.
> +static inline
> +void *_mmap(void *addr, size_t length, int prot, int flags,
> +            int fd, off_t offset)

Nit: the |void*| should be on the same line as the |static inline|.

@@ +38,5 @@
> +  return (void *) syscall(SYS_mmap2, addr, length, prot, flags,
> +                              fd, offset >> 12);
> +#else
> +  return (void *) syscall(SYS_mmap, addr, length, prot, flags,
> +                              fd, offset);

Nit: indentation in both function calls is slightly wrong.

@@ +79,5 @@
> +int
> +MozTaggedMemorySupported(void)
> +{
> +  static mozilla::Atomic<int> cache(-1);
> +  int supported = cache;

Is there any reason not to merge |supported| and |cache|?

@@ +95,5 @@
> +MozTagAnonymousMemory(const void *ptr, size_t length, const char *tag)
> +{
> +  if (MozTaggedMemorySupported()) {
> +    // The end of the range will be rounded up, but the bottom must be
> +    // rounded down (and the length adjusted).

I had to read this a few times to realize that we're doing the rounding down of the bottom ourselves here, but the system call will do the rounding up of the end. Re-word to clarify this?

@@ +112,5 @@
> +{
> +  void* mapped = _mmap(addr, length, prot, flags, fd, offset);
> +  if (MozTaggedMemorySupported()
> +      && (flags & MAP_ANONYMOUS) == MAP_ANONYMOUS
> +      && mapped != MAP_FAILED) {

Gecko style is to put the |&&| at the end of the previous line. (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures)

::: mfbt/TaggedAnonymousMemory.h
@@ +23,5 @@
> +// NOTE: The pointer given as the "tag" argument MUST remain valid as
> +// long as the mapping exists.  The referenced string is read when
> +// /proc/<pid>/smaps or /proc/<pid>/maps is read, not when the tag is
> +// established, so freeing it or changing its contents will have
> +// unexpected results.

Add something like "Using a static string is probably best." ?

@@ +45,5 @@
> +MozTagAnonymousMemory(const void *ptr, size_t length, const char *tag);
> +
> +MFBT_API void*
> +MozTaggedAnonymousMmap(void *addr, size_t length, int prot, int flags,
> +                         int fd, off_t offset, const char *tag);

Nit: Indentation isn't quite right.

@@ +76,5 @@
> +}
> +
> +#endif // ANDROID
> +
> +

Nit: No need for two blank lines.

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 8436223 [details] [diff] [review]
Part 2: Use TaggedAnonymousMemory to distinguish our various mmap call sites.

Review of attachment 8436223 [details] [diff] [review]:
-----------------------------------------------------------------

glandium, see my comment on the mozjemalloc change below.

::: js/src/assembler/jit/ExecutableAllocatorPosix.cpp
@@ +44,5 @@
>  }
>  
>  ExecutablePool::Allocation ExecutableAllocator::systemAlloc(size_t n)
>  {
> +    void *allocation = MozTaggedAnonymousMmap(NULL, n, INITIAL_PROTECTION_FLAGS, MAP_PRIVATE | MAP_ANON, VM_TAG_FOR_EXECUTABLEALLOCATOR_MEMORY, 0, "js-jit-code");

It's so much nicer being able to use strings directly rather than having to define a bunch of tag constants elsewhere that subsequently get mapped to strings :)

::: memory/mozjemalloc/jemalloc.c
@@ +1559,5 @@
>  #endif
>  
> +#include "mozilla/Assertions.h"
> +#include "mozilla/Attributes.h"
> +#include "mozilla/TaggedAnonymousMemory.h"

This is going to be another obstacle to getting an unmodified jemalloc3 working in the future. I'm ok with crossing that bridge when we get to it (and possibly losing this tagging at that point) but glandium might have other ideas.

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 8436224 [details] [diff] [review]
Part 3: Collect TaggedAnonymousMemory info in SystemMemoryReporter.

Review of attachment 8436224 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/base/SystemMemoryReporter.cpp
@@ -174,5 @@
> -  enum ProcessSizeKind {
> -    AnonymousOutsideBrk  = 0,
> -    AnonymousBrkHeap     = 1,
> -    SharedLibrariesRX    = 2,
> -    SharedLibrariesRW    = 3,

Getting rid of these constants is nice.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8436223 [details] [diff] [review]
Part 2: Use TaggedAnonymousMemory to distinguish our various mmap call sites.

Tagging glandium for Nick's comment 9.

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 8436222 [details] [diff] [review]
Part 1: Add mfbt support for PR_SET_VMA_ANON_NAME.

Review of attachment 8436222 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with Nick's changes and the ones below?

::: mfbt/TaggedAnonymousMemory.cpp
@@ +67,5 @@
> +
> +  if (mask == 0) {
> +    uintptr_t pageSize = sysconf(_SC_PAGESIZE);
> +    mask = ~(pageSize - 1);
> +    MOZ_ASSERT((mask & pageSize) == pageSize, "Page size must be a power of 2!");

I think it is slightly better to operate just on pageSize here, something like (pageSize - 1) & pageSize == 0.  But I don't know that it matters much.

@@ +96,5 @@
> +{
> +  if (MozTaggedMemorySupported()) {
> +    // The end of the range will be rounded up, but the bottom must be
> +    // rounded down (and the length adjusted).
> +    uintptr_t addr = (uintptr_t)ptr;

Nit: can you make this reinterpret_cast<uintptr_t> for C++-ness and consistency with the cast below?

@@ +112,5 @@
> +{
> +  void* mapped = _mmap(addr, length, prot, flags, fd, offset);
> +  if (MozTaggedMemorySupported()
> +      && (flags & MAP_ANONYMOUS) == MAP_ANONYMOUS
> +      && mapped != MAP_FAILED) {

I think reversing the order of these tests reads slightly better, as well as being epsilon faster for non-anonymous maps, but this is your call.

::: mfbt/TaggedAnonymousMemory.h
@@ +15,5 @@
> +// separating malloc() heap from JS heap).
> +//
> +// Existing memory can be tagged with MozTagAnonymousMemory(); it will
> +// tag the range of complete pages containing the given interval, so
> +// use it with caution.  MozTaggedAnonymousMmap() can be used like

What is the reason for using with caution?  Just that you can tag more pages than just the ones you specify?

@@ +41,5 @@
> +extern "C" {
> +#endif
> +
> +MFBT_API void
> +MozTagAnonymousMemory(const void *ptr, size_t length, const char *tag);

This file should include appropriate headers for the definition of size_t, rather than cargo-culting from Types.h.

@@ +45,5 @@
> +MozTagAnonymousMemory(const void *ptr, size_t length, const char *tag);
> +
> +MFBT_API void*
> +MozTaggedAnonymousMmap(void *addr, size_t length, int prot, int flags,
> +                         int fd, off_t offset, const char *tag);

Likewise on including appropriate headers for the definition of off_t.

@@ +48,5 @@
> +MozTaggedAnonymousMmap(void *addr, size_t length, int prot, int flags,
> +                         int fd, off_t offset, const char *tag);
> +
> +MFBT_API int
> +MozTaggedMemorySupported(void);

Nit: this should be MozTaggedMemoryIsSupported.  Also, this should return bool, not int.

@@ +65,5 @@
> +static inline void*
> +MozTaggedAnonymousMmap(void *addr, size_t length, int prot, int flags,
> +                       int fd, off_t offset, const char *)
> +{
> +  return mmap(addr, length, prot, flags, fs, offset);

Likewise, bring in the headers for mmap.

Comment 13

[Nicholas Nethercote [inactive]]

> > +MFBT_API int
> > +MozTaggedMemorySupported(void);
> 
> this should return bool, not int.

I think jld made it an int because that header file is also used in C code, for jemalloc.c. Having said that, this particular function isn't used in C code, so perhaps it could be made a C++-only function and then return bool?

Comment 14

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> @@ +79,5 @@
> > +int
> > +MozTaggedMemorySupported(void)
> > +{
> > +  static mozilla::Atomic<int> cache(-1);
> > +  int supported = cache;
> 
> Is there any reason not to merge |supported| and |cache|?

That would mean two loads instead of one in the common case.  It probably doesn't make much difference in practice, but it doesn't hurt.

(In reply to Nathan Froyd (:froydnj) from comment #12)
> > +// Existing memory can be tagged with MozTagAnonymousMemory(); it will
> > +// tag the range of complete pages containing the given interval, so
> > +// use it with caution.  MozTaggedAnonymousMmap() can be used like
> 
> What is the reason for using with caution?  Just that you can tag more pages
> than just the ones you specify?

It won't tag more pages, but it could tag more memory than intended (the entire page instead of part of it).  My reasoning for rounding like that, such as it is, is that it seems better to have inexact data in the unaligned case rather than none.

(In reply to Nathan Froyd (:froydnj) from comment #12)
> @@ +112,5 @@
> > +{
> > +  void* mapped = _mmap(addr, length, prot, flags, fd, offset);
> > +  if (MozTaggedMemorySupported()
> > +      && (flags & MAP_ANONYMOUS) == MAP_ANONYMOUS
> > +      && mapped != MAP_FAILED) {
> 
> I think reversing the order of these tests reads slightly better, as well as
> being epsilon faster for non-anonymous maps, but this is your call.

That seems backwards: if the map isn't anonymous, then the second condition will be false and the third won't be evaluated.  But the difference may be even less meaningful than that, given that compilers will recognize the lack of side-effects and rearrange conditionals like this if they feel like it (sometimes causing valgrind false positives in the process).  I don't particularly care, so I'll swap them.

Comment 15

[Nathan Froyd [:froydnj]]

(In reply to Jed Davis [:jld] from comment #14)
> > I think reversing the order of these tests reads slightly better, as well as
> > being epsilon faster for non-anonymous maps, but this is your call.
> 
> That seems backwards: if the map isn't anonymous, then the second condition
> will be false and the third won't be evaluated.  But the difference may be
> even less meaningful than that, given that compilers will recognize the lack
> of side-effects and rearrange conditionals like this if they feel like it
> (sometimes causing valgrind false positives in the process).  I don't
> particularly care, so I'll swap them.

My thinking was mostly that MozTaggedMemorySupported() is relatively expensive compared to the other two, so it seems better to place that call last.  Also that testing "did this call actually succeed?" prior to checking the other two seems better.  But given that failure it probably relative rare anyway, perhaps it doesn't matter.

Comment 16

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Nathan Froyd (:froydnj) from comment #15)
> My thinking was mostly that MozTaggedMemorySupported() is relatively
> expensive compared to the other two, so it seems better to place that call
> last.  Also that testing "did this call actually succeed?" prior to checking
> the other two seems better.  But given that failure it probably relative
> rare anyway, perhaps it doesn't matter.

Oh — you meant to reverse all three, not swap the last two.  But, yes: failure should be uncommon.  On {n+1}th thought, I think I'll leave this as it originally was.

Comment 17

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: Part 1: Add mfbt support for PR_SET_VMA_ANON_NAME. (v2)

Carrying over r+.

Comment 18

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: Part 2: Use TaggedAnonymousMemory to distinguish our various mmap call sites. (v2)

Carrying over r+ and f?.

Comment 19

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: Part 3: Collect TaggedAnonymousMemory info in SystemMemoryReporter. (v2)

Carrying over r+.

Comment 20

[Mike Hommey [:glandium]]

Comment on attachment 8439570 [details] [diff] [review]
Part 1: Add mfbt support for PR_SET_VMA_ANON_NAME. (v2)

Review of attachment 8439570 [details] [diff] [review]:
-----------------------------------------------------------------

Since I was looking...

::: mfbt/TaggedAnonymousMemory.cpp
@@ +32,5 @@
> +// call rather than the mmap libc function, thus bypassing any
> +// LD_PRELOAD interceptions, because such interceptions can perform
> +// reentrant calls to malloc and thus result in mozjemalloc deadlocks.
> +static inline void*
> +_mmap(void* aAddr, size_t aLength, int aProt, int aFlags, int aFd, off_t aOffset)

Note that's only really useful on systems where there is a chance of gecko running with an LD_PRELOAD that overrides mmap. This is true on desktop linux where e.g. DSP wrappers do that (with broken implementation, at that). But I doubt this ever happens on b2g or android. So you can just scrap that.

@@ +62,5 @@
> +// equivalent of PAGE_MASK.
> +static uintptr_t
> +GetPageMask()
> +{
> +  static mozilla::Atomic<uintptr_t> cache;

in practice, it doesn't matter if it's not atomic.

@@ +80,5 @@
> +
> +int
> +MozTaggedMemoryIsSupported(void)
> +{
> +  static mozilla::Atomic<int> cache(-1);

likewise

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 8439571 [details] [diff] [review]
Part 2: Use TaggedAnonymousMemory to distinguish our various mmap call sites. (v2)

Review of attachment 8439571 [details] [diff] [review]:
-----------------------------------------------------------------

::: memory/mozjemalloc/jemalloc.c
@@ +1977,5 @@
>  	VirtualFree(addr, size, MEM_DECOMMIT);
>  #else
> +	if (MozTaggedAnonymousMmap(addr, size, PROT_NONE, MAP_FIXED |
> +	    MAP_PRIVATE | MAP_ANON, -1, 0, "jemalloc-decommitted")
> +	    == MAP_FAILED)

It's probably possible to get this upstreamed, provided part 1 is rewritten in C for jemalloc.

Comment 22

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Mike Hommey [:glandium] from comment #20)
> > +static inline void*
> > +_mmap(void* aAddr, size_t aLength, int aProt, int aFlags, int aFd, off_t aOffset)
> 
> Note that's only really useful on systems where there is a chance of gecko
> running with an LD_PRELOAD that overrides mmap. This is true on desktop
> linux where e.g. DSP wrappers do that (with broken implementation, at that).
> But I doubt this ever happens on b2g or android. So you can just scrap that.

The problem here, which I just noticed, is: we need _mmap in the non-ANDROID case, in the header.  It might Just Work, because jemelloc.c does its #define of mmap before including TaggedAnonymousMemory.h, but if there's been an earlier include of it in the same (maybe unified?) source file, it will silently fail.  I'm not sure what the best way to fix this is.

Comment 23

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Jed Davis [:jld] from comment #22)
> The problem here, which I just noticed, is: we need _mmap in the non-ANDROID
> case, in the header.

Or not: jemalloc.c can map memory as before and MozTagAnonymousMemory() it afterwards.

Also, having looked at the code more closely, I don't think it makes sense to distinguish the mappings from pages_map, pages_map_align, and pages_commit; I'll just label them all "jemalloc".

Comment 24

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: Part 1: Add mfbt support for PR_SET_VMA_ANON_NAME. (v3)

Re-carrying over r+.

Comment 25

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: Part 2: Use TaggedAnonymousMemory to distinguish our various mmap call sites. (v3)

Re-carrying over r+.

Comment 26

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: Part 3: Collect TaggedAnonymousMemory info in SystemMemoryReporter. (v3)

Re-carrying over r+.

Comment 27

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Trying: https://tbpl.mozilla.org/?tree=Try&rev=afe6dc1f1f41

Comment 28

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(Try link in previous comment; failures apparently not mine and passed on retry.)

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7958c801c56a
https://hg.mozilla.org/integration/mozilla-inbound/rev/df4bfca6f533
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a2f8dead92b

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7958c801c56a
https://hg.mozilla.org/mozilla-central/rev/df4bfca6f533
https://hg.mozilla.org/mozilla-central/rev/6a2f8dead92b