On Thu, Jun 14, 2018 at 09:44:52AM -0700, John Fastabend wrote:
> Per the note in the TLS ULP (which is actually a generic statement
> regarding ULPs)
>
> /* The TLS ulp is currently supported only for TCP sockets
> * in ESTABLISHED state.
> * Supporting sockets in LISTEN state will require us
> * to modify the accept implementation to clone rather then
> * share the ulp context.
> */
Can you explain how that apply to bpf_tcp ulp?
My understanding is the "ulp context" referred in TLS ulp is
the tls_context stored in icsk_ulp_data but I don't see bpf_tcp's
ulp is using icsk_ulp_data.
Others LGTM.
>
> After this patch we only allow socks that are in ESTABLISHED state or
> are being added via a sock_ops event that is transitioning into an
> ESTABLISHED state. By allowing sock_ops events we allow users to
> manage sockmaps directly from sock ops programs. The two supported
> sock_ops ops are BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB and
> BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB.
>
> >From the userspace BPF update API the sock lock is also taken now
> to ensure we don't race with state changes after the ESTABLISHED
> check. The BPF program sock ops hook already has the sock lock
> taken.
>
> Also tested with 'netserver -6' and 'netperf -H [IPv6]' as well as
> 'netperf -H [IPv4]'.
>
> Reported-by: Eric Dumazet <eduma...@google.com>
> Signed-off-by: John Fastabend <john.fastab...@gmail.com>
> ---
> 0 files changed
>
> diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
> index f6dd4cd..f1ab52d 100644
> --- a/kernel/bpf/sockmap.c
> +++ b/kernel/bpf/sockmap.c
> @@ -1976,13 +1976,20 @@ static int sock_map_update_elem(struct bpf_map *map,
> return -EINVAL;
> }
>
> + lock_sock(skops.sk);
> + /* ULPs are currently supported only for TCP sockets in ESTABLISHED
> + * state.
> + */
> if (skops.sk->sk_type != SOCK_STREAM ||
> - skops.sk->sk_protocol != IPPROTO_TCP) {
> - fput(socket->file);
> - return -EOPNOTSUPP;
> + skops.sk->sk_protocol != IPPROTO_TCP ||
> + skops.sk->sk_state != TCP_ESTABLISHED) {
> + err = -EOPNOTSUPP;
> + goto out;
> }
>
> err = sock_map_ctx_update_elem(&skops, map, key, flags);
> +out:
> + release_sock(skops.sk);
> fput(socket->file);
> return err;
> }
> @@ -2247,10 +2254,6 @@ static int sock_hash_ctx_update_elem(struct
> bpf_sock_ops_kern *skops,
>
> sock = skops->sk;
>
> - if (sock->sk_type != SOCK_STREAM ||
> - sock->sk_protocol != IPPROTO_TCP)
> - return -EOPNOTSUPP;
> -
> if (unlikely(map_flags > BPF_EXIST))
> return -EINVAL;
>
> @@ -2338,7 +2341,20 @@ static int sock_hash_update_elem(struct bpf_map *map,
> return -EINVAL;
> }
>
> + lock_sock(skops.sk);
> + /* ULPs are currently supported only for TCP sockets in ESTABLISHED
> + * state.
> + */
> + if (skops.sk->sk_type != SOCK_STREAM ||
> + skops.sk->sk_protocol != IPPROTO_TCP ||
> + skops.sk->sk_state != TCP_ESTABLISHED) {
> + err = -EOPNOTSUPP;
> + goto out;
> + }
> +
> err = sock_hash_ctx_update_elem(&skops, map, key, flags);
> +out:
> + release_sock(skops.sk);
> fput(socket->file);
> return err;
> }
> @@ -2423,10 +2439,19 @@ struct sock *__sock_hash_lookup_elem(struct bpf_map
> *map, void *key)
> .map_delete_elem = sock_hash_delete_elem,
> };
>
> +static bool bpf_is_valid_sock(struct bpf_sock_ops_kern *ops)
> +{
> + return ops->op == BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB ||
> + ops->op == BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB;
> +}
> +
> BPF_CALL_4(bpf_sock_map_update, struct bpf_sock_ops_kern *, bpf_sock,
> struct bpf_map *, map, void *, key, u64, flags)
> {
> WARN_ON_ONCE(!rcu_read_lock_held());
> +
> + if (!bpf_is_valid_sock(bpf_sock))
> + return -EOPNOTSUPP;
> return sock_map_ctx_update_elem(bpf_sock, map, key, flags);
> }
>
> @@ -2445,6 +2470,9 @@ struct sock *__sock_hash_lookup_elem(struct bpf_map
> *map, void *key)
> struct bpf_map *, map, void *, key, u64, flags)
> {
> WARN_ON_ONCE(!rcu_read_lock_held());
> +
> + if (!bpf_is_valid_sock(bpf_sock))
> + return -EOPNOTSUPP;
> return sock_hash_ctx_update_elem(bpf_sock, map, key, flags);
> }
>
>
