Re: [Xen-devel] [PATCH v4] xmalloc: add support for checking the pool integrity

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/12/14 19:33, 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@xxxxxxxxxxxxxxx>

This review supersedes (and is adjusted accordingly for) the two
discussion threads which happened off my first review.

>
> ---
> Changes since v3:
>  - try harder to respect the 80 column limit
>  - use 'unsigned int' instead of 'int' where possible
>  - made the logged messages even shorter
>  - dropped the use of goto (didn't really make sense)
>
> Changes since v2:
>  - print the name of the corrupted pool
>  - adjusted the messages to better fit within 80 columns
>  - minor style change (a ? a : b -> a ?: b)
>
> Changes since v1:
>  - fixed the codingstyle
>  - swaped _locked/_unlocked naming
>  - reworked __xmem_pool_check_locked() a bit
>  - used bool_t where appropriate
>  - made xmem_pool_check() take a pool argument which can be NULL
> ---
>  xen/common/xmalloc_tlsf.c | 121 
> +++++++++++++++++++++++++++++++++++++++++++++-
>  xen/include/xen/xmalloc.h |   7 +++
>  2 files changed, 126 insertions(+), 2 deletions(-)
>
> diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
> index a5769c9..eca4f1c 100644
> --- a/xen/common/xmalloc_tlsf.c
> +++ b/xen/common/xmalloc_tlsf.c
> @@ -120,9 +120,122 @@ 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 bool_t xmem_pool_check_size(const struct xmem_pool *pool,
> +                                   int fl, int sl)
> +{
> +    const struct bhdr *b = pool->matrix[fl][sl];
> +
> +    while ( b )
> +    {
> +        int __fl;
> +        int __sl;
> +
> +        MAPPING_INSERT(b->size, &__fl, &__sl);
> +        if ( __fl != fl || __sl != sl )
> +        {
> +            printk(XENLOG_ERR
> +                   "xmem_pool: %s: misplaced block %p:%u ({%d,%d} -> 
> {%d,%d})\n",
> +                   pool->name, b, b->size, fl, sl, __fl, __sl);

Is it liable to get confusing with a hex number and a decimal number
combined with just a colon?

Is b even a useful pointer to print? You have the pool name, indicies
and size which seem to be the salient information.

> +            return 0;
> +        }
> +        b = b->ptr.free_ptr.next;
> +    }
> +    return 1;
> +}
> +
> +/*
> + * This function must be called from a context where pool->lock is
> + * already acquired.
> + *
> + * Returns true if the pool has been corrupted, false otherwise
> + */
> +#define xmem_pool_check_locked(pool) \
> +    __xmem_pool_check_locked(__FILE__, __LINE__, pool)
> +static bool_t __xmem_pool_check_locked(const char *file, int line,
> +                                       const struct xmem_pool *pool)
> +{
> +    unsigned int i;
> +    static bool_t once = 1;
> +
> +    if ( !once )
> +        return 1;
> +    for ( i = 0; i < REAL_FLI; i++ )
> +    {
> +        int fl = (pool->fl_bitmap & (1 << i)) ? i : -1;
> +
> +        if ( fl >= 0 )
> +        {
> +            unsigned int j;
> +
> +            if ( !pool->sl_bitmap[fl] )
> +            {
> +                printk(XENLOG_ERR
> +                       "xmem_pool: %s: TLSF bitmap corrupt (!empty FL, empty 
> SL)\n",
> +                       pool->name);
> +                __warn(file, line);
> +                once = 0;
> +                break;
> +            }
> +            for ( j = 0; j < MAX_SLI; j++ )
> +            {
> +                int sl = (pool->sl_bitmap[fl] & (1 << j)) ? j : -1;
> +
> +                if ( sl < 0 )
> +                    continue;
> +                if ( !pool->matrix[fl][sl] )
> +                {
> +                    printk(XENLOG_ERR
> +                           "xmem_pool: %s: TLSF bitmap corrupt 
> (!matrix[%d][%d])\n",
> +                           pool->name, fl, sl);
> +                    __warn(file, line);
> +                    once = 0;
> +                    break;
> +                }
> +                if ( !xmem_pool_check_size(pool, fl, sl) )
> +                {
> +                    printk(XENLOG_ERR "xmem_pool: %s: TLSF chunk matrix 
> corrupt\n",
> +                           pool->name);
> +                    __warn(file, line);
> +                    once = 0;
> +                    break;
> +                }
> +            }
> +            if ( !once )
> +                break;
> +        }
> +    }
> +    return !once;
> +}

I want to remove __bug() and __warn() because they are currently unused
in the code, and are a cludge on ARM (lack of working
run_in_exception_handler()).  You can use BUG()/WARN() instead, which
will work as expected ARM.

However, once memory corruption has been detected, all bets are off
regarding correct functioning of the host.  I would suggest printing as
much error information as possible and using BUG() to bring Xen down. 
This allows you to remove 'once' (dubious being static, yet applying to
arbitrary xmem pools), and to change the function to be void.

> +
> +#define xmem_pool_check_unlocked(pool) \
> +    __xmem_pool_check_unlocked(__FILE__, __LINE__, pool)
> +static bool_t __xmem_pool_check_unlocked(const char *file, int line,
> +                                         struct xmem_pool *pool)
> +{
> +    bool_t oops;
> +
> +    spin_lock(&pool->lock);
> +    oops = __xmem_pool_check_locked(file, line, pool);
> +    spin_unlock(&pool->lock);
> +    return oops;
> +}
> +
> +bool_t __xmem_pool_check(const char *file, int line, struct xmem_pool *pool)
> +{
> +    return __xmem_pool_check_unlocked(file, line, pool ?: xenpool);
> +}
> +#else
> +#define xmem_pool_check_locked(pool) ((void)(pool))
> +#define xmem_pool_check_unlocked(pool) ((void)(pool))
> +#endif
>  
>  /**
>   * Returns indexes (fl, sl) of the list used to serve request of size r
> @@ -381,6 +494,8 @@ void *xmem_pool_alloc(unsigned long size, struct 
> xmem_pool *pool)
>      int fl, sl;
>      unsigned long tmp_size;
>  
> +    xmem_pool_check_unlocked(pool);
> +
>      if ( pool->init_region == NULL )
>      {
>          if ( (region = pool->get_mem(pool->init_size)) == NULL )
> @@ -442,11 +557,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_locked(pool);
>      spin_unlock(&pool->lock);
>      return (void *)b->ptr.buffer;
>  
>      /* Failed alloc */
>   out_locked:
> +    xmem_pool_check_locked(pool);
>      spin_unlock(&pool->lock);
>  
>   out:
> @@ -464,6 +581,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_locked(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 +618,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_locked(pool);
>      spin_unlock(&pool->lock);
>  }
>  
> @@ -512,8 +631,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..ad48930 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(pool) __xmem_pool_check(__FILE__, __LINE__, pool)
> +bool_t __xmem_pool_check(const char *file, int line, struct xmem_pool *pool);
> +#else
> +#define xmem_pool_check(pool) ((void)0)

This needs to be ((void)(pool)) to evaluate the potential side effects.


What are the overheads of pool consistency checking?  It looks to be
moderately high, given a full inspection of the matrix on each
allocation and free.  Would it be possible to have one easier-to-alter
compile time knob so the default can be overridden in a single location?

Perhaps something like:

#ifndef NDEBUG
#define XMEM_POOL_CHECKS
#endif

#ifdef XMEM_POOL_CHECKS
...
#else
...
#endif

~Andrew


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