Re: Design for sk_lookup helper function in context of sk_lookup hook

On Wed, Mar 17, 2021 at 10:04:18AM +0100, Shanti Lombard née Bouchez-Mongardé wrote:

Hello everyone,

Background discussion: https://lore.kernel.org/bpf/CAADnVQJnX-+9u--px_VnhrMTPB=O9Y0LH9T7RJbqzfLchbUFvg@mail.gmail.com/ (local)

In a previous e-mail I was explaining that sk_lookup BPF programs had no way
to lookup existing sockets in kernel space. The sockets have to be provided
by userspace, and the userspace has to find a way to get them and update
them, which in some circumstances can be difficult. I'm working on a patch
to make socket lookup available to BPF programs in the context of the
sk_lookup hook.

There is also two helper function bpf_sk_lokup_tcp and bpf_sk_lookup_udp
which exists but are not available in the context of sk_lookup hooks. Making
them available in this context is not very difficult (just configure it in
net/core/filter.c) but those helpers will call back BPF code as part of the
socket lookup potentially causing an infinite loop. We need to find a way to
perform socket lookup but disable the BPF hook while doing so.

Around all this, I have a few design questions :

Q1: How do we prevent socket lookup from triggering BPF sk_lookup causing a
loop?

The bpf_sk_lookup_(tcp|udp) will be called from the BPF_PROG_TYPE_SK_LOOKUP program?

- Solution A: We add a flag to the existing inet_lookup exported function
(and similarly for inet6, udp4 and udp6). The INET_LOOKUP_SKIP_BPF_SK_LOOKUP
flag, when set, would prevent BPF sk_lookup from happening. It also requires
modifying every location making use of those functions.

- Solution B: We export a new symbol in inet_hashtables, a wrapper around
static function inet_lhash2_lookup for inet4 and similar functions for inet6
and udp4/6. Looking up specific IP/port and if not found looking up for
INADDR_ANY could be done in the helper function in net/core/filters.c or in
the BPF program.

Q2: Should we reuse the bpf_sk_lokup_tcp and bpf_sk_lookup_udp helper
functions or create new ones?

If the args passing to the bpf_sk_lookup_(tcp|udp) is the same,
it makes sense to reuse the same BPF_FUNC_sk_lookup_*.
The actual helper implementation could be different though.
Look at bpf_xdp_sk_lookup_tcp_proto and bpf_sk_lookup_tcp_proto.

For solution A above, the helper functions can be reused almose identically,
just adding a flag or boolean argument to tell if we are in a sk_lookup
program or not. In solution B is preferred, them perhaps it would make sense
to expose the new raw lookup function created, and the BPF program would be
responsible for falling back to INADDR_ANY if the specific socket is not
found. It adds more power to the BPF program in this case but requires to
create a new helper function.

I was going with Solution A abd identical function names, but as I am
touching the code it seems that maybe solution B with a new helper function
could be better. I'm open to ideas.

Thank you.

PS: please include me in replies if you are responding only to the netdev
mailing list as I'm not part of it. I'm subscribed to bpf.