Re: [bug report] mm/zswap: use only one pool in zswap

[Chengming Zhou <chengming.zhou@linux.dev>]

On 2024/6/20 16:52, Dan Carpenter wrote:
> Hello Chengming Zhou,
> 
> Commit 6193f190fe0a ("mm/zswap: use only one pool in zswap") from Jun
> 17, 2024 (linux-next), leads to the following Smatch static checker
> warning:

Thanks much for your report and explanation!

I will fix in the next version.

> 
> 	mm/zswap.c:306 zswap_pool_create()
> 	error: potential null dereference 'pool->zpool'.  (zpool_create_pool returns null)
> 
> mm/zswap.c
>      244 static struct zswap_pool *zswap_pool_create(char *type, char *compressor)
>      245 {
>      246         struct zswap_pool *pool;
>      247         char name[38]; /* 'zswap' + 32 char (max) num + \0 */
>      248         gfp_t gfp = __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM;
>      249         int ret;
>      250
>      251         if (!zswap_has_pool) {
>      252                 /* if either are unset, pool initialization failed, and we
>      253                  * need both params to be set correctly before trying to
>      254                  * create a pool.
>      255                  */
>      256                 if (!strcmp(type, ZSWAP_PARAM_UNSET))
>      257                         return NULL;
>      258                 if (!strcmp(compressor, ZSWAP_PARAM_UNSET))
>      259                         return NULL;
>      260         }
>      261
>      262         pool = kzalloc(sizeof(*pool), GFP_KERNEL);
>      263         if (!pool)
>      264                 return NULL;
>      265
>      266         /* unique name for each pool specifically required by zsmalloc */
>      267         snprintf(name, 38, "zswap%x", atomic_inc_return(&zswap_pools_count));
>      268         pool->zpool = zpool_create_pool(type, name, gfp);
>      269         if (!pool->zpool) {
>      270                 pr_err("%s zpool not available\n", type);
>      271                 goto error;
> 
> pool->zpool is NULL
> 
>      272         }
>      273         pr_debug("using %s zpool\n", zpool_get_type(pool->zpool));
>      274
>      275         strscpy(pool->tfm_name, compressor, sizeof(pool->tfm_name));
>      276
>      277         pool->acomp_ctx = alloc_percpu(*pool->acomp_ctx);
>      278         if (!pool->acomp_ctx) {
>      279                 pr_err("percpu alloc failed\n");
>      280                 goto error;
>      281         }
>      282
>      283         ret = cpuhp_state_add_instance(CPUHP_MM_ZSWP_POOL_PREPARE,
>      284                                        &pool->node);
>      285         if (ret)
>      286                 goto error;
>      287
>      288         /* being the current pool takes 1 ref; this func expects the
>      289          * caller to always add the new pool as the current pool
>      290          */
>      291         ret = percpu_ref_init(&pool->ref, __zswap_pool_empty,
>      292                               PERCPU_REF_ALLOW_REINIT, GFP_KERNEL);
>      293         if (ret)
>      294                 goto ref_fail;
>      295         INIT_LIST_HEAD(&pool->list);
>      296
>      297         zswap_pool_debug("created", pool);
>      298
>      299         return pool;
>      300
>      301 ref_fail:
>      302         cpuhp_state_remove_instance(CPUHP_MM_ZSWP_POOL_PREPARE, &pool->node);
>      303 error:
>      304         if (pool->acomp_ctx)
>      305                 free_percpu(pool->acomp_ctx);
> --> 306         zpool_destroy_pool(pool->zpool);
>                                     ^^^^^^^^^^^
> NULL dereference
> 
>      307         kfree(pool);
>      308         return NULL;
>      309 }
> 
> regards,
> dan carpenter