Thank you, Peter. ..Thomas
On Thu 19. Oct 2017 at 22:42, Peter Levart <[email protected]> wrote: > Hi Thomas, > > Right. I can understand the situation and potential problems of > introducing API for mmappedPageSize. So let's keep pretending that page > size is uniform for all memory regions used by VM and see where this brings > us. The change fixes the problem, although in a hack-ish way. It will be > good for now. > > Regards, Peter > > > > On 10/19/17 15:21, Thomas Stüfe wrote: > > Hi Peter, Christoph, > > if you have no objections, I'd like to push this change. As I explained in > my earlier mail, I'd prefer not to change MappedByteBuffer::load(), and if > you are fine with the change in its current form ( > http://cr.openjdk.java.net/~stuefe/webrevs/8186665-buffer-overflow-in-mincore/webrev.02/webrev/), > I'd like to push it. > > Thanks, Thomas > > > On Wed, Oct 18, 2017 at 12:24 PM, Thomas Stüfe <[email protected]> > wrote: > >> Hi Peter, Christoph, >> >> Thank you both for reviewing. >> >> New webrev: >> http://cr.openjdk.java.net/~stuefe/webrevs/8186665-buffer-overflow-in-mincore/webrev.02/webrev/ >> >> @Peter: >> >> >Shouldn't the following line: >> > >> > 47 return len2 + pagesize - 1 / pagesize; >> > >> >..be written as: >> > >> > return (len2 + pagesize - 1) / pagesize; >> >> You are right. Did not cause the mincore output buffer to be >> unnecessarily large. Thanks for catching this. >> >> As for your other concern: >> >> >> On Wed, Oct 18, 2017 at 10:32 AM, Peter Levart <[email protected]> >> wrote: >> >>> -- >>> In Java_java_nio_MappedByteBuffer_isLoaded0, we call mincore(2) to >>> retrieve the paging status of an address range. >>> >>> The size of the output buffer for mincore(2) depends on the number of >>> pages in *system page size* in the given memory range (this is spelled out >>> more or less explicitly on AIX and Linux, nothing is said on BSD/OSX, but I >>> assume the same). The number of pages in the memory range is calculated by >>> MappedByteBuffer.isLoaded() and handed down to >>> Java_java_nio_MappedByteBuffer_isLoaded0() together with the memory range >>> to test. >>> >>> MappedByteBuffer.isLoaded() calculates this number of pages based on >>> jjdk.internal.misc.Unsafe.pagesize(), which ultimately comes down to >>> os::vm_page_size(). >>> >>> For AIX, os::vm_page_size() may return a page size larger than the >>> system page size of 4K. The reason for this is that on AIX, memory can be >>> backed by different page sizes, usually either 4K or 64K - e.g. posix >>> thread stacks may have 4K pages, java heap (system V shared memory) with >>> 64K pages, but mmap memory is always 4K page backed... >>> >>> >>> If this is true and Unsafe.pagesize() returns a value > 4K, then perhaps >>> also the MappedByteBuffer.load() method is wrong for AIX? >>> >>> public final MappedByteBuffer load() { >>> checkMapped(); >>> if ((address == 0) || (capacity() == 0)) >>> return this; >>> long offset = mappingOffset(); >>> long length = mappingLength(offset); >>> load0(mappingAddress(offset), length); >>> >>> // Read a byte from each page to bring it into memory. A checksum >>> // is computed as we go along to prevent the compiler from >>> otherwise >>> // considering the loop as dead code. >>> Unsafe unsafe = Unsafe.getUnsafe(); >>> int ps = Bits.pageSize(); // << LOOK HERE >>> int count = Bits.pageCount(length); >>> long a = mappingAddress(offset); >>> byte x = 0; >>> for (int i=0; i<count; i++) { >>> x ^= unsafe.getByte(a); >>> a += ps; // << AND HERE >>> } >>> if (unused != 0) >>> unused = x; >>> >>> return this; >>> } >>> >>> ...this loop reads a byte from the start of each block in lumps of >>> Bits.pageSize(). Should it always read in lumps of 4K for AIX? Do we rather >>> need a special Unsafe.mmappedPageSize() method instead of just a hack in >>> isLoaded0 ? >>> >>> >> Yes, I considered this too. In effect, on AIX, we only touch every 16th >> page, thereby reducing MappedByteBuffer::load() to something like >> load_every_16th_page... However, this bug is very old (even our 1.4 VM >> already does this when the touching was still implemented in >> MappedByteBuffer.c) and did not cause any problems AFAIK. >> >> The story behind this is long and quite boring :) basically, 64k pages >> are used for the java heap and give a large performance bonus over 4K paged >> java heap. But we cannot switch all memory regions to 64K pages, so we live >> with multiple page sizes and above us we have a ton of code which assumes >> one consistent page size for everything. So we lie about the page size to >> everyone - claiming system page size to be 64k - and except for very rare >> cases like this one get away with this. >> >> I would like to keep lying consistently. There is not a hard reason for >> it, just that I am afraid that starting to publish a different page size to >> parts of the VM will confuse things and may introduce errors further down >> the line. >> >> I think a proper solution would be to keep page size on a per-ByteBuffer >> base, because ByteBuffers may be allocated in different memory regions - >> they are now allocated with mmap() in system page size, but that may change >> in the future. That is assuming that one byte buffer cannot span areas of >> multiple page sizes, which would complicate matters further. >> >> Btw, I also wondered whether other platforms could have a clash between >> the real memory page size and MappedByteBuffer's notion of that size - e.g. >> whether it is possible to have MappedByteBuffers with huge pages on Linux. >> But all cases I could think of are cases where the page size the JDK would >> assume is smaller than the actual page size, which would not be a problem >> for both mincore and load/touch. In the above example (huge pages on >> Linux), pages would be pinned anyway, so load() and isLoaded() would be >> noops. >> >> >> @Christoph: >> >> > apart from the point that Peter found, I’d also think it would look >> better if the typedef section (line 51-56) would be placed before the AIX >> only function (line 41-49). >> >> >> Sure. I moved the section up, below the includes. >> >> Kind Regards, Thomas >> >> > >
