Bug 77712 – [JSC] Range of cache flush is not guaranteed by Linux kernel

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Hojong Han]

Current MetaAllocator concept, always coalesces adjacent free spaces, doesn't meet memory management of Linux kernel.
In a certain case Linux kernel doesn't regard contiguous virtual memory areas as one but two.

Let's assume that mmap is called by MetaAllocator three times like below.

1. 8KB allocated from 0x41000000 to 0x41002000 by the 1st mmap call
2. 8KB allocated from 0x41004000 to 0x41006000 by the 2nd mmap call
3. 8KB allocated from 0x41002000 to 0x41004000 by the 3rd mmap call

These virtual memory areas(VMAs) above are contiguous from 0x41000000 to 0x41006000,
but the kernel merges only the third allocated 8KB with the first allocated 8KB although the second one is adjacent to the third.
Because newly allocated VMA can be coalesced with the previously allocated 
and it's not allowed between previously allocated VMAs which internally have different information about annoymous pages.

But current MetaAllocator is not considered on this kernel operation,
thereby it just coalesces adjacent spaces when those are freed.

This different mergence operation between MetaAllocator and Linux kernel finally causes the problem during cache flush.
MetaAllocator hands over coalesced free space that the kernel regards it just as parts of two separated VMAs.
The free space, coalesced by MetaAllocator, will be filled with JIT and cache flush will be run before the execution.
At that time the flush range of the coalesced space is not guaranteed by kernel.
It means inconsistency of data and instruction cache, and nobody knows what happens when this JIT code runs.

The bottom line is that routines, handling this kernel operation, is necessary in MetaAllocator.

[Hojong Han]

Created attachment 130982 [details]
Patch

[WebKit Review Bot]

Attachment 130982 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/jit/ExecutableAllocator.h:202:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andy Wingo]

Comment on attachment 130982 [details]
Patch

Is size_t the right type to use for startPage and endPage?  It seems safer to use intptr_t or something.

Also it seems to me that you would be better off doing

  uintptr_t start = reinterpret_cast<uintptr_t>(code) & ~(pageSize() - 1);

and just adding pageSize() each round instead of doing the divide-and-multiply dance.  Dunno.

I'm not a reviewer, so please take these comments with a grain of salt.

[Hojong Han]

(In reply to comment #3)
> (From update of attachment 130982 [details])
> Is size_t the right type to use for startPage and endPage?  It seems safer to use intptr_t or something.
> 
> Also it seems to me that you would be better off doing
> 
>   uintptr_t start = reinterpret_cast<uintptr_t>(code) & ~(pageSize() - 1);
> 
> and just adding pageSize() each round instead of doing the divide-and-multiply dance.  Dunno.
> 
> I'm not a reviewer, so please take these comments with a grain of salt.

Thank you. It's really necessary salt for me.
I'm going to put it into new patch.

[Geoffrey Garen]

Phil, does this mean that munmap will eventually fail as well?

[Filip Pizlo]

(In reply to comment #5)
> Phil, does this mean that munmap will eventually fail as well?

Nope.  We munmap one page at a time.

[Hojong Han]

Created attachment 131264 [details]
Patch

[WebKit Review Bot]

Attachment 131264 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/jit/ExecutableAllocator.h:202:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hojong Han]

Created attachment 131268 [details]
Patch

[WebKit Review Bot]

Attachment 131268 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/jit/ExecutableAllocator.h:202:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 131268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131268&action=review

> Source/JavaScriptCore/jit/ExecutableAllocator.h:209
> +            current = next;

You can eliminate the "next" variable if you change this line to "current += pageSize()".

> Source/JavaScriptCore/jit/ExecutableAllocator.h:211
> +        } while (end >= current);

In C++, "end" traditionally means one-past-the-end. That's not the case here, so I don't think you should use the word "end".

I'd suggest "currentPage" and "lastPage" for variable names.

[Hojong Han]

Created attachment 131496 [details]
Patch

[WebKit Review Bot]

Attachment 131496 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/jit/ExecutableAllocator.h:201:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hojong Han]

(In reply to comment #11)
> (From update of attachment 131268 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131268&action=review
> 
> > Source/JavaScriptCore/jit/ExecutableAllocator.h:209
> > +            current = next;
> 
> You can eliminate the "next" variable if you change this line to "current += pageSize()".
> 
> > Source/JavaScriptCore/jit/ExecutableAllocator.h:211
> > +        } while (end >= current);
> 
> In C++, "end" traditionally means one-past-the-end. That's not the case here, so I don't think you should use the word "end".
> 
> I'd suggest "currentPage" and "lastPage" for variable names.

Thanks for your kind comments. I reflect your advice.

[Geoffrey Garen]

Comment on attachment 131496 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 131496 [details]
Patch

Clearing flags on attachment: 131496

Committed r110792: <http://trac.webkit.org/changeset/110792>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.