Re: [PATCH v2 bpf-next 1/4] bpf: introduce task_vma bpf_iter

On Dec 16, 2020, at 4:34 PM, Andrii Nakryiko [off-list ref] wrote:

On Tue, Dec 15, 2020 at 3:37 PM Song Liu [off-list ref] wrote:

quoted

Introduce task_vma bpf_iter to print memory information of a process. It
can be used to print customized information similar to /proc/<pid>/maps.

task_vma iterator releases mmap_lock before calling the BPF program.
Therefore, we cannot pass vm_area_struct directly to the BPF program. A
new __vm_area_struct is introduced to keep key information of a vma. On
each iteration, task_vma gathers information in __vm_area_struct and
passes it to the BPF program.

If the vma maps to a file, task_vma also holds a reference to the file
while calling the BPF program.

Signed-off-by: Song Liu <redacted>
---
include/linux/bpf.h    |   2 +-
kernel/bpf/task_iter.c | 205 ++++++++++++++++++++++++++++++++++++++++-
2 files changed, 205 insertions(+), 2 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 07cb5d15e7439..49dd1e29c8118 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h

@@ -1325,7 +1325,7 @@ enum bpf_iter_feature {
       BPF_ITER_RESCHED        = BIT(0),

};

-#define BPF_ITER_CTX_ARG_MAX 2
+#define BPF_ITER_CTX_ARG_MAX 3
struct bpf_iter_reg {
       const char *target;
       bpf_iter_attach_target_t attach_target;

diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index 0458a40edf10a..15a066b442f75 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c

@@ -304,9 +304,183 @@ static const struct seq_operations task_file_seq_ops = {
       .show   = task_file_seq_show,

};

+/*
+ * Key information from vm_area_struct. We need this because we cannot
+ * assume the vm_area_struct is still valid after each iteration.
+ */
+struct __vm_area_struct {
+       __u64 start;
+       __u64 end;
+       __u64 flags;
+       __u64 pgoff;

I'd keep the original names of the fields (vm_start, vm_end, etc).

I thought about the names. Unlike the kernel fs/mm code, where there
are many different start/end/offset/flags, the prefix doesn't seem to 
be helpful in the BPF programs. In fact, it is probably easier for 
the developers to differentiate __vm_area_struct->start and 
vm_area_struct->vm_start.

Also, we have bpf_iter__task_vma->file, which is the same as 
vm_area_struct->vm_file. If we prefix __vm_area_struct members, it 
will be a little weird to name it either "vm_file" or "file".

Given these reasons, I decided to not have vm_ prefix. Does this make
sense? 

But
there are some more fields which seem useful, like vm_page_prot,
vm_mm, etc.

vm_page_prot doesn't really provide extra information than vm_flags. 
Please refer to mm/mmap.c vm_get_page_prot(). 

We have the vm_mm in task->mm, so no need to add it to __vm_area_struct.

It's quite unfortunate, actually, that bpf_iter program doesn't get
access to the real vm_area_struct, because it won't be able to do much
beyond using fields that we pre-defined here. E.g., there could be
interesting things to do with vm_mm, but unfortunately it won't be
possible.

Is there any way to still provide access to the original
vm_area_struct and let BPF programs use BTF magic to follow all those
pointers (like vm_mm) safely?

We hold a reference to task, and the task holds a reference to task->mm,
so we can safely probe_read information in mm_struct, like the page 
table. 

Thanks,
Song