[libvirt] [PATCH] nwfilter: Fix the test for the result of atomic dec and test
From: Stefan Berger <stefanb@linux.vnet.ibm.com> https://bugzilla.redhat.com/show_bug.cgi?id=1071181 Commit 49b59a15 fixed one problem but masks another one related to pointer freeing. Use virAtomicIntGet() to test for 0 rather than trying to test for 'true' after virAtomicIntDecAndTest(). Avoid putting of the virNWFilterSnoopReq once the thread has been started. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/nwfilter/nwfilter_dhcpsnoop.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index d2a8062..32ca304 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -720,7 +720,10 @@ virNWFilterSnoopReqPut(virNWFilterSnoopReqPtr req) virNWFilterSnoopLock(); - if (virAtomicIntDecAndTest(&req->refctr)) { + virAtomicIntDecAndTest(&req->refctr); + + /* make sure it's 0; virAtomitIntDecAndTest may return true on '1' */ + if (virAtomicIntGet(&req->refctr) == 0) { /* * delete the request: * - if we don't find req on the global list anymore @@ -1605,6 +1608,7 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver, int tmp; virThread thread; virNWFilterVarValuePtr dhcpsrvrs; + bool threadPuts = false; virNWFilterSnoopIFKeyFMT(ifkey, vmuuid, macaddr); @@ -1690,6 +1694,8 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver, /* prevent thread from holding req */ virNWFilterSnoopReqLock(req); + threadPuts = true; + if (virThreadCreate(&thread, false, virNWFilterDHCPSnoopThread, req) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1737,7 +1743,8 @@ exit_rem_ifnametokey: exit_snoopunlock: virNWFilterSnoopUnlock(); exit_snoopreqput: - virNWFilterSnoopReqPut(req); + if (!threadPuts) + virNWFilterSnoopReqPut(req); return -1; } -- 1.8.1.4
On Tue, Mar 18, 2014 at 09:45:14PM -0400, Stefan Berger wrote:
From: Stefan Berger <stefanb@linux.vnet.ibm.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1071181
Commit 49b59a15 fixed one problem but masks another one related to pointer freeing.
Use virAtomicIntGet() to test for 0 rather than trying to test for 'true' after virAtomicIntDecAndTest().
Avoid putting of the virNWFilterSnoopReq once the thread has been started.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/nwfilter/nwfilter_dhcpsnoop.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index d2a8062..32ca304 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -720,7 +720,10 @@ virNWFilterSnoopReqPut(virNWFilterSnoopReqPtr req)
virNWFilterSnoopLock();
- if (virAtomicIntDecAndTest(&req->refctr)) { + virAtomicIntDecAndTest(&req->refctr); + + /* make sure it's 0; virAtomitIntDecAndTest may return true on '1' */ + if (virAtomicIntGet(&req->refctr) == 0) {
NACK, using two atomic functions, you are making this non-atomic. between these two atomic operations many things can happen an it's not what you want I bet. The virAtomicIntDecAndTest() uses __sync_fetch_and_sub(atomic, 1) and compares it to 1, that's true, but since it is fetch_and_sub (and not the other way around, the value being compared to 1 is the value that the atomic had before it was decremented. That means it returns 1 (true) if and only if the current value is 0. virAtomicIntDecAndTest() is what you really want and should use here. Martin
/* * delete the request: * - if we don't find req on the global list anymore @@ -1605,6 +1608,7 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver, int tmp; virThread thread; virNWFilterVarValuePtr dhcpsrvrs; + bool threadPuts = false;
virNWFilterSnoopIFKeyFMT(ifkey, vmuuid, macaddr);
@@ -1690,6 +1694,8 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver, /* prevent thread from holding req */ virNWFilterSnoopReqLock(req);
+ threadPuts = true; + if (virThreadCreate(&thread, false, virNWFilterDHCPSnoopThread, req) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1737,7 +1743,8 @@ exit_rem_ifnametokey: exit_snoopunlock: virNWFilterSnoopUnlock(); exit_snoopreqput: - virNWFilterSnoopReqPut(req); + if (!threadPuts) + virNWFilterSnoopReqPut(req);
return -1; } -- 1.8.1.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Martin Kletzander <mkletzan@redhat.com> wrote on 03/19/2014 09:13:51 AM:
On Tue, Mar 18, 2014 at 09:45:14PM -0400, Stefan Berger wrote:
From: Stefan Berger <stefanb@linux.vnet.ibm.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1071181
Commit 49b59a15 fixed one problem but masks another one related to
pointer
freeing.
Use virAtomicIntGet() to test for 0 rather than trying to test for 'true' after virAtomicIntDecAndTest().
Avoid putting of the virNWFilterSnoopReq once the thread has been started.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/nwfilter/nwfilter_dhcpsnoop.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/ nwfilter_dhcpsnoop.c index d2a8062..32ca304 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -720,7 +720,10 @@ virNWFilterSnoopReqPut(virNWFilterSnoopReqPtr req)
virNWFilterSnoopLock();
- if (virAtomicIntDecAndTest(&req->refctr)) { + virAtomicIntDecAndTest(&req->refctr); + + /* make sure it's 0; virAtomitIntDecAndTest may return true on '1' */ + if (virAtomicIntGet(&req->refctr) == 0) {
NACK, using two atomic functions, you are making this non-atomic. between these two atomic operations many things can happen an it's not what you want I bet. The virAtomicIntDecAndTest() uses __sync_fetch_and_sub(atomic, 1) and compares it to 1, that's true, but since it is fetch_and_sub (and not the other way around, the value being compared to 1 is the value that the atomic had before it was decremented. That means it returns 1 (true) if and only if the current value is 0. virAtomicIntDecAndTest() is what you really want and should use here.
Right. I initially thought there was something wrong in this part here until I found the double free error is related to the other part. So this part you comment on doesn't need to be changed. Stefan
participants (2)
-
Martin Kletzander -
Stefan Berger