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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] mm: conditional definition of unconditionally declared API copy_remote_mm_str
--

commit 046f2d358ae7593df6c056f6c498b844e89a44bc
Author: Anastasios Papagiannis <tasos.papagiannnis@gmail.com>

mm: Add copy_remote_mm_str()

This commit adds copy_remote_mm_str() to operate directly on an mm_struct
rather than requiring a task_struct. It also moves the public wrappers
for these functions into mm/util.c to share a common internal interface
between MMU and NOMMU builds.

> diff --git a/include/linux/mm.h b/include/linux/mm.h
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
[ ... ]
> @@ -3325,10 +3325,10 @@ extern int access_process_vm(struct task_struct *tsk, unsigned long addr,
>  extern int access_remote_vm(struct mm_struct *mm, unsigned long addr,
>  		void *buf, int len, unsigned int gup_flags);
>  
> -#ifdef CONFIG_BPF_SYSCALL
> -extern int copy_remote_vm_str(struct task_struct *tsk, unsigned long addr,
> -			      void *buf, int len, unsigned int gup_flags);
> -#endif
> +int copy_remote_mm_str(struct mm_struct *mm, unsigned long addr,
> +		void *buf, int len, unsigned int gup_flags);
> +int copy_remote_vm_str(struct task_struct *tsk, unsigned long addr,
> +		void *buf, int len, unsigned int gup_flags);

[Severity: Low]
Are these functions intended to be available to all subsystems regardless of
configuration? The #ifdef CONFIG_BPF_SYSCALL guard was removed here, making
the declarations unconditional in the public header.

[ ... ]
> diff --git a/mm/util.c b/mm/util.c
> --- a/mm/util.c
> +++ b/mm/util.c
[ ... ]
> @@ -1061,6 +1061,68 @@ int get_cmdline(struct task_struct *task, char *buffer, int buflen)
>  	return res;
>  }
>  
> +#ifdef CONFIG_BPF_SYSCALL
> +/**
> + * copy_remote_mm_str - copy a string from a remote address space.
> + * @mm:         the remote address space
[ ... ]
> +int copy_remote_mm_str(struct mm_struct *mm, unsigned long addr,
> +		void *buf, int len, unsigned int gup_flags)
> +{
> +	if (unlikely(len == 0))
> +		return 0;
> +
> +	return __copy_remote_mm_str(mm, addr, buf, len, gup_flags);
> +}

[Severity: Low]
Since the implementations of copy_remote_mm_str() and copy_remote_vm_str()
are strictly enclosed within an #ifdef CONFIG_BPF_SYSCALL block, doesn't this
result in a link-time failure if code outside of BPF uses them in a
!CONFIG_BPF_SYSCALL build?

The underlying worker function __copy_remote_mm_str() in mm/memory.c appears
to have the same mismatch, where it is unconditionally declared in
mm/internal.h but defined conditionally here. 

Could the #ifdef CONFIG_BPF_SYSCALL blocks be retained in the headers, or
dummy static inline implementations be provided for the !CONFIG_BPF_SYSCALL
case?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915080255.48929-1-tasos.papagiannnis@gmail.com?part=1