[RESEND/PING] timers: limit heap size

Commit Message

Jan Beulich Aug. 30, 2019, 1:33 p.m. UTC

First and foremost make timer_softirq_action() avoid growing the heap
if its new size can't be stored without truncation. 64k entries is a
lot, and I don't think we're at risk of actually running into the issue,
but I also think it's better not to allow for hard to debug problems to
occur in the first place.

Furthermore also adjust the code such the size/limit fields becoming
unsigned int would at least work from a mere sizing point of view. For
this also switch various uses of plain int to unsigned int.

Signed-off-by: Jan Beulich <[e-mail]>
Reviewed-by: Roger Pau Monné <[e-mail]>
---
v2: Log (once) when heap limit would have been exceeded.

Comments

Andrew Cooper Sept. 2, 2019, 10:30 a.m. UTC | #1

On 30/08/2019 14:33, Jan Beulich wrote:
> First and foremost make timer_softirq_action() avoid growing the heap
> if its new size can't be stored without truncation. 64k entries is a
> lot, and I don't think we're at risk of actually running into the issue,
> but I also think it's better not to allow for hard to debug problems to
> occur in the first place.
>
> Furthermore also adjust the code such the size/limit fields becoming
> unsigned int would at least work from a mere sizing point of view. For
> this also switch various uses of plain int to unsigned int.
>
> Signed-off-by: Jan Beulich <[e-mail]>
> Reviewed-by: Roger Pau Monné <[e-mail]>

Acked-by: Andrew Cooper <[e-mail]>

Patch

--- a/xen/common/timer.c
+++ b/xen/common/timer.c
@@ -63,9 +63,9 @@  static struct heap_metadata *heap_metada
 }
 
 /* Sink down element @pos of @heap. */
-static void down_heap(struct timer **heap, int pos)
+static void down_heap(struct timer **heap, unsigned int pos)
 {
-    int sz = heap_metadata(heap)->size, nxt;
+    unsigned int sz = heap_metadata(heap)->size, nxt;
     struct timer *t = heap[pos];
 
     while ( (nxt = (pos << 1)) <= sz )
@@ -84,7 +84,7 @@  static void down_heap(struct timer **hea
 }
 
 /* Float element @pos up @heap. */
-static void up_heap(struct timer **heap, int pos)
+static void up_heap(struct timer **heap, unsigned int pos)
 {
     struct timer *t = heap[pos];
 
@@ -103,8 +103,8 @@  static void up_heap(struct timer **heap,
 /* Delete @t from @heap. Return TRUE if new top of heap. */
 static int remove_from_heap(struct timer **heap, struct timer *t)
 {
-    int sz = heap_metadata(heap)->size;
-    int pos = t->heap_offset;
+    unsigned int sz = heap_metadata(heap)->size;
+    unsigned int pos = t->heap_offset;
 
     if ( unlikely(pos == sz) )
     {
@@ -130,7 +130,7 @@  static int remove_from_heap(struct timer
 /* Add new entry @t to @heap. Return TRUE if new top of heap. */
 static int add_to_heap(struct timer **heap, struct timer *t)
 {
-    int sz = heap_metadata(heap)->size;
+    unsigned int sz = heap_metadata(heap)->size;
 
     /* Fail if the heap is full. */
     if ( unlikely(sz == heap_metadata(heap)->limit) )
@@ -462,9 +462,17 @@  static void timer_softirq_action(void)
     if ( unlikely(ts->list != NULL) )
     {
         /* old_limit == (2^n)-1; new_limit == (2^(n+4))-1 */
-        int old_limit = heap_metadata(heap)->limit;
-        int new_limit = ((old_limit + 1) << 4) - 1;
-        struct timer **newheap = xmalloc_array(struct timer *, new_limit + 1);
+        unsigned int old_limit = heap_metadata(heap)->limit;
+        unsigned int new_limit = ((old_limit + 1) << 4) - 1;
+        struct timer **newheap = NULL;
+
+        /* Don't grow the heap beyond what is representable in its metadata. */
+        if ( new_limit == (typeof(heap_metadata(heap)->limit))new_limit &&
+             new_limit + 1 )
+            newheap = xmalloc_array(struct timer *, new_limit + 1);
+        else
+            printk_once(XENLOG_WARNING "CPU%u: timer heap limit reached\n",
+                        smp_processor_id());
         if ( newheap != NULL )
         {
             spin_lock_irq(&ts->lock);
@@ -543,7 +551,7 @@  static void dump_timerq(unsigned char ke
     struct timers *ts;
     unsigned long  flags;
     s_time_t       now = NOW();
-    int            i, j;
+    unsigned int   i, j;
 
     printk("Dumping timer queues:\n");
 
@@ -555,7 +563,7 @@  static void dump_timerq(unsigned char ke
         spin_lock_irqsave(&ts->lock, flags);
         for ( j = 1; j <= heap_metadata(ts->heap)->size; j++ )
             dump_timer(ts->heap[j], now);
-        for ( t = ts->list, j = 0; t != NULL; t = t->list_next, j++ )
+        for ( t = ts->list; t != NULL; t = t->list_next )
             dump_timer(t, now);
         spin_unlock_irqrestore(&ts->lock, flags);
     }