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

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

[Jordan Rome]

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

[bpf-next v8 2/3] bpf: Add bpf_copy_from_user_task_str kfunc

[Jordan Rome]

This new kfunc will be able to copy a string
from another process's/task's address space.
This is similar to `bpf_copy_from_user_str`
but accepts a `struct task_struct*` argument.

Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Jordan Rome <linux@jordanrome.com>
---
 kernel/bpf/helpers.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index f27ce162427a..a33f72a4c31f 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -3082,6 +3082,53 @@ __bpf_kfunc void bpf_local_irq_restore(unsigned long *flags__irq_flag)
 	local_irq_restore(*flags__irq_flag);
 }

+/**
+ * bpf_copy_from_user_task_str() - Copy a string from an task's address space
+ * @dst:             Destination address, in kernel space.  This buffer must be
+ *                   at least @dst__sz bytes long.
+ * @dst__sz:         Maximum number of bytes to copy, includes the trailing NUL.
+ * @unsafe_ptr__ign: Source address in the task's address space.
+ * @tsk:             The task whose address space will be used
+ * @flags:           The only supported flag is BPF_F_PAD_ZEROS
+ *
+ * Copies a NUL terminated string from a task's address space to @dst__sz
+ * buffer. If user string is too long this will still ensure zero termination
+ * in the @dst__sz buffer unless buffer size is 0.
+ *
+ * If BPF_F_PAD_ZEROS flag is set, memset the tail of @dst__sz to 0 on success
+ * and memset all of @dst__sz on failure.
+ *
+ * Return: The number of copied bytes on success including the NUL terminator.
+ * A negative error code on failure.
+ */
+__bpf_kfunc int bpf_copy_from_user_task_str(void *dst,
+					    u32 dst__sz,
+					    const void __user *unsafe_ptr__ign,
+					    struct task_struct *tsk,
+					    u64 flags)
+{
+	int ret;
+
+	if (unlikely(flags & ~BPF_F_PAD_ZEROS))
+		return -EINVAL;
+
+	if (unlikely(!dst__sz))
+		return 0;
+
+	ret = copy_remote_vm_str(tsk, (unsigned long)unsafe_ptr__ign, dst, dst__sz, 0);
+
+	if (ret < 0) {
+		if (flags & BPF_F_PAD_ZEROS)
+			memset(dst, 0, dst__sz);
+		return ret;
+	}
+
+	if (flags & BPF_F_PAD_ZEROS)
+		memset(dst + ret, 0, dst__sz - ret);
+
+	return ret + 1;
+}
+
 __bpf_kfunc_end_defs();

 BTF_KFUNCS_START(generic_btf_ids)
@@ -3174,6 +3221,7 @@ BTF_ID_FLAGS(func, bpf_iter_bits_new, KF_ITER_NEW)
 BTF_ID_FLAGS(func, bpf_iter_bits_next, KF_ITER_NEXT | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_iter_bits_destroy, KF_ITER_DESTROY)
 BTF_ID_FLAGS(func, bpf_copy_from_user_str, KF_SLEEPABLE)
+BTF_ID_FLAGS(func, bpf_copy_from_user_task_str, KF_SLEEPABLE)
 BTF_ID_FLAGS(func, bpf_get_kmem_cache)
 BTF_ID_FLAGS(func, bpf_iter_kmem_cache_new, KF_ITER_NEW | KF_SLEEPABLE)
 BTF_ID_FLAGS(func, bpf_iter_kmem_cache_next, KF_ITER_NEXT | KF_RET_NULL | KF_SLEEPABLE)
--
2.43.5

[bpf-next v8 3/3] selftests/bpf: Add tests for bpf_copy_from_user_task_str

[Jordan Rome]

This adds tests for both the happy path and the
error path (with and without the BPF_F_PAD_ZEROS flag).

Signed-off-by: Jordan Rome <linux@jordanrome.com>
---
 .../selftests/bpf/prog_tests/bpf_iter.c       |  68 +++++++++++
 .../selftests/bpf/prog_tests/read_vsyscall.c  |   1 +
 .../selftests/bpf/progs/bpf_iter_tasks.c      | 110 ++++++++++++++++++
 .../selftests/bpf/progs/read_vsyscall.c       |  11 +-
 4 files changed, 188 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
index 6f1bfacd7375..add4a18c33bd 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
@@ -323,19 +323,87 @@ static void test_task_pidfd(void)
 static void test_task_sleepable(void)
 {
 	struct bpf_iter_tasks *skel;
+	int pid, status, err, data_pipe[2], finish_pipe[2], c;
+	char *test_data = NULL;
+	char *test_data_long = NULL;
+	char *data[2];
+
+	if (!ASSERT_OK(pipe(data_pipe), "data_pipe") ||
+	    !ASSERT_OK(pipe(finish_pipe), "finish_pipe"))
+		return;

 	skel = bpf_iter_tasks__open_and_load();
 	if (!ASSERT_OK_PTR(skel, "bpf_iter_tasks__open_and_load"))
 		return;

+	pid = fork();
+	if (!ASSERT_GE(pid, 0, "fork"))
+		return;
+
+	if (pid == 0) {
+		/* child */
+		close(data_pipe[0]);
+		close(finish_pipe[1]);
+
+		test_data = malloc(sizeof(char) * 10);
+		strncpy(test_data, "test_data", 10);
+		test_data[9] = '\0';
+
+		test_data_long = malloc(sizeof(char) * 5000);
+		for (int i = 0; i < 5000; ++i) {
+			if (i % 2 == 0)
+				test_data_long[i] = 'b';
+			else
+				test_data_long[i] = 'a';
+		}
+		test_data_long[4999] = '\0';
+
+		data[0] = test_data;
+		data[1] = test_data_long;
+
+		write(data_pipe[1], &data, sizeof(data));
+
+		/* keep child alive until after the test */
+		err = read(finish_pipe[0], &c, 1);
+		if (err != 1)
+			exit(-1);
+
+		close(data_pipe[1]);
+		close(finish_pipe[0]);
+		_exit(0);
+	}
+
+	/* parent */
+	close(data_pipe[1]);
+	close(finish_pipe[0]);
+
+	err = read(data_pipe[0], &data, sizeof(data));
+	ASSERT_EQ(err, sizeof(data), "read_check");
+
+	skel->bss->user_ptr = data[0];
+	skel->bss->user_ptr_long = data[1];
+	skel->bss->pid = pid;
+
 	do_dummy_read(skel->progs.dump_task_sleepable);

 	ASSERT_GT(skel->bss->num_expected_failure_copy_from_user_task, 0,
 		  "num_expected_failure_copy_from_user_task");
 	ASSERT_GT(skel->bss->num_success_copy_from_user_task, 0,
 		  "num_success_copy_from_user_task");
+	ASSERT_GT(skel->bss->num_expected_failure_copy_from_user_task_str, 0,
+		  "num_expected_failure_copy_from_user_task_str");
+	ASSERT_GT(skel->bss->num_success_copy_from_user_task_str, 0,
+		  "num_success_copy_from_user_task_str");

 	bpf_iter_tasks__destroy(skel);
+
+	write(finish_pipe[1], &c, 1);
+	err = waitpid(pid, &status, 0);
+	ASSERT_EQ(err, pid, "waitpid");
+	ASSERT_EQ(status, 0, "zero_child_exit");
+
+	close(data_pipe[0]);
+	close(finish_pipe[1]);
 }

 static void test_task_stack(void)
diff --git a/tools/testing/selftests/bpf/prog_tests/read_vsyscall.c b/tools/testing/selftests/bpf/prog_tests/read_vsyscall.c
index c7b9ba8b1d06..a8d1eaa67020 100644
--- a/tools/testing/selftests/bpf/prog_tests/read_vsyscall.c
+++ b/tools/testing/selftests/bpf/prog_tests/read_vsyscall.c
@@ -24,6 +24,7 @@ struct read_ret_desc {
 	{ .name = "copy_from_user", .ret = -EFAULT },
 	{ .name = "copy_from_user_task", .ret = -EFAULT },
 	{ .name = "copy_from_user_str", .ret = -EFAULT },
+	{ .name = "copy_from_user_task_str", .ret = -EFAULT },
 };

 void test_read_vsyscall(void)
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_tasks.c b/tools/testing/selftests/bpf/progs/bpf_iter_tasks.c
index bc10c4e4b4fa..966ee5a7b066 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_tasks.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_tasks.c
@@ -9,6 +9,13 @@ char _license[] SEC("license") = "GPL";
 uint32_t tid = 0;
 int num_unknown_tid = 0;
 int num_known_tid = 0;
+void *user_ptr = 0;
+void *user_ptr_long = 0;
+uint32_t pid = 0;
+
+static char big_str1[5000];
+static char big_str2[5005];
+static char big_str3[4996];

 SEC("iter/task")
 int dump_task(struct bpf_iter__task *ctx)
@@ -35,7 +42,9 @@ int dump_task(struct bpf_iter__task *ctx)
 }

 int num_expected_failure_copy_from_user_task = 0;
+int num_expected_failure_copy_from_user_task_str = 0;
 int num_success_copy_from_user_task = 0;
+int num_success_copy_from_user_task_str = 0;

 SEC("iter.s/task")
 int dump_task_sleepable(struct bpf_iter__task *ctx)
@@ -44,6 +53,9 @@ int dump_task_sleepable(struct bpf_iter__task *ctx)
 	struct task_struct *task = ctx->task;
 	static const char info[] = "    === END ===";
 	struct pt_regs *regs;
+	char task_str1[10] = "aaaaaaaaaa";
+	char task_str2[10], task_str3[10];
+	char task_str4[20] = "aaaaaaaaaaaaaaaaaaaa";
 	void *ptr;
 	uint32_t user_data = 0;
 	int ret;
@@ -78,8 +90,106 @@ int dump_task_sleepable(struct bpf_iter__task *ctx)
 		BPF_SEQ_PRINTF(seq, "%s\n", info);
 		return 0;
 	}
+
 	++num_success_copy_from_user_task;

+	/* Read an invalid pointer and ensure we get an error */
+	ptr = NULL;
+	ret = bpf_copy_from_user_task_str((char *)task_str1, sizeof(task_str1), ptr, task, 0);
+	if (ret >= 0 || task_str1[9] != 'a' || task_str1[0] != '\0') {
+		BPF_SEQ_PRINTF(seq, "%s\n", info);
+		return 0;
+	}
+
+	/* Read an invalid pointer and ensure we get error with pad zeros flag */
+	ptr = NULL;
+	ret = bpf_copy_from_user_task_str((char *)task_str1, sizeof(task_str1),
+					  ptr, task, BPF_F_PAD_ZEROS);
+	if (ret >= 0 || task_str1[9] != '\0' || task_str1[0] != '\0') {
+		BPF_SEQ_PRINTF(seq, "%s\n", info);
+		return 0;
+	}
+
+	++num_expected_failure_copy_from_user_task_str;
+
+	/* Same length as the string */
+	ret = bpf_copy_from_user_task_str((char *)task_str2, 10, user_ptr, task, 0);
+	/* only need to do the task pid check once */
+	if (bpf_strncmp(task_str2, 10, "test_data\0") != 0 || ret != 10 || task->tgid != pid) {
+		BPF_SEQ_PRINTF(seq, "%s\n", info);
+		return 0;
+	}
+
+	/* Shorter length than the string */
+	ret = bpf_copy_from_user_task_str((char *)task_str3, 2, user_ptr, task, 0);
+	if (bpf_strncmp(task_str3, 2, "t\0") != 0 || ret != 2) {
+		BPF_SEQ_PRINTF(seq, "%s\n", info);
+		return 0;
+	}
+
+	/* Longer length than the string */
+	ret = bpf_copy_from_user_task_str((char *)task_str4, 20, user_ptr, task, 0);
+	if (bpf_strncmp(task_str4, 10, "test_data\0") != 0 || ret != 10
+	    || task_str4[sizeof(task_str4) - 1] != 'a') {
+		BPF_SEQ_PRINTF(seq, "%s\n", info);
+		return 0;
+	}
+
+	/* Longer length than the string with pad zeros flag */
+	ret = bpf_copy_from_user_task_str((char *)task_str4, 20, user_ptr, task, BPF_F_PAD_ZEROS);
+	if (bpf_strncmp(task_str4, 10, "test_data\0") != 0 || ret != 10
+	    || task_str4[sizeof(task_str4) - 1] != '\0') {
+		BPF_SEQ_PRINTF(seq, "%s\n", info);
+		return 0;
+	}
+
+	/* Longer length than the string past a page boundary */
+	ret = bpf_copy_from_user_task_str(big_str1, 5000, user_ptr, task, 0);
+	if (bpf_strncmp(big_str1, 10, "test_data\0") != 0 || ret != 10) {
+		BPF_SEQ_PRINTF(seq, "%s\n", info);
+		return 0;
+	}
+
+	/* String that crosses a page boundary */
+	ret = bpf_copy_from_user_task_str(big_str1, 5000, user_ptr_long, task, BPF_F_PAD_ZEROS);
+	if (bpf_strncmp(big_str1, 4, "baba") != 0 || ret != 5000
+	    || bpf_strncmp(big_str1 + 4996, 4, "bab\0") != 0) {
+		BPF_SEQ_PRINTF(seq, "%s\n", info);
+		return 0;
+	}
+
+	for (int i = 0; i < 4999; ++i) {
+		if (i % 2 == 0) {
+			if (big_str1[i] != 'b') {
+				BPF_SEQ_PRINTF(seq, "%s\n", info);
+				return 0;
+			}
+		} else {
+			if (big_str1[i] != 'a') {
+				BPF_SEQ_PRINTF(seq, "%s\n", info);
+				return 0;
+			}
+		}
+	}
+
+	/* Longer length than the string that crosses a page boundary */
+	ret = bpf_copy_from_user_task_str(big_str2, 5005, user_ptr_long, task, BPF_F_PAD_ZEROS);
+	if (bpf_strncmp(big_str2, 4, "baba") != 0 || ret != 5000
+	    || bpf_strncmp(big_str2 + 4996, 5, "bab\0\0") != 0) {
+		BPF_SEQ_PRINTF(seq, "%s\n", info);
+		return 0;
+	}
+
+	/* Shorter length than the string that crosses a page boundary */
+	ret = bpf_copy_from_user_task_str(big_str3, 4996, user_ptr_long, task, 0);
+	if (bpf_strncmp(big_str3, 4, "baba") != 0 || ret != 4996
+	    || bpf_strncmp(big_str3 + 4992, 4, "bab\0") != 0) {
+		BPF_SEQ_PRINTF(seq, "%s\n", info);
+		return 0;
+	}
+
+	++num_success_copy_from_user_task_str;
+
 	if (ctx->meta->seq_num == 0)
 		BPF_SEQ_PRINTF(seq, "    tgid      gid     data\n");

diff --git a/tools/testing/selftests/bpf/progs/read_vsyscall.c b/tools/testing/selftests/bpf/progs/read_vsyscall.c
index 39ebef430059..395591374d4f 100644
--- a/tools/testing/selftests/bpf/progs/read_vsyscall.c
+++ b/tools/testing/selftests/bpf/progs/read_vsyscall.c
@@ -8,14 +8,16 @@

 int target_pid = 0;
 void *user_ptr = 0;
-int read_ret[9];
+int read_ret[10];

 char _license[] SEC("license") = "GPL";

 /*
- * This is the only kfunc, the others are helpers
+ * These are the kfuncs, the others are helpers
  */
 int bpf_copy_from_user_str(void *dst, u32, const void *, u64) __weak __ksym;
+int bpf_copy_from_user_task_str(void *dst, u32, const void *,
+				struct task_struct *, u64) __weak __ksym;

 SEC("fentry/" SYS_PREFIX "sys_nanosleep")
 int do_probe_read(void *ctx)
@@ -47,6 +49,11 @@ int do_copy_from_user(void *ctx)
 	read_ret[7] = bpf_copy_from_user_task(buf, sizeof(buf), user_ptr,
 					      bpf_get_current_task_btf(), 0);
 	read_ret[8] = bpf_copy_from_user_str((char *)buf, sizeof(buf), user_ptr, 0);
+	read_ret[9] = bpf_copy_from_user_task_str((char *)buf,
+						  sizeof(buf),
+						  user_ptr,
+						  bpf_get_current_task_btf(),
+						  0);

 	return 0;
 }
--
2.43.5

Re: [bpf-next v8 3/3] selftests/bpf: Add tests for bpf_copy_from_user_task_str

[Marcus Wichelmann]

Hi,

I'm not sure what I'm doing wrong, but after rebasing on latest bpf-next
which includes this patch, I'm no longer able to build the bpf selftests:

# pushd tools/testing/selftests/bpf/
# make -j80
[...]
   GEN-SKEL [test_progs] bpf_iter_task_vmas.skel.h
   CLNG-BPF [test_progs] bpf_iter_tasks.bpf.o
progs/bpf_iter_tasks.c:98:8: error: call to undeclared function 'bpf_copy_from_user_task_str'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    98 |         ret = bpf_copy_from_user_task_str((char *)task_str1, sizeof(task_str1), ptr, task, 0);
       |               ^
1 error generated.
make: *** [Makefile:733: /root/linux/tools/testing/selftests/bpf/bpf_iter_tasks.bpf.o] Error 1

I suppose the function definition should be in the vmlinux.h?

# grep bpf_copy tools/include/vmlinux.h
typedef u64 (*btf_bpf_copy_from_user)(void *, u32, const void *);
typedef u64 (*btf_bpf_copy_from_user_task)(void *, u32, const void *, struct task_struct *, u64);

The kernel built from this revision is booted while trying to compile
the selftests. I can also see that the kfunc is there when dumping without "format c":

# bpftool btf dump file /sys/kernel/btf/vmlinux | grep bpf_copy_from_user_task_str
[116060] FUNC 'bpf_copy_from_user_task_str' type_id=116059 linkage=static

But when dumping the vmlinux headers with "format c", I cannot see the kfunc.
I'm not sure if this is normal:

# bpftool btf dump file /sys/kernel/btf/vmlinux format c | grep bpf_copy_from_user_task_str
#

CONFIG_BPF SYSCALL is enabled. Are there other config options that
have to be enabled that I may be missing?

I'm trying this on a aarch64 system. I also tried to fully clean
my working tree using `git clean -d -x -f`, which didn't help
either.

Thanks!
Marcus


Am 13.02.25 um 16:21 schrieb Jordan Rome:
> This adds tests for both the happy path and the
> error path (with and without the BPF_F_PAD_ZEROS flag).
> 
> Signed-off-by: Jordan Rome <linux@jordanrome.com>
> ---
>   .../selftests/bpf/prog_tests/bpf_iter.c       |  68 +++++++++++
>   .../selftests/bpf/prog_tests/read_vsyscall.c  |   1 +
>   .../selftests/bpf/progs/bpf_iter_tasks.c      | 110 ++++++++++++++++++
>   .../selftests/bpf/progs/read_vsyscall.c       |  11 +-
>   4 files changed, 188 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> index 6f1bfacd7375..add4a18c33bd 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> @@ -323,19 +323,87 @@ static void test_task_pidfd(void)
>   static void test_task_sleepable(void)
>   {
>   	struct bpf_iter_tasks *skel;
> +	int pid, status, err, data_pipe[2], finish_pipe[2], c;
> +	char *test_data = NULL;
> +	char *test_data_long = NULL;
> +	char *data[2];
> +
> +	if (!ASSERT_OK(pipe(data_pipe), "data_pipe") ||
> +	    !ASSERT_OK(pipe(finish_pipe), "finish_pipe"))
> +		return;
> 
>   	skel = bpf_iter_tasks__open_and_load();
>   	if (!ASSERT_OK_PTR(skel, "bpf_iter_tasks__open_and_load"))
>   		return;
> 
> +	pid = fork();
> +	if (!ASSERT_GE(pid, 0, "fork"))
> +		return;
> +
> +	if (pid == 0) {
> +		/* child */
> +		close(data_pipe[0]);
> +		close(finish_pipe[1]);
> +
> +		test_data = malloc(sizeof(char) * 10);
> +		strncpy(test_data, "test_data", 10);
> +		test_data[9] = '\0';
> +
> +		test_data_long = malloc(sizeof(char) * 5000);
> +		for (int i = 0; i < 5000; ++i) {
> +			if (i % 2 == 0)
> +				test_data_long[i] = 'b';
> +			else
> +				test_data_long[i] = 'a';
> +		}
> +		test_data_long[4999] = '\0';
> +
> +		data[0] = test_data;
> +		data[1] = test_data_long;
> +
> +		write(data_pipe[1], &data, sizeof(data));
> +
> +		/* keep child alive until after the test */
> +		err = read(finish_pipe[0], &c, 1);
> +		if (err != 1)
> +			exit(-1);
> +
> +		close(data_pipe[1]);
> +		close(finish_pipe[0]);
> +		_exit(0);
> +	}
> +
> +	/* parent */
> +	close(data_pipe[1]);
> +	close(finish_pipe[0]);
> +
> +	err = read(data_pipe[0], &data, sizeof(data));
> +	ASSERT_EQ(err, sizeof(data), "read_check");
> +
> +	skel->bss->user_ptr = data[0];
> +	skel->bss->user_ptr_long = data[1];
> +	skel->bss->pid = pid;
> +
>   	do_dummy_read(skel->progs.dump_task_sleepable);
> 
>   	ASSERT_GT(skel->bss->num_expected_failure_copy_from_user_task, 0,
>   		  "num_expected_failure_copy_from_user_task");
>   	ASSERT_GT(skel->bss->num_success_copy_from_user_task, 0,
>   		  "num_success_copy_from_user_task");
> +	ASSERT_GT(skel->bss->num_expected_failure_copy_from_user_task_str, 0,
> +		  "num_expected_failure_copy_from_user_task_str");
> +	ASSERT_GT(skel->bss->num_success_copy_from_user_task_str, 0,
> +		  "num_success_copy_from_user_task_str");
> 
>   	bpf_iter_tasks__destroy(skel);
> +
> +	write(finish_pipe[1], &c, 1);
> +	err = waitpid(pid, &status, 0);
> +	ASSERT_EQ(err, pid, "waitpid");
> +	ASSERT_EQ(status, 0, "zero_child_exit");
> +
> +	close(data_pipe[0]);
> +	close(finish_pipe[1]);
>   }
> 
>   static void test_task_stack(void)
> diff --git a/tools/testing/selftests/bpf/prog_tests/read_vsyscall.c b/tools/testing/selftests/bpf/prog_tests/read_vsyscall.c
> index c7b9ba8b1d06..a8d1eaa67020 100644
> --- a/tools/testing/selftests/bpf/prog_tests/read_vsyscall.c
> +++ b/tools/testing/selftests/bpf/prog_tests/read_vsyscall.c
> @@ -24,6 +24,7 @@ struct read_ret_desc {
>   	{ .name = "copy_from_user", .ret = -EFAULT },
>   	{ .name = "copy_from_user_task", .ret = -EFAULT },
>   	{ .name = "copy_from_user_str", .ret = -EFAULT },
> +	{ .name = "copy_from_user_task_str", .ret = -EFAULT },
>   };
> 
>   void test_read_vsyscall(void)
> diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_tasks.c b/tools/testing/selftests/bpf/progs/bpf_iter_tasks.c
> index bc10c4e4b4fa..966ee5a7b066 100644
> --- a/tools/testing/selftests/bpf/progs/bpf_iter_tasks.c
> +++ b/tools/testing/selftests/bpf/progs/bpf_iter_tasks.c
> @@ -9,6 +9,13 @@ char _license[] SEC("license") = "GPL";
>   uint32_t tid = 0;
>   int num_unknown_tid = 0;
>   int num_known_tid = 0;
> +void *user_ptr = 0;
> +void *user_ptr_long = 0;
> +uint32_t pid = 0;
> +
> +static char big_str1[5000];
> +static char big_str2[5005];
> +static char big_str3[4996];
> 
>   SEC("iter/task")
>   int dump_task(struct bpf_iter__task *ctx)
> @@ -35,7 +42,9 @@ int dump_task(struct bpf_iter__task *ctx)
>   }
> 
>   int num_expected_failure_copy_from_user_task = 0;
> +int num_expected_failure_copy_from_user_task_str = 0;
>   int num_success_copy_from_user_task = 0;
> +int num_success_copy_from_user_task_str = 0;
> 
>   SEC("iter.s/task")
>   int dump_task_sleepable(struct bpf_iter__task *ctx)
> @@ -44,6 +53,9 @@ int dump_task_sleepable(struct bpf_iter__task *ctx)
>   	struct task_struct *task = ctx->task;
>   	static const char info[] = "    === END ===";
>   	struct pt_regs *regs;
> +	char task_str1[10] = "aaaaaaaaaa";
> +	char task_str2[10], task_str3[10];
> +	char task_str4[20] = "aaaaaaaaaaaaaaaaaaaa";
>   	void *ptr;
>   	uint32_t user_data = 0;
>   	int ret;
> @@ -78,8 +90,106 @@ int dump_task_sleepable(struct bpf_iter__task *ctx)
>   		BPF_SEQ_PRINTF(seq, "%s\n", info);
>   		return 0;
>   	}
> +
>   	++num_success_copy_from_user_task;
> 
> +	/* Read an invalid pointer and ensure we get an error */
> +	ptr = NULL;
> +	ret = bpf_copy_from_user_task_str((char *)task_str1, sizeof(task_str1), ptr, task, 0);
> +	if (ret >= 0 || task_str1[9] != 'a' || task_str1[0] != '\0') {
> +		BPF_SEQ_PRINTF(seq, "%s\n", info);
> +		return 0;
> +	}
> +
> +	/* Read an invalid pointer and ensure we get error with pad zeros flag */
> +	ptr = NULL;
> +	ret = bpf_copy_from_user_task_str((char *)task_str1, sizeof(task_str1),
> +					  ptr, task, BPF_F_PAD_ZEROS);
> +	if (ret >= 0 || task_str1[9] != '\0' || task_str1[0] != '\0') {
> +		BPF_SEQ_PRINTF(seq, "%s\n", info);
> +		return 0;
> +	}
> +
> +	++num_expected_failure_copy_from_user_task_str;
> +
> +	/* Same length as the string */
> +	ret = bpf_copy_from_user_task_str((char *)task_str2, 10, user_ptr, task, 0);
> +	/* only need to do the task pid check once */
> +	if (bpf_strncmp(task_str2, 10, "test_data\0") != 0 || ret != 10 || task->tgid != pid) {
> +		BPF_SEQ_PRINTF(seq, "%s\n", info);
> +		return 0;
> +	}
> +
> +	/* Shorter length than the string */
> +	ret = bpf_copy_from_user_task_str((char *)task_str3, 2, user_ptr, task, 0);
> +	if (bpf_strncmp(task_str3, 2, "t\0") != 0 || ret != 2) {
> +		BPF_SEQ_PRINTF(seq, "%s\n", info);
> +		return 0;
> +	}
> +
> +	/* Longer length than the string */
> +	ret = bpf_copy_from_user_task_str((char *)task_str4, 20, user_ptr, task, 0);
> +	if (bpf_strncmp(task_str4, 10, "test_data\0") != 0 || ret != 10
> +	    || task_str4[sizeof(task_str4) - 1] != 'a') {
> +		BPF_SEQ_PRINTF(seq, "%s\n", info);
> +		return 0;
> +	}
> +
> +	/* Longer length than the string with pad zeros flag */
> +	ret = bpf_copy_from_user_task_str((char *)task_str4, 20, user_ptr, task, BPF_F_PAD_ZEROS);
> +	if (bpf_strncmp(task_str4, 10, "test_data\0") != 0 || ret != 10
> +	    || task_str4[sizeof(task_str4) - 1] != '\0') {
> +		BPF_SEQ_PRINTF(seq, "%s\n", info);
> +		return 0;
> +	}
> +
> +	/* Longer length than the string past a page boundary */
> +	ret = bpf_copy_from_user_task_str(big_str1, 5000, user_ptr, task, 0);
> +	if (bpf_strncmp(big_str1, 10, "test_data\0") != 0 || ret != 10) {
> +		BPF_SEQ_PRINTF(seq, "%s\n", info);
> +		return 0;
> +	}
> +
> +	/* String that crosses a page boundary */
> +	ret = bpf_copy_from_user_task_str(big_str1, 5000, user_ptr_long, task, BPF_F_PAD_ZEROS);
> +	if (bpf_strncmp(big_str1, 4, "baba") != 0 || ret != 5000
> +	    || bpf_strncmp(big_str1 + 4996, 4, "bab\0") != 0) {
> +		BPF_SEQ_PRINTF(seq, "%s\n", info);
> +		return 0;
> +	}
> +
> +	for (int i = 0; i < 4999; ++i) {
> +		if (i % 2 == 0) {
> +			if (big_str1[i] != 'b') {
> +				BPF_SEQ_PRINTF(seq, "%s\n", info);
> +				return 0;
> +			}
> +		} else {
> +			if (big_str1[i] != 'a') {
> +				BPF_SEQ_PRINTF(seq, "%s\n", info);
> +				return 0;
> +			}
> +		}
> +	}
> +
> +	/* Longer length than the string that crosses a page boundary */
> +	ret = bpf_copy_from_user_task_str(big_str2, 5005, user_ptr_long, task, BPF_F_PAD_ZEROS);
> +	if (bpf_strncmp(big_str2, 4, "baba") != 0 || ret != 5000
> +	    || bpf_strncmp(big_str2 + 4996, 5, "bab\0\0") != 0) {
> +		BPF_SEQ_PRINTF(seq, "%s\n", info);
> +		return 0;
> +	}
> +
> +	/* Shorter length than the string that crosses a page boundary */
> +	ret = bpf_copy_from_user_task_str(big_str3, 4996, user_ptr_long, task, 0);
> +	if (bpf_strncmp(big_str3, 4, "baba") != 0 || ret != 4996
> +	    || bpf_strncmp(big_str3 + 4992, 4, "bab\0") != 0) {
> +		BPF_SEQ_PRINTF(seq, "%s\n", info);
> +		return 0;
> +	}
> +
> +	++num_success_copy_from_user_task_str;
> +
>   	if (ctx->meta->seq_num == 0)
>   		BPF_SEQ_PRINTF(seq, "    tgid      gid     data\n");
> 
> diff --git a/tools/testing/selftests/bpf/progs/read_vsyscall.c b/tools/testing/selftests/bpf/progs/read_vsyscall.c
> index 39ebef430059..395591374d4f 100644
> --- a/tools/testing/selftests/bpf/progs/read_vsyscall.c
> +++ b/tools/testing/selftests/bpf/progs/read_vsyscall.c
> @@ -8,14 +8,16 @@
> 
>   int target_pid = 0;
>   void *user_ptr = 0;
> -int read_ret[9];
> +int read_ret[10];
> 
>   char _license[] SEC("license") = "GPL";
> 
>   /*
> - * This is the only kfunc, the others are helpers
> + * These are the kfuncs, the others are helpers
>    */
>   int bpf_copy_from_user_str(void *dst, u32, const void *, u64) __weak __ksym;
> +int bpf_copy_from_user_task_str(void *dst, u32, const void *,
> +				struct task_struct *, u64) __weak __ksym;
> 
>   SEC("fentry/" SYS_PREFIX "sys_nanosleep")
>   int do_probe_read(void *ctx)
> @@ -47,6 +49,11 @@ int do_copy_from_user(void *ctx)
>   	read_ret[7] = bpf_copy_from_user_task(buf, sizeof(buf), user_ptr,
>   					      bpf_get_current_task_btf(), 0);
>   	read_ret[8] = bpf_copy_from_user_str((char *)buf, sizeof(buf), user_ptr, 0);
> +	read_ret[9] = bpf_copy_from_user_task_str((char *)buf,
> +						  sizeof(buf),
> +						  user_ptr,
> +						  bpf_get_current_task_btf(),
> +						  0);
> 
>   	return 0;
>   }
> --
> 2.43.5
> 
> 

-- 
Best regards,
Marcus Wichelmann
Linux Networking Specialist
Team SDN

______________________________

Hetzner Cloud GmbH
Feringastraße 12A
85774 Unterföhring
Germany

Phone: +49 89 381690 150
E-Mail: marcus.wichelmann@hetzner-cloud.de

Handelsregister München HRB 226782
Geschäftsführer: Sebastian Färber, Markus Kalmuk

------------------
For information on the processing of your personal
data in the context of this contact, please see
https://hetzner-cloud.de/datenschutz
------------------

Re: [bpf-next v8 3/3] selftests/bpf: Add tests for bpf_copy_from_user_task_str

[Andrii Nakryiko]

On Fri, Feb 21, 2025 at 7:01 AM Marcus Wichelmann
<marcus.wichelmann@hetzner-cloud.de> wrote:
>
> Hi,
>
> I'm not sure what I'm doing wrong, but after rebasing on latest bpf-next
> which includes this patch, I'm no longer able to build the bpf selftests:
>
> # pushd tools/testing/selftests/bpf/
> # make -j80
> [...]
>    GEN-SKEL [test_progs] bpf_iter_task_vmas.skel.h
>    CLNG-BPF [test_progs] bpf_iter_tasks.bpf.o
> progs/bpf_iter_tasks.c:98:8: error: call to undeclared function 'bpf_copy_from_user_task_str'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>     98 |         ret = bpf_copy_from_user_task_str((char *)task_str1, sizeof(task_str1), ptr, task, 0);
>        |               ^
> 1 error generated.
> make: *** [Makefile:733: /root/linux/tools/testing/selftests/bpf/bpf_iter_tasks.bpf.o] Error 1
>
> I suppose the function definition should be in the vmlinux.h?
>

Yes, it should be in vmlinux.h, and if you don't have it, then you
must have a bit too old pahole.

$ git tag --contains ce4d0bc0200e3
v1.27
v1.28

> # grep bpf_copy tools/include/vmlinux.h
> typedef u64 (*btf_bpf_copy_from_user)(void *, u32, const void *);
> typedef u64 (*btf_bpf_copy_from_user_task)(void *, u32, const void *, struct task_struct *, u64);
>
> The kernel built from this revision is booted while trying to compile
> the selftests. I can also see that the kfunc is there when dumping without "format c":
>
> # bpftool btf dump file /sys/kernel/btf/vmlinux | grep bpf_copy_from_user_task_str
> [116060] FUNC 'bpf_copy_from_user_task_str' type_id=116059 linkage=static
>
> But when dumping the vmlinux headers with "format c", I cannot see the kfunc.
> I'm not sure if this is normal:
>
> # bpftool btf dump file /sys/kernel/btf/vmlinux format c | grep bpf_copy_from_user_task_str
> #
>
> CONFIG_BPF SYSCALL is enabled. Are there other config options that
> have to be enabled that I may be missing?
>
> I'm trying this on a aarch64 system. I also tried to fully clean
> my working tree using `git clean -d -x -f`, which didn't help
> either.
>
> Thanks!
> Marcus
>
>
> Am 13.02.25 um 16:21 schrieb Jordan Rome:
> > This adds tests for both the happy path and the
> > error path (with and without the BPF_F_PAD_ZEROS flag).
> >
> > Signed-off-by: Jordan Rome <linux@jordanrome.com>
> > ---
> >   .../selftests/bpf/prog_tests/bpf_iter.c       |  68 +++++++++++
> >   .../selftests/bpf/prog_tests/read_vsyscall.c  |   1 +
> >   .../selftests/bpf/progs/bpf_iter_tasks.c      | 110 ++++++++++++++++++
> >   .../selftests/bpf/progs/read_vsyscall.c       |  11 +-
> >   4 files changed, 188 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> > index 6f1bfacd7375..add4a18c33bd 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> > @@ -323,19 +323,87 @@ static void test_task_pidfd(void)
> >   static void test_task_sleepable(void)
> >   {
> >       struct bpf_iter_tasks *skel;
> > +     int pid, status, err, data_pipe[2], finish_pipe[2], c;
> > +     char *test_data = NULL;
> > +     char *test_data_long = NULL;
> > +     char *data[2];
> > +
> > +     if (!ASSERT_OK(pipe(data_pipe), "data_pipe") ||
> > +         !ASSERT_OK(pipe(finish_pipe), "finish_pipe"))
> > +             return;
> >
> >       skel = bpf_iter_tasks__open_and_load();
> >       if (!ASSERT_OK_PTR(skel, "bpf_iter_tasks__open_and_load"))
> >               return;
> >
> > +     pid = fork();
> > +     if (!ASSERT_GE(pid, 0, "fork"))
> > +             return;
> > +
> > +     if (pid == 0) {
> > +             /* child */
> > +             close(data_pipe[0]);
> > +             close(finish_pipe[1]);
> > +
> > +             test_data = malloc(sizeof(char) * 10);
> > +             strncpy(test_data, "test_data", 10);
> > +             test_data[9] = '\0';
> > +
> > +             test_data_long = malloc(sizeof(char) * 5000);
> > +             for (int i = 0; i < 5000; ++i) {
> > +                     if (i % 2 == 0)
> > +                             test_data_long[i] = 'b';
> > +                     else
> > +                             test_data_long[i] = 'a';
> > +             }
> > +             test_data_long[4999] = '\0';
> > +
> > +             data[0] = test_data;
> > +             data[1] = test_data_long;
> > +
> > +             write(data_pipe[1], &data, sizeof(data));
> > +
> > +             /* keep child alive until after the test */
> > +             err = read(finish_pipe[0], &c, 1);
> > +             if (err != 1)
> > +                     exit(-1);
> > +
> > +             close(data_pipe[1]);
> > +             close(finish_pipe[0]);
> > +             _exit(0);
> > +     }
> > +
> > +     /* parent */
> > +     close(data_pipe[1]);
> > +     close(finish_pipe[0]);
> > +
> > +     err = read(data_pipe[0], &data, sizeof(data));
> > +     ASSERT_EQ(err, sizeof(data), "read_check");
> > +
> > +     skel->bss->user_ptr = data[0];
> > +     skel->bss->user_ptr_long = data[1];
> > +     skel->bss->pid = pid;
> > +
> >       do_dummy_read(skel->progs.dump_task_sleepable);
> >
> >       ASSERT_GT(skel->bss->num_expected_failure_copy_from_user_task, 0,
> >                 "num_expected_failure_copy_from_user_task");
> >       ASSERT_GT(skel->bss->num_success_copy_from_user_task, 0,
> >                 "num_success_copy_from_user_task");
> > +     ASSERT_GT(skel->bss->num_expected_failure_copy_from_user_task_str, 0,
> > +               "num_expected_failure_copy_from_user_task_str");
> > +     ASSERT_GT(skel->bss->num_success_copy_from_user_task_str, 0,
> > +               "num_success_copy_from_user_task_str");
> >
> >       bpf_iter_tasks__destroy(skel);
> > +
> > +     write(finish_pipe[1], &c, 1);
> > +     err = waitpid(pid, &status, 0);
> > +     ASSERT_EQ(err, pid, "waitpid");
> > +     ASSERT_EQ(status, 0, "zero_child_exit");
> > +
> > +     close(data_pipe[0]);
> > +     close(finish_pipe[1]);
> >   }
> >
> >   static void test_task_stack(void)
> > diff --git a/tools/testing/selftests/bpf/prog_tests/read_vsyscall.c b/tools/testing/selftests/bpf/prog_tests/read_vsyscall.c
> > index c7b9ba8b1d06..a8d1eaa67020 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/read_vsyscall.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/read_vsyscall.c
> > @@ -24,6 +24,7 @@ struct read_ret_desc {
> >       { .name = "copy_from_user", .ret = -EFAULT },
> >       { .name = "copy_from_user_task", .ret = -EFAULT },
> >       { .name = "copy_from_user_str", .ret = -EFAULT },
> > +     { .name = "copy_from_user_task_str", .ret = -EFAULT },
> >   };
> >
> >   void test_read_vsyscall(void)
> > diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_tasks.c b/tools/testing/selftests/bpf/progs/bpf_iter_tasks.c
> > index bc10c4e4b4fa..966ee5a7b066 100644
> > --- a/tools/testing/selftests/bpf/progs/bpf_iter_tasks.c
> > +++ b/tools/testing/selftests/bpf/progs/bpf_iter_tasks.c
> > @@ -9,6 +9,13 @@ char _license[] SEC("license") = "GPL";
> >   uint32_t tid = 0;
> >   int num_unknown_tid = 0;
> >   int num_known_tid = 0;
> > +void *user_ptr = 0;
> > +void *user_ptr_long = 0;
> > +uint32_t pid = 0;
> > +
> > +static char big_str1[5000];
> > +static char big_str2[5005];
> > +static char big_str3[4996];
> >
> >   SEC("iter/task")
> >   int dump_task(struct bpf_iter__task *ctx)
> > @@ -35,7 +42,9 @@ int dump_task(struct bpf_iter__task *ctx)
> >   }
> >
> >   int num_expected_failure_copy_from_user_task = 0;
> > +int num_expected_failure_copy_from_user_task_str = 0;
> >   int num_success_copy_from_user_task = 0;
> > +int num_success_copy_from_user_task_str = 0;
> >
> >   SEC("iter.s/task")
> >   int dump_task_sleepable(struct bpf_iter__task *ctx)
> > @@ -44,6 +53,9 @@ int dump_task_sleepable(struct bpf_iter__task *ctx)
> >       struct task_struct *task = ctx->task;
> >       static const char info[] = "    === END ===";
> >       struct pt_regs *regs;
> > +     char task_str1[10] = "aaaaaaaaaa";
> > +     char task_str2[10], task_str3[10];
> > +     char task_str4[20] = "aaaaaaaaaaaaaaaaaaaa";
> >       void *ptr;
> >       uint32_t user_data = 0;
> >       int ret;
> > @@ -78,8 +90,106 @@ int dump_task_sleepable(struct bpf_iter__task *ctx)
> >               BPF_SEQ_PRINTF(seq, "%s\n", info);
> >               return 0;
> >       }
> > +
> >       ++num_success_copy_from_user_task;
> >
> > +     /* Read an invalid pointer and ensure we get an error */
> > +     ptr = NULL;
> > +     ret = bpf_copy_from_user_task_str((char *)task_str1, sizeof(task_str1), ptr, task, 0);
> > +     if (ret >= 0 || task_str1[9] != 'a' || task_str1[0] != '\0') {
> > +             BPF_SEQ_PRINTF(seq, "%s\n", info);
> > +             return 0;
> > +     }
> > +
> > +     /* Read an invalid pointer and ensure we get error with pad zeros flag */
> > +     ptr = NULL;
> > +     ret = bpf_copy_from_user_task_str((char *)task_str1, sizeof(task_str1),
> > +                                       ptr, task, BPF_F_PAD_ZEROS);
> > +     if (ret >= 0 || task_str1[9] != '\0' || task_str1[0] != '\0') {
> > +             BPF_SEQ_PRINTF(seq, "%s\n", info);
> > +             return 0;
> > +     }
> > +
> > +     ++num_expected_failure_copy_from_user_task_str;
> > +
> > +     /* Same length as the string */
> > +     ret = bpf_copy_from_user_task_str((char *)task_str2, 10, user_ptr, task, 0);
> > +     /* only need to do the task pid check once */
> > +     if (bpf_strncmp(task_str2, 10, "test_data\0") != 0 || ret != 10 || task->tgid != pid) {
> > +             BPF_SEQ_PRINTF(seq, "%s\n", info);
> > +             return 0;
> > +     }
> > +
> > +     /* Shorter length than the string */
> > +     ret = bpf_copy_from_user_task_str((char *)task_str3, 2, user_ptr, task, 0);
> > +     if (bpf_strncmp(task_str3, 2, "t\0") != 0 || ret != 2) {
> > +             BPF_SEQ_PRINTF(seq, "%s\n", info);
> > +             return 0;
> > +     }
> > +
> > +     /* Longer length than the string */
> > +     ret = bpf_copy_from_user_task_str((char *)task_str4, 20, user_ptr, task, 0);
> > +     if (bpf_strncmp(task_str4, 10, "test_data\0") != 0 || ret != 10
> > +         || task_str4[sizeof(task_str4) - 1] != 'a') {
> > +             BPF_SEQ_PRINTF(seq, "%s\n", info);
> > +             return 0;
> > +     }
> > +
> > +     /* Longer length than the string with pad zeros flag */
> > +     ret = bpf_copy_from_user_task_str((char *)task_str4, 20, user_ptr, task, BPF_F_PAD_ZEROS);
> > +     if (bpf_strncmp(task_str4, 10, "test_data\0") != 0 || ret != 10
> > +         || task_str4[sizeof(task_str4) - 1] != '\0') {
> > +             BPF_SEQ_PRINTF(seq, "%s\n", info);
> > +             return 0;
> > +     }
> > +
> > +     /* Longer length than the string past a page boundary */
> > +     ret = bpf_copy_from_user_task_str(big_str1, 5000, user_ptr, task, 0);
> > +     if (bpf_strncmp(big_str1, 10, "test_data\0") != 0 || ret != 10) {
> > +             BPF_SEQ_PRINTF(seq, "%s\n", info);
> > +             return 0;
> > +     }
> > +
> > +     /* String that crosses a page boundary */
> > +     ret = bpf_copy_from_user_task_str(big_str1, 5000, user_ptr_long, task, BPF_F_PAD_ZEROS);
> > +     if (bpf_strncmp(big_str1, 4, "baba") != 0 || ret != 5000
> > +         || bpf_strncmp(big_str1 + 4996, 4, "bab\0") != 0) {
> > +             BPF_SEQ_PRINTF(seq, "%s\n", info);
> > +             return 0;
> > +     }
> > +
> > +     for (int i = 0; i < 4999; ++i) {
> > +             if (i % 2 == 0) {
> > +                     if (big_str1[i] != 'b') {
> > +                             BPF_SEQ_PRINTF(seq, "%s\n", info);
> > +                             return 0;
> > +                     }
> > +             } else {
> > +                     if (big_str1[i] != 'a') {
> > +                             BPF_SEQ_PRINTF(seq, "%s\n", info);
> > +                             return 0;
> > +                     }
> > +             }
> > +     }
> > +
> > +     /* Longer length than the string that crosses a page boundary */
> > +     ret = bpf_copy_from_user_task_str(big_str2, 5005, user_ptr_long, task, BPF_F_PAD_ZEROS);
> > +     if (bpf_strncmp(big_str2, 4, "baba") != 0 || ret != 5000
> > +         || bpf_strncmp(big_str2 + 4996, 5, "bab\0\0") != 0) {
> > +             BPF_SEQ_PRINTF(seq, "%s\n", info);
> > +             return 0;
> > +     }
> > +
> > +     /* Shorter length than the string that crosses a page boundary */
> > +     ret = bpf_copy_from_user_task_str(big_str3, 4996, user_ptr_long, task, 0);
> > +     if (bpf_strncmp(big_str3, 4, "baba") != 0 || ret != 4996
> > +         || bpf_strncmp(big_str3 + 4992, 4, "bab\0") != 0) {
> > +             BPF_SEQ_PRINTF(seq, "%s\n", info);
> > +             return 0;
> > +     }
> > +
> > +     ++num_success_copy_from_user_task_str;
> > +
> >       if (ctx->meta->seq_num == 0)
> >               BPF_SEQ_PRINTF(seq, "    tgid      gid     data\n");
> >
> > diff --git a/tools/testing/selftests/bpf/progs/read_vsyscall.c b/tools/testing/selftests/bpf/progs/read_vsyscall.c
> > index 39ebef430059..395591374d4f 100644
> > --- a/tools/testing/selftests/bpf/progs/read_vsyscall.c
> > +++ b/tools/testing/selftests/bpf/progs/read_vsyscall.c
> > @@ -8,14 +8,16 @@
> >
> >   int target_pid = 0;
> >   void *user_ptr = 0;
> > -int read_ret[9];
> > +int read_ret[10];
> >
> >   char _license[] SEC("license") = "GPL";
> >
> >   /*
> > - * This is the only kfunc, the others are helpers
> > + * These are the kfuncs, the others are helpers
> >    */
> >   int bpf_copy_from_user_str(void *dst, u32, const void *, u64) __weak __ksym;
> > +int bpf_copy_from_user_task_str(void *dst, u32, const void *,
> > +                             struct task_struct *, u64) __weak __ksym;
> >
> >   SEC("fentry/" SYS_PREFIX "sys_nanosleep")
> >   int do_probe_read(void *ctx)
> > @@ -47,6 +49,11 @@ int do_copy_from_user(void *ctx)
> >       read_ret[7] = bpf_copy_from_user_task(buf, sizeof(buf), user_ptr,
> >                                             bpf_get_current_task_btf(), 0);
> >       read_ret[8] = bpf_copy_from_user_str((char *)buf, sizeof(buf), user_ptr, 0);
> > +     read_ret[9] = bpf_copy_from_user_task_str((char *)buf,
> > +                                               sizeof(buf),
> > +                                               user_ptr,
> > +                                               bpf_get_current_task_btf(),
> > +                                               0);
> >
> >       return 0;
> >   }
> > --
> > 2.43.5
> >
> >
>
> --
> Best regards,
> Marcus Wichelmann
> Linux Networking Specialist
> Team SDN
>
> ______________________________
>
> Hetzner Cloud GmbH
> Feringastraße 12A
> 85774 Unterföhring
> Germany
>
> Phone: +49 89 381690 150
> E-Mail: marcus.wichelmann@hetzner-cloud.de
>
> Handelsregister München HRB 226782
> Geschäftsführer: Sebastian Färber, Markus Kalmuk
>
> ------------------
> For information on the processing of your personal
> data in the context of this contact, please see
> https://hetzner-cloud.de/datenschutz
> ------------------
>

Re: [bpf-next v8 3/3] selftests/bpf: Add tests for bpf_copy_from_user_task_str

[Marcus Wichelmann]

Am 21.02.25 um 19:26 schrieb Andrii Nakryiko:
> On Fri, Feb 21, 2025 at 7:01 AM Marcus Wichelmann
> <marcus.wichelmann@hetzner-cloud.de> wrote:
>>
>> Hi,
>>
>> I'm not sure what I'm doing wrong, but after rebasing on latest bpf-next
>> which includes this patch, I'm no longer able to build the bpf selftests:
>>
>> # pushd tools/testing/selftests/bpf/
>> # make -j80
>> [...]
>>     GEN-SKEL [test_progs] bpf_iter_task_vmas.skel.h
>>     CLNG-BPF [test_progs] bpf_iter_tasks.bpf.o
>> progs/bpf_iter_tasks.c:98:8: error: call to undeclared function 'bpf_copy_from_user_task_str'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>>      98 |         ret = bpf_copy_from_user_task_str((char *)task_str1, sizeof(task_str1), ptr, task, 0);
>>         |               ^
>> 1 error generated.
>> make: *** [Makefile:733: /root/linux/tools/testing/selftests/bpf/bpf_iter_tasks.bpf.o] Error 1
>>
>> I suppose the function definition should be in the vmlinux.h?
>>
> 
> Yes, it should be in vmlinux.h, and if you don't have it, then you
> must have a bit too old pahole.
> 
> $ git tag --contains ce4d0bc0200e3
> v1.27
> v1.28

Ah, my pahole version was 1.25. Compiling a newer version from source did the trick.
This was really the last thing I would have thought of.

Thank you very much for your help!

Marcus

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

[Shakeel Butt]

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
> 

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

[Andrew Morton]

On Wed, 19 Feb 2025 15:33:12 -0800 Shakeel Butt <shakeel.butt@linux.dev> wrote:

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

I'm seeing no merge issues at this time.  Via the bpf tree, please.

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

[Andrii Nakryiko]

On Wed, Feb 19, 2025 at 4:19 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed, 19 Feb 2025 15:33:12 -0800 Shakeel Butt <shakeel.butt@linux.dev> wrote:
>
> > Hi Andrew,
> >
> > Do you prefer this patch series to go though mm-tree or routing these
> > through bpf tree is fine with you?
> >
>
> I'm seeing no merge issues at this time.  Via the bpf tree, please.


Great, I'll put it into bpf-next tree, thanks!

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

[Jordan Rome]

On Wed, Feb 19, 2025 at 8:02 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Feb 19, 2025 at 4:19 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Wed, 19 Feb 2025 15:33:12 -0800 Shakeel Butt <shakeel.butt@linux.dev> wrote:
> >
> > > Hi Andrew,
> > >
> > > Do you prefer this patch series to go though mm-tree or routing these
> > > through bpf tree is fine with you?
> > >
> >
> > I'm seeing no merge issues at this time.  Via the bpf tree, please.
>
>
> Great, I'll put it into bpf-next tree, thanks!

Thanks everyone for the review and guidance!

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

[Andrew Morton]

On Thu, 13 Feb 2025 07:21:23 -0800 Jordan Rome <linux@jordanrome.com> 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.
> 
> ...
>
>  include/linux/mm.h |   3 ++
>  mm/memory.c        | 122 +++++++++++++++++++++++++++++++++++++++++++++
>  mm/nommu.c         |  76 ++++++++++++++++++++++++++++

Is there any way in which we can avoid adding all this to vmlinux if
it's unneeded?

Any such ifdeffery would of course need removal or alteration if
callers other than BPF emerge.

> ...
>
> +/*
> + * 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);

Well that's a crappy little comment which you copied-n-pasted.  It
tells us "what" (which is utterly obvious) but not "why".  whodidthat.

> +/**
> + * 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;

I wonder if this can ever happen.  And if it does, should it WARN?  And
returen -Efoo?

> +	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;
> +}

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

[Andrii Nakryiko]

On Wed, Feb 19, 2025 at 4:18 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Thu, 13 Feb 2025 07:21:23 -0800 Jordan Rome <linux@jordanrome.com> 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.
> >
> > ...
> >
> >  include/linux/mm.h |   3 ++
> >  mm/memory.c        | 122 +++++++++++++++++++++++++++++++++++++++++++++
> >  mm/nommu.c         |  76 ++++++++++++++++++++++++++++
>
> Is there any way in which we can avoid adding all this to vmlinux if
> it's unneeded?
>
> Any such ifdeffery would of course need removal or alteration if
> callers other than BPF emerge.
>

yeah, it's a straightforward #ifdef CONFIG_BPF_SYSCALL guard, I'll add
it while applying

> > ...
> >
> > +/*
> > + * 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);
>
> Well that's a crappy little comment which you copied-n-pasted.  It
> tells us "what" (which is utterly obvious) but not "why".  whodidthat.

:) dropped the comment, but yeah, it's coming from
__access_remote_vm(), of course. Seems other users of
untagged_addr_remote() don't bother leaving comment, so dropping the
comment seems appropriate (and anyone can actually read more expanded
comment in include/linux/uaccess.h, if curious)

>
> > +/**
> > + * 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;
>
> I wonder if this can ever happen.  And if it does, should it WARN?  And
> returen -Efoo?

so this was not meant to catch negative len (that's assumed invalid
API usage), it was more about handling len == 0 case, which is legal
for access_remote_vm(). So I fixed it up to `if (unlikely(len == 0))
return 0;`  explicitly to keep behavior consistent with
access_remote_vm().

>
> > +     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;
> > +}
>

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

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Thu, 13 Feb 2025 07:21:23 -0800 you 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.
> 
> [...]

Here is the summary with links:
  - [bpf-next,v8,1/3] mm: add copy_remote_vm_str
    https://git.kernel.org/bpf/bpf-next/c/f0b79944e6f4
  - [bpf-next,v8,2/3] bpf: Add bpf_copy_from_user_task_str kfunc
    https://git.kernel.org/bpf/bpf-next/c/f0f8a5b58f78
  - [bpf-next,v8,3/3] selftests/bpf: Add tests for bpf_copy_from_user_task_str
    https://git.kernel.org/bpf/bpf-next/c/7042882abc04

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html