Hello everyone,
Background discussion:
https://lore.kernel.org/bpf/CAADnVQJnX-+9u--px_VnhrMTPB=O9Y0LH9T7RJbqzfLchbUFvg@mail.gmail.com/
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?
- 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?
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.
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/
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.
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
On Fri, Mar 19, 2021 at 06:05:20PM +0100, Shanti Lombard née Bouchez-Mongardé wrote:
Le 19/03/2021 à 17:55, Martin KaFai Lau a écrit :
quoted
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
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.
For TCP, it is only for lhash lookup, right?
quoted
quoted
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.