Jakub Sitnicki wrote:
> In order for sockmap type to become a generic collection for storing TCP
> sockets we need to loosen the checks during map update, while tightening
> the checks in redirect helpers.
>
> Currently sockmap requires the TCP socket to be in established state (or
> transitioning out of SYN_RECV into established state when done from BPF),
> which prevents inserting listening sockets.
>
> Change the update pre-checks so that the socket can also be in listening
> state. If the state is not white-listed, return -EINVAL to be consistent
> with REUSEPORT_SOCKARRY map type.
>
> Since it doesn't make sense to redirect with sockmap to listening sockets,
> add appropriate socket state checks to BPF redirect helpers too.
>
> Signed-off-by: Jakub Sitnicki <jakub@xxxxxxxxxxxxxx>
> ---
> net/core/sock_map.c | 46 ++++++++++++++++++++-----
> tools/testing/selftests/bpf/test_maps.c | 6 +---
> 2 files changed, 39 insertions(+), 13 deletions(-)
>
> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index eb114ee419b6..99daea502508 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -396,6 +396,23 @@ static bool sock_map_sk_is_suitable(const struct sock *sk)
> sk->sk_protocol == IPPROTO_TCP;
> }
>
> +/* Is sock in a state that allows inserting into the map?
> + * SYN_RECV is needed for updates on BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB.
> + */
> +static bool sock_map_update_okay(const struct sock *sk)
> +{
> + return (1 << sk->sk_state) & (TCPF_ESTABLISHED |
> + TCPF_SYN_RECV |
> + TCPF_LISTEN);
> +}
> +
> +/* Is sock in a state that allows redirecting into it? */
> +static bool sock_map_redirect_okay(const struct sock *sk)
> +{
> + return (1 << sk->sk_state) & (TCPF_ESTABLISHED |
> + TCPF_SYN_RECV);
> +}
> +
> static int sock_map_update_elem(struct bpf_map *map, void *key,
> void *value, u64 flags)
> {
> @@ -413,11 +430,14 @@ static int sock_map_update_elem(struct bpf_map *map, void *key,
> ret = -EINVAL;
> goto out;
> }
> - if (!sock_map_sk_is_suitable(sk) ||
> - sk->sk_state != TCP_ESTABLISHED) {
> + if (!sock_map_sk_is_suitable(sk)) {
> ret = -EOPNOTSUPP;
> goto out;
> }
> + if (!sock_map_update_okay(sk)) {
> + ret = -EINVAL;
> + goto out;
> + }
I nit but seeing we need a v3 anyways. How about consolidating
this state checks into sock_map_sk_is_suitable() so we don't have
multiple if branches or this '|| TCP_ESTABLISHED' like we do now.
>
> sock_map_sk_acquire(sk);
> ret = sock_map_update_common(map, idx, sk, flags);
> @@ -433,6 +453,7 @@ BPF_CALL_4(bpf_sock_map_update, struct bpf_sock_ops_kern *, sops,
> WARN_ON_ONCE(!rcu_read_lock_held());
>
> if (likely(sock_map_sk_is_suitable(sops->sk) &&
> + sock_map_update_okay(sops->sk) &&
> sock_map_op_okay(sops)))
> return sock_map_update_common(map, *(u32 *)key, sops->sk,
> flags);
> @@ -454,13 +475,17 @@ BPF_CALL_4(bpf_sk_redirect_map, struct sk_buff *, skb,
> struct bpf_map *, map, u32, key, u64, flags)
> {
> struct tcp_skb_cb *tcb = TCP_SKB_CB(skb);
> + struct sock *sk;
>
> if (unlikely(flags & ~(BPF_F_INGRESS)))
> return SK_DROP;
> - tcb->bpf.flags = flags;
> - tcb->bpf.sk_redir = __sock_map_lookup_elem(map, key);
> - if (!tcb->bpf.sk_redir)
> +
> + sk = __sock_map_lookup_elem(map, key);
> + if (!sk || !sock_map_redirect_okay(sk))
> return SK_DROP;
unlikely(!sock_map_redirect_okay)? Or perhaps unlikely the entire case,
if (unlikely(!sk || !sock_map_redirect_okay(sk)). I think users should
know if the sk is a valid sock or not and this is just catching the
error case. Any opinion?
Otherwise looks good.
