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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> 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.

Thanks for finding this case!

Song



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux