linux-kernel - Re: [PATCH v11 bpf-next 5/7] bpf: Use btf_kfunc_id_set.remap logic for bpf_dynptr_from

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: <58833120-DD06-4024-B7F5-E255AC9261E6@fb.com>
Date: Thu, 30 Jan 2025 17:49:50 +0000
From: Song Liu <songliubraving@...a.com>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
CC: Song Liu <song@...nel.org>, bpf <bpf@...r.kernel.org>,
        Linux-Fsdevel
	<linux-fsdevel@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        LSM
 List <linux-security-module@...r.kernel.org>,
        Kernel Team
	<kernel-team@...a.com>,
        Andrii Nakryiko <andrii@...nel.org>, Eddy Z
	<eddyz87@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann
	<daniel@...earbox.net>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Alexander
 Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>, Jan
 Kara <jack@...e.cz>,
        KP Singh <kpsingh@...nel.org>,
        Matt Bobrowski
	<mattbobrowski@...gle.com>,
        Liam Wisehart <liamwisehart@...a.com>,
        Shankaran
 Gnanashanmugam <shankaran@...a.com>
Subject: Re: [PATCH v11 bpf-next 5/7] bpf: Use btf_kfunc_id_set.remap logic
 for bpf_dynptr_from_skb

Hi Alexei, 

Thanks for the review!

> On Jan 29, 2025, at 6:32 PM, Alexei Starovoitov <alexei.starovoitov@...il.com> wrote:

[...]

>> 
>> +BTF_ID_LIST(bpf_dynptr_from_skb_list)
>> +BTF_ID(func, bpf_dynptr_from_skb)
>> +BTF_ID(func, bpf_dynptr_from_skb_rdonly)
>> +
>> +static u32 bpf_kfunc_set_skb_remap(const struct bpf_prog *prog, u32 kfunc_id)
>> +{
>> +       if (kfunc_id != bpf_dynptr_from_skb_list[0])
>> +               return 0;
>> +
>> +       switch (resolve_prog_type(prog)) {
>> +       /* Program types only with direct read access go here! */
>> +       case BPF_PROG_TYPE_LWT_IN:
>> +       case BPF_PROG_TYPE_LWT_OUT:
>> +       case BPF_PROG_TYPE_LWT_SEG6LOCAL:
>> +       case BPF_PROG_TYPE_SK_REUSEPORT:
>> +       case BPF_PROG_TYPE_FLOW_DISSECTOR:
>> +       case BPF_PROG_TYPE_CGROUP_SKB:
> 
> This copy pastes the logic from may_access_direct_pkt_data(),
> so any future change to that helper would need to update
> this one as well.

We can probably improve this with some helpers/macros. 

> 
>> +               return bpf_dynptr_from_skb_list[1];
> 
> The [0] and [1] stuff is quite error prone.
> 
>> +
>> +       /* Program types with direct read + write access go here! */
>> +       case BPF_PROG_TYPE_SCHED_CLS:
>> +       case BPF_PROG_TYPE_SCHED_ACT:
>> +       case BPF_PROG_TYPE_XDP:
>> +       case BPF_PROG_TYPE_LWT_XMIT:
>> +       case BPF_PROG_TYPE_SK_SKB:
>> +       case BPF_PROG_TYPE_SK_MSG:
>> +       case BPF_PROG_TYPE_CGROUP_SOCKOPT:
>> +               return kfunc_id;
>> +
>> +       default:
>> +               break;
>> +       }
>> +       return bpf_dynptr_from_skb_list[1];
>> +}
>> +
>> static const struct btf_kfunc_id_set bpf_kfunc_set_skb = {
>>        .owner = THIS_MODULE,
>>        .set = &bpf_kfunc_check_set_skb,
>> +       .hidden_set = &bpf_kfunc_check_hidden_set_skb,
> 
> If I'm reading it correctly the hidden_set serves no additional purpose.
> It splits the set into two, but patch 4 just adds them together.

hidden_set does not have BTF_SET8_KFUNCS, so pahole will not export 
these kfuncs to vmlinux.h. 

> 
>> +       .remap = &bpf_kfunc_set_skb_remap,
> 
> I'm not a fan of callbacks in general.
> The makes everything harder to follow.

This motivation here is to move polymorphism logic from verifier
core to kfuncs owners. I guess we will need some callback to 
achieve this goal. Of course, we don't have to do it in this set. 
 

> For all these reasons I don't like this approach.
> This "generality" doesn't make it cleaner or easier to extend.
> For the patch 6... just repeat what specialize_kfunc()
> currently does for dynptr ?

Yes, specialize_kfunc() can handle this. But we will need to use
d_inode_locked_hooks from 6/7 in specialize_kfunc(). It works, 
but it is not clean (to me). 

I will revise this set so that the polymorphism logic in handled
in specialize_kfunc(). For longer term, maybe we should discuss 
"move some logic from verifier core to kfuncs" in the upcoming 
LSF/MM/BPF? 

Thanks,
Song