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

2014-12-05 Thread Jan Beulich 
 On 04.12.14 at 18:01, mdo...@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


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

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

2014-12-04 Thread 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 mdo...@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

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

2014-12-04 Thread 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 mdo...@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);