Message ID | 20170110235403.15331-1-joe@ovn.org |
---|---|
State | Accepted |
Headers | show |
Acked-by: Jarno Rajahalme <jarno@ovn.org> > On Jan 10, 2017, at 3:54 PM, Joe Stringer <joe@ovn.org> wrote: > > revalidator_sweep__() splits checking for whether to delete a ukey from > the actual deletion to prevent taking the umap lock for too long. > However it uses information gathered from the first critical section to > decide to call ukey_delete() - ie, the second critical section. > > Since 67f08985d769 ("upcall: Replace ukeys for deleted flows."), it is > possible for a handler thread to receive an upcall for the same flow and > to replace the ukey which is being deleted with a new one, in between > these critical sections. This will remove the ukey from the cmap, > rcu-defer its deletion, and update the existing ukey. > > If this occurs in between the critical sections of revalidator cleanup > of the flow, then the revalidator will subsequently call ukey_delete() > to delete the original ukey, which was already deleted by the handler > thread. > > Guard against this by checking the ukey state in ukey_delete(). > > Backtrace: > Program terminated with signal 11, Segmentation fault. > #0 0x00007fe969b13da3 in cmap_replace__ () > #1 0x00007fe969b14491 in cmap_replace () > #2 0x00007fe969aee9ff in ukey_delete () > #3 0x00007fe969aefd42 in revalidator_sweep__ () > #4 0x00007fe969af1bad in udpif_revalidator () > #5 0x00007fe969b8b2a6 in ovsthread_wrapper () > #6 0x00007fe968e07dc5 in start_thread () from /lib64/libpthread.so.0 > #7 0x00007fe96862c73d in clone () from /lib64/libc.so.6 > > Fixes: 54ebeff4c03d ("upcall: Track ukey states.") > Fixes: 67f08985d769 ("upcall: Replace ukeys for deleted flows.") > Reported-by: Numan Siddique <nusiddiq@redhat.com> > Signed-off-by: Joe Stringer <joe@ovn.org> > --- > ofproto/ofproto-dpif-upcall.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index cd2445fc4ab9..1ffeaabf7d8e 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -1794,9 +1794,11 @@ ukey_delete(struct umap *umap, struct udpif_key *ukey) > OVS_REQUIRES(umap->mutex) > { > ovs_mutex_lock(&ukey->mutex); > - cmap_remove(&umap->cmap, &ukey->cmap_node, ukey->hash); > - ovsrcu_postpone(ukey_delete__, ukey); > - transition_ukey(ukey, UKEY_DELETED); > + if (ukey->state < UKEY_DELETED) { > + cmap_remove(&umap->cmap, &ukey->cmap_node, ukey->hash); > + ovsrcu_postpone(ukey_delete__, ukey); > + transition_ukey(ukey, UKEY_DELETED); > + } > ovs_mutex_unlock(&ukey->mutex); > } > > -- > 2.10.2 >
On 10 January 2017 at 16:00, Jarno Rajahalme <jarno@ovn.org> wrote:
> Acked-by: Jarno Rajahalme <jarno@ovn.org>
Thanks, applied to master.
Numan, I believe that this is an issue regardless of whether it fixes
your issue. Let us know if you have any further trouble.
On Thu, Jan 12, 2017 at 4:05 AM, Joe Stringer <joe@ovn.org> wrote: > On 10 January 2017 at 16:00, Jarno Rajahalme <jarno@ovn.org> wrote: > > Acked-by: Jarno Rajahalme <jarno@ovn.org> > > Thanks, applied to master. > > Numan, I believe that this is an issue regardless of whether it fixes > your issue. Let us know if you have any further trouble. > Sure. I will do. Thanks for the fix. Numan
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index cd2445fc4ab9..1ffeaabf7d8e 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -1794,9 +1794,11 @@ ukey_delete(struct umap *umap, struct udpif_key *ukey) OVS_REQUIRES(umap->mutex) { ovs_mutex_lock(&ukey->mutex); - cmap_remove(&umap->cmap, &ukey->cmap_node, ukey->hash); - ovsrcu_postpone(ukey_delete__, ukey); - transition_ukey(ukey, UKEY_DELETED); + if (ukey->state < UKEY_DELETED) { + cmap_remove(&umap->cmap, &ukey->cmap_node, ukey->hash); + ovsrcu_postpone(ukey_delete__, ukey); + transition_ukey(ukey, UKEY_DELETED); + } ovs_mutex_unlock(&ukey->mutex); }
revalidator_sweep__() splits checking for whether to delete a ukey from the actual deletion to prevent taking the umap lock for too long. However it uses information gathered from the first critical section to decide to call ukey_delete() - ie, the second critical section. Since 67f08985d769 ("upcall: Replace ukeys for deleted flows."), it is possible for a handler thread to receive an upcall for the same flow and to replace the ukey which is being deleted with a new one, in between these critical sections. This will remove the ukey from the cmap, rcu-defer its deletion, and update the existing ukey. If this occurs in between the critical sections of revalidator cleanup of the flow, then the revalidator will subsequently call ukey_delete() to delete the original ukey, which was already deleted by the handler thread. Guard against this by checking the ukey state in ukey_delete(). Backtrace: Program terminated with signal 11, Segmentation fault. #0 0x00007fe969b13da3 in cmap_replace__ () #1 0x00007fe969b14491 in cmap_replace () #2 0x00007fe969aee9ff in ukey_delete () #3 0x00007fe969aefd42 in revalidator_sweep__ () #4 0x00007fe969af1bad in udpif_revalidator () #5 0x00007fe969b8b2a6 in ovsthread_wrapper () #6 0x00007fe968e07dc5 in start_thread () from /lib64/libpthread.so.0 #7 0x00007fe96862c73d in clone () from /lib64/libc.so.6 Fixes: 54ebeff4c03d ("upcall: Track ukey states.") Fixes: 67f08985d769 ("upcall: Replace ukeys for deleted flows.") Reported-by: Numan Siddique <nusiddiq@redhat.com> Signed-off-by: Joe Stringer <joe@ovn.org> --- ofproto/ofproto-dpif-upcall.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)