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
The solution in this commit is to switch to mutex + RCU. With the RCU
API we can first stop new buffers from being handed out, then wait for
existing users to finish, and *then* free the buffers.
I have a simple reproducer program which spins up two pools of threads,
doing the following in a tight loop:
Pool 1:
mmap(NULL, 4096, PROT_READ | PROT_WRITE,
MAP_PRIVATE | MAP_ANONYMOUS, -1, 0)
munmap()
Pool 2:
echo 1 > /sys/kernel/debug/tracing/events/mmap_lock/enable
echo 0 > /sys/kernel/debug/tracing/events/mmap_lock/enable
This triggers the use-after-free very quickly. With this patch, I let it
run for an hour without any BUGs.
While fixing this, I also noticed and fixed a css ref leak. Previously
we called get_mem_cgroup_from_mm(), but we never called css_put() to
release that reference. get_mm_memcg_path() now does this properly.
[1]: https://syzkaller.appspot.com/bug?extid=19e6dd9943972fa1c58a
Signed-off-by: Axel Rasmussen <[email protected]>
---
mm/mmap_lock.c | 104 +++++++++++++++++++++++++++++++------------------
1 file changed, 66 insertions(+), 38 deletions(-)
diff --git a/mm/mmap_lock.c b/mm/mmap_lock.c
index 12af8f1b8a14..5a3349bf1501 100644
--- a/mm/mmap_lock.c
+++ b/mm/mmap_lock.c
@@ -6,9 +6,10 @@
#include <linux/cgroup.h>
#include <linux/memcontrol.h>
#include <linux/mmap_lock.h>
+#include <linux/mutex.h>
#include <linux/percpu.h>
+#include <linux/rcupdate.h>
#include <linux/smp.h>
-#include <linux/spinlock.h>
#include <linux/trace_events.h>
EXPORT_TRACEPOINT_SYMBOL(mmap_lock_start_locking);
@@ -23,8 +24,8 @@ EXPORT_TRACEPOINT_SYMBOL(mmap_lock_released);
* concurrent _reg() and _unreg() calls, and count how many _reg() calls have
* been made.
*/
-static DEFINE_SPINLOCK(reg_lock);
-static int reg_refcount;
+static DEFINE_MUTEX(reg_lock);
+static int reg_refcount; /* Protected by reg_lock. */
/*
* Size of the buffer for memcg path names. Ignoring stack trace support,
@@ -38,99 +39,126 @@ static int reg_refcount;
*/
#define CONTEXT_COUNT 4
-DEFINE_PER_CPU(char *, memcg_path_buf);
-DEFINE_PER_CPU(int, memcg_path_buf_idx);
+static DEFINE_PER_CPU(char __rcu *, memcg_path_buf);
+static DEFINE_PER_CPU(int, memcg_path_buf_idx);
+
+/* Called with reg_lock held. */
+static void free_memcg_path_bufs(void)
+{
+ int cpu;
+ char *old;
+
+ for_each_possible_cpu(cpu) {
+ old = rcu_dereference_protected(per_cpu(memcg_path_buf, cpu),
+ lockdep_is_held(®_lock));
+ if (old == NULL)
+ break;
+ rcu_assign_pointer(per_cpu(memcg_path_buf, cpu), NULL);
+ /* Wait for inflight memcg_path_buf users to finish. */
+ synchronize_rcu();
+ kfree(old);
+ }
+}
int trace_mmap_lock_reg(void)
{
- unsigned long flags;
int cpu;
+ char *new;
- spin_lock_irqsave(®_lock, flags);
+ mutex_lock(®_lock);
+ /* If the refcount is going 0->1, proceed with allocating buffers. */
if (reg_refcount++)
goto out;
for_each_possible_cpu(cpu) {
- per_cpu(memcg_path_buf, cpu) = NULL;
- }
- for_each_possible_cpu(cpu) {
- per_cpu(memcg_path_buf, cpu) = kmalloc(
- MEMCG_PATH_BUF_SIZE * CONTEXT_COUNT, GFP_NOWAIT);
- if (per_cpu(memcg_path_buf, cpu) == NULL)
+ new = kmalloc(MEMCG_PATH_BUF_SIZE * CONTEXT_COUNT, GFP_KERNEL);
+ if (new == NULL)
goto out_fail;
- per_cpu(memcg_path_buf_idx, cpu) = 0;
+ rcu_assign_pointer(per_cpu(memcg_path_buf, cpu), new);
+ /* Don't need to wait for inflights, they'd have gotten NULL. */
}
out:
- spin_unlock_irqrestore(®_lock, flags);
+ mutex_unlock(®_lock);
return 0;
out_fail:
- for_each_possible_cpu(cpu) {
- if (per_cpu(memcg_path_buf, cpu) != NULL)
- kfree(per_cpu(memcg_path_buf, cpu));
- else
- break;
- }
+ free_memcg_path_bufs();
+ /* Since we failed, undo the earlier ref increment. */
--reg_refcount;
- spin_unlock_irqrestore(®_lock, flags);
+ mutex_unlock(®_lock);
return -ENOMEM;
}
void trace_mmap_lock_unreg(void)
{
- unsigned long flags;
- int cpu;
-
- spin_lock_irqsave(®_lock, flags);
+ mutex_lock(®_lock);
+ /* If the refcount is going 1->0, proceed with freeing buffers. */
if (--reg_refcount)
goto out;
- for_each_possible_cpu(cpu) {
- kfree(per_cpu(memcg_path_buf, cpu));
- }
+ free_memcg_path_bufs();
out:
- spin_unlock_irqrestore(®_lock, flags);
+ mutex_unlock(®_lock);
}
static inline char *get_memcg_path_buf(void)
{
+ char *buf;
int idx;
+ rcu_read_lock();
+ buf = rcu_dereference(*this_cpu_ptr(&memcg_path_buf));
+ if (buf == NULL)
+ return NULL;
idx = this_cpu_add_return(memcg_path_buf_idx, MEMCG_PATH_BUF_SIZE) -
MEMCG_PATH_BUF_SIZE;
- return &this_cpu_read(memcg_path_buf)[idx];
+ return &buf[idx];
}
static inline void put_memcg_path_buf(void)
{
this_cpu_sub(memcg_path_buf_idx, MEMCG_PATH_BUF_SIZE);
+ rcu_read_unlock();
}
/*
* Write the given mm_struct's memcg path to a percpu buffer, and return a
- * pointer to it. If the path cannot be determined, NULL is returned.
+ * pointer to it. If the path cannot be determined, or no buffer was available
+ * (because the trace event is being unregistered), NULL is returned.
*
* Note: buffers are allocated per-cpu to avoid locking, so preemption must be
* disabled by the caller before calling us, and re-enabled only after the
* caller is done with the pointer.
+ *
+ * The caller must call put_memcg_path_buf() once the buffer is no longer
+ * needed. This must be done while preemption is still disabled.
*/
static const char *get_mm_memcg_path(struct mm_struct *mm)
{
+ char *buf = NULL;
struct mem_cgroup *memcg = get_mem_cgroup_from_mm(mm);
- if (memcg != NULL && likely(memcg->css.cgroup != NULL)) {
- char *buf = get_memcg_path_buf();
+ if (memcg == NULL)
+ goto out;
+ if (unlikely(memcg->css.cgroup == NULL))
+ goto out_put;
- cgroup_path(memcg->css.cgroup, buf, MEMCG_PATH_BUF_SIZE);
- return buf;
- }
- return NULL;
+ buf = get_memcg_path_buf();
+ if (buf == NULL)
+ goto out_put;
+
+ cgroup_path(memcg->css.cgroup, buf, MEMCG_PATH_BUF_SIZE);
+
+out_put:
+ css_put(&memcg->css);
+out:
+ return buf;
}
#define TRACE_MMAP_LOCK_EVENT(type, mm, ...) \
--
2.29.2.454.gaff20da3a2-goog
On Tue, 1 Dec 2020 12:32:49 -0800
Axel Rasmussen <[email protected]> wrote:
> +/* Called with reg_lock held. */
The above comment is reduntant, as the lockdep_is_held() below also suggest
that it is ;-)
> +static void free_memcg_path_bufs(void)
> +{
> + int cpu;
> + char *old;
> +
> + for_each_possible_cpu(cpu) {
> + old = rcu_dereference_protected(per_cpu(memcg_path_buf, cpu),
> + lockdep_is_held(®_lock));
> + if (old == NULL)
> + break;
Hmm, what if the topology of the system has missing CPU numbers (this is
possible I believe on some systems)?
> + rcu_assign_pointer(per_cpu(memcg_path_buf, cpu), NULL);
> + /* Wait for inflight memcg_path_buf users to finish. */
> + synchronize_rcu();
Please break this up into two loops. You will need to have another array
that is created in trace_mmap_lock_reg() function:
static char **path_holders;
trace_mmap_lock_reg()
{
[..]
path_holders = kmalloc(num_possible_cpus * sizeof(*path_holders));
[..]
}
Then this function can be:
static void free_memcg_path_bufs(void)
{
int cpu;
for_each_possible_cpu(cpu) {
path_holders[cpu] = rcu_dereference_protected(per_cpu(memcg_path_buf, cpu),
lockdep_is_held(®_lock));
rcu_assign_pointer(per_cpu(memcg_path_buf, cpu), NULL);
}
/* Wait for inflight memcg_path_buf users to finish. */
synchronize_rcu();
for_each_possible_cpu(cpu) {
kfree(path_holders[cpu]);
}
kfree(path_holders);
path_holders = NULL;
}
Otherwise, if you have a machine with 128 possible CPUs, doing 128
synchronize_rcu()s is going to be expensive!
> + kfree(old);
> + }
> +}
>
> static inline char *get_memcg_path_buf(void)
> {
> + char *buf;
> int idx;
>
> + rcu_read_lock();
The caller of get_mm_memcg_path() has preemption disabled, which is also
now an RCU lock. So the rcu_read_lock() is somewhat redundant.
Oh, and looking at the original patch:
+ memcg_path != NULL ? memcg_path : "", \
The above could be shorten to:
memcg_path ? : "",
As gcc has a trick with the "? :" which is if there's nothing in between
the "?" and ":" it will use what was tested as the result if it is not zero
or NULL.
-- Steve
> + buf = rcu_dereference(*this_cpu_ptr(&memcg_path_buf));
> + if (buf == NULL)
> + return NULL;
> idx = this_cpu_add_return(memcg_path_buf_idx, MEMCG_PATH_BUF_SIZE) -
> MEMCG_PATH_BUF_SIZE;
> - return &this_cpu_read(memcg_path_buf)[idx];
> + return &buf[idx];
> }
On Tue, 1 Dec 2020 16:28:47 -0500
Steven Rostedt <[email protected]> wrote:
> On Tue, 1 Dec 2020 12:32:49 -0800
> Axel Rasmussen <[email protected]> wrote:
>
> > +/* Called with reg_lock held. */
>
> The above comment is reduntant, as the lockdep_is_held() below also suggest
> that it is ;-)
>
>
> > static inline char *get_memcg_path_buf(void)
> > {
> > + char *buf;
> > int idx;
> >
> > + rcu_read_lock();
>
> The caller of get_mm_memcg_path() has preemption disabled, which is also
> now an RCU lock. So the rcu_read_lock() is somewhat redundant.
BTW, both of these comments are FYI. You can keep the comment and keep the
rcu_read_lock(). I was just stating that they are redundant, but keeping
them may make the code a bit more robust.
-- Steve