| Message ID | 20200122130549.832236-7-jakub@cloudflare.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | Extend SOCKMAP to store listening sockets | expand |
On Wed, 22 Jan 2020 at 13:06, Jakub Sitnicki <jakub@cloudflare.com> wrote: > @@ -352,7 +376,15 @@ static int sock_map_update_common(struct bpf_map *map, u32 idx, > if (!link) > return -ENOMEM; > > - ret = sock_map_link(map, &stab->progs, sk); > + /* Only established or almost established sockets leaving > + * SYN_RECV state need to hold refs to parser/verdict progs > + * and have their sk_data_ready and sk_write_space callbacks > + * overridden. > + */ > + if (sk->sk_state == TCP_LISTEN) > + ret = sock_map_link_no_progs(map, sk); > + else > + ret = sock_map_link(map, &stab->progs, sk); Could you use sock_map_redirect_okay from the previous patch here instead of checking for TCP_LISTEN?
On Wed, Jan 22, 2020 at 05:24 PM CET, Lorenz Bauer wrote: > On Wed, 22 Jan 2020 at 13:06, Jakub Sitnicki <jakub@cloudflare.com> wrote: >> @@ -352,7 +376,15 @@ static int sock_map_update_common(struct bpf_map *map, u32 idx, >> if (!link) >> return -ENOMEM; >> >> - ret = sock_map_link(map, &stab->progs, sk); >> + /* Only established or almost established sockets leaving >> + * SYN_RECV state need to hold refs to parser/verdict progs >> + * and have their sk_data_ready and sk_write_space callbacks >> + * overridden. >> + */ >> + if (sk->sk_state == TCP_LISTEN) >> + ret = sock_map_link_no_progs(map, sk); >> + else >> + ret = sock_map_link(map, &stab->progs, sk); > > Could you use sock_map_redirect_okay from the previous patch here > instead of checking for TCP_LISTEN? Makes sense. Queuing it for next iteration if more things pile up. To give the rest of reviewers some context - Lorenz started looking at adding bare-bones support for UDP to sockmap. Bare-bones meaning that UDP sockets could be inserted/deleted into/from sockmap, but not spliced with sockmap. Being consistent about how we check if a socket can be used for splicing will make extending it for UDP easier. Thanks, -jkbs
On Wed, Jan 22, 2020 at 02:05:43PM +0100, Jakub Sitnicki wrote: > Now that sockmap can hold listening sockets, when setting up the psock we > will (i) grab references to verdict/parser progs, and (2) override socket > upcalls sk_data_ready and sk_write_space. > > We cannot redirect to listening sockets so we don't need to link the socket > to the BPF progs, but more importantly we don't want the listening socket > to have overridden upcalls because they would get inherited by child > sockets cloned from it. > > Introduce a separate initialization path for listening sockets that does > not change the upcalls and ignores the BPF progs. The change makes sense to me. Acked-by: Martin KaFai Lau <kafai@fb.com>
diff --git a/net/core/sock_map.c b/net/core/sock_map.c index 97bdceb29f09..439f1e0b995e 100644 --- a/net/core/sock_map.c +++ b/net/core/sock_map.c @@ -228,6 +228,30 @@ static int sock_map_link(struct bpf_map *map, struct sk_psock_progs *progs, return ret; } +static int sock_map_link_no_progs(struct bpf_map *map, struct sock *sk) +{ + struct sk_psock *psock; + int ret; + + psock = sk_psock_get_checked(sk); + if (IS_ERR(psock)) + return PTR_ERR(psock); + + if (psock) { + tcp_bpf_reinit(sk); + return 0; + } + + psock = sk_psock_init(sk, map->numa_node); + if (!psock) + return -ENOMEM; + + ret = tcp_bpf_init(sk); + if (ret < 0) + sk_psock_put(sk, psock); + return ret; +} + static void sock_map_free(struct bpf_map *map) { struct bpf_stab *stab = container_of(map, struct bpf_stab, map); @@ -352,7 +376,15 @@ static int sock_map_update_common(struct bpf_map *map, u32 idx, if (!link) return -ENOMEM; - ret = sock_map_link(map, &stab->progs, sk); + /* Only established or almost established sockets leaving + * SYN_RECV state need to hold refs to parser/verdict progs + * and have their sk_data_ready and sk_write_space callbacks + * overridden. + */ + if (sk->sk_state == TCP_LISTEN) + ret = sock_map_link_no_progs(map, sk); + else + ret = sock_map_link(map, &stab->progs, sk); if (ret < 0) goto out_free;
Now that sockmap can hold listening sockets, when setting up the psock we will (i) grab references to verdict/parser progs, and (2) override socket upcalls sk_data_ready and sk_write_space. We cannot redirect to listening sockets so we don't need to link the socket to the BPF progs, but more importantly we don't want the listening socket to have overridden upcalls because they would get inherited by child sockets cloned from it. Introduce a separate initialization path for listening sockets that does not change the upcalls and ignores the BPF progs. Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> --- net/core/sock_map.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-)