Re: [bpf-next v8 1/3] mm: add copy_remote_vm_str

[Shakeel Butt <shakeel.butt@linux.dev>]

Hi Andrew,

Do you prefer this patch series to go though mm-tree or routing these
through bpf tree is fine with you?

Shakeel

On Thu, Feb 13, 2025 at 07:21:23AM -0800, Jordan Rome wrote:
> Similar to `access_process_vm` but specific to strings.
> Also chunks reads by page and utilizes `strscpy`
> for handling null termination.
> 
> The primary motivation for this change is to copy
> strings from a non-current task/process in BPF.
> There is already a helper `bpf_copy_from_user_task`,
> which uses `access_process_vm` but one to handle
> strings would be very helpful.
> 
> Reviewed-by: Shakeel Butt <shakeel.butt@linux.dev>
> Signed-off-by: Jordan Rome <linux@jordanrome.com>
> ---
>  include/linux/mm.h |   3 ++
>  mm/memory.c        | 122 +++++++++++++++++++++++++++++++++++++++++++++
>  mm/nommu.c         |  76 ++++++++++++++++++++++++++++
>  3 files changed, 201 insertions(+)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 7b1068ddcbb7..aee23d84ce01 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2486,6 +2486,9 @@ 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);
> 
> +extern int copy_remote_vm_str(struct task_struct *tsk, unsigned long addr,
> +		void *buf, int len, unsigned int gup_flags);
> +
>  long get_user_pages_remote(struct mm_struct *mm,
>  			   unsigned long start, unsigned long nr_pages,
>  			   unsigned int gup_flags, struct page **pages,
> diff --git a/mm/memory.c b/mm/memory.c
> index 539c0f7c6d54..014fe35af071 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -6803,6 +6803,128 @@ int access_process_vm(struct task_struct *tsk, unsigned long addr,
>  }
>  EXPORT_SYMBOL_GPL(access_process_vm);
> 
> +/*
> + * 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, unsigned int gup_flags)
> +{
> +	void *old_buf = buf;
> +	int err = 0;
> +
> +	*(char *)buf = '\0';
> +
> +	if (mmap_read_lock_killable(mm))
> +		return -EFAULT;
> +
> +	/* Untag the address before looking up the VMA */
> +	addr = untagged_addr_remote(mm, addr);
> +
> +	/* Avoid triggering the temporary warning in __get_user_pages */
> +	if (!vma_lookup(mm, addr)) {
> +		err = -EFAULT;
> +		goto out;
> +	}
> +
> +	while (len) {
> +		int bytes, offset, retval;
> +		void *maddr;
> +		struct page *page;
> +		struct vm_area_struct *vma = NULL;
> +
> +		page = get_user_page_vma_remote(mm, addr, gup_flags, &vma);
> +
> +		if (IS_ERR(page)) {
> +			/*
> +			 * Treat as a total failure for now until we decide how
> +			 * to handle the CONFIG_HAVE_IOREMAP_PROT case and
> +			 * stack expansion.
> +			 */
> +			*(char *)buf = '\0';
> +			err = -EFAULT;
> +			goto out;
> +		}
> +
> +		bytes = len;
> +		offset = addr & (PAGE_SIZE - 1);
> +		if (bytes > PAGE_SIZE - offset)
> +			bytes = PAGE_SIZE - offset;
> +
> +		maddr = kmap_local_page(page);
> +		retval = strscpy(buf, maddr + offset, bytes);
> +
> +		if (retval >= 0) {
> +			/* Found the end of the string */
> +			buf += retval;
> +			unmap_and_put_page(page, maddr);
> +			break;
> +		}
> +
> +		buf += bytes - 1;
> +		/*
> +		 * Because strscpy always NUL terminates we need to
> +		 * copy the last byte in the page if we are going to
> +		 * load more pages
> +		 */
> +		if (bytes != len) {
> +			addr += bytes - 1;
> +			copy_from_user_page(vma, page, addr, buf,
> +					maddr + (PAGE_SIZE - 1), 1);
> +
> +			buf += 1;
> +			addr += 1;
> +		}
> +		len -= bytes;
> +
> +		unmap_and_put_page(page, maddr);
> +	}
> +
> +out:
> +	mmap_read_unlock(mm);
> +	if (err)
> +		return err;
> +
> +	return buf - old_buf;
> +}
> +
> +/**
> + * copy_remote_vm_str - copy a string from another process's address space.
> + * @tsk:	the task of the target address space
> + * @addr:	start address to read from
> + * @buf:	destination buffer
> + * @len:	number of bytes to copy
> + * @gup_flags:	flags modifying lookup behaviour
> + *
> + * The caller must hold a reference on @mm.
> + *
> + * Return: number of bytes copied from @addr (source) to @buf (destination);
> + * not including the trailing NUL. Always guaranteed to leave NUL-terminated
> + * buffer. On any error, return -EFAULT.
> + */
> +int copy_remote_vm_str(struct task_struct *tsk, unsigned long addr,
> +		void *buf, int len, unsigned int gup_flags)
> +{
> +	struct mm_struct *mm;
> +	int ret;
> +
> +	if (unlikely(len < 1))
> +		return 0;
> +
> +	mm = get_task_mm(tsk);
> +	if (!mm) {
> +		*(char *)buf = '\0';
> +		return -EFAULT;
> +	}
> +
> +	ret = __copy_remote_vm_str(mm, addr, buf, len, gup_flags);
> +
> +	mmput(mm);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(copy_remote_vm_str);
> +
>  /*
>   * Print the name of a VMA.
>   */
> diff --git a/mm/nommu.c b/mm/nommu.c
> index baa79abdaf03..11d2341c634e 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -1708,6 +1708,82 @@ int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, in
>  }
>  EXPORT_SYMBOL_GPL(access_process_vm);
> 
> +/*
> + * 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)
> +{
> +	unsigned long addr_end;
> +	struct vm_area_struct *vma;
> +	int ret = -EFAULT;
> +
> +	*(char *)buf = '\0';
> +
> +	if (mmap_read_lock_killable(mm))
> +		return ret;
> +
> +	/* the access must start within one of the target process's mappings */
> +	vma = find_vma(mm, addr);
> +	if (!vma)
> +		goto out;
> +
> +	if (check_add_overflow(addr, len, &addr_end))
> +		goto out;
> +	/* don't overrun this mapping */
> +	if (addr_end > vma->vm_end)
> +		len = vma->vm_end - addr;
> +
> +	/* only read mappings where it is permitted */
> +	if (vma->vm_flags & VM_MAYREAD) {
> +		ret = strscpy(buf, (char *)addr, len);
> +		if (ret < 0)
> +			ret = len - 1;
> +	}
> +
> +out:
> +	mmap_read_unlock(mm);
> +	return ret;
> +}
> +
> +/**
> + * copy_remote_vm_str - copy a string from another process's address space.
> + * @tsk:	the task of the target address space
> + * @addr:	start address to read from
> + * @buf:	destination buffer
> + * @len:	number of bytes to copy
> + * @gup_flags:	flags modifying lookup behaviour (unused)
> + *
> + * The caller must hold a reference on @mm.
> + *
> + * Return: number of bytes copied from @addr (source) to @buf (destination);
> + * not including the trailing NUL. Always guaranteed to leave NUL-terminated
> + * buffer. On any error, return -EFAULT.
> + */
> +int copy_remote_vm_str(struct task_struct *tsk, unsigned long addr,
> +		void *buf, int len, unsigned int gup_flags)
> +{
> +	struct mm_struct *mm;
> +	int ret;
> +
> +	if (unlikely(len < 1))
> +		return 0;
> +
> +	mm = get_task_mm(tsk);
> +	if (!mm) {
> +		*(char *)buf = '\0';
> +		return -EFAULT;
> +	}
> +
> +	ret = __copy_remote_vm_str(mm, addr, buf, len);
> +
> +	mmput(mm);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(copy_remote_vm_str);
> +
>  /**
>   * nommu_shrink_inode_mappings - Shrink the shared mappings on an inode
>   * @inode: The inode to check
> --
> 2.43.5
>