On Thu, Jun 20, 2024 at 6:08 PM Tetsuo Handa
<penguin-kernel@xxxxxxxxxxxxxxxxxxx> wrote:
>
> Commit 2b5067a8143e ("mm: mmap_lock: add tracepoints around lock
> acquisition") introduced TRACE_MMAP_LOCK_EVENT() macro using
> preempt_disable() in order to let get_mm_memcg_path() return a percpu
> buffer exclusively used by normal, softirq, irq and NMI contexts
> respectively.
>
> Commit 832b50725373 ("mm: mmap_lock: use local locks instead of disabling
> preemption") replaced preempt_disable() with local_lock(&memcg_paths.lock)
> based on an argument that preempt_disable() has to be avoided because
> get_mm_memcg_path() might sleep if PREEMPT_RT=y.
>
> But syzbot started reporting
>
> inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage.
>
> and
>
> inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
>
> messages, for local_lock() does not disable IRQ.
>
> We could replace local_lock() with local_lock_irqsave() in order to
> suppress these messages. But this patch instead replaces percpu buffers
> with on-stack buffer, for the size of each buffer returned by
> get_memcg_path_buf() is only 256 bytes which is tolerable for allocating
> from current thread's kernel stack memory.
>
> Reported-by: syzbot <syzbot+40905bca570ae6784745@xxxxxxxxxxxxxxxxxxxxxxxxx>
> Closes: https://syzkaller.appspot.com/bug?extid=40905bca570ae6784745
> Fixes: 832b50725373 ("mm: mmap_lock: use local locks instead of disabling preemption")
> Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> ---
> Only compile tested.
>
> mm/mmap_lock.c | 175 ++++++-------------------------------------------
> 1 file changed, 20 insertions(+), 155 deletions(-)
No objections. Looking back all the way to the first version [1] the
buffers were already percpu, instead of on the stack like this. IOW,
there was no on-list discussion about why this shouldn't go on the
stack. It has been a while, but if memory serves I opted to do it that
way just out of paranoia around putting large buffers on the stack.
But, I agree 256 bytes isn't all that large.
That v1 patch wasn't all that complex, but then again it didn't deal
with various edge cases properly :) so it has grown significantly more
complex over time. Reconsidering the approach seems reasonable now,
given how much code this removes.
This change looks straightforwardly correct to me. You can take:
Reviewed-by: Axel Rasmussen <axelrasmussen@xxxxxxxxxx>
[1]: https://lore.kernel.org/all/20200917154258.1a364cdf@xxxxxxxxxxxxxxxxxx/T/
>
> diff --git a/mm/mmap_lock.c b/mm/mmap_lock.c
> index 1854850b4b89..368b840e7508 100644
> --- a/mm/mmap_lock.c
> +++ b/mm/mmap_lock.c
> @@ -19,14 +19,7 @@ EXPORT_TRACEPOINT_SYMBOL(mmap_lock_released);
>
> #ifdef CONFIG_MEMCG
>
> -/*
> - * Our various events all share the same buffer (because we don't want or need
> - * to allocate a set of buffers *per event type*), so we need to protect against
> - * concurrent _reg() and _unreg() calls, and count how many _reg() calls have
> - * been made.
> - */
> -static DEFINE_MUTEX(reg_lock);
> -static int reg_refcount; /* Protected by reg_lock. */
> +static atomic_t reg_refcount;
>
> /*
> * Size of the buffer for memcg path names. Ignoring stack trace support,
> @@ -34,136 +27,22 @@ static int reg_refcount; /* Protected by reg_lock. */
> */
> #define MEMCG_PATH_BUF_SIZE MAX_FILTER_STR_VAL
>
> -/*
> - * How many contexts our trace events might be called in: normal, softirq, irq,
> - * and NMI.
> - */
> -#define CONTEXT_COUNT 4
> -
> -struct memcg_path {
> - local_lock_t lock;
> - char __rcu *buf;
> - local_t buf_idx;
> -};
> -static DEFINE_PER_CPU(struct memcg_path, memcg_paths) = {
> - .lock = INIT_LOCAL_LOCK(lock),
> - .buf_idx = LOCAL_INIT(0),
> -};
> -
> -static char **tmp_bufs;
> -
> -/* Called with reg_lock held. */
> -static void free_memcg_path_bufs(void)
> -{
> - struct memcg_path *memcg_path;
> - int cpu;
> - char **old = tmp_bufs;
> -
> - for_each_possible_cpu(cpu) {
> - memcg_path = per_cpu_ptr(&memcg_paths, cpu);
> - *(old++) = rcu_dereference_protected(memcg_path->buf,
> - lockdep_is_held(®_lock));
> - rcu_assign_pointer(memcg_path->buf, NULL);
> - }
> -
> - /* Wait for inflight memcg_path_buf users to finish. */
> - synchronize_rcu();
> -
> - old = tmp_bufs;
> - for_each_possible_cpu(cpu) {
> - kfree(*(old++));
> - }
> -
> - kfree(tmp_bufs);
> - tmp_bufs = NULL;
> -}
> -
> int trace_mmap_lock_reg(void)
> {
> - int cpu;
> - char *new;
> -
> - mutex_lock(®_lock);
> -
> - /* If the refcount is going 0->1, proceed with allocating buffers. */
> - if (reg_refcount++)
> - goto out;
> -
> - tmp_bufs = kmalloc_array(num_possible_cpus(), sizeof(*tmp_bufs),
> - GFP_KERNEL);
> - if (tmp_bufs == NULL)
> - goto out_fail;
> -
> - for_each_possible_cpu(cpu) {
> - new = kmalloc(MEMCG_PATH_BUF_SIZE * CONTEXT_COUNT, GFP_KERNEL);
> - if (new == NULL)
> - goto out_fail_free;
> - rcu_assign_pointer(per_cpu_ptr(&memcg_paths, cpu)->buf, new);
> - /* Don't need to wait for inflights, they'd have gotten NULL. */
> - }
> -
> -out:
> - mutex_unlock(®_lock);
> + atomic_inc(®_refcount);
> return 0;
> -
> -out_fail_free:
> - free_memcg_path_bufs();
> -out_fail:
> - /* Since we failed, undo the earlier ref increment. */
> - --reg_refcount;
> -
> - mutex_unlock(®_lock);
> - return -ENOMEM;
> }
>
> void trace_mmap_lock_unreg(void)
> {
> - mutex_lock(®_lock);
> -
> - /* If the refcount is going 1->0, proceed with freeing buffers. */
> - if (--reg_refcount)
> - goto out;
> -
> - free_memcg_path_bufs();
> -
> -out:
> - mutex_unlock(®_lock);
> -}
> -
> -static inline char *get_memcg_path_buf(void)
> -{
> - struct memcg_path *memcg_path = this_cpu_ptr(&memcg_paths);
> - char *buf;
> - int idx;
> -
> - rcu_read_lock();
> - buf = rcu_dereference(memcg_path->buf);
> - if (buf == NULL) {
> - rcu_read_unlock();
> - return NULL;
> - }
> - idx = local_add_return(MEMCG_PATH_BUF_SIZE, &memcg_path->buf_idx) -
> - MEMCG_PATH_BUF_SIZE;
> - return &buf[idx];
> + atomic_dec(®_refcount);
> }
>
> -static inline void put_memcg_path_buf(void)
> -{
> - local_sub(MEMCG_PATH_BUF_SIZE, &this_cpu_ptr(&memcg_paths)->buf_idx);
> - rcu_read_unlock();
> -}
> -
> -#define TRACE_MMAP_LOCK_EVENT(type, mm, ...) \
> - do { \
> - const char *memcg_path; \
> - local_lock(&memcg_paths.lock); \
> - memcg_path = get_mm_memcg_path(mm); \
> - trace_mmap_lock_##type(mm, \
> - memcg_path != NULL ? memcg_path : "", \
> - ##__VA_ARGS__); \
> - if (likely(memcg_path != NULL)) \
> - put_memcg_path_buf(); \
> - local_unlock(&memcg_paths.lock); \
> +#define TRACE_MMAP_LOCK_EVENT(type, mm, ...) \
> + do { \
> + char buf[MEMCG_PATH_BUF_SIZE]; \
> + get_mm_memcg_path(mm, buf, sizeof(buf)); \
> + trace_mmap_lock_##type(mm, buf, ##__VA_ARGS__); \
> } while (0)
>
> #else /* !CONFIG_MEMCG */
> @@ -185,37 +64,23 @@ void trace_mmap_lock_unreg(void)
> #ifdef CONFIG_TRACING
> #ifdef CONFIG_MEMCG
> /*
> - * Write the given mm_struct's memcg path to a percpu buffer, and return a
> - * 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.
> + * Write the given mm_struct's memcg path to a buffer. If the path cannot be
> + * determined or the trace event is being unregistered, empty string is written.
> */
> -static const char *get_mm_memcg_path(struct mm_struct *mm)
> +static void get_mm_memcg_path(struct mm_struct *mm, char *buf, size_t buflen)
> {
> - char *buf = NULL;
> - struct mem_cgroup *memcg = get_mem_cgroup_from_mm(mm);
> + struct mem_cgroup *memcg;
>
> + buf[0] = '\0';
> + /* No need to get path if no trace event is registered. */
> + if (!atomic_read(®_refcount))
> + return;
> + memcg = get_mem_cgroup_from_mm(mm);
> if (memcg == NULL)
> - goto out;
> - if (unlikely(memcg->css.cgroup == NULL))
> - goto out_put;
> -
> - buf = get_memcg_path_buf();
> - if (buf == NULL)
> - goto out_put;
> -
> - cgroup_path(memcg->css.cgroup, buf, MEMCG_PATH_BUF_SIZE);
> -
> -out_put:
> + return;
> + if (memcg->css.cgroup)
> + cgroup_path(memcg->css.cgroup, buf, buflen);
> css_put(&memcg->css);
> -out:
> - return buf;
> }
>
> #endif /* CONFIG_MEMCG */
> --
> 2.43.0
>
