[bug report] mm: add zblock - new allocator for use via zpool API

[bug report] mm: add zblock - new allocator for use via zpool API

[Dan Carpenter]

Hello Ananda,

The patch 9097e28c25c8: "mm: add zblock - new allocator for use via
zpool API" from Nov 4, 2022, leads to the following Smatch static
checker warning:

mm/zblock.c:341 zblock_alloc() error: buffer overflow 'block_desc' 29 <= 29 (assuming for loop doesn't break)
mm/zblock.c:165 cache_insert_block() error: uninitialized symbol 'min_index'.
mm/zblock.c:412 zblock_reclaim_block() warn: always true condition '(block_type >= 0) => (0-u64max >= 0)'

mm/zblock.c
    297 static int zblock_alloc(struct zblock_pool *pool, size_t size, gfp_t gfp,
    298                         unsigned long *handle)
    299 {
    300         unsigned int block_type, slot;
    301         struct zblock_block *block;
    302         struct block_list *list;
    303 
    304         if (!size)
    305                 return -EINVAL;
    306 
    307         if (size > PAGE_SIZE)
    308                 return -ENOSPC;
    309 
    310         /* find basic block type with suitable slot size */
    311         for (block_type = 0; block_type < ARRAY_SIZE(block_desc); block_type++) {
    312                 if (size <= block_desc[block_type].slot_size)
    313                         break;
    314         }

"size" is always <= PAGE_SIZE.  Is PAGE_SIZE always 4k?  If so then this
code is fine.  Smatch is bad at handling arrays.

If we don't hit the break then this code has an issue.

    315         list = &(pool->block_lists[block_type]);
    316 
    317 check:

[ Snip ] Similar thing, with breaking from loops:

   150  static void cache_insert_block(struct zblock_block *block, struct block_list *list)
   151  {
   152          unsigned int i, min_free_slots, min_index;
   153  
   154          min_free_slots = MAX_SLOTS;
   155          for (i = 0; i < BLOCK_CACHE_SIZE; i++) {
   156                  if (!list->block_cache[i] || !(list->block_cache[i])->free_slots) {
   157                          list->block_cache[i] = block;
   158                          return;
   159                  }
   160                  if ((list->block_cache[i])->free_slots < min_free_slots) {
   161                          min_free_slots = (list->block_cache[i])->free_slots;
   162                          min_index = i;

Smatch cannot figure out if this condition *must* be true.

   163                  }
   164          }
   165          list->block_cache[min_index] = block;
                                  ^^^^^^^^^
Smatch says this can be uninitialized.

   166  }

[ snip ]

   405  static int zblock_reclaim_block(struct zblock_pool *pool)
   406  {
   407          struct zblock_block *block;
   408          struct block_list *list;
   409          unsigned long handle, block_type, slot;
                ^^^^^^^^^^^^^         ^^^^^^^^^^

   410          int ret, i, reclaimed;
   411  
   412          /* start with list storing blocks with the worst compression and try
   413           * to evict the first added (oldest) block in this list
   414           */
   415          for (block_type = ARRAY_SIZE(block_desc) - 1; block_type >= 0; --block_type) {
                                                              ^^^^^^^^^^^^^^^
The condition is always true.  Just declare "block_type" as an int.

   416                  list = &(pool->block_lists[block_type]);
   417                  spin_lock(&list->lock);
   418  
   419                  /* find the oldest block in list */
   420                  block = list_last_entry(&list->head, struct zblock_block, block_node);
   421  
   422                  if (!block) {




regards,
dan carpenter

Re: [bug report] mm: add zblock - new allocator for use via zpool API

[Matthew Wilcox]

On Fri, Nov 18, 2022 at 03:20:32PM +0300, Dan Carpenter wrote:
> Hello Ananda,
> 
> The patch 9097e28c25c8: "mm: add zblock - new allocator for use via
> zpool API" from Nov 4, 2022, leads to the following Smatch static
> checker warning:
> 
> mm/zblock.c:341 zblock_alloc() error: buffer overflow 'block_desc' 29 <= 29 (assuming for loop doesn't break)
> mm/zblock.c:165 cache_insert_block() error: uninitialized symbol 'min_index'.
> mm/zblock.c:412 zblock_reclaim_block() warn: always true condition '(block_type >= 0) => (0-u64max >= 0)'
> 
> mm/zblock.c
>     297 static int zblock_alloc(struct zblock_pool *pool, size_t size, gfp_t gfp,
>     298                         unsigned long *handle)
>     299 {
>     300         unsigned int block_type, slot;
>     301         struct zblock_block *block;
>     302         struct block_list *list;
>     303 
>     304         if (!size)
>     305                 return -EINVAL;
>     306 
>     307         if (size > PAGE_SIZE)
>     308                 return -ENOSPC;
>     309 
>     310         /* find basic block type with suitable slot size */
>     311         for (block_type = 0; block_type < ARRAY_SIZE(block_desc); block_type++) {
>     312                 if (size <= block_desc[block_type].slot_size)
>     313                         break;
>     314         }
> 
> "size" is always <= PAGE_SIZE.  Is PAGE_SIZE always 4k?  If so then this
> code is fine.  Smatch is bad at handling arrays.

PAGE_SIZE is 8kB on SPARC/Alpha.  It can be 64kB on PPC and ARM.  It can
even be 256kB on one of the weirdo architectures (but, honestly, it's
OK if that breaks; it's not well-tested)..

Re: [bug report] mm: add zblock - new allocator for use via zpool API

[Ananda Badmaev]

22.11.2022 06:29, Matthew Wilcox пишет:
> On Fri, Nov 18, 2022 at 03:20:32PM +0300, Dan Carpenter wrote:
>> Hello Ananda,
>>
>> The patch 9097e28c25c8: "mm: add zblock - new allocator for use via
>> zpool API" from Nov 4, 2022, leads to the following Smatch static
>> checker warning:
>>
>> mm/zblock.c:341 zblock_alloc() error: buffer overflow 'block_desc' 29 <= 29 (assuming for loop doesn't break)
>> mm/zblock.c:165 cache_insert_block() error: uninitialized symbol 'min_index'.
>> mm/zblock.c:412 zblock_reclaim_block() warn: always true condition '(block_type >= 0) => (0-u64max >= 0)'
>>
>> mm/zblock.c
>>      297 static int zblock_alloc(struct zblock_pool *pool, size_t size, gfp_t gfp,
>>      298                         unsigned long *handle)
>>      299 {
>>      300         unsigned int block_type, slot;
>>      301         struct zblock_block *block;
>>      302         struct block_list *list;
>>      303
>>      304         if (!size)
>>      305                 return -EINVAL;
>>      306
>>      307         if (size > PAGE_SIZE)
>>      308                 return -ENOSPC;
>>      309
>>      310         /* find basic block type with suitable slot size */
>>      311         for (block_type = 0; block_type < ARRAY_SIZE(block_desc); block_type++) {
>>      312                 if (size <= block_desc[block_type].slot_size)
>>      313                         break;
>>      314         }
>>
>> "size" is always <= PAGE_SIZE.  Is PAGE_SIZE always 4k?  If so then this
>> code is fine.  Smatch is bad at handling arrays.
> 
> PAGE_SIZE is 8kB on SPARC/Alpha.  It can be 64kB on PPC and ARM.  It can
> even be 256kB on one of the weirdo architectures (but, honestly, it's
> OK if that breaks; it's not well-tested)..

The block_desc table uses fractions of PAGE_SIZE, so with larger 
PAGE_SIZE, the size of slots will also increase. Should work fine with a 
page size of 8, 64, 256k.