[bug report] mm: shrinker: add a secondary array for shrinker_info::{map, nr_deferred}

[bug report] mm: shrinker: add a secondary array for shrinker_info::{map, nr_deferred}

[Dan Carpenter]

Hello Qi Zheng,

The patch b6884b5f15cf: "mm: shrinker: add a secondary array for
shrinker_info::{map, nr_deferred}" from Sep 11, 2023 (linux-next),
leads to the following Smatch static checker warning:

	mm/shrinker.c:100 alloc_shrinker_info()
	warn: inconsistent returns '&shrinker_mutex'.

mm/shrinker.c
    77 int alloc_shrinker_info(struct mem_cgroup *memcg)
    78 {
    79         struct shrinker_info *info;
    80         int nid, ret = 0;
    81         int array_size = 0;
    82 
    83         mutex_lock(&shrinker_mutex);
    84         array_size = shrinker_unit_size(shrinker_nr_max);
    85         for_each_node(nid) {
    86                 info = kvzalloc_node(sizeof(*info) + array_size, GFP_KERNEL, nid);
    87                 if (!info)
    88                         goto err;
                               ^^^^^^^^^

    89                 info->map_nr_max = shrinker_nr_max;
    90                 if (shrinker_unit_alloc(info, NULL, nid))
    91                         goto err;
                               ^^^^^^^^
These error paths need to unlock.  I'm not 100% positive if you want to
unlock before or after the call to free_shrinker_info().

    92                 rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_info, info);
    93         }
    94         mutex_unlock(&shrinker_mutex);
    95 
    96         return ret;
    97 
    98 err:
    99         free_shrinker_info(memcg);
-->100         return -ENOMEM;
   101 }

regards,
dan carpenter

Re: [External] [bug report] mm: shrinker: add a secondary array for shrinker_info::{map, nr_deferred}

[Qi Zheng]

Hi Dan,

On 2023/9/28 21:03, Dan Carpenter wrote:
> Hello Qi Zheng,
> 
> The patch b6884b5f15cf: "mm: shrinker: add a secondary array for
> shrinker_info::{map, nr_deferred}" from Sep 11, 2023 (linux-next),
> leads to the following Smatch static checker warning:
> 
> 	mm/shrinker.c:100 alloc_shrinker_info()
> 	warn: inconsistent returns '&shrinker_mutex'.
> 
> mm/shrinker.c
>      77 int alloc_shrinker_info(struct mem_cgroup *memcg)
>      78 {
>      79         struct shrinker_info *info;
>      80         int nid, ret = 0;
>      81         int array_size = 0;
>      82
>      83         mutex_lock(&shrinker_mutex);
>      84         array_size = shrinker_unit_size(shrinker_nr_max);
>      85         for_each_node(nid) {
>      86                 info = kvzalloc_node(sizeof(*info) + array_size, GFP_KERNEL, nid);
>      87                 if (!info)
>      88                         goto err;
>                                 ^^^^^^^^^
> 
>      89                 info->map_nr_max = shrinker_nr_max;
>      90                 if (shrinker_unit_alloc(info, NULL, nid))
>      91                         goto err;
>                                 ^^^^^^^^
> These error paths need to unlock.  I'm not 100% positive if you want to
> unlock before or after the call to free_shrinker_info().

Thanks for reporting this. The shrinker_mutex should be unlocked before
the call to free_shrinker_info(). I will send a fix patch ASAP.

Thanks,
Qi

> 
>      92                 rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_info, info);
>      93         }
>      94         mutex_unlock(&shrinker_mutex);
>      95
>      96         return ret;
>      97
>      98 err:
>      99         free_shrinker_info(memcg);
> -->100         return -ENOMEM;
>     101 }
> 
> regards,
> dan carpenter