[PATCH v2 bpf-next 0/2] introduce bpf_find_vma

[PATCH v2 bpf-next 0/2] introduce bpf_find_vma

[Song Liu]

Changes v1 => v2:
1. Share irq_work with stackmap.c. (Daniel)
2. Add tests for illegal writes to task/vma from the callback function.
   (Daniel)
3. Other small fixes.

Add helper bpf_find_vma. This can be used in some profiling use cases. It
might also be useful for LSM.

Song Liu (2):
  bpf: introduce helper bpf_find_vma
  selftests/bpf: add tests for bpf_find_vma

 include/linux/bpf.h                           |   1 +
 include/uapi/linux/bpf.h                      |  20 +++
 kernel/bpf/mmap_unlock_work.h                 |  65 ++++++++++
 kernel/bpf/stackmap.c                         |  71 +++--------
 kernel/bpf/task_iter.c                        |  49 ++++++++
 kernel/bpf/verifier.c                         |  36 ++++++
 kernel/trace/bpf_trace.c                      |   2 +
 tools/include/uapi/linux/bpf.h                |  20 +++
 .../selftests/bpf/prog_tests/find_vma.c       | 115 ++++++++++++++++++
 tools/testing/selftests/bpf/progs/find_vma.c  |  70 +++++++++++
 .../selftests/bpf/progs/find_vma_fail1.c      |  30 +++++
 .../selftests/bpf/progs/find_vma_fail2.c      |  30 +++++
 12 files changed, 454 insertions(+), 55 deletions(-)
 create mode 100644 kernel/bpf/mmap_unlock_work.h
 create mode 100644 tools/testing/selftests/bpf/prog_tests/find_vma.c
 create mode 100644 tools/testing/selftests/bpf/progs/find_vma.c
 create mode 100644 tools/testing/selftests/bpf/progs/find_vma_fail1.c
 create mode 100644 tools/testing/selftests/bpf/progs/find_vma_fail2.c

--
2.30.2

[PATCH v2 bpf-next 1/2] bpf: introduce helper bpf_find_vma

[Song Liu]

In some profiler use cases, it is necessary to map an address to the
backing file, e.g., a shared library. bpf_find_vma helper provides a
flexible way to achieve this. bpf_find_vma maps an address of a task to
the vma (vm_area_struct) for this address, and feed the vma to an callback
BPF function. The callback function is necessary here, as we need to
ensure mmap_sem is unlocked.

It is necessary to lock mmap_sem for find_vma. To lock and unlock mmap_sem
safely when irqs are disable, we use the same mechanism as stackmap with
build_id. Specifically, when irqs are disabled, the unlocked is postponed
in an irq_work. Refactor stackmap.c so that the irq_work is shared among
bpf_find_vma and stackmap helpers.

Signed-off-by: Song Liu <songliubraving@fb.com>
---
 include/linux/bpf.h            |  1 +
 include/uapi/linux/bpf.h       | 20 ++++++++++
 kernel/bpf/mmap_unlock_work.h  | 65 +++++++++++++++++++++++++++++++
 kernel/bpf/stackmap.c          | 71 ++++++++--------------------------
 kernel/bpf/task_iter.c         | 49 +++++++++++++++++++++++
 kernel/bpf/verifier.c          | 36 +++++++++++++++++
 kernel/trace/bpf_trace.c       |  2 +
 tools/include/uapi/linux/bpf.h | 20 ++++++++++
 8 files changed, 209 insertions(+), 55 deletions(-)
 create mode 100644 kernel/bpf/mmap_unlock_work.h

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 2be6dfd68df99..df3410bff4b06 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2157,6 +2157,7 @@ extern const struct bpf_func_proto bpf_btf_find_by_name_kind_proto;
 extern const struct bpf_func_proto bpf_sk_setsockopt_proto;
 extern const struct bpf_func_proto bpf_sk_getsockopt_proto;
 extern const struct bpf_func_proto bpf_kallsyms_lookup_name_proto;
+extern const struct bpf_func_proto bpf_find_vma_proto;
 
 const struct bpf_func_proto *tracing_prog_func_proto(
   enum bpf_func_id func_id, const struct bpf_prog *prog);
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index ba5af15e25f5c..22fa7b74de451 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -4938,6 +4938,25 @@ union bpf_attr {
  *		**-ENOENT** if symbol is not found.
  *
  *		**-EPERM** if caller does not have permission to obtain kernel address.
+ *
+ * long bpf_find_vma(struct task_struct *task, u64 addr, void *callback_fn, void *callback_ctx, u64 flags)
+ *	Description
+ *		Find vma of *task* that contains *addr*, call *callback_fn*
+ *		function with *task*, *vma*, and *callback_ctx*.
+ *		The *callback_fn* should be a static function and
+ *		the *callback_ctx* should be a pointer to the stack.
+ *		The *flags* is used to control certain aspects of the helper.
+ *		Currently, the *flags* must be 0.
+ *
+ *		The expected callback signature is
+ *
+ *		long (\*callback_fn)(struct task_struct \*task, struct vm_area_struct \*vma, void \*ctx);
+ *
+ *	Return
+ *		0 on success.
+ *		**-ENOENT** if *task->mm* is NULL, or no vma contains *addr*.
+ *		**-EBUSY** if failed to try lock mmap_lock.
+ *		**-EINVAL** for invalid **flags**.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -5120,6 +5139,7 @@ union bpf_attr {
 	FN(trace_vprintk),		\
 	FN(skc_to_unix_sock),		\
 	FN(kallsyms_lookup_name),	\
+	FN(find_vma),			\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
diff --git a/kernel/bpf/mmap_unlock_work.h b/kernel/bpf/mmap_unlock_work.h
new file mode 100644
index 0000000000000..5d18d7d85bef9
--- /dev/null
+++ b/kernel/bpf/mmap_unlock_work.h
@@ -0,0 +1,65 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Copyright (c) 2021 Facebook
+ */
+
+#ifndef __MMAP_UNLOCK_WORK_H__
+#define __MMAP_UNLOCK_WORK_H__
+#include <linux/irq_work.h>
+
+/* irq_work to run mmap_read_unlock() in irq_work */
+struct mmap_unlock_irq_work {
+	struct irq_work irq_work;
+	struct mm_struct *mm;
+};
+
+DECLARE_PER_CPU(struct mmap_unlock_irq_work, mmap_unlock_work);
+
+/*
+ * We cannot do mmap_read_unlock() when the irq is disabled, because of
+ * risk to deadlock with rq_lock. To look up vma when the irqs are
+ * disabled, we need to run mmap_read_unlock() in irq_work. We use a
+ * percpu variable to do the irq_work. If the irq_work is already used
+ * by another lookup, we fall over.
+ */
+static inline bool bpf_mmap_unlock_get_irq_work(struct mmap_unlock_irq_work **work_ptr)
+{
+	struct mmap_unlock_irq_work *work = NULL;
+	bool irq_work_busy = false;
+
+	if (irqs_disabled()) {
+		if (!IS_ENABLED(CONFIG_PREEMPT_RT)) {
+			work = this_cpu_ptr(&mmap_unlock_work);
+			if (irq_work_is_busy(&work->irq_work)) {
+				/* cannot queue more up_read, fallback */
+				irq_work_busy = true;
+			}
+		} else {
+			/*
+			 * PREEMPT_RT does not allow to trylock mmap sem in
+			 * interrupt disabled context. Force the fallback code.
+			 */
+			irq_work_busy = true;
+		}
+	}
+
+	*work_ptr = work;
+	return irq_work_busy;
+}
+
+static inline void bpf_mmap_unlock_mm(struct mmap_unlock_irq_work *work, struct mm_struct *mm)
+{
+	if (!work) {
+		mmap_read_unlock(mm);
+	} else {
+		work->mm = mm;
+
+		/* The lock will be released once we're out of interrupt
+		 * context. Tell lockdep that we've released it now so
+		 * it doesn't complain that we forgot to release it.
+		 */
+		rwsem_release(&mm->mmap_lock.dep_map, _RET_IP_);
+		irq_work_queue(&work->irq_work);
+	}
+}
+
+#endif /* __MMAP_UNLOCK_WORK_H__ */
diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index 6e75bbee39f0b..77e366f69212b 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -7,10 +7,10 @@
 #include <linux/kernel.h>
 #include <linux/stacktrace.h>
 #include <linux/perf_event.h>
-#include <linux/irq_work.h>
 #include <linux/btf_ids.h>
 #include <linux/buildid.h>
 #include "percpu_freelist.h"
+#include "mmap_unlock_work.h"
 
 #define STACK_CREATE_FLAG_MASK					\
 	(BPF_F_NUMA_NODE | BPF_F_RDONLY | BPF_F_WRONLY |	\
@@ -31,25 +31,19 @@ struct bpf_stack_map {
 	struct stack_map_bucket *buckets[];
 };
 
-/* irq_work to run up_read() for build_id lookup in nmi context */
-struct stack_map_irq_work {
-	struct irq_work irq_work;
-	struct mm_struct *mm;
-};
+DEFINE_PER_CPU(struct mmap_unlock_irq_work, mmap_unlock_work);
 
-static void do_up_read(struct irq_work *entry)
+static void do_mmap_read_unlock(struct irq_work *entry)
 {
-	struct stack_map_irq_work *work;
+	struct mmap_unlock_irq_work *work;
 
 	if (WARN_ON_ONCE(IS_ENABLED(CONFIG_PREEMPT_RT)))
 		return;
 
-	work = container_of(entry, struct stack_map_irq_work, irq_work);
+	work = container_of(entry, struct mmap_unlock_irq_work, irq_work);
 	mmap_read_unlock_non_owner(work->mm);
 }
 
-static DEFINE_PER_CPU(struct stack_map_irq_work, up_read_work);
-
 static inline bool stack_map_use_build_id(struct bpf_map *map)
 {
 	return (map->map_flags & BPF_F_STACK_BUILD_ID);
@@ -145,39 +139,18 @@ static struct bpf_map *stack_map_alloc(union bpf_attr *attr)
 	return ERR_PTR(err);
 }
 
+
 static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
 					  u64 *ips, u32 trace_nr, bool user)
 {
-	int i;
+	struct mmap_unlock_irq_work *work = NULL;
+	bool irq_work_busy = bpf_mmap_unlock_get_irq_work(&work);
 	struct vm_area_struct *vma;
-	bool irq_work_busy = false;
-	struct stack_map_irq_work *work = NULL;
-
-	if (irqs_disabled()) {
-		if (!IS_ENABLED(CONFIG_PREEMPT_RT)) {
-			work = this_cpu_ptr(&up_read_work);
-			if (irq_work_is_busy(&work->irq_work)) {
-				/* cannot queue more up_read, fallback */
-				irq_work_busy = true;
-			}
-		} else {
-			/*
-			 * PREEMPT_RT does not allow to trylock mmap sem in
-			 * interrupt disabled context. Force the fallback code.
-			 */
-			irq_work_busy = true;
-		}
-	}
+	int i;
 
-	/*
-	 * We cannot do up_read() when the irq is disabled, because of
-	 * risk to deadlock with rq_lock. To do build_id lookup when the
-	 * irqs are disabled, we need to run up_read() in irq_work. We use
-	 * a percpu variable to do the irq_work. If the irq_work is
-	 * already used by another lookup, we fall back to report ips.
-	 *
-	 * Same fallback is used for kernel stack (!user) on a stackmap
-	 * with build_id.
+	/* If the irq_work is in use, fall back to report ips. Same
+	 * fallback is used for kernel stack (!user) on a stackmap with
+	 * build_id.
 	 */
 	if (!user || !current || !current->mm || irq_work_busy ||
 	    !mmap_read_trylock(current->mm)) {
@@ -203,19 +176,7 @@ static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
 			- vma->vm_start;
 		id_offs[i].status = BPF_STACK_BUILD_ID_VALID;
 	}
-
-	if (!work) {
-		mmap_read_unlock(current->mm);
-	} else {
-		work->mm = current->mm;
-
-		/* The lock will be released once we're out of interrupt
-		 * context. Tell lockdep that we've released it now so
-		 * it doesn't complain that we forgot to release it.
-		 */
-		rwsem_release(&current->mm->mmap_lock.dep_map, _RET_IP_);
-		irq_work_queue(&work->irq_work);
-	}
+	bpf_mmap_unlock_mm(work, current->mm);
 }
 
 static struct perf_callchain_entry *
@@ -723,11 +684,11 @@ const struct bpf_map_ops stack_trace_map_ops = {
 static int __init stack_map_init(void)
 {
 	int cpu;
-	struct stack_map_irq_work *work;
+	struct mmap_unlock_irq_work *work;
 
 	for_each_possible_cpu(cpu) {
-		work = per_cpu_ptr(&up_read_work, cpu);
-		init_irq_work(&work->irq_work, do_up_read);
+		work = per_cpu_ptr(&mmap_unlock_work, cpu);
+		init_irq_work(&work->irq_work, do_mmap_read_unlock);
 	}
 	return 0;
 }
diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index b48750bfba5aa..9d92e14a6eea4 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c
@@ -8,6 +8,7 @@
 #include <linux/fdtable.h>
 #include <linux/filter.h>
 #include <linux/btf_ids.h>
+#include "mmap_unlock_work.h"
 
 struct bpf_iter_seq_task_common {
 	struct pid_namespace *ns;
@@ -586,6 +587,54 @@ static struct bpf_iter_reg task_vma_reg_info = {
 	.seq_info		= &task_vma_seq_info,
 };
 
+BPF_CALL_5(bpf_find_vma, struct task_struct *, task, u64, start,
+	   bpf_callback_t, callback_fn, void *, callback_ctx, u64, flags)
+{
+	struct mmap_unlock_irq_work *work = NULL;
+	struct vm_area_struct *vma;
+	bool irq_work_busy = false;
+	struct mm_struct *mm;
+	int ret = -ENOENT;
+
+	if (flags)
+		return -EINVAL;
+
+	if (!task)
+		return -ENOENT;
+
+	mm = task->mm;
+	if (!mm)
+		return -ENOENT;
+
+	irq_work_busy = bpf_mmap_unlock_get_irq_work(&work);
+
+	if (irq_work_busy || !mmap_read_trylock(mm))
+		return -EBUSY;
+
+	vma = find_vma(mm, start);
+
+	if (vma && vma->vm_start <= start && vma->vm_end > start) {
+		callback_fn((u64)(long)task, (u64)(long)vma,
+			    (u64)(long)callback_ctx, 0, 0);
+		ret = 0;
+	}
+	bpf_mmap_unlock_mm(work, mm);
+	return ret;
+}
+
+BTF_ID_LIST_SINGLE(btf_find_vma_ids, struct, task_struct)
+
+const struct bpf_func_proto bpf_find_vma_proto = {
+	.func		= bpf_find_vma,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_BTF_ID,
+	.arg1_btf_id	= &btf_find_vma_ids[0],
+	.arg2_type	= ARG_ANYTHING,
+	.arg3_type	= ARG_PTR_TO_FUNC,
+	.arg4_type	= ARG_PTR_TO_STACK_OR_NULL,
+	.arg5_type	= ARG_ANYTHING,
+};
+
 static int __init task_iter_init(void)
 {
 	int ret;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index f0dca726ebfde..a65526112924a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6132,6 +6132,35 @@ static int set_timer_callback_state(struct bpf_verifier_env *env,
 	return 0;
 }
 
+BTF_ID_LIST_SINGLE(btf_set_find_vma_ids, struct, vm_area_struct)
+
+static int set_find_vma_callback_state(struct bpf_verifier_env *env,
+				       struct bpf_func_state *caller,
+				       struct bpf_func_state *callee,
+				       int insn_idx)
+{
+	/* bpf_find_vma(struct task_struct *task, u64 start,
+	 *               void *callback_fn, void *callback_ctx, u64 flags)
+	 * (callback_fn)(struct task_struct *task,
+	 *               struct vm_area_struct *vma, void *ctx);
+	 */
+	callee->regs[BPF_REG_1] = caller->regs[BPF_REG_1];
+
+	callee->regs[BPF_REG_2].type = PTR_TO_BTF_ID;
+	__mark_reg_known_zero(&callee->regs[BPF_REG_2]);
+	callee->regs[BPF_REG_2].btf =  btf_vmlinux;
+	callee->regs[BPF_REG_2].btf_id = btf_set_find_vma_ids[0];
+
+	/* pointer to stack or null */
+	callee->regs[BPF_REG_3] = caller->regs[BPF_REG_4];
+
+	/* unused */
+	__mark_reg_not_init(env, &callee->regs[BPF_REG_4]);
+	__mark_reg_not_init(env, &callee->regs[BPF_REG_5]);
+	callee->in_callback_fn = true;
+	return 0;
+}
+
 static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
 {
 	struct bpf_verifier_state *state = env->cur_state;
@@ -6489,6 +6518,13 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 			return -EINVAL;
 	}
 
+	if (func_id == BPF_FUNC_find_vma) {
+		err = __check_func_call(env, insn, insn_idx_p, meta.subprogno,
+					set_find_vma_callback_state);
+		if (err < 0)
+			return -EINVAL;
+	}
+
 	if (func_id == BPF_FUNC_snprintf) {
 		err = check_bpf_snprintf_call(env, regs);
 		if (err < 0)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 7396488793ff7..390176a3031ab 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1208,6 +1208,8 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_get_func_ip_proto_tracing;
 	case BPF_FUNC_get_branch_snapshot:
 		return &bpf_get_branch_snapshot_proto;
+	case BPF_FUNC_find_vma:
+		return &bpf_find_vma_proto;
 	case BPF_FUNC_trace_vprintk:
 		return bpf_get_trace_vprintk_proto();
 	default:
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index ba5af15e25f5c..22fa7b74de451 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -4938,6 +4938,25 @@ union bpf_attr {
  *		**-ENOENT** if symbol is not found.
  *
  *		**-EPERM** if caller does not have permission to obtain kernel address.
+ *
+ * long bpf_find_vma(struct task_struct *task, u64 addr, void *callback_fn, void *callback_ctx, u64 flags)
+ *	Description
+ *		Find vma of *task* that contains *addr*, call *callback_fn*
+ *		function with *task*, *vma*, and *callback_ctx*.
+ *		The *callback_fn* should be a static function and
+ *		the *callback_ctx* should be a pointer to the stack.
+ *		The *flags* is used to control certain aspects of the helper.
+ *		Currently, the *flags* must be 0.
+ *
+ *		The expected callback signature is
+ *
+ *		long (\*callback_fn)(struct task_struct \*task, struct vm_area_struct \*vma, void \*ctx);
+ *
+ *	Return
+ *		0 on success.
+ *		**-ENOENT** if *task->mm* is NULL, or no vma contains *addr*.
+ *		**-EBUSY** if failed to try lock mmap_lock.
+ *		**-EINVAL** for invalid **flags**.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -5120,6 +5139,7 @@ union bpf_attr {
 	FN(trace_vprintk),		\
 	FN(skc_to_unix_sock),		\
 	FN(kallsyms_lookup_name),	\
+	FN(find_vma),			\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
-- 
2.30.2

Re: [PATCH v2 bpf-next 1/2] bpf: introduce helper bpf_find_vma

[Yonghong Song]

On 11/4/21 12:00 AM, Song Liu wrote:
> In some profiler use cases, it is necessary to map an address to the
> backing file, e.g., a shared library. bpf_find_vma helper provides a
> flexible way to achieve this. bpf_find_vma maps an address of a task to
> the vma (vm_area_struct) for this address, and feed the vma to an callback
> BPF function. The callback function is necessary here, as we need to
> ensure mmap_sem is unlocked.
> 
> It is necessary to lock mmap_sem for find_vma. To lock and unlock mmap_sem
> safely when irqs are disable, we use the same mechanism as stackmap with
> build_id. Specifically, when irqs are disabled, the unlocked is postponed
> in an irq_work. Refactor stackmap.c so that the irq_work is shared among
> bpf_find_vma and stackmap helpers.
> 
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
>   include/linux/bpf.h            |  1 +
>   include/uapi/linux/bpf.h       | 20 ++++++++++
>   kernel/bpf/mmap_unlock_work.h  | 65 +++++++++++++++++++++++++++++++
>   kernel/bpf/stackmap.c          | 71 ++++++++--------------------------
>   kernel/bpf/task_iter.c         | 49 +++++++++++++++++++++++
>   kernel/bpf/verifier.c          | 36 +++++++++++++++++
>   kernel/trace/bpf_trace.c       |  2 +
>   tools/include/uapi/linux/bpf.h | 20 ++++++++++
>   8 files changed, 209 insertions(+), 55 deletions(-)
>   create mode 100644 kernel/bpf/mmap_unlock_work.h
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 2be6dfd68df99..df3410bff4b06 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -2157,6 +2157,7 @@ extern const struct bpf_func_proto bpf_btf_find_by_name_kind_proto;
>   extern const struct bpf_func_proto bpf_sk_setsockopt_proto;
>   extern const struct bpf_func_proto bpf_sk_getsockopt_proto;
>   extern const struct bpf_func_proto bpf_kallsyms_lookup_name_proto;
> +extern const struct bpf_func_proto bpf_find_vma_proto;
>   
>   const struct bpf_func_proto *tracing_prog_func_proto(
>     enum bpf_func_id func_id, const struct bpf_prog *prog);
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index ba5af15e25f5c..22fa7b74de451 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -4938,6 +4938,25 @@ union bpf_attr {
>    *		**-ENOENT** if symbol is not found.
>    *
>    *		**-EPERM** if caller does not have permission to obtain kernel address.
> + *
> + * long bpf_find_vma(struct task_struct *task, u64 addr, void *callback_fn, void *callback_ctx, u64 flags)
> + *	Description
> + *		Find vma of *task* that contains *addr*, call *callback_fn*
> + *		function with *task*, *vma*, and *callback_ctx*.
> + *		The *callback_fn* should be a static function and
> + *		the *callback_ctx* should be a pointer to the stack.
> + *		The *flags* is used to control certain aspects of the helper.
> + *		Currently, the *flags* must be 0.
> + *
> + *		The expected callback signature is
> + *
> + *		long (\*callback_fn)(struct task_struct \*task, struct vm_area_struct \*vma, void \*ctx);

ctx => callback_ctx
this should make it clear what this 'ctx' is.

> + *
> + *	Return
> + *		0 on success.
> + *		**-ENOENT** if *task->mm* is NULL, or no vma contains *addr*.
> + *		**-EBUSY** if failed to try lock mmap_lock.
> + *		**-EINVAL** for invalid **flags**.
>    */
>   #define __BPF_FUNC_MAPPER(FN)		\
>   	FN(unspec),			\
> @@ -5120,6 +5139,7 @@ union bpf_attr {
>   	FN(trace_vprintk),		\
>   	FN(skc_to_unix_sock),		\
>   	FN(kallsyms_lookup_name),	\
> +	FN(find_vma),			\
>   	/* */
>   
>   /* integer value in 'imm' field of BPF_CALL instruction selects which helper
[...]
> +
>   static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
>   					  u64 *ips, u32 trace_nr, bool user)
>   {
> -	int i;
> +	struct mmap_unlock_irq_work *work = NULL;
> +	bool irq_work_busy = bpf_mmap_unlock_get_irq_work(&work);
>   	struct vm_area_struct *vma;
> -	bool irq_work_busy = false;
> -	struct stack_map_irq_work *work = NULL;
> -
> -	if (irqs_disabled()) {
> -		if (!IS_ENABLED(CONFIG_PREEMPT_RT)) {
> -			work = this_cpu_ptr(&up_read_work);
> -			if (irq_work_is_busy(&work->irq_work)) {
> -				/* cannot queue more up_read, fallback */
> -				irq_work_busy = true;
> -			}
> -		} else {
> -			/*
> -			 * PREEMPT_RT does not allow to trylock mmap sem in
> -			 * interrupt disabled context. Force the fallback code.
> -			 */
> -			irq_work_busy = true;
> -		}
> -	}
> +	int i;

I think moving 'int i' is unnecessary here since there is no 
functionality change.

>   
> -	/*
> -	 * We cannot do up_read() when the irq is disabled, because of
> -	 * risk to deadlock with rq_lock. To do build_id lookup when the
> -	 * irqs are disabled, we need to run up_read() in irq_work. We use
> -	 * a percpu variable to do the irq_work. If the irq_work is
> -	 * already used by another lookup, we fall back to report ips.
> -	 *
> -	 * Same fallback is used for kernel stack (!user) on a stackmap
> -	 * with build_id.
> +	/* If the irq_work is in use, fall back to report ips. Same
> +	 * fallback is used for kernel stack (!user) on a stackmap with
> +	 * build_id.
>   	 */
>   	if (!user || !current || !current->mm || irq_work_busy ||
>   	    !mmap_read_trylock(current->mm)) {
[...]
> +
> +	irq_work_busy = bpf_mmap_unlock_get_irq_work(&work);
> +
> +	if (irq_work_busy || !mmap_read_trylock(mm))
> +		return -EBUSY;
> +
> +	vma = find_vma(mm, start);
> +
> +	if (vma && vma->vm_start <= start && vma->vm_end > start) {
> +		callback_fn((u64)(long)task, (u64)(long)vma,
> +			    (u64)(long)callback_ctx, 0, 0);
> +		ret = 0;
> +	}
> +	bpf_mmap_unlock_mm(work, mm);
> +	return ret;
> +}
> +
> +BTF_ID_LIST_SINGLE(btf_find_vma_ids, struct, task_struct)

We have global btf_task_struct_ids, maybe reuse it?

> +
> +const struct bpf_func_proto bpf_find_vma_proto = {
> +	.func		= bpf_find_vma,
> +	.ret_type	= RET_INTEGER,
> +	.arg1_type	= ARG_PTR_TO_BTF_ID,
> +	.arg1_btf_id	= &btf_find_vma_ids[0],
> +	.arg2_type	= ARG_ANYTHING,
> +	.arg3_type	= ARG_PTR_TO_FUNC,
> +	.arg4_type	= ARG_PTR_TO_STACK_OR_NULL,
> +	.arg5_type	= ARG_ANYTHING,
> +};
> +
>   static int __init task_iter_init(void)
>   {
>   	int ret;
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index f0dca726ebfde..a65526112924a 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -6132,6 +6132,35 @@ static int set_timer_callback_state(struct bpf_verifier_env *env,
>   	return 0;
>   }
>   
> +BTF_ID_LIST_SINGLE(btf_set_find_vma_ids, struct, vm_area_struct)

In task_iter.c, we have

BTF_ID_LIST(btf_task_file_ids)
BTF_ID(struct, file)
BTF_ID(struct, vm_area_struct)

Maybe it is worthwhile to separate them so we can put vm_area_struct as 
global to be reused.

> +
> +static int set_find_vma_callback_state(struct bpf_verifier_env *env,
> +				       struct bpf_func_state *caller,
> +				       struct bpf_func_state *callee,
> +				       int insn_idx)
> +{
> +	/* bpf_find_vma(struct task_struct *task, u64 start,

start => addr ?

> +	 *               void *callback_fn, void *callback_ctx, u64 flags)
> +	 * (callback_fn)(struct task_struct *task,
> +	 *               struct vm_area_struct *vma, void *ctx);

ctx => callback_ctx ?

> +	 */
> +	callee->regs[BPF_REG_1] = caller->regs[BPF_REG_1];
> +
> +	callee->regs[BPF_REG_2].type = PTR_TO_BTF_ID;
> +	__mark_reg_known_zero(&callee->regs[BPF_REG_2]);
> +	callee->regs[BPF_REG_2].btf =  btf_vmlinux;
> +	callee->regs[BPF_REG_2].btf_id = btf_set_find_vma_ids[0];
> +
> +	/* pointer to stack or null */
> +	callee->regs[BPF_REG_3] = caller->regs[BPF_REG_4];
> +
> +	/* unused */
> +	__mark_reg_not_init(env, &callee->regs[BPF_REG_4]);
> +	__mark_reg_not_init(env, &callee->regs[BPF_REG_5]);
> +	callee->in_callback_fn = true;
> +	return 0;
> +}
> +
[...]

Re: [PATCH v2 bpf-next 1/2] bpf: introduce helper bpf_find_vma

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1643 bytes --]

Hi Song,

I love your patch! Yet something to improve:

[auto build test ERROR on bpf-next/master]

url:    https://github.com/0day-ci/linux/commits/Song-Liu/introduce-bpf_find_vma/20211104-150210
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: riscv-randconfig-r034-20211104 (attached as .config)
compiler: riscv64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/e219efba8d04dede08b1d87fa1e8c5c01180caaf
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Song-Liu/introduce-bpf_find_vma/20211104-150210
        git checkout e219efba8d04dede08b1d87fa1e8c5c01180caaf
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=riscv SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   riscv64-linux-ld: kernel/bpf/task_iter.o: in function `bpf_find_vma':
   task_iter.c:(.text+0x3dc): undefined reference to `mmap_unlock_work'
>> riscv64-linux-ld: task_iter.c:(.text+0x3e0): undefined reference to `mmap_unlock_work'
   riscv64-linux-ld: task_iter.c:(.text+0x400): undefined reference to `mmap_unlock_work'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31824 bytes --]

Re: [PATCH v2 bpf-next 1/2] bpf: introduce helper bpf_find_vma

[Song Liu]

> On Nov 4, 2021, at 1:37 PM, kernel test robot <lkp@intel.com> wrote:
> 
> Hi Song,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on bpf-next/master]
> 
> url:    https://github.com/0day-ci/linux/commits/Song-Liu/introduce-bpf_find_vma/20211104-150210
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> config: riscv-randconfig-r034-20211104 (attached as .config)
> compiler: riscv64-linux-gcc (GCC) 11.2.0
> reproduce (this is a W=1 build):
>        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross  -O ~/bin/make.cross
>        chmod +x ~/bin/make.cross
>        # https://github.com/0day-ci/linux/commit/e219efba8d04dede08b1d87fa1e8c5c01180caaf
>        git remote add linux-review https://github.com/0day-ci/linux
>        git fetch --no-tags linux-review Song-Liu/introduce-bpf_find_vma/20211104-150210
>        git checkout e219efba8d04dede08b1d87fa1e8c5c01180caaf
>        # save the attached .config to linux build tree
>        mkdir build_dir
>        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=riscv SHELL=/bin/bash
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>   riscv64-linux-ld: kernel/bpf/task_iter.o: in function `bpf_find_vma':
>   task_iter.c:(.text+0x3dc): undefined reference to `mmap_unlock_work'
>>> riscv64-linux-ld: task_iter.c:(.text+0x3e0): undefined reference to `mmap_unlock_work'
>   riscv64-linux-ld: task_iter.c:(.text+0x400): undefined reference to `mmap_unlock_work'

Sigh, I didn't see this before sending v3. Will fix in v4. 

> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org 
> <.config.gz>

[PATCH v2 bpf-next 2/2] selftests/bpf: add tests for bpf_find_vma

[Song Liu]

Add tests for bpf_find_vma in perf_event program and kprobe program. The
perf_event program is triggered from NMI context, so the second call of
bpf_find_vma() will return -EBUSY (irq_work busy). The kprobe program,
on the other hand, does not have this constraint.

Also add test for illegal writes to task or vma from the callback
function. The verifier should reject both cases.

Signed-off-by: Song Liu <songliubraving@fb.com>
---
 .../selftests/bpf/prog_tests/find_vma.c       | 115 ++++++++++++++++++
 tools/testing/selftests/bpf/progs/find_vma.c  |  70 +++++++++++
 .../selftests/bpf/progs/find_vma_fail1.c      |  30 +++++
 .../selftests/bpf/progs/find_vma_fail2.c      |  30 +++++
 4 files changed, 245 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/find_vma.c
 create mode 100644 tools/testing/selftests/bpf/progs/find_vma.c
 create mode 100644 tools/testing/selftests/bpf/progs/find_vma_fail1.c
 create mode 100644 tools/testing/selftests/bpf/progs/find_vma_fail2.c

diff --git a/tools/testing/selftests/bpf/prog_tests/find_vma.c b/tools/testing/selftests/bpf/prog_tests/find_vma.c
new file mode 100644
index 0000000000000..3955a92d4c152
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/find_vma.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Facebook */
+#include <test_progs.h>
+#include <sys/types.h>
+#include <unistd.h>
+#include "find_vma.skel.h"
+#include "find_vma_fail1.skel.h"
+#include "find_vma_fail2.skel.h"
+
+static void test_and_reset_skel(struct find_vma *skel, int expected_find_zero_ret)
+{
+	ASSERT_EQ(skel->bss->found_vm_exec, 1, "found_vm_exec");
+	ASSERT_EQ(skel->data->find_addr_ret, 0, "find_addr_ret");
+	ASSERT_EQ(skel->data->find_zero_ret, expected_find_zero_ret, "find_zero_ret");
+	ASSERT_OK_PTR(strstr(skel->bss->d_iname, "test_progs"), "find_test_progs");
+
+	skel->bss->found_vm_exec = 0;
+	skel->data->find_addr_ret = -1;
+	skel->data->find_zero_ret = -1;
+	skel->bss->d_iname[0] = 0;
+}
+
+static int open_pe(void)
+{
+	struct perf_event_attr attr = {0};
+	int pfd;
+
+	/* create perf event */
+	attr.size = sizeof(attr);
+	attr.type = PERF_TYPE_HARDWARE;
+	attr.config = PERF_COUNT_HW_CPU_CYCLES;
+	attr.freq = 1;
+	attr.sample_freq = 4000;
+	pfd = syscall(__NR_perf_event_open, &attr, 0, -1, -1, PERF_FLAG_FD_CLOEXEC);
+
+	return pfd >= 0 ? pfd : -errno;
+}
+
+static void test_find_vma_pe(struct find_vma *skel)
+{
+	struct bpf_link *link = NULL;
+	volatile int j = 0;
+	int pfd = -1, i;
+
+	pfd = open_pe();
+	if (pfd < 0) {
+		if (pfd == -ENOENT || pfd == -EOPNOTSUPP) {
+			printf("%s:SKIP:no PERF_COUNT_HW_CPU_CYCLES\n", __func__);
+			test__skip();
+		}
+		if (!ASSERT_GE(pfd, 0, "perf_event_open"))
+			goto cleanup;
+	}
+
+	link = bpf_program__attach_perf_event(skel->progs.handle_pe, pfd);
+	if (!ASSERT_OK_PTR(link, "attach_perf_event"))
+		goto cleanup;
+
+	for (i = 0; i < 1000000; ++i)
+		++j;
+
+	test_and_reset_skel(skel, -EBUSY /* in nmi, irq_work is busy */);
+cleanup:
+	bpf_link__destroy(link);
+	close(pfd);
+	/* caller will clean up skel */
+}
+
+static void test_find_vma_kprobe(struct find_vma *skel)
+{
+	int err;
+
+	err = find_vma__attach(skel);
+	if (!ASSERT_OK(err, "get_branch_snapshot__attach"))
+		return;  /* caller will cleanup skel */
+
+	getpgid(skel->bss->target_pid);
+	test_and_reset_skel(skel, -ENOENT /* could not find vma for ptr 0 */);
+}
+
+static void test_illegal_write_vma(void)
+{
+	struct find_vma_fail1 *skel;
+
+	skel = find_vma_fail1__open_and_load();
+	ASSERT_ERR_PTR(skel, "find_vma_fail1__open_and_load");
+}
+
+static void test_illegal_write_task(void)
+{
+	struct find_vma_fail2 *skel;
+
+	skel = find_vma_fail2__open_and_load();
+	ASSERT_ERR_PTR(skel, "find_vma_fail2__open_and_load");
+}
+
+void serial_test_find_vma(void)
+{
+	struct find_vma *skel;
+
+	skel = find_vma__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "find_vma__open_and_load"))
+		return;
+
+	skel->bss->target_pid = getpid();
+	skel->bss->addr = (__u64)test_find_vma_pe;
+
+	test_find_vma_pe(skel);
+	usleep(100000); /* allow the irq_work to finish */
+	test_find_vma_kprobe(skel);
+
+	find_vma__destroy(skel);
+	test_illegal_write_vma();
+	test_illegal_write_task();
+}
diff --git a/tools/testing/selftests/bpf/progs/find_vma.c b/tools/testing/selftests/bpf/progs/find_vma.c
new file mode 100644
index 0000000000000..2776718a54e29
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/find_vma.c
@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Facebook */
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+struct callback_ctx {
+	int dummy;
+};
+
+#define VM_EXEC		0x00000004
+#define DNAME_INLINE_LEN 32
+
+pid_t target_pid = 0;
+char d_iname[DNAME_INLINE_LEN] = {0};
+__u32 found_vm_exec = 0;
+__u64 addr = 0;
+int find_zero_ret = -1;
+int find_addr_ret = -1;
+
+static __u64
+check_vma(struct task_struct *task, struct vm_area_struct *vma,
+	  struct callback_ctx *data)
+{
+	if (vma->vm_file)
+		bpf_probe_read_kernel_str(d_iname, DNAME_INLINE_LEN - 1,
+					  vma->vm_file->f_path.dentry->d_iname);
+
+	/* check for VM_EXEC */
+	if (vma->vm_flags & VM_EXEC)
+		found_vm_exec = 1;
+
+	return 0;
+}
+
+SEC("kprobe/__x64_sys_getpgid")
+int handle_getpid(void)
+{
+	struct task_struct *task = bpf_get_current_task_btf();
+	struct callback_ctx data = {0};
+
+	if (task->pid != target_pid)
+		return 0;
+
+	find_addr_ret = bpf_find_vma(task, addr, check_vma, &data, 0);
+
+	/* this should return -ENOENT */
+	find_zero_ret = bpf_find_vma(task, 0, check_vma, &data, 0);
+	return 0;
+}
+
+SEC("perf_event")
+int handle_pe(void)
+{
+	struct task_struct *task = bpf_get_current_task_btf();
+	struct callback_ctx data = {0};
+
+	if (task->pid != target_pid)
+		return 0;
+
+	find_addr_ret = bpf_find_vma(task, addr, check_vma, &data, 0);
+
+	/* In NMI, this should return -EBUSY, as the previous call is using
+	 * the irq_work.
+	 */
+	find_zero_ret = bpf_find_vma(task, 0, check_vma, &data, 0);
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/progs/find_vma_fail1.c b/tools/testing/selftests/bpf/progs/find_vma_fail1.c
new file mode 100644
index 0000000000000..d17bdcdf76f07
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/find_vma_fail1.c
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Facebook */
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+
+char _license[] SEC("license") = "GPL";
+
+struct callback_ctx {
+	int dummy;
+};
+
+static __u64
+write_vma(struct task_struct *task, struct vm_area_struct *vma,
+	  struct callback_ctx *data)
+{
+	/* writing to vma, which is illegal */
+	vma->vm_flags |= 0x55;
+
+	return 0;
+}
+
+SEC("kprobe/__x64_sys_getpgid")
+int handle_getpid(void)
+{
+	struct task_struct *task = bpf_get_current_task_btf();
+	struct callback_ctx data = {0};
+
+	bpf_find_vma(task, 0, write_vma, &data, 0);
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/progs/find_vma_fail2.c b/tools/testing/selftests/bpf/progs/find_vma_fail2.c
new file mode 100644
index 0000000000000..079c4594c095d
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/find_vma_fail2.c
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Facebook */
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+
+char _license[] SEC("license") = "GPL";
+
+struct callback_ctx {
+	int dummy;
+};
+
+static __u64
+write_task(struct task_struct *task, struct vm_area_struct *vma,
+	   struct callback_ctx *data)
+{
+	/* writing to task, which is illegal */
+	task->mm = NULL;
+
+	return 0;
+}
+
+SEC("kprobe/__x64_sys_getpgid")
+int handle_getpid(void)
+{
+	struct task_struct *task = bpf_get_current_task_btf();
+	struct callback_ctx data = {0};
+
+	bpf_find_vma(task, 0, write_task, &data, 0);
+	return 0;
+}
-- 
2.30.2

Re: [PATCH v2 bpf-next 2/2] selftests/bpf: add tests for bpf_find_vma

[Song Liu]

> On Nov 4, 2021, at 12:00 AM, Song Liu <songliubraving@fb.com> wrote:
> 
> Add tests for bpf_find_vma in perf_event program and kprobe program. The
> perf_event program is triggered from NMI context, so the second call of
> bpf_find_vma() will return -EBUSY (irq_work busy). The kprobe program,
> on the other hand, does not have this constraint.
> 
> Also add test for illegal writes to task or vma from the callback
> function. The verifier should reject both cases.
> 
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
> .../selftests/bpf/prog_tests/find_vma.c       | 115 ++++++++++++++++++
> tools/testing/selftests/bpf/progs/find_vma.c  |  70 +++++++++++
> .../selftests/bpf/progs/find_vma_fail1.c      |  30 +++++
> .../selftests/bpf/progs/find_vma_fail2.c      |  30 +++++
> 4 files changed, 245 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/find_vma.c
> create mode 100644 tools/testing/selftests/bpf/progs/find_vma.c
> create mode 100644 tools/testing/selftests/bpf/progs/find_vma_fail1.c
> create mode 100644 tools/testing/selftests/bpf/progs/find_vma_fail2.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/find_vma.c b/tools/testing/selftests/bpf/prog_tests/find_vma.c
> new file mode 100644
> index 0000000000000..3955a92d4c152
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/find_vma.c
> @@ -0,0 +1,115 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2021 Facebook */
> +#include <test_progs.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +#include "find_vma.skel.h"
> +#include "find_vma_fail1.skel.h"
> +#include "find_vma_fail2.skel.h"
> +
> +static void test_and_reset_skel(struct find_vma *skel, int expected_find_zero_ret)
> +{
> +	ASSERT_EQ(skel->bss->found_vm_exec, 1, "found_vm_exec");
> +	ASSERT_EQ(skel->data->find_addr_ret, 0, "find_addr_ret");
> +	ASSERT_EQ(skel->data->find_zero_ret, expected_find_zero_ret, "find_zero_ret");
> +	ASSERT_OK_PTR(strstr(skel->bss->d_iname, "test_progs"), "find_test_progs");
> +
> +	skel->bss->found_vm_exec = 0;
> +	skel->data->find_addr_ret = -1;
> +	skel->data->find_zero_ret = -1;
> +	skel->bss->d_iname[0] = 0;
> +}
> +
> +static int open_pe(void)
> +{
> +	struct perf_event_attr attr = {0};
> +	int pfd;
> +
> +	/* create perf event */
> +	attr.size = sizeof(attr);
> +	attr.type = PERF_TYPE_HARDWARE;
> +	attr.config = PERF_COUNT_HW_CPU_CYCLES;
> +	attr.freq = 1;
> +	attr.sample_freq = 4000;
> +	pfd = syscall(__NR_perf_event_open, &attr, 0, -1, -1, PERF_FLAG_FD_CLOEXEC);
> +
> +	return pfd >= 0 ? pfd : -errno;
> +}
> +
> +static void test_find_vma_pe(struct find_vma *skel)
> +{
> +	struct bpf_link *link = NULL;
> +	volatile int j = 0;
> +	int pfd = -1, i;
> +
> +	pfd = open_pe();
> +	if (pfd < 0) {
> +		if (pfd == -ENOENT || pfd == -EOPNOTSUPP) {
> +			printf("%s:SKIP:no PERF_COUNT_HW_CPU_CYCLES\n", __func__);
> +			test__skip();

Ah, I missed a goto cleanup here, so it breaks vmtest. I can fix this in 
v3, or we can fix it when applying the patch. 

> +		}
> +		if (!ASSERT_GE(pfd, 0, "perf_event_open"))
> +			goto cleanup;
> +	}
> +
> +	link = bpf_program__attach_perf_event(skel->progs.handle_pe, pfd);
> +	if (!ASSERT_OK_PTR(link, "attach_perf_event"))
> +		goto cleanup;
> +
> +	for (i = 0; i < 1000000; ++i)
> +		++j;
> +
> +	test_and_reset_skel(skel, -EBUSY /* in nmi, irq_work is busy */);
> +cleanup:
> +	bpf_link__destroy(link);
> +	close(pfd);
> +	/* caller will clean up skel */
> +}
> +
> +static void test_find_vma_kprobe(struct find_vma *skel)
> +{
> +	int err;
> +
> +	err = find_vma__attach(skel);
> +	if (!ASSERT_OK(err, "get_branch_snapshot__attach"))
> +		return;  /* caller will cleanup skel */
> +
> +	getpgid(skel->bss->target_pid);
> +	test_and_reset_skel(skel, -ENOENT /* could not find vma for ptr 0 */);
> +}
> +
> +static void test_illegal_write_vma(void)
> +{
> +	struct find_vma_fail1 *skel;
> +
> +	skel = find_vma_fail1__open_and_load();
> +	ASSERT_ERR_PTR(skel, "find_vma_fail1__open_and_load");
> +}
> +
> +static void test_illegal_write_task(void)
> +{
> +	struct find_vma_fail2 *skel;
> +
> +	skel = find_vma_fail2__open_and_load();
> +	ASSERT_ERR_PTR(skel, "find_vma_fail2__open_and_load");
> +}
> +
> +void serial_test_find_vma(void)
> +{
> +	struct find_vma *skel;
> +
> +	skel = find_vma__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "find_vma__open_and_load"))
> +		return;
> +
> +	skel->bss->target_pid = getpid();
> +	skel->bss->addr = (__u64)test_find_vma_pe;
> +
> +	test_find_vma_pe(skel);
> +	usleep(100000); /* allow the irq_work to finish */
> +	test_find_vma_kprobe(skel);
> +
> +	find_vma__destroy(skel);
> +	test_illegal_write_vma();
> +	test_illegal_write_task();
> +}
> diff --git a/tools/testing/selftests/bpf/progs/find_vma.c b/tools/testing/selftests/bpf/progs/find_vma.c
> new file mode 100644
> index 0000000000000..2776718a54e29
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/find_vma.c
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2021 Facebook */
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +char _license[] SEC("license") = "GPL";
> +
> +struct callback_ctx {
> +	int dummy;
> +};
> +
> +#define VM_EXEC		0x00000004
> +#define DNAME_INLINE_LEN 32
> +
> +pid_t target_pid = 0;
> +char d_iname[DNAME_INLINE_LEN] = {0};
> +__u32 found_vm_exec = 0;
> +__u64 addr = 0;
> +int find_zero_ret = -1;
> +int find_addr_ret = -1;
> +
> +static __u64
> +check_vma(struct task_struct *task, struct vm_area_struct *vma,
> +	  struct callback_ctx *data)
> +{
> +	if (vma->vm_file)
> +		bpf_probe_read_kernel_str(d_iname, DNAME_INLINE_LEN - 1,
> +					  vma->vm_file->f_path.dentry->d_iname);
> +
> +	/* check for VM_EXEC */
> +	if (vma->vm_flags & VM_EXEC)
> +		found_vm_exec = 1;
> +
> +	return 0;
> +}
> +
> +SEC("kprobe/__x64_sys_getpgid")
> +int handle_getpid(void)
> +{
> +	struct task_struct *task = bpf_get_current_task_btf();
> +	struct callback_ctx data = {0};
> +
> +	if (task->pid != target_pid)
> +		return 0;
> +
> +	find_addr_ret = bpf_find_vma(task, addr, check_vma, &data, 0);
> +
> +	/* this should return -ENOENT */
> +	find_zero_ret = bpf_find_vma(task, 0, check_vma, &data, 0);
> +	return 0;
> +}
> +
> +SEC("perf_event")
> +int handle_pe(void)
> +{
> +	struct task_struct *task = bpf_get_current_task_btf();
> +	struct callback_ctx data = {0};
> +
> +	if (task->pid != target_pid)
> +		return 0;
> +
> +	find_addr_ret = bpf_find_vma(task, addr, check_vma, &data, 0);
> +
> +	/* In NMI, this should return -EBUSY, as the previous call is using
> +	 * the irq_work.
> +	 */
> +	find_zero_ret = bpf_find_vma(task, 0, check_vma, &data, 0);
> +	return 0;
> +}
> diff --git a/tools/testing/selftests/bpf/progs/find_vma_fail1.c b/tools/testing/selftests/bpf/progs/find_vma_fail1.c
> new file mode 100644
> index 0000000000000..d17bdcdf76f07
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/find_vma_fail1.c
> @@ -0,0 +1,30 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2021 Facebook */
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +
> +char _license[] SEC("license") = "GPL";
> +
> +struct callback_ctx {
> +	int dummy;
> +};
> +
> +static __u64
> +write_vma(struct task_struct *task, struct vm_area_struct *vma,
> +	  struct callback_ctx *data)
> +{
> +	/* writing to vma, which is illegal */
> +	vma->vm_flags |= 0x55;
> +
> +	return 0;
> +}
> +
> +SEC("kprobe/__x64_sys_getpgid")
> +int handle_getpid(void)
> +{
> +	struct task_struct *task = bpf_get_current_task_btf();
> +	struct callback_ctx data = {0};
> +
> +	bpf_find_vma(task, 0, write_vma, &data, 0);
> +	return 0;
> +}
> diff --git a/tools/testing/selftests/bpf/progs/find_vma_fail2.c b/tools/testing/selftests/bpf/progs/find_vma_fail2.c
> new file mode 100644
> index 0000000000000..079c4594c095d
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/find_vma_fail2.c
> @@ -0,0 +1,30 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2021 Facebook */
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +
> +char _license[] SEC("license") = "GPL";
> +
> +struct callback_ctx {
> +	int dummy;
> +};
> +
> +static __u64
> +write_task(struct task_struct *task, struct vm_area_struct *vma,
> +	   struct callback_ctx *data)
> +{
> +	/* writing to task, which is illegal */
> +	task->mm = NULL;
> +
> +	return 0;
> +}
> +
> +SEC("kprobe/__x64_sys_getpgid")
> +int handle_getpid(void)
> +{
> +	struct task_struct *task = bpf_get_current_task_btf();
> +	struct callback_ctx data = {0};
> +
> +	bpf_find_vma(task, 0, write_task, &data, 0);
> +	return 0;
> +}
> -- 
> 2.30.2
> 

Re: [PATCH v2 bpf-next 2/2] selftests/bpf: add tests for bpf_find_vma

[Yonghong Song]

On 11/4/21 12:00 AM, Song Liu wrote:
> Add tests for bpf_find_vma in perf_event program and kprobe program. The
> perf_event program is triggered from NMI context, so the second call of
> bpf_find_vma() will return -EBUSY (irq_work busy). The kprobe program,
> on the other hand, does not have this constraint.
> 
> Also add test for illegal writes to task or vma from the callback
> function. The verifier should reject both cases.
> 
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
>   .../selftests/bpf/prog_tests/find_vma.c       | 115 ++++++++++++++++++
>   tools/testing/selftests/bpf/progs/find_vma.c  |  70 +++++++++++
>   .../selftests/bpf/progs/find_vma_fail1.c      |  30 +++++
>   .../selftests/bpf/progs/find_vma_fail2.c      |  30 +++++
>   4 files changed, 245 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/find_vma.c
>   create mode 100644 tools/testing/selftests/bpf/progs/find_vma.c
>   create mode 100644 tools/testing/selftests/bpf/progs/find_vma_fail1.c
>   create mode 100644 tools/testing/selftests/bpf/progs/find_vma_fail2.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/find_vma.c b/tools/testing/selftests/bpf/prog_tests/find_vma.c
> new file mode 100644
> index 0000000000000..3955a92d4c152
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/find_vma.c
> @@ -0,0 +1,115 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2021 Facebook */
> +#include <test_progs.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +#include "find_vma.skel.h"
> +#include "find_vma_fail1.skel.h"
> +#include "find_vma_fail2.skel.h"
> +
> +static void test_and_reset_skel(struct find_vma *skel, int expected_find_zero_ret)
> +{
> +	ASSERT_EQ(skel->bss->found_vm_exec, 1, "found_vm_exec");
> +	ASSERT_EQ(skel->data->find_addr_ret, 0, "find_addr_ret");
> +	ASSERT_EQ(skel->data->find_zero_ret, expected_find_zero_ret, "find_zero_ret");
> +	ASSERT_OK_PTR(strstr(skel->bss->d_iname, "test_progs"), "find_test_progs");
> +
> +	skel->bss->found_vm_exec = 0;
> +	skel->data->find_addr_ret = -1;
> +	skel->data->find_zero_ret = -1;
> +	skel->bss->d_iname[0] = 0;
> +}
> +
> +static int open_pe(void)
> +{
> +	struct perf_event_attr attr = {0};
> +	int pfd;
> +
> +	/* create perf event */
> +	attr.size = sizeof(attr);
> +	attr.type = PERF_TYPE_HARDWARE;
> +	attr.config = PERF_COUNT_HW_CPU_CYCLES;
> +	attr.freq = 1;
> +	attr.sample_freq = 4000;
> +	pfd = syscall(__NR_perf_event_open, &attr, 0, -1, -1, PERF_FLAG_FD_CLOEXEC);
> +
> +	return pfd >= 0 ? pfd : -errno;
> +}
> +
> +static void test_find_vma_pe(struct find_vma *skel)
> +{
> +	struct bpf_link *link = NULL;
> +	volatile int j = 0;
> +	int pfd = -1, i;
> +
> +	pfd = open_pe();
> +	if (pfd < 0) {
> +		if (pfd == -ENOENT || pfd == -EOPNOTSUPP) {
> +			printf("%s:SKIP:no PERF_COUNT_HW_CPU_CYCLES\n", __func__);
> +			test__skip();
> +		}
> +		if (!ASSERT_GE(pfd, 0, "perf_event_open"))
> +			goto cleanup;
> +	}
> +
> +	link = bpf_program__attach_perf_event(skel->progs.handle_pe, pfd);
> +	if (!ASSERT_OK_PTR(link, "attach_perf_event"))
> +		goto cleanup;
> +
> +	for (i = 0; i < 1000000; ++i)
> +		++j;

Does this really work? Compiler could do
   j += 1000000;

> +
> +	test_and_reset_skel(skel, -EBUSY /* in nmi, irq_work is busy */);
> +cleanup:
> +	bpf_link__destroy(link);
> +	close(pfd);
> +	/* caller will clean up skel */

Above comment is not needed. It should be clear from the code.

> +}
> +
> +static void test_find_vma_kprobe(struct find_vma *skel)
> +{
> +	int err;
> +
> +	err = find_vma__attach(skel);
> +	if (!ASSERT_OK(err, "get_branch_snapshot__attach"))
> +		return;  /* caller will cleanup skel */
> +
> +	getpgid(skel->bss->target_pid);
> +	test_and_reset_skel(skel, -ENOENT /* could not find vma for ptr 0 */);
> +}
> +
> +static void test_illegal_write_vma(void)
> +{
> +	struct find_vma_fail1 *skel;
> +
> +	skel = find_vma_fail1__open_and_load();
> +	ASSERT_ERR_PTR(skel, "find_vma_fail1__open_and_load");
> +}
> +
> +static void test_illegal_write_task(void)
> +{
> +	struct find_vma_fail2 *skel;
> +
> +	skel = find_vma_fail2__open_and_load();
> +	ASSERT_ERR_PTR(skel, "find_vma_fail2__open_and_load");
> +}
> +
> +void serial_test_find_vma(void)
> +{
> +	struct find_vma *skel;
> +
> +	skel = find_vma__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "find_vma__open_and_load"))
> +		return;
> +
> +	skel->bss->target_pid = getpid();
> +	skel->bss->addr = (__u64)test_find_vma_pe;
> +
> +	test_find_vma_pe(skel);
> +	usleep(100000); /* allow the irq_work to finish */
> +	test_find_vma_kprobe(skel);
> +
> +	find_vma__destroy(skel);
> +	test_illegal_write_vma();
> +	test_illegal_write_task();
> +}
> diff --git a/tools/testing/selftests/bpf/progs/find_vma.c b/tools/testing/selftests/bpf/progs/find_vma.c
> new file mode 100644
> index 0000000000000..2776718a54e29
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/find_vma.c
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2021 Facebook */
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +char _license[] SEC("license") = "GPL";
> +
> +struct callback_ctx {
> +	int dummy;
> +};
> +
> +#define VM_EXEC		0x00000004
> +#define DNAME_INLINE_LEN 32
> +
> +pid_t target_pid = 0;
> +char d_iname[DNAME_INLINE_LEN] = {0};
> +__u32 found_vm_exec = 0;
> +__u64 addr = 0;
> +int find_zero_ret = -1;
> +int find_addr_ret = -1;
> +
> +static __u64

Let us 'long' instead of '__u64' to match uapi bpf.h.

> +check_vma(struct task_struct *task, struct vm_area_struct *vma,
> +	  struct callback_ctx *data)
> +{
> +	if (vma->vm_file)
> +		bpf_probe_read_kernel_str(d_iname, DNAME_INLINE_LEN - 1,
> +					  vma->vm_file->f_path.dentry->d_iname);
> +
> +	/* check for VM_EXEC */
> +	if (vma->vm_flags & VM_EXEC)
> +		found_vm_exec = 1;
> +
> +	return 0;
> +}
> +
> +SEC("kprobe/__x64_sys_getpgid")

The test will fail for non x86_64 architecture.
I had some tweaks in test_probe_user.c. Please take a look.
We can refactor to make tweaks in test_probe_user.c reusable
by other files.

> +int handle_getpid(void)
> +{
> +	struct task_struct *task = bpf_get_current_task_btf();
> +	struct callback_ctx data = {0};
> +
> +	if (task->pid != target_pid)
> +		return 0;
> +
> +	find_addr_ret = bpf_find_vma(task, addr, check_vma, &data, 0);
> +
> +	/* this should return -ENOENT */
> +	find_zero_ret = bpf_find_vma(task, 0, check_vma, &data, 0);
> +	return 0;
> +}
> +
> +SEC("perf_event")
> +int handle_pe(void)
> +{
> +	struct task_struct *task = bpf_get_current_task_btf();
> +	struct callback_ctx data = {0};
> +
> +	if (task->pid != target_pid)
> +		return 0;

This is tricky. How do we guarantee task->pid == target_pid hit?
This probably mostly okay in serial running mode. But it may
become more challenging if test_progs is running in parallel mode?

> +
> +	find_addr_ret = bpf_find_vma(task, addr, check_vma, &data, 0);
> +
> +	/* In NMI, this should return -EBUSY, as the previous call is using
> +	 * the irq_work.
> +	 */
> +	find_zero_ret = bpf_find_vma(task, 0, check_vma, &data, 0);
> +	return 0;
> +}
> diff --git a/tools/testing/selftests/bpf/progs/find_vma_fail1.c b/tools/testing/selftests/bpf/progs/find_vma_fail1.c
> new file mode 100644
> index 0000000000000..d17bdcdf76f07
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/find_vma_fail1.c
> @@ -0,0 +1,30 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2021 Facebook */
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +
> +char _license[] SEC("license") = "GPL";
> +
> +struct callback_ctx {
> +	int dummy;
> +};
> +
> +static __u64

__u64 => long

> +write_vma(struct task_struct *task, struct vm_area_struct *vma,
> +	  struct callback_ctx *data)
> +{
> +	/* writing to vma, which is illegal */
> +	vma->vm_flags |= 0x55;
> +
> +	return 0;
> +}
> +
> +SEC("kprobe/__x64_sys_getpgid")
> +int handle_getpid(void)
> +{
> +	struct task_struct *task = bpf_get_current_task_btf();
> +	struct callback_ctx data = {0};
> +
> +	bpf_find_vma(task, 0, write_vma, &data, 0);
> +	return 0;
> +}
> diff --git a/tools/testing/selftests/bpf/progs/find_vma_fail2.c b/tools/testing/selftests/bpf/progs/find_vma_fail2.c
> new file mode 100644
> index 0000000000000..079c4594c095d
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/find_vma_fail2.c
> @@ -0,0 +1,30 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2021 Facebook */
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +
> +char _license[] SEC("license") = "GPL";
> +
> +struct callback_ctx {
> +	int dummy;
> +};
> +
> +static __u64

__u64 => long

> +write_task(struct task_struct *task, struct vm_area_struct *vma,
> +	   struct callback_ctx *data)
> +{
> +	/* writing to task, which is illegal */
> +	task->mm = NULL;
> +
> +	return 0;
> +}
> +
> +SEC("kprobe/__x64_sys_getpgid")
> +int handle_getpid(void)
> +{
> +	struct task_struct *task = bpf_get_current_task_btf();
> +	struct callback_ctx data = {0};
> +
> +	bpf_find_vma(task, 0, write_task, &data, 0);
> +	return 0;
> +}
> 

Re: [PATCH v2 bpf-next 2/2] selftests/bpf: add tests for bpf_find_vma

[Song Liu]

> On Nov 4, 2021, at 10:07 AM, Yonghong Song <yhs@fb.com> wrote:
> 
> 
> 
> On 11/4/21 12:00 AM, Song Liu wrote:
>> Add tests for bpf_find_vma in perf_event program and kprobe program. The
>> perf_event program is triggered from NMI context, so the second call of
>> bpf_find_vma() will return -EBUSY (irq_work busy). The kprobe program,
>> on the other hand, does not have this constraint.
>> Also add test for illegal writes to task or vma from the callback
>> function. The verifier should reject both cases.
>> Signed-off-by: Song Liu <songliubraving@fb.com>

[...]

>> +static void test_find_vma_pe(struct find_vma *skel)
>> +{
>> +	struct bpf_link *link = NULL;
>> +	volatile int j = 0;
>> +	int pfd = -1, i;
>> +
>> +	pfd = open_pe();
>> +	if (pfd < 0) {
>> +		if (pfd == -ENOENT || pfd == -EOPNOTSUPP) {
>> +			printf("%s:SKIP:no PERF_COUNT_HW_CPU_CYCLES\n", __func__);
>> +			test__skip();
>> +		}
>> +		if (!ASSERT_GE(pfd, 0, "perf_event_open"))
>> +			goto cleanup;
>> +	}
>> +
>> +	link = bpf_program__attach_perf_event(skel->progs.handle_pe, pfd);
>> +	if (!ASSERT_OK_PTR(link, "attach_perf_event"))
>> +		goto cleanup;
>> +
>> +	for (i = 0; i < 1000000; ++i)
>> +		++j;
> 
> Does this really work? Compiler could do
>  j += 1000000;

I think compiler won't do it with volatile j? 

> 
>> +
>> +	test_and_reset_skel(skel, -EBUSY /* in nmi, irq_work is busy */);
>> +cleanup:
>> +	bpf_link__destroy(link);
>> +	close(pfd);
>> +	/* caller will clean up skel */
> 
> Above comment is not needed. It should be clear from the code.
> 
>> +}
>> +
>> +static void test_find_vma_kprobe(struct find_vma *skel)
>> +{
>> +	int err;
>> +
>> +	err = find_vma__attach(skel);
>> +	if (!ASSERT_OK(err, "get_branch_snapshot__attach"))
>> +		return;  /* caller will cleanup skel */
>> +
>> +	getpgid(skel->bss->target_pid);
>> +	test_and_reset_skel(skel, -ENOENT /* could not find vma for ptr 0 */);
>> +}
>> +
>> +static void test_illegal_write_vma(void)
>> +{
>> +	struct find_vma_fail1 *skel;
>> +
>> +	skel = find_vma_fail1__open_and_load();
>> +	ASSERT_ERR_PTR(skel, "find_vma_fail1__open_and_load");
>> +}
>> +
>> +static void test_illegal_write_task(void)
>> +{
>> +	struct find_vma_fail2 *skel;
>> +
>> +	skel = find_vma_fail2__open_and_load();
>> +	ASSERT_ERR_PTR(skel, "find_vma_fail2__open_and_load");
>> +}
>> +
>> +void serial_test_find_vma(void)
>> +{
>> +	struct find_vma *skel;
>> +
>> +	skel = find_vma__open_and_load();
>> +	if (!ASSERT_OK_PTR(skel, "find_vma__open_and_load"))
>> +		return;
>> +
>> +	skel->bss->target_pid = getpid();
>> +	skel->bss->addr = (__u64)test_find_vma_pe;
>> +
>> +	test_find_vma_pe(skel);
>> +	usleep(100000); /* allow the irq_work to finish */
>> +	test_find_vma_kprobe(skel);
>> +
>> +	find_vma__destroy(skel);
>> +	test_illegal_write_vma();
>> +	test_illegal_write_task();
>> +}
>> diff --git a/tools/testing/selftests/bpf/progs/find_vma.c b/tools/testing/selftests/bpf/progs/find_vma.c
>> new file mode 100644
>> index 0000000000000..2776718a54e29
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/find_vma.c
>> @@ -0,0 +1,70 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2021 Facebook */
>> +#include "vmlinux.h"
>> +#include <bpf/bpf_helpers.h>
>> +#include <bpf/bpf_tracing.h>
>> +
>> +char _license[] SEC("license") = "GPL";
>> +
>> +struct callback_ctx {
>> +	int dummy;
>> +};
>> +
>> +#define VM_EXEC		0x00000004
>> +#define DNAME_INLINE_LEN 32
>> +
>> +pid_t target_pid = 0;
>> +char d_iname[DNAME_INLINE_LEN] = {0};
>> +__u32 found_vm_exec = 0;
>> +__u64 addr = 0;
>> +int find_zero_ret = -1;
>> +int find_addr_ret = -1;
>> +
>> +static __u64
> 
> Let us 'long' instead of '__u64' to match uapi bpf.h.
> 
>> +check_vma(struct task_struct *task, struct vm_area_struct *vma,
>> +	  struct callback_ctx *data)
>> +{
>> +	if (vma->vm_file)
>> +		bpf_probe_read_kernel_str(d_iname, DNAME_INLINE_LEN - 1,
>> +					  vma->vm_file->f_path.dentry->d_iname);
>> +
>> +	/* check for VM_EXEC */
>> +	if (vma->vm_flags & VM_EXEC)
>> +		found_vm_exec = 1;
>> +
>> +	return 0;
>> +}
>> +
>> +SEC("kprobe/__x64_sys_getpgid")
> 
> The test will fail for non x86_64 architecture.
> I had some tweaks in test_probe_user.c. Please take a look.
> We can refactor to make tweaks in test_probe_user.c reusable
> by other files.

Good point. I will look into this. 

> 
>> +int handle_getpid(void)
>> +{
>> +	struct task_struct *task = bpf_get_current_task_btf();
>> +	struct callback_ctx data = {0};
>> +
>> +	if (task->pid != target_pid)
>> +		return 0;
>> +
>> +	find_addr_ret = bpf_find_vma(task, addr, check_vma, &data, 0);
>> +
>> +	/* this should return -ENOENT */
>> +	find_zero_ret = bpf_find_vma(task, 0, check_vma, &data, 0);
>> +	return 0;
>> +}
>> +
>> +SEC("perf_event")
>> +int handle_pe(void)
>> +{
>> +	struct task_struct *task = bpf_get_current_task_btf();
>> +	struct callback_ctx data = {0};
>> +
>> +	if (task->pid != target_pid)
>> +		return 0;
> 
> This is tricky. How do we guarantee task->pid == target_pid hit?
> This probably mostly okay in serial running mode. But it may
> become more challenging if test_progs is running in parallel mode?

This is on a per task perf_event, so it shouldn't hit other tasks. 

> 
>> +
>> +	find_addr_ret = bpf_find_vma(task, addr, check_vma, &data, 0);
>> +
>> +	/* In NMI, this should return -EBUSY, as the previous call is using
>> +	 * the irq_work.
>> +	 */
>> +	find_zero_ret = bpf_find_vma(task, 0, check_vma, &data, 0);
>> +	return 0;
>> +}
>> diff --git a/tools/testing/selftests/bpf/progs/find_vma_fail1.c b/tools/testing/selftests/bpf/progs/find_vma_fail1.c
>> new file mode 100644
>> index 0000000000000..d17bdcdf76f07
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/find_vma_fail1.c
>> @@ -0,0 +1,30 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2021 Facebook */
>> +#include "vmlinux.h"
>> +#include <bpf/bpf_helpers.h>
>> +
>> +char _license[] SEC("license") = "GPL";
>> +
>> +struct callback_ctx {
>> +	int dummy;
>> +};
>> +
>> +static __u64
> 
> __u64 => long
> 
>> +write_vma(struct task_struct *task, struct vm_area_struct *vma,
>> +	  struct callback_ctx *data)
>> +{
>> +	/* writing to vma, which is illegal */
>> +	vma->vm_flags |= 0x55;
>> +
>> +	return 0;
>> +}
>> +
>> +SEC("kprobe/__x64_sys_getpgid")
>> +int handle_getpid(void)
>> +{
>> +	struct task_struct *task = bpf_get_current_task_btf();
>> +	struct callback_ctx data = {0};
>> +
>> +	bpf_find_vma(task, 0, write_vma, &data, 0);
>> +	return 0;
>> +}
>> diff --git a/tools/testing/selftests/bpf/progs/find_vma_fail2.c b/tools/testing/selftests/bpf/progs/find_vma_fail2.c
>> new file mode 100644
>> index 0000000000000..079c4594c095d
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/find_vma_fail2.c
>> @@ -0,0 +1,30 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2021 Facebook */
>> +#include "vmlinux.h"
>> +#include <bpf/bpf_helpers.h>
>> +
>> +char _license[] SEC("license") = "GPL";
>> +
>> +struct callback_ctx {
>> +	int dummy;
>> +};
>> +
>> +static __u64
> 
> __u64 => long
> 
>> +write_task(struct task_struct *task, struct vm_area_struct *vma,
>> +	   struct callback_ctx *data)
>> +{
>> +	/* writing to task, which is illegal */
>> +	task->mm = NULL;
>> +
>> +	return 0;
>> +}
>> +
>> +SEC("kprobe/__x64_sys_getpgid")
>> +int handle_getpid(void)
>> +{
>> +	struct task_struct *task = bpf_get_current_task_btf();
>> +	struct callback_ctx data = {0};
>> +
>> +	bpf_find_vma(task, 0, write_task, &data, 0);
>> +	return 0;
>> +}

Re: [PATCH v2 bpf-next 2/2] selftests/bpf: add tests for bpf_find_vma

[Yonghong Song]

On 11/4/21 10:17 AM, Song Liu wrote:
> 
> 
>> On Nov 4, 2021, at 10:07 AM, Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 11/4/21 12:00 AM, Song Liu wrote:
>>> Add tests for bpf_find_vma in perf_event program and kprobe program. The
>>> perf_event program is triggered from NMI context, so the second call of
>>> bpf_find_vma() will return -EBUSY (irq_work busy). The kprobe program,
>>> on the other hand, does not have this constraint.
>>> Also add test for illegal writes to task or vma from the callback
>>> function. The verifier should reject both cases.
>>> Signed-off-by: Song Liu <songliubraving@fb.com>
> 
> [...]
> 
>>> +static void test_find_vma_pe(struct find_vma *skel)
>>> +{
>>> +	struct bpf_link *link = NULL;
>>> +	volatile int j = 0;
>>> +	int pfd = -1, i;
>>> +
>>> +	pfd = open_pe();
>>> +	if (pfd < 0) {
>>> +		if (pfd == -ENOENT || pfd == -EOPNOTSUPP) {
>>> +			printf("%s:SKIP:no PERF_COUNT_HW_CPU_CYCLES\n", __func__);
>>> +			test__skip();
>>> +		}
>>> +		if (!ASSERT_GE(pfd, 0, "perf_event_open"))
>>> +			goto cleanup;
>>> +	}
>>> +
>>> +	link = bpf_program__attach_perf_event(skel->progs.handle_pe, pfd);
>>> +	if (!ASSERT_OK_PTR(link, "attach_perf_event"))
>>> +		goto cleanup;
>>> +
>>> +	for (i = 0; i < 1000000; ++i)
>>> +		++j;
>>
>> Does this really work? Compiler could do
>>   j += 1000000;
> 
> I think compiler won't do it with volatile j?

Ya. volatile j should be fine.

> 
>>
>>> +
>>> +	test_and_reset_skel(skel, -EBUSY /* in nmi, irq_work is busy */);
>>> +cleanup:
>>> +	bpf_link__destroy(link);
>>> +	close(pfd);
>>> +	/* caller will clean up skel */
>>
>> Above comment is not needed. It should be clear from the code.
>>
[...]
>>
>>> +int handle_getpid(void)
>>> +{
>>> +	struct task_struct *task = bpf_get_current_task_btf();
>>> +	struct callback_ctx data = {0};
>>> +
>>> +	if (task->pid != target_pid)
>>> +		return 0;
>>> +
>>> +	find_addr_ret = bpf_find_vma(task, addr, check_vma, &data, 0);
>>> +
>>> +	/* this should return -ENOENT */
>>> +	find_zero_ret = bpf_find_vma(task, 0, check_vma, &data, 0);
>>> +	return 0;
>>> +}
>>> +
>>> +SEC("perf_event")
>>> +int handle_pe(void)
>>> +{
>>> +	struct task_struct *task = bpf_get_current_task_btf();
>>> +	struct callback_ctx data = {0};
>>> +
>>> +	if (task->pid != target_pid)
>>> +		return 0;
>>
>> This is tricky. How do we guarantee task->pid == target_pid hit?
>> This probably mostly okay in serial running mode. But it may
>> become more challenging if test_progs is running in parallel mode?
> 
> This is on a per task perf_event, so it shouldn't hit other tasks.

I see. we have the following parameters for perf_event open.

        pid == 0 and cpu == -1
               This measures the calling process/thread on any CPU.

So yes, we are fine then.

> 
>>
>>> +
>>> +	find_addr_ret = bpf_find_vma(task, addr, check_vma, &data, 0);
>>> +
>>> +	/* In NMI, this should return -EBUSY, as the previous call is using
>>> +	 * the irq_work.
>>> +	 */
>>> +	find_zero_ret = bpf_find_vma(task, 0, check_vma, &data, 0);
>>> +	return 0;
>>> +}
[...]

Re: [PATCH v2 bpf-next 2/2] selftests/bpf: add tests for bpf_find_vma

[Song Liu]

> On Nov 4, 2021, at 10:17 AM, Song Liu <songliubraving@fb.com> wrote:
> 
> 
> 
>> On Nov 4, 2021, at 10:07 AM, Yonghong Song <yhs@fb.com> wrote:
>> 
>> 
>> 
>> On 11/4/21 12:00 AM, Song Liu wrote:
>>> Add tests for bpf_find_vma in perf_event program and kprobe program. The
>>> perf_event program is triggered from NMI context, so the second call of
>>> bpf_find_vma() will return -EBUSY (irq_work busy). The kprobe program,
>>> on the other hand, does not have this constraint.
>>> Also add test for illegal writes to task or vma from the callback
>>> function. The verifier should reject both cases.
>>> Signed-off-by: Song Liu <songliubraving@fb.com>
> 
> [...]
> 
>>> +static void test_find_vma_pe(struct find_vma *skel)
>>> +{
>>> +	struct bpf_link *link = NULL;
>>> +	volatile int j = 0;
>>> +	int pfd = -1, i;
>>> +
>>> +	pfd = open_pe();
>>> +	if (pfd < 0) {
>>> +		if (pfd == -ENOENT || pfd == -EOPNOTSUPP) {
>>> +			printf("%s:SKIP:no PERF_COUNT_HW_CPU_CYCLES\n", __func__);
>>> +			test__skip();
>>> +		}
>>> +		if (!ASSERT_GE(pfd, 0, "perf_event_open"))
>>> +			goto cleanup;
>>> +	}
>>> +
>>> +	link = bpf_program__attach_perf_event(skel->progs.handle_pe, pfd);
>>> +	if (!ASSERT_OK_PTR(link, "attach_perf_event"))
>>> +		goto cleanup;
>>> +
>>> +	for (i = 0; i < 1000000; ++i)
>>> +		++j;
>> 
>> Does this really work? Compiler could do
>> j += 1000000;
> 
> I think compiler won't do it with volatile j? 
> 
>> 
>>> +
>>> +	test_and_reset_skel(skel, -EBUSY /* in nmi, irq_work is busy */);
>>> +cleanup:
>>> +	bpf_link__destroy(link);
>>> +	close(pfd);
>>> +	/* caller will clean up skel */
>> 
>> Above comment is not needed. It should be clear from the code.
>> 
>>> +}
>>> +
>>> +static void test_find_vma_kprobe(struct find_vma *skel)
>>> +{
>>> +	int err;
>>> +
>>> +	err = find_vma__attach(skel);
>>> +	if (!ASSERT_OK(err, "get_branch_snapshot__attach"))
>>> +		return;  /* caller will cleanup skel */
>>> +
>>> +	getpgid(skel->bss->target_pid);
>>> +	test_and_reset_skel(skel, -ENOENT /* could not find vma for ptr 0 */);
>>> +}
>>> +
>>> +static void test_illegal_write_vma(void)
>>> +{
>>> +	struct find_vma_fail1 *skel;
>>> +
>>> +	skel = find_vma_fail1__open_and_load();
>>> +	ASSERT_ERR_PTR(skel, "find_vma_fail1__open_and_load");
>>> +}
>>> +
>>> +static void test_illegal_write_task(void)
>>> +{
>>> +	struct find_vma_fail2 *skel;
>>> +
>>> +	skel = find_vma_fail2__open_and_load();
>>> +	ASSERT_ERR_PTR(skel, "find_vma_fail2__open_and_load");
>>> +}
>>> +
>>> +void serial_test_find_vma(void)
>>> +{
>>> +	struct find_vma *skel;
>>> +
>>> +	skel = find_vma__open_and_load();
>>> +	if (!ASSERT_OK_PTR(skel, "find_vma__open_and_load"))
>>> +		return;
>>> +
>>> +	skel->bss->target_pid = getpid();
>>> +	skel->bss->addr = (__u64)test_find_vma_pe;
>>> +
>>> +	test_find_vma_pe(skel);
>>> +	usleep(100000); /* allow the irq_work to finish */
>>> +	test_find_vma_kprobe(skel);
>>> +
>>> +	find_vma__destroy(skel);
>>> +	test_illegal_write_vma();
>>> +	test_illegal_write_task();
>>> +}
>>> diff --git a/tools/testing/selftests/bpf/progs/find_vma.c b/tools/testing/selftests/bpf/progs/find_vma.c
>>> new file mode 100644
>>> index 0000000000000..2776718a54e29
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/bpf/progs/find_vma.c
>>> @@ -0,0 +1,70 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/* Copyright (c) 2021 Facebook */
>>> +#include "vmlinux.h"
>>> +#include <bpf/bpf_helpers.h>
>>> +#include <bpf/bpf_tracing.h>
>>> +
>>> +char _license[] SEC("license") = "GPL";
>>> +
>>> +struct callback_ctx {
>>> +	int dummy;
>>> +};
>>> +
>>> +#define VM_EXEC		0x00000004
>>> +#define DNAME_INLINE_LEN 32
>>> +
>>> +pid_t target_pid = 0;
>>> +char d_iname[DNAME_INLINE_LEN] = {0};
>>> +__u32 found_vm_exec = 0;
>>> +__u64 addr = 0;
>>> +int find_zero_ret = -1;
>>> +int find_addr_ret = -1;
>>> +
>>> +static __u64
>> 
>> Let us 'long' instead of '__u64' to match uapi bpf.h.
>> 
>>> +check_vma(struct task_struct *task, struct vm_area_struct *vma,
>>> +	  struct callback_ctx *data)
>>> +{
>>> +	if (vma->vm_file)
>>> +		bpf_probe_read_kernel_str(d_iname, DNAME_INLINE_LEN - 1,
>>> +					  vma->vm_file->f_path.dentry->d_iname);
>>> +
>>> +	/* check for VM_EXEC */
>>> +	if (vma->vm_flags & VM_EXEC)
>>> +		found_vm_exec = 1;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +SEC("kprobe/__x64_sys_getpgid")
>> 
>> The test will fail for non x86_64 architecture.
>> I had some tweaks in test_probe_user.c. Please take a look.
>> We can refactor to make tweaks in test_probe_user.c reusable
>> by other files.
> 
> Good point. I will look into this. 

Actually, we can just use SEC("raw_tp/sys_enter") here. 

Thanks,
Song