[PATCH] mm: memcomtrol: add warning in case of rtpn = NULL

[PATCH] mm: memcomtrol: add warning in case of rtpn = NULL

[Anastasia Belova]

kzalloc_node may return NULL. So rtpn should be
checked before dereference in mem_cgroup_init.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 95a045f63d98 ("mm: memcontrol: consolidate memory controller initialization")
Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
---
 mm/memcontrol.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index e8ca4bdcb03c..7998dbf3c7cd 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -7377,6 +7377,7 @@ static int __init mem_cgroup_init(void)
 		struct mem_cgroup_tree_per_node *rtpn;
 
 		rtpn = kzalloc_node(sizeof(*rtpn), GFP_KERNEL, node);
+		WARN_ON(!rtpn);
 
 		rtpn->rb_root = RB_ROOT;
 		rtpn->rb_rightmost = NULL;
-- 
2.30.2

Re: [PATCH] mm: memcomtrol: add warning in case of rtpn = NULL

[Matthew Wilcox]

On Mon, Aug 21, 2023 at 05:20:51PM +0300, Anastasia Belova wrote:
> kzalloc_node may return NULL. So rtpn should be
> checked before dereference in mem_cgroup_init.

What good will this do?

>  
>  		rtpn = kzalloc_node(sizeof(*rtpn), GFP_KERNEL, node);
> +		WARN_ON(!rtpn);
>  
>  		rtpn->rb_root = RB_ROOT;

so you'll get a warning right before you get a crash, which will provide
exactly the same information.  And you'd already have a warning that
the memory allocation had failed (because __GFP_NOWARN wasn't set).
So you've increased the amount of data from 2 dumps to 3 without providing
any neww information.

If you _did_ something to avoid the crash, there might be a point to
this, but the fact is that small GFP_KERNEL allocations _don't_ fail.
You need to teach your tool that, or you'll be overwhelmed with
false positives.

Re: [PATCH] mm: memcomtrol: add warning in case of rtpn = NULL

[Michal Hocko]

On Mon 21-08-23 17:20:51, Anastasia Belova wrote:
> kzalloc_node may return NULL. So rtpn should be
> checked before dereference in mem_cgroup_init.

I do not think this is an appropriate fix. If the allocation really
fails (which is essentially impossible as this is an early boot code and
failing 24B req. this early means the system will likely die anyway)
then we will crash on NULL ptr dereference right afterwards so WARN will
not give us anything usefull on top. So either this needs a graceful way
to deal with the allocation failure or considering the previously said
just keep the code alone and teach the tooling that this should be
ignored.

> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: 95a045f63d98 ("mm: memcontrol: consolidate memory controller initialization")
> Signed-off-by: Anastasia Belova <abelova@astralinux.ru>

Nacked-by: Michal Hocko <mhocko@suse.com>

Thanks!

> ---
>  mm/memcontrol.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index e8ca4bdcb03c..7998dbf3c7cd 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -7377,6 +7377,7 @@ static int __init mem_cgroup_init(void)
>  		struct mem_cgroup_tree_per_node *rtpn;
>  
>  		rtpn = kzalloc_node(sizeof(*rtpn), GFP_KERNEL, node);
> +		WARN_ON(!rtpn);
>  
>  		rtpn->rb_root = RB_ROOT;
>  		rtpn->rb_rightmost = NULL;
> -- 
> 2.30.2

-- 
Michal Hocko
SUSE Labs