Message-ID: <CAPhsuW5oFPri17tTn0P0=B+Hv1H8BUoza27p_U4sM4kOLEKQdA@mail.gmail.com>
Date:   Fri, 25 May 2018 23:07:38 -0700
From:   Song Liu <liu.song.a23@...il.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     ast@...nel.org, daniel@...earbox.net, netdev@...r.kernel.org
Subject: Re: [bpf-next PATCH] bpf: sockhash fix race with bpf_tcp_close and
 map delete

On Fri, May 25, 2018 at 10:37 AM, John Fastabend
<john.fastabend@...il.com> wrote:
> syzbot reported two related splats, a use after free and null
> pointer dereference, when a TCP socket is closed while the map is
> also being removed.
>
> The psock keeps a reference to all map slots that have a reference
> to the sock so that when the sock is closed we can clean up any
> outstanding sock{map|hash} entries. This avoids pinning a sock
> forever if the map owner fails to do proper cleanup. However, the
> result is we have two paths that can free an entry in the map. Even
> the comment in the sock{map|hash} tear down function, sock_hash_free()
> notes this:
>
>  At this point no update, lookup or delete operations can happen.
>  However, be aware we can still get a socket state event updates,
>  and data ready callbacks that reference the psock from sk_user_data.
>
> Both removal paths omitted taking the hash bucket lock resulting
> in the case where we have two references that are in the process
> of being free'd.
>
> Reported-by: syzbot+a761b81c211794fa1072@...kaller.appspotmail.com
> Signed-off-by: John Fastabend <john.fastabend@...il.com>

Acked-by: Song Liu <songliubraving@...com>

> ---
>  kernel/bpf/sockmap.c |   33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
> index 52a91d8..b508141f 100644
> --- a/kernel/bpf/sockmap.c
> +++ b/kernel/bpf/sockmap.c
> @@ -225,6 +225,16 @@ static void free_htab_elem(struct bpf_htab *htab, struct htab_elem *l)
>         kfree_rcu(l, rcu);
>  }
>
> +static inline struct bucket *__select_bucket(struct bpf_htab *htab, u32 hash)
> +{
> +       return &htab->buckets[hash & (htab->n_buckets - 1)];
> +}
> +
> +static inline struct hlist_head *select_bucket(struct bpf_htab *htab, u32 hash)
> +{
> +       return &__select_bucket(htab, hash)->head;
> +}
> +
>  static void bpf_tcp_close(struct sock *sk, long timeout)
>  {
>         void (*close_fun)(struct sock *sk, long timeout);
> @@ -268,9 +278,15 @@ static void bpf_tcp_close(struct sock *sk, long timeout)
>                                 smap_release_sock(psock, sk);
>                         }
>                 } else {
> +                       u32 hash = e->hash_link->hash;
> +                       struct bucket *b;
> +
> +                       b = __select_bucket(e->htab, hash);
> +                       raw_spin_lock_bh(&b->lock);
>                         hlist_del_rcu(&e->hash_link->hash_node);
>                         smap_release_sock(psock, e->hash_link->sk);
>                         free_htab_elem(e->htab, e->hash_link);
> +                       raw_spin_unlock_bh(&b->lock);
>                 }
>         }
>         write_unlock_bh(&sk->sk_callback_lock);
> @@ -2043,16 +2059,6 @@ static struct bpf_map *sock_hash_alloc(union bpf_attr *attr)
>         return ERR_PTR(err);
>  }
>
> -static inline struct bucket *__select_bucket(struct bpf_htab *htab, u32 hash)
> -{
> -       return &htab->buckets[hash & (htab->n_buckets - 1)];
> -}
> -
> -static inline struct hlist_head *select_bucket(struct bpf_htab *htab, u32 hash)
> -{
> -       return &__select_bucket(htab, hash)->head;
> -}
> -
>  static void sock_hash_free(struct bpf_map *map)
>  {
>         struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
> @@ -2069,10 +2075,12 @@ static void sock_hash_free(struct bpf_map *map)
>          */
>         rcu_read_lock();
>         for (i = 0; i < htab->n_buckets; i++) {
> -               struct hlist_head *head = select_bucket(htab, i);
> +               struct bucket *b = __select_bucket(htab, i);
> +               struct hlist_head *head = &b->head;
>                 struct hlist_node *n;
>                 struct htab_elem *l;
>
> +               raw_spin_lock_bh(&b->lock);
>                 hlist_for_each_entry_safe(l, n, head, hash_node) {
>                         struct sock *sock = l->sk;
>                         struct smap_psock *psock;
> @@ -2090,8 +2098,9 @@ static void sock_hash_free(struct bpf_map *map)
>                                 smap_release_sock(psock, sock);
>                         }
>                         write_unlock_bh(&sock->sk_callback_lock);
> -                       kfree(l);
> +                       free_htab_elem(htab, l);
>                 }
> +               raw_spin_unlock_bh(&b->lock);
>         }
>         rcu_read_unlock();
>         bpf_map_area_free(htab->buckets);
>

Powered by blists - more mailing lists