> On Nov 5, 2021, at 2:16 PM, Alexei Starovoitov <ast@xxxxxx> wrote: > > On 11/5/21 2:11 PM, Song Liu wrote: >>> On Nov 5, 2021, at 8:23 AM, Hengqi Chen <hengqi.chen@xxxxxxxxx> wrote: >>> >>> Hi, Song >>> >>> On 2021/11/5 5:31 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. >>>> >>>> Reported-by: kernel test robot <lkp@xxxxxxxxx> >>>> Signed-off-by: Song Liu <songliubraving@xxxxxx> >>>> --- >>> >>> [...] >>> >>>> >>>> -BTF_ID_LIST(btf_task_file_ids) >>>> -BTF_ID(struct, file) >>>> -BTF_ID(struct, vm_area_struct) >>>> - >>>> static const struct bpf_iter_seq_info task_seq_info = { >>>> .seq_ops = &task_seq_ops, >>>> .init_seq_private = init_seq_pidns, >>>> @@ -586,9 +583,74 @@ 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); >>>> + >>> >>> I found that when a BPF program attach to security_file_open which is in >>> the bpf_d_path helper's allowlist, the bpf_d_path helper is also allowed >>> to be called inside the callback function. So we can have this in callback >>> function: >>> >>> bpf_d_path(&vma->vm_file->f_path, path, sizeof(path)); >>> >>> >>> I wonder whether there is a guarantee that vma->vm_file will never be null, >>> as you said in the commit message, a backing file. >> I don't think we can guarantee vma->vm_file never be NULL here, so this is >> a real problem. Let me see how to fix it. > > It's unrelated. There was a separate thread about this. I see. I will not fix it here then. Thanks, Song