Re: [PATCH bpf-next v8 1/5] mm: Add copy_remote_mm_str()

[bot+bpf-ci]

> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index dd09c438fa23e..63f40e615754d 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h

[ ... ]

> diff --git a/mm/internal.h b/mm/internal.h
> index 38b1165212c94..557b293813557 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -25,6 +25,9 @@
>  struct folio_batch;
>  struct hstate;
>
> +int __copy_remote_mm_str(struct mm_struct *mm, unsigned long addr,
> +			 void *buf, int len, unsigned int gup_flags);
> +

__copy_remote_mm_str() goes from 'static' to an mm-wide helper, but its
'len >= 1' precondition is left undocumented and is now enforced in a
different translation unit from the implementations.

Both implementations begin by writing to the buffer unconditionally,
before any length check:

mm/memory.c:__copy_remote_mm_str() {
    void *old_buf = buf;
    int err = 0;

    if (mmap_read_lock_killable(mm))
	    return -EINTR;

    *(char *)buf = '\0';
    ...
}

mm/nommu.c:__copy_remote_mm_str() {
    unsigned long addr_end;
    struct vm_area_struct *vma;
    int ret = 0;

    if (mmap_read_lock_killable(mm))
	    return -EINTR;

    *(char *)buf = '\0';
    ...
}

Neither ever tests 'len == 0'. The guard lives only in the two wrappers,
in a third file (mm/util.c), so calling __copy_remote_mm_str() with
len == 0 writes one byte past the end of a zero-length buffer.

Before the patch this was structurally impossible: __copy_remote_vm_str()
was 'static' in mm/memory.c and mm/nommu.c and its single caller was the
copy_remote_vm_str() immediately below it in the same file, so the guard
and the unguarded write could not drift apart. Now any file in mm/ that
includes internal.h can call it, and the declaration carries no comment
about the contract.

Should either the 'len must be >= 1' requirement be documented at the
mm/internal.h declaration, or should the 'if (unlikely(len == 0))
return 0;' test be moved into __copy_remote_mm_str() so the guard sits
with the write it protects?


> diff --git a/mm/memory.c b/mm/memory.c
> index 8b0c2c735d3de..fc6933d7e9d36 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c

[ ... ]

> diff --git a/mm/nommu.c b/mm/nommu.c
> index 498e01ee40b05..9a810c35e7e90 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -1741,13 +1741,12 @@ int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, in
>  }
>  EXPORT_SYMBOL_GPL(access_process_vm);
>
> -#ifdef CONFIG_BPF_SYSCALL
>  /*
>   * Copy a string from another process's address space as given in mm.
>   * If there is any error return -EFAULT.
>   */
> -static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
> -				void *buf, int len)
> +int __copy_remote_mm_str(struct mm_struct *mm, unsigned long addr,
> +			 void *buf, int len, unsigned int gup_flags)
>  {

The NOMMU implementation gains an 'unsigned int gup_flags' parameter that
it never references, and at the same time the patch deletes the only
documentation that said so.

The pre-patch NOMMU kernel-doc read:

 * @gup_flags:  flags modifying lookup behaviour (unused)

The replacement is a single shared kernel-doc in mm/util.c, compiled for
both MMU and NOMMU:

mm/util.c:copy_remote_mm_str():
 * @gup_flags:  flags modifying lookup behaviour

mm/util.c:copy_remote_vm_str():
 * @gup_flags:  flags modifying lookup behaviour

The '(unused)' caveat is gone from both blocks.

The full NOMMU body makes no use of the argument - it goes from
find_vma()/check_add_overflow() to 'ret = strscpy(buf, (char *)addr, len)'
with no reference to gup_flags.

Should the kernel-doc in mm/util.c note that the flags are ignored on
!CONFIG_MMU, or should the '(unused)' comment be restored somewhere in
the NOMMU code itself?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/35330379263