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

Le 19/03/2021 à 17:55, Martin KaFai Lau a écrit :

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

quoted

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?

Yes, the idea is to allow the BPF program to redirect incoming 
connections for 0.0.0.0:1234 to a specific IP address such as 
127.0.12.34:1234 or any other combinaison with a binding not done based 
on a predefined socket file descriptor but based on a listening IP 
address for a socket.

See linked discussion in the original message

quoted

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

I was thinking that perhaps a different helper method which would take 
IPPROTO_TCP or IPPROTO_UDP parameter would be better suited. it would 
avoid BPF code such as :

     switch(ctx->protocol){
         case IPPROTO_TCP:
             sk = bpf_sk_lookup_tcp(ctx, &tuple, tuple_size, -1, 0);
             break;
         case IPPROTO_UDP:
             sk = bpf_sk_lookup_udp(ctx, &tuple, tuple_size, -1, 0);
             break;
         default:
             return SK_PASS;
     }

But then there is the limit of 5 arguments, isn't it, so perhaps the 
_tcp/_udp functions are not such a bad idea after all.

I saw already that the same helper functions could be given different 
implementations. And if there is no way to have more than 5 arguments 
then this is probably better to reuse the same helper function name and 
signature.

Thank you