[PATCH] xmalloc: add support for checking the pool integrity

[PATCH] xmalloc: add support for checking the pool integrity

[Mihai Donțu]

Implemented xmem_pool_check(), xmem_pool_check_locked() and
xmem_pool_check_unlocked() to verity the integrity of the TLSF matrix.

Signed-off-by: Mihai Donțu <mdontu@bitdefender.com>
---
 xen/common/xmalloc_tlsf.c | 119 +++++++++++++++++++++++++++++++++++++++++++++-
 xen/include/xen/xmalloc.h |   7 +++
 2 files changed, 124 insertions(+), 2 deletions(-)

diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
index a5769c9..009ba60 100644
--- a/xen/common/xmalloc_tlsf.c
+++ b/xen/common/xmalloc_tlsf.c
@@ -120,9 +120,120 @@ struct xmem_pool {
     char name[MAX_POOL_NAME_LEN];
 };
 
+static struct xmem_pool *xenpool;
+
+static inline void MAPPING_INSERT(unsigned long r, int *fl, int *sl);
+
 /*
  * Helping functions
  */
+#ifndef NDEBUG
+static int xmem_pool_check_size(const struct bhdr *b, int fl, int sl)
+{
+    while ( b )
+    {
+        int __fl;
+        int __sl;
+
+        MAPPING_INSERT(b->size, &__fl, &__sl);
+        if ( __fl != fl || __sl != sl )
+        {
+            printk(XENLOG_ERR "xmem_pool: for block %p size = %u, { fl = %d, sl = %d } should be { fl = %d, sl = %d }\n", b, b->size, fl, sl, __fl, __sl);
+            return 0;
+        }
+        b = b->ptr.free_ptr.next;
+    }
+    return 1;
+}
+
+/*
+ * This function must be called from a context where pool->lock is
+ * already acquired
+ */
+#define xmem_pool_check_unlocked(__pool) __xmem_pool_check_unlocked(__FILE__, __LINE__, __pool)
+static int __xmem_pool_check_unlocked(const char *file, int line, const struct xmem_pool *pool)
+{
+    int i;
+    int woops = 0;
+    static int once = 1;
+
+    for ( i = 0; i < REAL_FLI; i++ )
+    {
+        int fl = ( pool->fl_bitmap & (1 << i) ) ? i : -1;
+
+        if ( fl >= 0 )
+        {
+            int j;
+            int bitmap_empty = 1;
+            int matrix_empty = 1;
+
+            for ( j = 0; j < MAX_SLI; j++ )
+            {
+                int sl = ( pool->sl_bitmap[fl] & (1 << j) ) ? j : -1;
+
+                if ( sl < 0 )
+                    continue;
+
+                if ( once && !pool->matrix[fl][sl] )
+                {
+                    /* The bitmap is corrupted */
+                    printk(XENLOG_ERR "xmem_pool:%s:%d the TLSF bitmap is corrupted\n", file, line);
+                    __warn((char *)file, line);
+                    once = 0;
+                    woops = 1;
+                }
+                else if ( once && !xmem_pool_check_size(pool->matrix[fl][sl], fl, sl))
+                {
+                    printk(XENLOG_ERR "xmem_pool:%s:%d the TLSF chunk matrix is corrupted\n", file, line);
+                    __warn((char *)file, line);
+                    once = 0;
+                    woops = 1;
+                }
+                if ( pool->matrix[fl][sl] )
+                    matrix_empty = 0;
+                bitmap_empty = 0;
+            }
+            if ( once && bitmap_empty )
+            {
+                /* The bitmap is corrupted */
+                printk(XENLOG_ERR "xmem_pool:%s:%d the TLSF bitmap is corrupted (non-empty FL with empty SL)\n", file, line);
+                __warn((char *)file, line);
+                once = 0;
+                woops = 1;
+            }
+            if ( once && matrix_empty )
+            {
+                /* The bitmap is corrupted */
+                printk(XENLOG_ERR "xmem_pool:%s:%d the TLSF bitmap is corrupted (empty matrix)\n", file, line);
+                __warn((char *)file, line);
+                once = 0;
+                woops = 1;
+            }
+        }
+    }
+
+    return woops;
+}
+
+#define xmem_pool_check_locked(__pool) __xmem_pool_check_locked(__FILE__, __LINE__, __pool)
+static int __xmem_pool_check_locked(const char *file, int line, struct xmem_pool *pool)
+{
+    int err;
+
+    spin_lock(&pool->lock);
+    err = __xmem_pool_check_unlocked(file, line, pool);
+    spin_unlock(&pool->lock);
+    return err;
+}
+
+int __xmem_pool_check(const char *file, int line)
+{
+    return __xmem_pool_check_locked(file, line, xenpool);
+}
+#else
+#define xmem_pool_check_locked(__pool) do { if ( 0 && (__pool) ); } while (0)
+#define xmem_pool_check_unlocked(__pool) do { if ( 0 && (__pool) ); } while (0)
+#endif
 
 /**
  * Returns indexes (fl, sl) of the list used to serve request of size r
@@ -381,6 +492,8 @@ void *xmem_pool_alloc(unsigned long size, struct xmem_pool *pool)
     int fl, sl;
     unsigned long tmp_size;
 
+    xmem_pool_check_locked(pool);
+
     if ( pool->init_region == NULL )
     {
         if ( (region = pool->get_mem(pool->init_size)) == NULL )
@@ -442,11 +555,13 @@ void *xmem_pool_alloc(unsigned long size, struct xmem_pool *pool)
 
     pool->used_size += (b->size & BLOCK_SIZE_MASK) + BHDR_OVERHEAD;
 
+    xmem_pool_check_unlocked(pool);
     spin_unlock(&pool->lock);
     return (void *)b->ptr.buffer;
 
     /* Failed alloc */
  out_locked:
+    xmem_pool_check_unlocked(pool);
     spin_unlock(&pool->lock);
 
  out:
@@ -464,6 +579,7 @@ void xmem_pool_free(void *ptr, struct xmem_pool *pool)
     b = (struct bhdr *)((char *) ptr - BHDR_OVERHEAD);
 
     spin_lock(&pool->lock);
+    xmem_pool_check_unlocked(pool);
     b->size |= FREE_BLOCK;
     pool->used_size -= (b->size & BLOCK_SIZE_MASK) + BHDR_OVERHEAD;
     b->ptr.free_ptr = (struct free_ptr) { NULL, NULL};
@@ -500,6 +616,7 @@ void xmem_pool_free(void *ptr, struct xmem_pool *pool)
     tmp_b->size |= PREV_FREE;
     tmp_b->prev_hdr = b;
  out:
+    xmem_pool_check_unlocked(pool);
     spin_unlock(&pool->lock);
 }
 
@@ -512,8 +629,6 @@ int xmem_pool_maxalloc(struct xmem_pool *pool)
  * Glue for xmalloc().
  */
 
-static struct xmem_pool *xenpool;
-
 static void *xmalloc_pool_get(unsigned long size)
 {
     ASSERT(size == PAGE_SIZE);
diff --git a/xen/include/xen/xmalloc.h b/xen/include/xen/xmalloc.h
index 24a99ac..626ead0 100644
--- a/xen/include/xen/xmalloc.h
+++ b/xen/include/xen/xmalloc.h
@@ -123,4 +123,11 @@ unsigned long xmem_pool_get_used_size(struct xmem_pool *pool);
  */
 unsigned long xmem_pool_get_total_size(struct xmem_pool *pool);
 
+#ifndef NDEBUG
+#define xmem_pool_check() __xmem_pool_check(__FILE__, __LINE__)
+int __xmem_pool_check(const char *file, int line);
+#else
+#define xmem_pool_check() do { if ( 0 ); } while (0)
+#endif
+
 #endif /* __XMALLOC_H__ */
-- 
2.2.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] xmalloc: add support for checking the pool integrity

[Mihai Donțu]

On Thursday 04 December 2014 19:01:40 Mihai Donțu wrote:
> Implemented xmem_pool_check(), xmem_pool_check_locked() and
> xmem_pool_check_unlocked() to verity the integrity of the TLSF matrix.
> 
> Signed-off-by: Mihai Donțu <mdontu@bitdefender.com>
> ---
>  xen/common/xmalloc_tlsf.c | 119 +++++++++++++++++++++++++++++++++++++++++++++-
>  xen/include/xen/xmalloc.h |   7 +++
>  2 files changed, 124 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
> index a5769c9..009ba60 100644
> --- a/xen/common/xmalloc_tlsf.c
> +++ b/xen/common/xmalloc_tlsf.c
> @@ -120,9 +120,120 @@ struct xmem_pool {
>      char name[MAX_POOL_NAME_LEN];
>  };
>  
> +static struct xmem_pool *xenpool;
> +
> +static inline void MAPPING_INSERT(unsigned long r, int *fl, int *sl);
> +
>  /*
>   * Helping functions
>   */
> +#ifndef NDEBUG
> +static int xmem_pool_check_size(const struct bhdr *b, int fl, int sl)
> +{
> +    while ( b )
> +    {
> +        int __fl;
> +        int __sl;
> +
> +        MAPPING_INSERT(b->size, &__fl, &__sl);
> +        if ( __fl != fl || __sl != sl )
> +        {
> +            printk(XENLOG_ERR "xmem_pool: for block %p size = %u, { fl = %d, sl = %d } should be { fl = %d, sl = %d }\n", b, b->size, fl, sl, __fl, __sl);
> +            return 0;
> +        }
> +        b = b->ptr.free_ptr.next;
> +    }
> +    return 1;
> +}
> +
> +/*
> + * This function must be called from a context where pool->lock is
> + * already acquired
> + */
> +#define xmem_pool_check_unlocked(__pool) __xmem_pool_check_unlocked(__FILE__, __LINE__, __pool)
> +static int __xmem_pool_check_unlocked(const char *file, int line, const struct xmem_pool *pool)
> +{
> +    int i;
> +    int woops = 0;
> +    static int once = 1;
> +
> +    for ( i = 0; i < REAL_FLI; i++ )
> +    {
> +        int fl = ( pool->fl_bitmap & (1 << i) ) ? i : -1;
> +
> +        if ( fl >= 0 )
> +        {
> +            int j;
> +            int bitmap_empty = 1;
> +            int matrix_empty = 1;
> +
> +            for ( j = 0; j < MAX_SLI; j++ )
> +            {
> +                int sl = ( pool->sl_bitmap[fl] & (1 << j) ) ? j : -1;
> +
> +                if ( sl < 0 )
> +                    continue;
> +
> +                if ( once && !pool->matrix[fl][sl] )
> +                {
> +                    /* The bitmap is corrupted */
> +                    printk(XENLOG_ERR "xmem_pool:%s:%d the TLSF bitmap is corrupted\n", file, line);
> +                    __warn((char *)file, line);
> +                    once = 0;
> +                    woops = 1;
> +                }
> +                else if ( once && !xmem_pool_check_size(pool->matrix[fl][sl], fl, sl))
> +                {
> +                    printk(XENLOG_ERR "xmem_pool:%s:%d the TLSF chunk matrix is corrupted\n", file, line);
> +                    __warn((char *)file, line);
> +                    once = 0;
> +                    woops = 1;
> +                }
> +                if ( pool->matrix[fl][sl] )
> +                    matrix_empty = 0;
> +                bitmap_empty = 0;
> +            }
> +            if ( once && bitmap_empty )
> +            {
> +                /* The bitmap is corrupted */
> +                printk(XENLOG_ERR "xmem_pool:%s:%d the TLSF bitmap is corrupted (non-empty FL with empty SL)\n", file, line);
> +                __warn((char *)file, line);
> +                once = 0;
> +                woops = 1;
> +            }
> +            if ( once && matrix_empty )
> +            {
> +                /* The bitmap is corrupted */
> +                printk(XENLOG_ERR "xmem_pool:%s:%d the TLSF bitmap is corrupted (empty matrix)\n", file, line);
> +                __warn((char *)file, line);
> +                once = 0;
> +                woops = 1;
> +            }
> +        }
> +    }
> +
> +    return woops;
> +}
> +
> +#define xmem_pool_check_locked(__pool) __xmem_pool_check_locked(__FILE__, __LINE__, __pool)
> +static int __xmem_pool_check_locked(const char *file, int line, struct xmem_pool *pool)
> +{
> +    int err;
> +
> +    spin_lock(&pool->lock);
> +    err = __xmem_pool_check_unlocked(file, line, pool);
> +    spin_unlock(&pool->lock);
> +    return err;
> +}
> +
> +int __xmem_pool_check(const char *file, int line)
> +{
> +    return __xmem_pool_check_locked(file, line, xenpool);
> +}
> +#else
> +#define xmem_pool_check_locked(__pool) do { if ( 0 && (__pool) ); } while (0)
> +#define xmem_pool_check_unlocked(__pool) do { if ( 0 && (__pool) ); } while (0)
> +#endif
>  
>  /**
>   * Returns indexes (fl, sl) of the list used to serve request of size r
> @@ -381,6 +492,8 @@ void *xmem_pool_alloc(unsigned long size, struct xmem_pool *pool)
>      int fl, sl;
>      unsigned long tmp_size;
>  
> +    xmem_pool_check_locked(pool);
> +
>      if ( pool->init_region == NULL )
>      {
>          if ( (region = pool->get_mem(pool->init_size)) == NULL )
> @@ -442,11 +555,13 @@ void *xmem_pool_alloc(unsigned long size, struct xmem_pool *pool)
>  
>      pool->used_size += (b->size & BLOCK_SIZE_MASK) + BHDR_OVERHEAD;
>  
> +    xmem_pool_check_unlocked(pool);
>      spin_unlock(&pool->lock);
>      return (void *)b->ptr.buffer;
>  
>      /* Failed alloc */
>   out_locked:
> +    xmem_pool_check_unlocked(pool);
>      spin_unlock(&pool->lock);
>  
>   out:
> @@ -464,6 +579,7 @@ void xmem_pool_free(void *ptr, struct xmem_pool *pool)
>      b = (struct bhdr *)((char *) ptr - BHDR_OVERHEAD);
>  
>      spin_lock(&pool->lock);
> +    xmem_pool_check_unlocked(pool);
>      b->size |= FREE_BLOCK;
>      pool->used_size -= (b->size & BLOCK_SIZE_MASK) + BHDR_OVERHEAD;
>      b->ptr.free_ptr = (struct free_ptr) { NULL, NULL};
> @@ -500,6 +616,7 @@ void xmem_pool_free(void *ptr, struct xmem_pool *pool)
>      tmp_b->size |= PREV_FREE;
>      tmp_b->prev_hdr = b;
>   out:
> +    xmem_pool_check_unlocked(pool);
>      spin_unlock(&pool->lock);
>  }
>  
> @@ -512,8 +629,6 @@ int xmem_pool_maxalloc(struct xmem_pool *pool)
>   * Glue for xmalloc().
>   */
>  
> -static struct xmem_pool *xenpool;
> -
>  static void *xmalloc_pool_get(unsigned long size)
>  {
>      ASSERT(size == PAGE_SIZE);
> diff --git a/xen/include/xen/xmalloc.h b/xen/include/xen/xmalloc.h
> index 24a99ac..626ead0 100644
> --- a/xen/include/xen/xmalloc.h
> +++ b/xen/include/xen/xmalloc.h
> @@ -123,4 +123,11 @@ unsigned long xmem_pool_get_used_size(struct xmem_pool *pool);
>   */
>  unsigned long xmem_pool_get_total_size(struct xmem_pool *pool);
>  
> +#ifndef NDEBUG
> +#define xmem_pool_check() __xmem_pool_check(__FILE__, __LINE__)
> +int __xmem_pool_check(const char *file, int line);
> +#else
> +#define xmem_pool_check() do { if ( 0 ); } while (0)
> +#endif
> +
>  #endif /* __XMALLOC_H__ */

This change will generate a message like this one:

[2014-12-04 15:41:23] (XEN) [ 1374.507125] xmem_pool: for block ffff8304004fb9b0 size = 0, { fl = 3, sl = 9 } should be { fl = 0, sl = 0 }
[2014-12-04 15:41:23] (XEN) [ 1374.507127] xmem_pool:xmalloc_tlsf.c:582 the TLSF chunk matrix is corrupted
[2014-12-04 15:41:23] (XEN) [ 1374.507128] Xen WARN at xmalloc_tlsf.c:582
[2014-12-04 15:41:23] (XEN) [ 1374.507131] ----[ Xen-4.4.1  x86_64  debug=y  Not tainted ]----
[2014-12-04 15:41:23] (XEN) [ 1374.507132] CPU:    3
[2014-12-04 15:41:23] (XEN) [ 1374.507133] RIP:    e008:[<ffff82d0801428b0>] __warn+0x1a/0x1e
[2014-12-04 15:41:23] (XEN) [ 1374.507136] RFLAGS: 0000000000010282   CONTEXT: hypervisor
[2014-12-04 15:41:23] (XEN) [ 1374.507138] rax: 0000000000000000   rbx: 0000000000000009   rcx: 0000000000000000
[2014-12-04 15:41:23] (XEN) [ 1374.507139] rdx: ffff830414388000   rsi: 000000000000000a   rdi: ffff82d0802926dc
[2014-12-04 15:41:23] (XEN) [ 1374.507141] rbp: ffff83041438fd38   rsp: ffff83041438fd38   r8:  ffff8304143b0000
[2014-12-04 15:41:23] (XEN) [ 1374.507142] r9:  0000000000000006   r10: 000000000007bcf8   r11: 0000000000000006
[2014-12-04 15:41:23] (XEN) [ 1374.507143] r12: 0000000000000003   r13: 0000000000000001   r14: 0000000000000003
[2014-12-04 15:41:23] (XEN) [ 1374.507145] r15: ffff8304143ca300   cr0: 000000008005003b   cr4: 00000000001526f0
[2014-12-04 15:41:23] (XEN) [ 1374.507146] cr3: 0000000403af6000   cr2: ffffffffff600400
[2014-12-04 15:41:23] (XEN) [ 1374.507147] ds: 002b   es: 002b   fs: 0000   gs: 0000   ss: e010   cs: e008
[2014-12-04 15:41:23] (XEN) [ 1374.507149] Xen stack trace from rsp=ffff83041438fd38:
[2014-12-04 15:41:23] (XEN) [ 1374.507150]    ffff83041438fda8 ffff82d0801324d7 0000000000000000 0000013ff87075a2
[2014-12-04 15:41:23] (XEN) [ 1374.507152]    000000ff96ef1b38 000002468016e35d ffff82d08025a8b9 ffff8304143ca000
[2014-12-04 15:41:23] (XEN) [ 1374.507155]    0000000000000001 ffff83040fc67010 ffff8304143ca000 ffff8304143cb868
[2014-12-04 15:41:23] (XEN) [ 1374.507157]    ffff830402338000 ffff830410dc7dec ffff83041438fdd8 ffff82d080132dc5
[2014-12-04 15:41:23] (XEN) [ 1374.507159]    ffff8300dbfb0030 ffff8300d4bfa000 ffff83040fc67010 ffff830402338da8
[2014-12-04 15:41:23] (XEN) [ 1374.507161]    ffff83041438fe18 ffff82d08013358d ffff8300dbfb0000 ffff8300d4bfa000
[2014-12-04 15:41:23] (XEN) [ 1374.507164]    0000000000000000 ffff830402338da8 ffff830402338000 ffff830410dc7dec
[2014-12-04 15:41:23] (XEN) [ 1374.507166]    ffff83041438fe38 ffff82d0801a2da2 ffff83041438fe48 ffff8300d4bfa000
[2014-12-04 15:41:23] (XEN) [ 1374.507168]    ffff83041438fe48 ffff82d0801675bc ffff83041438fe68 ffff82d080161015
[2014-12-04 15:41:23] (XEN) [ 1374.507171]    ffff8300d4bfa000 ffff8300d4bfa000 ffff83041438fe98 ffff82d080105094
[2014-12-04 15:41:23] (XEN) [ 1374.507173]    ffff8304143962c0 0000000000000000 0000000000000000 ffff830414388000
[2014-12-04 15:41:23] (XEN) [ 1374.507175]    ffff83041438fec8 ffff82d0801337d0 ffff830414363dec ffff82d080308180
[2014-12-04 15:41:23] (XEN) [ 1374.507177]    ffff82d080308000 ffffffffffffffff ffff83041438fef8 ffff82d08012aa36
[2014-12-04 15:41:23] (XEN) [ 1374.507180]    ffff8300d47fe000 ffff8300dbfb0000 0000000000000001 ffff830410dc7000
[2014-12-04 15:41:23] (XEN) [ 1374.507182]    ffff83041438ff08 ffff82d08012aa8f ffff83041438fda0 ffff82d08022bd91
[2014-12-04 15:41:23] (XEN) [ 1374.507184]    ffff880024c43fd8 ffffffff81ab1b38 0000000000000000 0000000000000000
[2014-12-04 15:41:23] (XEN) [ 1374.507186]    ffff880024c43ec0 0000000000000002 0000000000000246 0000000000000000
[2014-12-04 15:41:23] (XEN) [ 1374.507188]    0000000000000000 0000000000000000 0000000000000000 ffffffff810013aa
[2014-12-04 15:41:23] (XEN) [ 1374.507190]    0000000000000000 00000000deadbeef 00000000deadbeef 0002010000000000
[2014-12-04 15:41:23] (XEN) [ 1374.507192]    ffffffff810013aa 000000000000e033 0000000000000246 ffff880024c43ea8
[2014-12-04 15:41:23] (XEN) [ 1374.507194] Xen call trace:
[2014-12-04 15:41:23] (XEN) [ 1374.507196]    [<ffff82d0801428b0>] __warn+0x1a/0x1e
[2014-12-04 15:41:23] (XEN) [ 1374.507198]    [<ffff82d0801324d7>] __xmem_pool_check_unlocked+0x143/0x24f
[2014-12-04 15:41:23] (XEN) [ 1374.507200]    [<ffff82d080132dc5>] xmem_pool_free+0x3f/0x2d9
[2014-12-04 15:41:23] (XEN) [ 1374.507201]    [<ffff82d08013358d>] xfree+0x1d5/0x222
[2014-12-04 15:41:23] (XEN) [ 1374.507203]    [<ffff82d0801a2da2>] xstate_free_save_area+0x18/0x2a
[2014-12-04 15:41:23] (XEN) [ 1374.507205]    [<ffff82d0801675bc>] vcpu_destroy_fpu+0x13/0x23
[2014-12-04 15:41:23] (XEN) [ 1374.507207]    [<ffff82d080161015>] vcpu_destroy+0x26/0x50
[2014-12-04 15:41:23] (XEN) [ 1374.507209]    [<ffff82d080105094>] complete_domain_destroy+0x49/0x16a
[2014-12-04 15:41:23] (XEN) [ 1374.507211]    [<ffff82d0801337d0>] rcu_process_callbacks+0x145/0x1a6
[2014-12-04 15:41:23] (XEN) [ 1374.507213]    [<ffff82d08012aa36>] __do_softirq+0x81/0x8c
[2014-12-04 15:41:23] (XEN) [ 1374.507215]    [<ffff82d08012aa8f>] do_softirq+0x13/0x15
[2014-12-04 15:41:23] (XEN) [ 1374.507253]    [<ffff82d08022bd91>] process_softirqs+0x21/0x30

every time the xmem pool gets corrupted. This is only a hint, though. A
developer will have to place xmem_pool_check() in various places to
determine the source of the corruption. I used it to track down a use
after free bug (details will follow later).

Thanks,

-- 
Mihai DONȚU

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] xmalloc: add support for checking the pool integrity

[Jan Beulich]

>>> On 04.12.14 at 18:01, <mdontu@bitdefender.com> wrote:
> --- a/xen/common/xmalloc_tlsf.c
> +++ b/xen/common/xmalloc_tlsf.c
> @@ -120,9 +120,120 @@ struct xmem_pool {
>      char name[MAX_POOL_NAME_LEN];
>  };
>  
> +static struct xmem_pool *xenpool;
> +
> +static inline void MAPPING_INSERT(unsigned long r, int *fl, int *sl);
> +
>  /*
>   * Helping functions
>   */
> +#ifndef NDEBUG
> +static int xmem_pool_check_size(const struct bhdr *b, int fl, int sl)
> +{
> +    while ( b )
> +    {
> +        int __fl;
> +        int __sl;
> +
> +        MAPPING_INSERT(b->size, &__fl, &__sl);
> +        if ( __fl != fl || __sl != sl )
> +        {
> +            printk(XENLOG_ERR "xmem_pool: for block %p size = %u, { fl = %d, sl = %d } should be { fl = %d, sl = %d }\n", b, b->size, fl, sl, __fl, __sl);

Long line. Only the format message alone is allowed to exceed 80
characters.

> +            return 0;
> +        }
> +        b = b->ptr.free_ptr.next;
> +    }
> +    return 1;
> +}
> +
> +/*
> + * This function must be called from a context where pool->lock is
> + * already acquired
> + */
> +#define xmem_pool_check_unlocked(__pool) __xmem_pool_check_unlocked(__FILE__, __LINE__, __pool)

No need for the double underscores on the macro parameter.

> +static int __xmem_pool_check_unlocked(const char *file, int line, const 
> struct xmem_pool *pool)
> +{
> +    int i;
> +    int woops = 0;
> +    static int once = 1;

bool_t

> +
> +    for ( i = 0; i < REAL_FLI; i++ )
> +    {
> +        int fl = ( pool->fl_bitmap & (1 << i) ) ? i : -1;

Bogus spaces inside parentheses.

> +
> +        if ( fl >= 0 )
> +        {
> +            int j;
> +            int bitmap_empty = 1;
> +            int matrix_empty = 1;

For any of the int-s here and above - can they really all become
negative? If not, they ought to be unsigned int or bool_t.

> +
> +            for ( j = 0; j < MAX_SLI; j++ )
> +            {
> +                int sl = ( pool->sl_bitmap[fl] & (1 << j) ) ? j : -1;
> +
> +                if ( sl < 0 )
> +                    continue;
> +
> +                if ( once && !pool->matrix[fl][sl] )
> +                {
> +                    /* The bitmap is corrupted */
> +                    printk(XENLOG_ERR "xmem_pool:%s:%d the TLSF bitmap is corrupted\n", file, line);
> +                    __warn((char *)file, line);

Please constify the first parameter of __warn() instead of adding
fragile casts. I also don't see why file and line need printing twice.

> +static int __xmem_pool_check_locked(const char *file, int line, struct 
> xmem_pool *pool)
> +{
> +    int err;
> +
> +    spin_lock(&pool->lock);
> +    err = __xmem_pool_check_unlocked(file, line, pool);

Inversed naming: The caller here should be _unlocked, and the
callee _locked.

> +#define xmem_pool_check_locked(__pool) do { if ( 0 && (__pool) ); } while (0)
> +#define xmem_pool_check_unlocked(__pool) do { if ( 0 && (__pool) ); } while (0)

((void)(pool)) or at least drop the "0 &&" - after all you _want_ the
macro argument to be evaluated (in order to carry out side effects).

> --- a/xen/include/xen/xmalloc.h
> +++ b/xen/include/xen/xmalloc.h
> @@ -123,4 +123,11 @@ unsigned long xmem_pool_get_used_size(struct xmem_pool 
> *pool);
>   */
>  unsigned long xmem_pool_get_total_size(struct xmem_pool *pool);
>  
> +#ifndef NDEBUG
> +#define xmem_pool_check() __xmem_pool_check(__FILE__, __LINE__)
> +int __xmem_pool_check(const char *file, int line);
> +#else
> +#define xmem_pool_check() do { if ( 0 ); } while (0)

((void)0)

or

do {} while (0)

Also perhaps __xmem_pool_check() should have a pool parameter,
with NULL meaning the default one.

Jan

Re: [PATCH] xmalloc: add support for checking the pool integrity

[Mihai Donțu]

On Friday 05 December 2014 12:09:02 Jan Beulich wrote:
> >>> On 04.12.14 at 18:01, <mdontu@bitdefender.com> wrote:
> > --- a/xen/common/xmalloc_tlsf.c
> > +++ b/xen/common/xmalloc_tlsf.c
> > @@ -120,9 +120,120 @@ struct xmem_pool {
> >      char name[MAX_POOL_NAME_LEN];
> >  };
> >  
> > +static struct xmem_pool *xenpool;
> > +
> > +static inline void MAPPING_INSERT(unsigned long r, int *fl, int *sl);
> > +
> >  /*
> >   * Helping functions
> >   */
> > +#ifndef NDEBUG
> > +static int xmem_pool_check_size(const struct bhdr *b, int fl, int sl)
> > +{
> > +    while ( b )
> > +    {
> > +        int __fl;
> > +        int __sl;
> > +
> > +        MAPPING_INSERT(b->size, &__fl, &__sl);
> > +        if ( __fl != fl || __sl != sl )
> > +        {
> > +            printk(XENLOG_ERR "xmem_pool: for block %p size = %u, { fl = %d, sl = %d } should be { fl = %d, sl = %d }\n", b, b->size, fl, sl, __fl, __sl);
> 
> Long line. Only the format message alone is allowed to exceed 80
> characters.
> 
> > +            return 0;
> > +        }
> > +        b = b->ptr.free_ptr.next;
> > +    }
> > +    return 1;
> > +}
> > +
> > +/*
> > + * This function must be called from a context where pool->lock is
> > + * already acquired
> > + */
> > +#define xmem_pool_check_unlocked(__pool) __xmem_pool_check_unlocked(__FILE__, __LINE__, __pool)
> 
> No need for the double underscores on the macro parameter.
> 
> > +static int __xmem_pool_check_unlocked(const char *file, int line, const 
> > struct xmem_pool *pool)
> > +{
> > +    int i;
> > +    int woops = 0;
> > +    static int once = 1;
> 
> bool_t
> 
> > +
> > +    for ( i = 0; i < REAL_FLI; i++ )
> > +    {
> > +        int fl = ( pool->fl_bitmap & (1 << i) ) ? i : -1;
> 
> Bogus spaces inside parentheses.
> 
> > +
> > +        if ( fl >= 0 )
> > +        {
> > +            int j;
> > +            int bitmap_empty = 1;
> > +            int matrix_empty = 1;
> 
> For any of the int-s here and above - can they really all become
> negative? If not, they ought to be unsigned int or bool_t.
> 
> > +
> > +            for ( j = 0; j < MAX_SLI; j++ )
> > +            {
> > +                int sl = ( pool->sl_bitmap[fl] & (1 << j) ) ? j : -1;
> > +
> > +                if ( sl < 0 )
> > +                    continue;
> > +
> > +                if ( once && !pool->matrix[fl][sl] )
> > +                {
> > +                    /* The bitmap is corrupted */
> > +                    printk(XENLOG_ERR "xmem_pool:%s:%d the TLSF bitmap is corrupted\n", file, line);
> > +                    __warn((char *)file, line);
> 
> Please constify the first parameter of __warn() instead of adding
> fragile casts. I also don't see why file and line need printing twice.
> 
> > +static int __xmem_pool_check_locked(const char *file, int line, struct 
> > xmem_pool *pool)
> > +{
> > +    int err;
> > +
> > +    spin_lock(&pool->lock);
> > +    err = __xmem_pool_check_unlocked(file, line, pool);
> 
> Inversed naming: The caller here should be _unlocked, and the
> callee _locked.
> 
> > +#define xmem_pool_check_locked(__pool) do { if ( 0 && (__pool) ); } while (0)
> > +#define xmem_pool_check_unlocked(__pool) do { if ( 0 && (__pool) ); } while (0)
> 
> ((void)(pool)) or at least drop the "0 &&" - after all you _want_ the
> macro argument to be evaluated (in order to carry out side effects).
> 
> > --- a/xen/include/xen/xmalloc.h
> > +++ b/xen/include/xen/xmalloc.h
> > @@ -123,4 +123,11 @@ unsigned long xmem_pool_get_used_size(struct xmem_pool 
> > *pool);
> >   */
> >  unsigned long xmem_pool_get_total_size(struct xmem_pool *pool);
> >  
> > +#ifndef NDEBUG
> > +#define xmem_pool_check() __xmem_pool_check(__FILE__, __LINE__)
> > +int __xmem_pool_check(const char *file, int line);
> > +#else
> > +#define xmem_pool_check() do { if ( 0 ); } while (0)
> 
> ((void)0)
> 
> or
> 
> do {} while (0)
> 
> Also perhaps __xmem_pool_check() should have a pool parameter,
> with NULL meaning the default one.
> 

Thank you for your review Jan. I'll follow up with an update soon, as
well as a patch for __warn() (and __bug() while I'm at it).

-- 
Mihai DONȚU

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel