On Fri, Jun 22, 2018 at 08:21:39AM -0700, John Fastabend wrote:
> If a hashmap is free'd with open socks it removes the reference to
> the hash entry from the psock. If that is the last reference to the
> psock then it will also be free'd by the reference counting logic.
> However the current logic that removes the hash reference from the
> list of references is broken. In map_list_map_remove() we first check
s/map_list_map_remove/smap_list_remove/
> if the sockmap entry matches and then check if the hashmap entry
> matches. But, the sockmap entry sill always match because its NULL in
> this case which causes the first entry to be removed from the list.
> If this is always the "right" entry (because the user adds/removes
> entries in order) then everything is OK but otherwise a subsequent
> bpf_tcp_close() may reference a free'd object.
>
> To fix this create two list handlers one for sockmap and one for
> sockhash.
>
> Reported-by: syzbot+0ce137753c78f7b6a...@syzkaller.appspotmail.com
> Fixes: 81110384441a ("bpf: sockmap, add hash map support")
> Signed-off-by: John Fastabend <john.fastab...@gmail.com>
One nit. Other than that,
Acked-by: Martin KaFai Lau <ka...@fb.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 d7fd17a..69b26af 100644
> --- a/kernel/bpf/sockmap.c
> +++ b/kernel/bpf/sockmap.c
> @@ -1602,17 +1602,26 @@ static struct bpf_map *sock_map_alloc(union bpf_attr
> *attr)
> return ERR_PTR(err);
> }
>
> -static void smap_list_remove(struct smap_psock *psock,
> - struct sock **entry,
> - struct htab_elem *hash_link)
> +static void smap_list_map_remove(struct smap_psock *psock,
> + struct sock **entry)
> {
> struct smap_psock_map_entry *e, *tmp;
>
> list_for_each_entry_safe(e, tmp, &psock->maps, list) {
> - if (e->entry == entry || e->hash_link == hash_link) {
> + if (e->entry == entry)
> + list_del(&e->list);
> + }
> +}
Nit. Add an empty line.
> +static void smap_list_hash_remove(struct smap_psock *psock,
> + struct htab_elem *hash_link)
> +{
> + struct smap_psock_map_entry *e, *tmp;
> +
> + list_for_each_entry_safe(e, tmp, &psock->maps, list) {
> + struct htab_elem *c = e->hash_link;
> +
> + if (c == hash_link)
> list_del(&e->list);
> - break;
> - }
> }
> }
>
> @@ -1647,7 +1656,7 @@ static void sock_map_free(struct bpf_map *map)
> * to be null and queued for garbage collection.
> */
> if (likely(psock)) {
> - smap_list_remove(psock, &stab->sock_map[i], NULL);
> + smap_list_map_remove(psock, &stab->sock_map[i]);
> smap_release_sock(psock, sock);
> }
> write_unlock_bh(&sock->sk_callback_lock);
> @@ -1706,7 +1715,7 @@ static int sock_map_delete_elem(struct bpf_map *map,
> void *key)
>
> if (psock->bpf_parse)
> smap_stop_sock(psock, sock);
> - smap_list_remove(psock, &stab->sock_map[k], NULL);
> + smap_list_map_remove(psock, &stab->sock_map[k]);
> smap_release_sock(psock, sock);
> out:
> write_unlock_bh(&sock->sk_callback_lock);
> @@ -1908,7 +1917,7 @@ static int sock_map_ctx_update_elem(struct
> bpf_sock_ops_kern *skops,
> struct smap_psock *opsock = smap_psock_sk(osock);
>
> write_lock_bh(&osock->sk_callback_lock);
> - smap_list_remove(opsock, &stab->sock_map[i], NULL);
> + smap_list_map_remove(opsock, &stab->sock_map[i]);
> smap_release_sock(opsock, osock);
> write_unlock_bh(&osock->sk_callback_lock);
> }
> @@ -2124,7 +2133,7 @@ static void sock_hash_free(struct bpf_map *map)
> * (psock) to be null and queued for garbage collection.
> */
> if (likely(psock)) {
> - smap_list_remove(psock, NULL, l);
> + smap_list_hash_remove(psock, l);
> smap_release_sock(psock, sock);
> }
> write_unlock_bh(&sock->sk_callback_lock);
> @@ -2304,7 +2313,7 @@ static int sock_hash_ctx_update_elem(struct
> bpf_sock_ops_kern *skops,
> psock = smap_psock_sk(l_old->sk);
>
> hlist_del_rcu(&l_old->hash_node);
> - smap_list_remove(psock, NULL, l_old);
> + smap_list_hash_remove(psock, l_old);
> smap_release_sock(psock, l_old->sk);
> free_htab_elem(htab, l_old);
> }
> @@ -2372,7 +2381,7 @@ static int sock_hash_delete_elem(struct bpf_map *map,
> void *key)
> * to be null and queued for garbage collection.
> */
> if (likely(psock)) {
> - smap_list_remove(psock, NULL, l);
> + smap_list_hash_remove(psock, l);
> smap_release_sock(psock, sock);
> }
> write_unlock_bh(&sock->sk_callback_lock);
>
