On 06/30/2018 03:51 PM, John Fastabend wrote:
> The current code, in the error path of sock_hash_ctx_update_elem,
> checks if the sock has a psock in the user data and if so decrements
> the reference count of the psock. However, if the error happens early
> in the error path we may have never incremented the psock reference
> count and if the psock exists because the sock is in another map then
> we may inadvertently decrement the reference count.
>
> Fix this by making the error path only call smap_release_sock if the
> error happens after the increment.
>
> Reported-by: [email protected]
> Fixes: 81110384441a ("bpf: sockmap, add hash map support")
> Signed-off-by: John Fastabend <[email protected]>
> ---
> 0 files changed
>
> diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
> index 4fc2cb1..63fb047 100644
> --- a/kernel/bpf/sockmap.c
> +++ b/kernel/bpf/sockmap.c
> @@ -1896,7 +1896,7 @@ static int __sock_map_ctx_update_elem(struct bpf_map
> *map,
> e = kzalloc(sizeof(*e), GFP_ATOMIC | __GFP_NOWARN);
> if (!e) {
> err = -ENOMEM;
> - goto out_progs;
> + goto out_free;
> }
> }
>
> @@ -2324,7 +2324,12 @@ static int sock_hash_ctx_update_elem(struct
> bpf_sock_ops_kern *skops,
> if (err)
> goto err;
>
> - /* bpf_map_update_elem() can be called in_irq() */
> + psock = smap_psock_sk(sock);
> + if (unlikely(!psock)) {
> + err = -EINVAL;
> + goto err;
> + }
Is an error even possible at this point? If __sock_map_ctx_update_elem()
succeeds,
we either allocated and linked a new psock to the sock or we inc'ed the existing
one's refcount. From my reading it seems we should always succeed the subsequent
smap_psock_sk(). If we would have failed here in between it would mean we'd have
a refcount imbalance somewhere?
> +
> raw_spin_lock_bh(&b->lock);
> l_old = lookup_elem_raw(head, hash, key, key_size);
> if (l_old && map_flags == BPF_NOEXIST) {
> @@ -2342,12 +2347,6 @@ static int sock_hash_ctx_update_elem(struct
> bpf_sock_ops_kern *skops,
> goto bucket_err;
> }
>
> - psock = smap_psock_sk(sock);
> - if (unlikely(!psock)) {
> - err = -EINVAL;
> - goto bucket_err;
> - }
> -
> rcu_assign_pointer(e->hash_link, l_new);
> rcu_assign_pointer(e->htab,
> container_of(map, struct bpf_htab, map));
> @@ -2370,12 +2369,10 @@ static int sock_hash_ctx_update_elem(struct
> bpf_sock_ops_kern *skops,
> raw_spin_unlock_bh(&b->lock);
> return 0;
> bucket_err:
> + smap_release_sock(psock, sock);
> raw_spin_unlock_bh(&b->lock);
> err:
> kfree(e);
> - psock = smap_psock_sk(sock);
> - if (psock)
> - smap_release_sock(psock, sock);
> return err;
> }
>
>
Thanks,
Daniel
