netdev - Re: [PATCH bpf-next v2 1/2] bpf: Add ifindex to bpf_sk

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] [day] [month] [year] [list]

Message-ID: <87y262hd5u.fsf@cloudflare.com>
Date:   Fri, 05 Nov 2021 15:47:57 +0100
From:   Jakub Sitnicki <jakub@...udflare.com>
To:     Yonghong Song <yhs@...com>,
        Mark Pashmfouroush <markpash@...udflare.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>, kernel-team@...udflare.com,
        netdev@...r.kernel.org, bpf@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v2 1/2] bpf: Add ifindex to bpf_sk_lookup

On Thu, Nov 04, 2021 at 07:06 PM CET, 'Yonghong Song' via kernel-team+notifications wrote:
> On 11/4/21 5:23 AM, Mark Pashmfouroush wrote:
>> It may be helpful to have access to the ifindex during bpf socket
>> lookup. An example may be to scope certain socket lookup logic to
>> specific interfaces, i.e. an interface may be made exempt from custom
>> lookup code.
>> Add the ifindex of the arriving connection to the bpf_sk_lookup API.
>> Signed-off-by: Mark Pashmfouroush <markpash@...udflare.com>
>> diff --git a/include/linux/filter.h b/include/linux/filter.h
>> index 24b7ed2677af..0012a5176a32 100644
>> --- a/include/linux/filter.h
>> +++ b/include/linux/filter.h
>> @@ -1374,6 +1374,7 @@ struct bpf_sk_lookup_kern {
>>   		const struct in6_addr *daddr;
>>   	} v6;
>>   	struct sock	*selected_sk;
>> +	u32		ifindex;
>
> In struct __sk_buff, we have two ifindex related fields:
>
>         __u32 ingress_ifindex;
>         __u32 ifindex;
>
> Does newly-added ifindex corresponds to skb->ingress_ifindex or
> skb->ifindex? From comments:
>   > +	__u32 ifindex;		/* The arriving interface. Determined by inet_iif. */
>
> looks like it corresponds to ingress? Should be use the name
> ingress_ifindex to be consistent with __sk_buff?
>

On ingress these two (skb->skb_iif and skb->dev-ifindex) are the same,
if I read the code correctly [1].

That said, I agree that ingress_ifindex would be less ambiguous (iif ->
ingress interface, can't get that wrong).

Also, as Yonghong points out __sk_buff and xdp_md context objects
already use this identifier for the same bit of information, so it will
be less of surprise.

[1] https://elixir.bootlin.com/linux/latest/source/net/core/dev.c#L5258

[...]