Re: [PATCH] mm: mmap_lock: fix use-after-free race and css ref leak in tracepoints

From: Shakeel Butt <shakeelb@google.com>
To: Greg Thelen <gthelen@google.com>
Cc: Axel Rasmussen <axelrasmussen@google.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Chinwen Chang <chinwen.chang@mediatek.com>,
	Daniel Jordan <daniel.m.jordan@oracle.com>,
	David Rientjes <rientjes@google.com>,
	Davidlohr Bueso <dbueso@suse.de>, Ingo Molnar <mingo@redhat.com>,
	Jann Horn <jannh@google.com>,
	Laurent Dufour <ldufour@linux.ibm.com>,
	Michel Lespinasse <walken@google.com>,
	Stephen Rothwell <sfr@canb.auug.org.au>,
	Steven Rostedt <rostedt@goodmis.org>,
	Vlastimil Babka <vbabka@suse.cz>,
	Yafang Shao <laoar.shao@gmail.com>,
	"David S . Miller" <davem@davemloft.net>,
	dsahern@kernel.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jakub Kicinski <kuba@kernel.org>,
	liuhangbin@gmail.com, Tejun Heo <tj@kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Linux MM <linux-mm@kvack.org>
Subject: Re: [PATCH] mm: mmap_lock: fix use-after-free race and css ref leak in tracepoints
Date: Tue, 1 Dec 2020 10:42:31 -0800	[thread overview]
Message-ID: <CALvZod4j9fFpGRfkios1ef0D5qhyw3XA_VSVm0k__RuMG1Qhwg@mail.gmail.com> (raw)
In-Reply-To: <xr93lfehl8al.fsf@gthelen.svl.corp.google.com>

On Tue, Dec 1, 2020 at 9:56 AM Greg Thelen <gthelen@google.com> wrote:
>
> Axel Rasmussen <axelrasmussen@google.com> wrote:
>
> > On Mon, Nov 30, 2020 at 5:34 PM Shakeel Butt <shakeelb@google.com> wrote:
> >>
> >> On Mon, Nov 30, 2020 at 3:43 PM Axel Rasmussen <axelrasmussen@google.com> wrote:
> >> >
> >> > syzbot reported[1] a use-after-free introduced in 0f818c4bc1f3. The bug
> >> > is that an ongoing trace event might race with the tracepoint being
> >> > disabled (and therefore the _unreg() callback being called). Consider
> >> > this ordering:
> >> >
> >> > T1: trace event fires, get_mm_memcg_path() is called
> >> > T1: get_memcg_path_buf() returns a buffer pointer
> >> > T2: trace_mmap_lock_unreg() is called, buffers are freed
> >> > T1: cgroup_path() is called with the now-freed buffer
> >>
> >> Any reason to use the cgroup_path instead of the cgroup_ino? There are
> >> other examples of trace points using cgroup_ino and no need to
> >> allocate buffers. Also cgroup namespace might complicate the path
> >> usage.
> >
> > Hmm, so in general I would love to use a numeric identifier instead of a string.
> >
> > I did some reading, and it looks like the cgroup_ino() mainly has to
> > do with writeback, instead of being just a general identifier?
> > https://www.kernel.org/doc/Documentation/cgroup-v2.txt

I think you are confusing cgroup inodes with real filesystem inodes in that doc.

> >
> > There is cgroup_id() which I think is almost what I'd want, but there
> > are a couple problems with it:
> >
> > - I don't know of a way for userspace to translate IDs -> paths, to
> > make them human readable?
>
> The id => name map can be built from user space with a tree walk.
> Example:
>
> $ find /sys/fs/cgroup/memory -type d -printf '%i %P\n'                                                                          # ~ [main]
> 20387 init.scope
> 31 system.slice
>
> > - Also I think the ID implementation we use for this is "dense",
> > meaning if a cgroup is removed, its ID is likely to be quickly reused.
> >

The ID for cgroup nodes (underlying it is kernfs) are allocated from
idr_alloc_cyclic() which gives new ID after the last allocated ID and
wrap after around INT_MAX IDs. So, likeliness of repetition is very
low. Also the file_handle returned by name_to_handle_at() for cgroupfs
returns the inode ID which gives confidence to the claim of low chance
of ID reusing.