From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001

From: Daniel Axtens <dja@axtens.net>

Date: Thu, 25 Nov 2021 02:22:46 +1100

Subject: [PATCH] mm: Clarify grub_real_malloc()

When iterating through the singly linked list of free blocks,

grub_real_malloc() uses p and q for the current and previous blocks

respectively. This isn't super clear, so swap to using prev and cur.

This makes another quirk more obvious. The comment at the top of

grub_real_malloc() might lead you to believe that the function will

allocate from *first if there is space in that block.

It actually doesn't do that, and it can't do that with the current

data structures. If we used up all of *first, we would need to change

the ->next of the previous block to point to *first->next, but we

can't do that because it's a singly linked list and we don't have

access to *first's previous block.

What grub_real_malloc() actually does is set *first to the initial

previous block, and *first->next is the block we try to allocate

from. That allows us to keep all the data structures consistent.

Document that.

Signed-off-by: Daniel Axtens <dja@axtens.net>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

(cherry picked from commit 246ad6a44c281bb13486ddea0a26bb661db73106)

---

 grub-core/kern/mm.c | 76 +++++++++++++++++++++++++++++------------------------

 1 file changed, 41 insertions(+), 35 deletions(-)

diff --git a/grub-core/kern/mm.c b/grub-core/kern/mm.c

index d8c8377578..fb20e93acf 100644

--- a/grub-core/kern/mm.c

+++ b/grub-core/kern/mm.c

@@ -178,13 +178,20 @@ grub_mm_init_region (void *addr, grub_size_t size)

 }

 /* Allocate the number of units N with the alignment ALIGN from the ring

-   buffer starting from *FIRST.  ALIGN must be a power of two. Both N and

-   ALIGN are in units of GRUB_MM_ALIGN.  Return a non-NULL if successful,

-   otherwise return NULL.  */

+ * buffer given in *FIRST.  ALIGN must be a power of two. Both N and

+ * ALIGN are in units of GRUB_MM_ALIGN.  Return a non-NULL if successful,

+ * otherwise return NULL.

+ *

+ * Note: because in certain circumstances we need to adjust the ->next

+ * pointer of the previous block, we iterate over the singly linked

+ * list with the pair (prev, cur). *FIRST is our initial previous, and

+ * *FIRST->next is our initial current pointer. So we will actually

+ * allocate from *FIRST->next first and *FIRST itself last.

+ */

 static void *

 grub_real_malloc (grub_mm_header_t *first, grub_size_t n, grub_size_t align)

 {

-  grub_mm_header_t p, q;

+  grub_mm_header_t cur, prev;

   /* When everything is allocated side effect is that *first will have alloc

      magic marked, meaning that there is no room in this region.  */

@@ -192,24 +199,24 @@ grub_real_malloc (grub_mm_header_t *first, grub_size_t n, grub_size_t align)

     return 0;

   /* Try to search free slot for allocation in this memory region.  */

-  for (q = *first, p = q->next; ; q = p, p = p->next)

+  for (prev = *first, cur = prev->next; ; prev = cur, cur = cur->next)

     {

       grub_off_t extra;

-      extra = ((grub_addr_t) (p + 1) >> GRUB_MM_ALIGN_LOG2) & (align - 1);

+      extra = ((grub_addr_t) (cur + 1) >> GRUB_MM_ALIGN_LOG2) & (align - 1);

       if (extra)

 	extra = align - extra;

-      if (! p)

+      if (! cur)

 	grub_fatal ("null in the ring");

-      if (p->magic != GRUB_MM_FREE_MAGIC)

-	grub_fatal ("free magic is broken at %p: 0x%x", p, p->magic);

+      if (cur->magic != GRUB_MM_FREE_MAGIC)

+	grub_fatal ("free magic is broken at %p: 0x%x", cur, cur->magic);

-      if (p->size >= n + extra)

+      if (cur->size >= n + extra)

 	{

-	  extra += (p->size - extra - n) & (~(align - 1));

-	  if (extra == 0 && p->size == n)

+	  extra += (cur->size - extra - n) & (~(align - 1));

+	  if (extra == 0 && cur->size == n)

 	    {

 	      /* There is no special alignment requirement and memory block

 	         is complete match.

@@ -222,9 +229,9 @@ grub_real_malloc (grub_mm_header_t *first, grub_size_t n, grub_size_t align)

 	         | alloc, size=n |          |

 	         +---------------+          v

 	       */

-	      q->next = p->next;

+	      prev->next = cur->next;

 	    }

-	  else if (align == 1 || p->size == n + extra)

+	  else if (align == 1 || cur->size == n + extra)

 	    {

 	      /* There might be alignment requirement, when taking it into

 	         account memory block fits in.

@@ -241,23 +248,22 @@ grub_real_malloc (grub_mm_header_t *first, grub_size_t n, grub_size_t align)

 	         | alloc, size=n |        |

 	         +---------------+        v

 	       */

-

-	      p->size -= n;

-	      p += p->size;

+	      cur->size -= n;

+	      cur += cur->size;

 	    }

 	  else if (extra == 0)

 	    {

 	      grub_mm_header_t r;

-	      r = p + extra + n;

+	      r = cur + extra + n;

 	      r->magic = GRUB_MM_FREE_MAGIC;

-	      r->size = p->size - extra - n;

-	      r->next = p->next;

-	      q->next = r;

+	      r->size = cur->size - extra - n;

+	      r->next = cur->next;

+	      prev->next = r;

-	      if (q == p)

+	      if (prev == cur)

 		{

-		  q = r;

+		  prev = r;

 		  r->next = r;

 		}

 	    }

@@ -284,32 +290,32 @@ grub_real_malloc (grub_mm_header_t *first, grub_size_t n, grub_size_t align)

 	       */

 	      grub_mm_header_t r;

-	      r = p + extra + n;

+	      r = cur + extra + n;

 	      r->magic = GRUB_MM_FREE_MAGIC;

-	      r->size = p->size - extra - n;

-	      r->next = p;

+	      r->size = cur->size - extra - n;

+	      r->next = cur;

-	      p->size = extra;

-	      q->next = r;

-	      p += extra;

+	      cur->size = extra;

+	      prev->next = r;

+	      cur += extra;

 	    }

-	  p->magic = GRUB_MM_ALLOC_MAGIC;

-	  p->size = n;

+	  cur->magic = GRUB_MM_ALLOC_MAGIC;

+	  cur->size = n;

 	  /* Mark find as a start marker for next allocation to fasten it.

 	     This will have side effect of fragmenting memory as small

 	     pieces before this will be un-used.  */

 	  /* So do it only for chunks under 64K.  */

 	  if (n < (0x8000 >> GRUB_MM_ALIGN_LOG2)

-	      || *first == p)

-	    *first = q;

+	      || *first == cur)

+	    *first = prev;

-	  return p + 1;

+	  return cur + 1;

 	}

       /* Search was completed without result.  */

-      if (p == *first)

+      if (cur == *first)

 	break;

     }