lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20231116155627.3686da61@yea>
Date:   Thu, 16 Nov 2023 15:56:27 +0100
From:   Erhard Furtner <erhard_f@...lbox.org>
To:     Vlastimil Babka <vbabka@...e.cz>
Cc:     Roman Gushchin <roman.gushchin@...ux.dev>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, Shakeel Butt <shakeelb@...gle.com>,
        David Rientjes <rientjes@...gle.com>,
        Dennis Zhou <dennis@...nel.org>,
        Johannes Weiner <hannes@...xchg.org>,
        Michal Hocko <mhocko@...nel.org>,
        Muchun Song <muchun.song@...ux.dev>, stable@...r.kernel.org
Subject: Re: [PATCH] mm: kmem: properly initialize local objcg variable in
 current_obj_cgroup()

On Thu, 16 Nov 2023 08:04:18 +0100
Vlastimil Babka <vbabka@...e.cz> wrote:

> On 11/16/23 03:51, Roman Gushchin wrote:
> > Actually the problem is caused by uninitialized local variable in
> > current_obj_cgroup(). If the root memory cgroup is set as an active
> > memory cgroup for a charging scope (as in the trace, where systemd
> > tries to create the first non-root cgroup, so the parent cgroup is
> > the root cgroup), the "for" loop is skipped and uninitialized objcg is
> > returned, causing a panic down the accounting stack.
> > 
> > The fix is trivial: initialize the objcg variable to NULL
> > unconditionally before the "for" loop.
> > 
> > Fixes: e86828e5446d ("mm: kmem: scoped objcg protection")
> > Reported-by: Erhard Furtner <erhard_f@...lbox.org>
> > Closes: https://github.com/ClangBuiltLinux/linux/issues/1959
> > Signed-off-by: Roman Gushchin (Cruise) <roman.gushchin@...ux.dev>
> > Cc: Shakeel Butt <shakeelb@...gle.com>
> > Cc: Vlastimil Babka <vbabka@...e.cz>
> > Cc: David Rientjes <rientjes@...gle.com>
> > Cc: Dennis Zhou <dennis@...nel.org>
> > Cc: Johannes Weiner <hannes@...xchg.org>
> > Cc: Michal Hocko <mhocko@...nel.org>
> > Cc: Muchun Song <muchun.song@...ux.dev>
> > Cc: stable@...r.kernel.org  
> 
> Acked-by: Vlastimil Babka <vbabka@...e.cz>
> 
> We could also do this to make it less confusing?
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 774bd6e21e27..a08bcec661b6 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3175,7 +3175,6 @@ __always_inline struct obj_cgroup *current_obj_cgroup(void)
>  		objcg = rcu_dereference_check(memcg->objcg, 1);
>  		if (likely(objcg))
>  			break;
> -		objcg = NULL;
>  	}
>  
>  	return objcg;
> 
> 

I can confirm the 1st patch from Roman fixes the issue on my amd64 and on my i686 box.

The 2nd patch from Vlastimil unfortunately does not (only tried on amd64).

Regards,
Erhard

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ