| Message ID | 20180622152139.24502.24474.stgit@john-Precision-Tower-5810 |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | BPF fixes for sockhash | expand |
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+0ce137753c78f7b6acc1@syzkaller.appspotmail.com > Fixes: 81110384441a ("bpf: sockmap, add hash map support") > Signed-off-by: John Fastabend <john.fastabend@gmail.com> One nit. Other than that, Acked-by: Martin KaFai Lau <kafai@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); >
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); + } +} +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);
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 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+0ce137753c78f7b6acc1@syzkaller.appspotmail.com Fixes: 81110384441a ("bpf: sockmap, add hash map support") Signed-off-by: John Fastabend <john.fastabend@gmail.com> --- kernel/bpf/sockmap.c | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-)