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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <deed01c3-83aa-9d18-b803-ba0b427c58af@fb.com>
Date:   Fri, 5 Nov 2021 14:16:35 -0700
From:   Alexei Starovoitov <ast@...com>
To:     Song Liu <songliubraving@...com>,
        Hengqi Chen <hengqi.chen@...il.com>
CC:     bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Kernel Team <Kernel-team@...com>,
        KP Singh <kpsingh@...nel.org>,
        kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v4 bpf-next 1/2] bpf: introduce helper bpf_find_vma

On 11/5/21 2:11 PM, Song Liu wrote:
> 
> 
>> On Nov 5, 2021, at 8:23 AM, Hengqi Chen <hengqi.chen@...il.com> 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@...el.com>
>>> Signed-off-by: Song Liu <songliubraving@...com>
>>> ---
>>
>> [...]
>>
>>>
>>> -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.