Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
References: <20211104122304.962104-1-markpash@cloudflare.com>
 <20211104122304.962104-2-markpash@cloudflare.com>
 <32332bb4-1848-0280-9482-5189ab912b02@fb.com>
User-agent: mu4e 1.1.0; emacs 27.2
From:   Jakub Sitnicki <jakub@cloudflare.com>
To:     Yonghong Song <yhs@fb.com>,
        Mark Pashmfouroush <markpash@cloudflare.com>
Cc:     Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
        David Ahern <dsahern@kernel.org>, kernel-team@cloudflare.com,
        netdev@vger.kernel.org, bpf@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH bpf-next v2 1/2] bpf: Add ifindex to bpf_sk_lookup
In-reply-to: <32332bb4-1848-0280-9482-5189ab912b02@fb.com>
Date:   Fri, 05 Nov 2021 15:47:57 +0100
Message-ID: <87y262hd5u.fsf@cloudflare.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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

[...]