* [bpf PATCH 0/2] sockmap, syzbot fix error path and RCU fix
@ 2018-06-30 13:51 John Fastabend
2018-06-30 13:51 ` [bpf PATCH 1/2] bpf: sockmap, error path can not release psock in multi-map case John Fastabend
2018-06-30 13:51 ` [bpf PATCH 2/2] bpf: sockmap, hash table is RCU so readers do not need locks John Fastabend
0 siblings, 2 replies; 5+ messages in thread
From: John Fastabend @ 2018-06-30 13:51 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, kafai
This applies on top of "BPF fixes for sockhash" I just didn't
want to confuse that series yet again by re-ordering/adding
these patches in it
I missed fixing the error path in the sockhash code to align with
supporting socks in multiple maps. Simply checking if the psock is
present does not mean we can decrement the reference count because
it could be part of another map. Fix this by cleaning up the error
path so this situation does not happen.
As far as I know this should be the last fix to the fallout from
relaxing the single map restriction. Sorry about the multiple fixes
but these patches were all written before the initial submission then
converted and I missed this detail. But at least we caught these early
in the net cycle. Will continue reviewing/testing however to see if
we catch anything else.
Also we need one more series to check ESTABLISH state as Eric noted.
That will be sent out shortly just going over the patches once more.
The ESTABLISH/unhash fix is also needed in kTLS.
---
John Fastabend (2):
bpf: sockmap, error path can not release psock in multi-map case
bpf: sockmap, hash table is RCU so readers do not need locks
0 files changed
^ permalink raw reply [flat|nested] 5+ messages in thread* [bpf PATCH 1/2] bpf: sockmap, error path can not release psock in multi-map case 2018-06-30 13:51 [bpf PATCH 0/2] sockmap, syzbot fix error path and RCU fix John Fastabend @ 2018-06-30 13:51 ` John Fastabend 2018-07-03 14:40 ` Daniel Borkmann 2018-06-30 13:51 ` [bpf PATCH 2/2] bpf: sockmap, hash table is RCU so readers do not need locks John Fastabend 1 sibling, 1 reply; 5+ messages in thread From: John Fastabend @ 2018-06-30 13:51 UTC (permalink / raw) To: ast, daniel; +Cc: netdev, kafai 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: syzbot+d464d2c20c717ef5a6a8@syzkaller.appspotmail.com Fixes: 81110384441a ("bpf: sockmap, add hash map support") Signed-off-by: John Fastabend <john.fastabend@gmail.com> --- 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; + } + 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; } ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [bpf PATCH 1/2] bpf: sockmap, error path can not release psock in multi-map case 2018-06-30 13:51 ` [bpf PATCH 1/2] bpf: sockmap, error path can not release psock in multi-map case John Fastabend @ 2018-07-03 14:40 ` Daniel Borkmann 2018-07-05 14:41 ` John Fastabend 0 siblings, 1 reply; 5+ messages in thread From: Daniel Borkmann @ 2018-07-03 14:40 UTC (permalink / raw) To: John Fastabend, ast; +Cc: netdev, kafai 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: syzbot+d464d2c20c717ef5a6a8@syzkaller.appspotmail.com > Fixes: 81110384441a ("bpf: sockmap, add hash map support") > Signed-off-by: John Fastabend <john.fastabend@gmail.com> > --- > 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bpf PATCH 1/2] bpf: sockmap, error path can not release psock in multi-map case 2018-07-03 14:40 ` Daniel Borkmann @ 2018-07-05 14:41 ` John Fastabend 0 siblings, 0 replies; 5+ messages in thread From: John Fastabend @ 2018-07-05 14:41 UTC (permalink / raw) To: Daniel Borkmann, ast; +Cc: netdev, kafai On 07/03/2018 07:40 AM, Daniel Borkmann wrote: > 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: syzbot+d464d2c20c717ef5a6a8@syzkaller.appspotmail.com >> Fixes: 81110384441a ("bpf: sockmap, add hash map support") >> Signed-off-by: John Fastabend <john.fastabend@gmail.com> >> --- [...] >> @@ -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? > Its not possible will replace with a comment. Thanks. ^ permalink raw reply [flat|nested] 5+ messages in thread
* [bpf PATCH 2/2] bpf: sockmap, hash table is RCU so readers do not need locks 2018-06-30 13:51 [bpf PATCH 0/2] sockmap, syzbot fix error path and RCU fix John Fastabend 2018-06-30 13:51 ` [bpf PATCH 1/2] bpf: sockmap, error path can not release psock in multi-map case John Fastabend @ 2018-06-30 13:51 ` John Fastabend 1 sibling, 0 replies; 5+ messages in thread From: John Fastabend @ 2018-06-30 13:51 UTC (permalink / raw) To: ast, daniel; +Cc: netdev, kafai This removes locking from readers of RCU hash table. Its not necessary. Fixes: 81110384441a ("bpf: sockmap, add hash map support") Signed-off-by: John Fastabend <john.fastabend@gmail.com> --- 0 files changed diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c index 63fb047..12ac10a 100644 --- a/kernel/bpf/sockmap.c +++ b/kernel/bpf/sockmap.c @@ -2451,10 +2451,8 @@ struct sock *__sock_hash_lookup_elem(struct bpf_map *map, void *key) b = __select_bucket(htab, hash); head = &b->head; - raw_spin_lock_bh(&b->lock); l = lookup_elem_raw(head, hash, key, key_size); sk = l ? l->sk : NULL; - raw_spin_unlock_bh(&b->lock); return sk; } ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-07-05 14:41 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-30 13:51 [bpf PATCH 0/2] sockmap, syzbot fix error path and RCU fix John Fastabend 2018-06-30 13:51 ` [bpf PATCH 1/2] bpf: sockmap, error path can not release psock in multi-map case John Fastabend 2018-07-03 14:40 ` Daniel Borkmann 2018-07-05 14:41 ` John Fastabend 2018-06-30 13:51 ` [bpf PATCH 2/2] bpf: sockmap, hash table is RCU so readers do not need locks John Fastabend
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.