On Tue, Jan 05, 2021 at 07:38:08PM +0000, Song Liu wrote:

quoted

On Jan 5, 2021, at 9:27 AM, Alexei Starovoitov [off-list ref] wrote:

On Tue, Jan 5, 2021 at 9:11 AM Song Liu [off-list ref] wrote:

quoted

quoted

On Jan 5, 2021, at 8:27 AM, Alexei Starovoitov [off-list ref] wrote:

On Mon, Jan 4, 2021 at 9:47 PM Song Liu [off-list ref] wrote:

quoted

quoted

On Jan 4, 2021, at 5:46 PM, Alexei Starovoitov [off-list ref] wrote:

On Fri, Dec 18, 2020 at 05:23:25PM +0000, Song Liu wrote:

quoted

quoted

On Dec 18, 2020, at 8:38 AM, Yonghong Song [off-list ref] wrote:



On 12/17/20 9:23 PM, Alexei Starovoitov wrote:

quoted

On Thu, Dec 17, 2020 at 8:33 PM Song Liu [off-list ref] wrote:

quoted

quoted

ahh. I missed that. Makes sense.
vm_file needs to be accurate, but vm_area_struct should be accessed as ptr_to_btf_id.

Passing pointer of vm_area_struct into BPF will be tricky. For example, shall we
allow the user to access vma->vm_file? IIUC, with ptr_to_btf_id the verifier will
allow access of vma->vm_file as a valid pointer to struct file. However, since the
vma might be freed, vma->vm_file could point to random data.

I don't think so. The proposed patch will do get_file() on it.
There is actually no need to assign it into a different variable.
Accessing it via vma->vm_file is safe and cleaner.

I did not check the code but do you have scenarios where vma is freed but old vma->vm_file is not freed due to reference counting, but
freed vma area is reused so vma->vm_file could be garbage?

AFAIK, once we unlock mmap_sem, the vma could be freed and reused. I guess ptr_to_btf_id
or probe_read would not help with this?

Theoretically we can hack the verifier to treat some ptr_to_btf_id as "less
valid" than the other ptr_to_btf_id, but the user experience will not be great.
Reading such bpf prog will not be obvious. I think it's better to run bpf prog
in mmap_lock then and let it access vma->vm_file. After prog finishes the iter
bit can do if (mmap_lock_is_contended()) before iterating. That will deliver
better performance too. Instead of task_vma_seq_get_next() doing
mmap_lock/unlock at every vma. No need for get_file() either. And no
__vm_area_struct exposure.

I think there might be concern calling BPF program with mmap_lock, especially that
the program is sleepable (for bpf_d_path). It shouldn't be a problem for common
cases, but I am not 100% sure for corner cases (many instructions in BPF + sleep).
Current version is designed to be very safe for the workload, which might be too
conservative.

I know and I agree with all that, but how do you propose to fix the
vm_file concern
without holding the lock while prog is running? I couldn't come up with a way.

I guess the gap here is that I don't see why __vm_area_struct exposure is an issue.
It is similar to __sk_buff, and simpler (though we had more reasons to introduce
__sk_buff back when there wasn't BTF).

If we can accept __vm_area_struct, current version should work, as it doesn't have
problem with vm_file

True. The problem with __vm_area_struct is that it is a hard coded
uapi with little to none
extensibility. In this form vma iterator is not really useful beyond
the example in selftest.

With __vm_area_struct, we can still probe_read the page table, so we can 
cover more use cases than the selftest. But I agree that it is not as
extensible as feeding real vm_area_struct with BTF to the BPF program. 

Another confusing thing with __vm_area_struct is vm_flags field.
It's copied directly. As __vm_area_struct->flags this field is uapi field,
but its contents are kernel internal. We avoided such corner cases in the past.
When flags field is copied into uapi the kernel internal flags are encoded
and exposed as separate uapi flags. That was the case with
BPF_TCP_* flags. If we have to do this with vm_flags (VM_EXEC, etc) that
might kill the patchset due to abi concerns.