Bug: chunk.used should not be up-aligned before the size check

[jsjolen]

Hi,

Consider these lines from dma_alloc:

   -- Skip allocation forward pointer to suit alignment
   chunk.used = lib.align(chunk.used, align)
   -- Need a new chunk to service this allocation?
   if chunk.used + bytes > chunk.size then

I assume that lib.align does up-alignment (down would be very strange). Then there are two issues:

1. If lib.align(chunk.used, align) > chunk.sizethen the chunk will have used more than is available, then that's probably an invariant violation as chunk.used > chunk.size. Let's say there's 2 bytes left and it's on a 2-byte alignment, but needs 16-byte alignment, then oops, it'll overshoot by 14 bytes.
2. If the branch is reached and issue 1. doesn't occur, then it will still 'leak' a few bytes, leading to a small amount of internal fragmentation.

To be fair, I haven't looked at the surrounding context, so this may not be an issue anyway. I'm assuming that the latest chunk will at some time be deallocated and the previous invariant-breaking chunk will at some point be accessed.

I bet the issue doesn't manifest because the last valid address in a huge page is 2M aligned, and nothing uses such a high alignment anyway. Since the fix is so easy, maybe it's worth fixing anyway?

Full function for reference:

function dma_alloc (bytes,  align)
   align = align or 128
   assert(bytes <= huge_page_size)
   -- Get current chunk of memory to allocate from
   if #chunks == 0 then allocate_next_chunk() end
   local chunk = chunks[#chunks]
   -- Skip allocation forward pointer to suit alignment
   chunk.used = lib.align(chunk.used, align)
   -- Need a new chunk to service this allocation?
   if chunk.used + bytes > chunk.size then
      allocate_next_chunk()
      chunk = chunks[#chunks]
   end
   -- Slice out the memory we need
   local where = chunk.used
   chunk.used = chunk.used + bytes
   return chunk.pointer + where, chunk.physical + where, bytes
end

[alexandergall]

This is a simple linear memory allocator, i.e. a chunk is never revisited once a new chunk has been allocated. There is no way to free memory (all allocated hugepages are only freed during process termination) and unused regions can't be claimed by later requests. Therefore, all regions of a chunk that are actually being used are always within bounds and chunk.used > chunk.size is irrelevant. Fragmentation is not an issue either for the same reason.

I haven't looked at this code for a while, maybe @eugeneia wants to weigh in.