[PATCH] mm: memcontrol-v1: use nofail allocations for soft limit trees

[PATCH] mm: memcontrol-v1: use nofail allocations for soft limit trees

[Ruoyu Wang]

memcg1_init() allocates one soft-limit tree node per NUMA node and
then initializes the returned object. If kzalloc_node() fails, the rb_root
and lock initialization dereference NULL.

The per-node soft-limit tree is required by memcg v1. Use nofail
GFP_KERNEL allocations for these init-time objects so the init path does
not continue without the required tree nodes.

Signed-off-by: Ruoyu Wang <ruoyuw560@gmail.com>
---
 mm/memcontrol-v1.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/memcontrol-v1.c b/mm/memcontrol-v1.c
index 433bba9dfe715..3f41a15d8a8cf 100644
--- a/mm/memcontrol-v1.c
+++ b/mm/memcontrol-v1.c
@@ -2246,7 +2246,8 @@ static int __init memcg1_init(void)
 	for_each_node(node) {
 		struct mem_cgroup_tree_per_node *rtpn;
 
-		rtpn = kzalloc_node(sizeof(*rtpn), GFP_KERNEL, node);
+		rtpn = kzalloc_node(sizeof(*rtpn), GFP_KERNEL | __GFP_NOFAIL,
+				    node);
 
 		rtpn->rb_root = RB_ROOT;
 		rtpn->rb_rightmost = NULL;
-- 
2.51.0

Re: [PATCH] mm: memcontrol-v1: use nofail allocations for soft limit trees

[Michal Hocko]

On Mon 08-06-26 14:36:44, Ruoyu Wang wrote:
> memcg1_init() allocates one soft-limit tree node per NUMA node and
> then initializes the returned object. If kzalloc_node() fails, the rb_root
> and lock initialization dereference NULL.
> 
> The per-node soft-limit tree is required by memcg v1. Use nofail
> GFP_KERNEL allocations for these init-time objects so the init path does
> not continue without the required tree nodes.

This is an early init code executing in during boot. Have you really
seen this allocation failing?

> Signed-off-by: Ruoyu Wang <ruoyuw560@gmail.com>
> ---
>  mm/memcontrol-v1.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/memcontrol-v1.c b/mm/memcontrol-v1.c
> index 433bba9dfe715..3f41a15d8a8cf 100644
> --- a/mm/memcontrol-v1.c
> +++ b/mm/memcontrol-v1.c
> @@ -2246,7 +2246,8 @@ static int __init memcg1_init(void)
>  	for_each_node(node) {
>  		struct mem_cgroup_tree_per_node *rtpn;
>  
> -		rtpn = kzalloc_node(sizeof(*rtpn), GFP_KERNEL, node);
> +		rtpn = kzalloc_node(sizeof(*rtpn), GFP_KERNEL | __GFP_NOFAIL,
> +				    node);
>  
>  		rtpn->rb_root = RB_ROOT;
>  		rtpn->rb_rightmost = NULL;
> -- 
> 2.51.0

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: memcontrol-v1: use nofail allocations for soft limit trees

[Michal Hocko]

On Mon 08-06-26 16:34:48, Ruoyu Wang wrote:
> No, I have not observed this allocation failing in practice.
> 
> This was found by static analysis and then checked by reading the code:
> memcg1_init() dereferences rtpn unconditionally after kzalloc_node(). I
> treated the soft-limit tree as mandatory memcg v1 init state and used
> __GFP_NOFAIL because continuing without it would not be useful.

This should have been part of the changelog because it provides an
insight into how have you reached your conclusion.

> I agree this is early boot init code, and I do not have a
> runtime failure report or fault-injection reproduction for it. If such
> allocations are considered not worth handling in this path, feel
> free to drop the patch.

Yes, there is simply no point in handling these failures because early
allocation failure like this one would very likely lead to massive
failure before userspace is brought up anyway so there is no practical
way to trigger the NULL ptr.

-- 
Michal Hocko
SUSE Labs