External/libvirt
1
0
Fork 0
mirror of https://gitlab.com/libvirt/libvirt.git synced 2024-07-07 18:35:46 +00:00
Code Issues Packages Projects Releases Wiki Activity

nwfilter_dhcpsnoop: Replace virNWFilterSnoopReqLock functions

Browse Source 
Use automatic mutex management instead.

Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This commit is contained in:
Tim Wiederhake 2022-03-16 23:00:45 +01:00
parent 5e6442b903
commit 2d7682dd3b
1 changed files with 98 additions and 185 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

283
src/nwfilter/nwfilter_dhcpsnoop.c
View File

@ -139,7 +139,7 @@ struct _virNWFilterSnoopReq {
/* /*
* Note about lock-order: * Note about lock-order:
* 1st: virNWFilterSnoopState.snoopLock * 1st: virNWFilterSnoopState.snoopLock
* 2nd: virNWFilterSnoopReqLock(req) * 2nd: &req->lock
* *
* Rationale: Former protects the SnoopReqs hash, latter its contents * Rationale: Former protects the SnoopReqs hash, latter its contents
*/ */
@ -246,9 +246,6 @@ static int virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReq *req,
bool update_leasefile, bool update_leasefile,
bool instantiate); bool instantiate);
static void virNWFilterSnoopReqLock(virNWFilterSnoopReq *req);
static void virNWFilterSnoopReqUnlock(virNWFilterSnoopReq *req);
static void virNWFilterSnoopLeaseFileLoad(void); static void virNWFilterSnoopLeaseFileLoad(void);
static void virNWFilterSnoopLeaseFileSave(virNWFilterSnoopIPLease *ipl); static void virNWFilterSnoopLeaseFileSave(virNWFilterSnoopIPLease *ipl);
@ -362,11 +359,9 @@ virNWFilterSnoopIPLeaseTimerAdd(virNWFilterSnoopIPLease *plnew)
virNWFilterSnoopReq *req = plnew->snoopReq; virNWFilterSnoopReq *req = plnew->snoopReq;
/* protect req->start / req->end */ /* protect req->start / req->end */
virNWFilterSnoopReqLock(req); VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock);
virNWFilterSnoopListAdd(plnew, &req->start, &req->end); virNWFilterSnoopListAdd(plnew, &req->start, &req->end);
virNWFilterSnoopReqUnlock(req);
} }
/* /*
@ -378,12 +373,9 @@ virNWFilterSnoopIPLeaseTimerDel(virNWFilterSnoopIPLease *ipl)
virNWFilterSnoopReq *req = ipl->snoopReq; virNWFilterSnoopReq *req = ipl->snoopReq;
/* protect req->start / req->end */ /* protect req->start / req->end */
virNWFilterSnoopReqLock(req); VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock);
virNWFilterSnoopListDel(ipl, &req->start, &req->end); virNWFilterSnoopListDel(ipl, &req->start, &req->end);
virNWFilterSnoopReqUnlock(req);
ipl->timeout = 0; ipl->timeout = 0;
} }
@ -401,36 +393,24 @@ virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLease *ipl,
bool instantiate) bool instantiate)
{ {
g_autofree char *ipaddr = virSocketAddrFormat(&ipl->ipAddress); g_autofree char *ipaddr = virSocketAddrFormat(&ipl->ipAddress);
int rc = -1; virNWFilterSnoopReq *req = ipl->snoopReq;
virNWFilterSnoopReq *req; VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock);
if (!ipaddr) if (!ipaddr)
return -1; return -1;
req = ipl->snoopReq;
/* protect req->binding->portdevname */
virNWFilterSnoopReqLock(req);
if (virNWFilterIPAddrMapAddIPAddr(req->binding->portdevname, ipaddr) < 0) if (virNWFilterIPAddrMapAddIPAddr(req->binding->portdevname, ipaddr) < 0)
goto cleanup; return -1;
if (!instantiate) { if (!instantiate)
rc = 0; return 0;
goto cleanup;
}
/* instantiate the filters */ /* instantiate the filters */
if (req->binding->portdevname) { if (!req->binding->portdevname)
rc = virNWFilterInstantiateFilterLate(req->driver, return -1;
req->binding,
req->ifindex);
}
cleanup: return virNWFilterInstantiateFilterLate(req->driver, req->binding, req->ifindex);
virNWFilterSnoopReqUnlock(req);
return rc;
} }
/* /*
@ -473,7 +453,7 @@ virNWFilterSnoopReqLeaseTimerRun(virNWFilterSnoopReq *req)
bool is_last = false; bool is_last = false;
/* protect req->start */ /* protect req->start */
virNWFilterSnoopReqLock(req); VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock);
while (req->start && req->start->timeout <= now) { while (req->start && req->start->timeout <= now) {
if (req->start->next == NULL || if (req->start->next == NULL ||
@ -483,8 +463,6 @@ virNWFilterSnoopReqLeaseTimerRun(virNWFilterSnoopReq *req)
is_last); is_last);
} }
virNWFilterSnoopReqUnlock(req);
return 0; return 0;
} }
@ -562,24 +540,6 @@ virNWFilterSnoopReqFree(virNWFilterSnoopReq *req)
g_free(req); g_free(req);
} }
/*
* Lock a Snoop request 'req'
*/
static void
virNWFilterSnoopReqLock(virNWFilterSnoopReq *req)
{
virMutexLock(&req->lock);
}
/*
* Unlock a Snoop request 'req'
*/
static void
virNWFilterSnoopReqUnlock(virNWFilterSnoopReq *req)
{
virMutexUnlock(&req->lock);
}
/* /*
* virNWFilterSnoopReqRelease - hash table free function to kill a request * virNWFilterSnoopReqRelease - hash table free function to kill a request
*/ */
@ -592,12 +552,10 @@ virNWFilterSnoopReqRelease(void *req0)
return; return;
/* protect req->threadkey */ /* protect req->threadkey */
virNWFilterSnoopReqLock(req); VIR_WITH_MUTEX_LOCK_GUARD(&req->lock) {
if (req->threadkey)
if (req->threadkey) virNWFilterSnoopCancel(&req->threadkey);
virNWFilterSnoopCancel(&req->threadkey); }
virNWFilterSnoopReqUnlock(req);
virNWFilterSnoopReqFree(req); virNWFilterSnoopReqFree(req);
} }
@ -658,45 +616,33 @@ virNWFilterSnoopReqLeaseAdd(virNWFilterSnoopReq *req,
virNWFilterSnoopIPLease *plnew, virNWFilterSnoopIPLease *plnew,
bool update_leasefile) bool update_leasefile)
{ {
VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock);
virNWFilterSnoopIPLease *pl; virNWFilterSnoopIPLease *pl;
plnew->snoopReq = req; plnew->snoopReq = req;
/* protect req->start and the lease */
virNWFilterSnoopReqLock(req);
pl = virNWFilterSnoopIPLeaseGetByIP(req->start, &plnew->ipAddress); pl = virNWFilterSnoopIPLeaseGetByIP(req->start, &plnew->ipAddress);
if (pl) { if (pl) {
virNWFilterSnoopIPLeaseUpdate(pl, plnew->timeout); virNWFilterSnoopIPLeaseUpdate(pl, plnew->timeout);
virLockGuardUnlock(&lock);
} else {
pl = g_new0(virNWFilterSnoopIPLease, 1);
*pl = *plnew;
virNWFilterSnoopReqUnlock(req); if (req->threadkey && virNWFilterSnoopIPLeaseInstallRule(pl, true) < 0) {
g_free(pl);
return -1;
}
goto cleanup; virLockGuardUnlock(&lock);
/* put the lease on the req's list */
virNWFilterSnoopIPLeaseTimerAdd(pl);
g_atomic_int_add(&virNWFilterSnoopState.nLeases, 1);
} }
virNWFilterSnoopReqUnlock(req);
pl = g_new0(virNWFilterSnoopIPLease, 1);
*pl = *plnew;
/* protect req->threadkey */
virNWFilterSnoopReqLock(req);
if (req->threadkey && virNWFilterSnoopIPLeaseInstallRule(pl, true) < 0) {
virNWFilterSnoopReqUnlock(req);
g_free(pl);
return -1;
}
virNWFilterSnoopReqUnlock(req);
/* put the lease on the req's list */
virNWFilterSnoopIPLeaseTimerAdd(pl);
g_atomic_int_add(&virNWFilterSnoopState.nLeases, 1);
cleanup:
if (update_leasefile) if (update_leasefile)
virNWFilterSnoopLeaseFileSave(pl); virNWFilterSnoopLeaseFileSave(pl);
@ -710,24 +656,19 @@ virNWFilterSnoopReqLeaseAdd(virNWFilterSnoopReq *req,
static int static int
virNWFilterSnoopReqRestore(virNWFilterSnoopReq *req) virNWFilterSnoopReqRestore(virNWFilterSnoopReq *req)
{ {
int ret = 0;
virNWFilterSnoopIPLease *ipl; virNWFilterSnoopIPLease *ipl;
/* protect req->start */ /* protect req->start */
virNWFilterSnoopReqLock(req); VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock);
for (ipl = req->start; ipl; ipl = ipl->next) { for (ipl = req->start; ipl; ipl = ipl->next) {
/* instantiate the rules at the last lease */ /* instantiate the rules at the last lease */
bool is_last = (ipl->next == NULL); bool is_last = (ipl->next == NULL);
if (virNWFilterSnoopIPLeaseInstallRule(ipl, is_last) < 0) { if (virNWFilterSnoopIPLeaseInstallRule(ipl, is_last) < 0)
ret = -1; return -1;
break;
}
} }
virNWFilterSnoopReqUnlock(req); return 0;
return ret;
} }
/* /*
@ -759,16 +700,15 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReq *req,
g_autofree char *ipstr = NULL; g_autofree char *ipstr = NULL;
/* protect req->start, req->ifname and the lease */ /* protect req->start, req->ifname and the lease */
virNWFilterSnoopReqLock(req); VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock);
ipl = virNWFilterSnoopIPLeaseGetByIP(req->start, ipaddr); ipl = virNWFilterSnoopIPLeaseGetByIP(req->start, ipaddr);
if (ipl == NULL) if (ipl == NULL)
goto lease_not_found; return 0;
ipstr = virSocketAddrFormat(&ipl->ipAddress); ipstr = virSocketAddrFormat(&ipl->ipAddress);
if (!ipstr) { if (!ipstr) {
ret = -1; return -1;
goto lease_not_found;
} }
virNWFilterSnoopIPLeaseTimerDel(ipl); virNWFilterSnoopIPLeaseTimerDel(ipl);
@ -807,10 +747,6 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReq *req,
g_free(ipl); g_free(ipl);
ignore_value(!!g_atomic_int_dec_and_test(&virNWFilterSnoopState.nLeases)); ignore_value(!!g_atomic_int_dec_and_test(&virNWFilterSnoopState.nLeases));
lease_not_found:
virNWFilterSnoopReqUnlock(req);
return ret; return ret;
} }
@ -1279,43 +1215,37 @@ virNWFilterDHCPSnoopThread(void *req0)
/* whoever started us increased the reference counter for the req for us */ /* whoever started us increased the reference counter for the req for us */
/* protect req->binding->portdevname & req->threadkey */ /* protect req->binding->portdevname & req->threadkey */
virNWFilterSnoopReqLock(req); VIR_WITH_MUTEX_LOCK_GUARD(&req->lock) {
if (req->binding->portdevname && req->threadkey) {
if (req->binding->portdevname && req->threadkey) { for (i = 0; i < G_N_ELEMENTS(pcapConf); i++) {
for (i = 0; i < G_N_ELEMENTS(pcapConf); i++) { pcapConf[i].handle =
pcapConf[i].handle = virNWFilterSnoopDHCPOpen(req->binding->portdevname,
virNWFilterSnoopDHCPOpen(req->binding->portdevname, &req->binding->mac,
&req->binding->mac, pcapConf[i].filter,
pcapConf[i].filter, pcapConf[i].dir);
pcapConf[i].dir); if (!pcapConf[i].handle) {
if (!pcapConf[i].handle) { error = true;
error = true; break;
break; }
fds[i].fd = pcap_fileno(pcapConf[i].handle);
} }
fds[i].fd = pcap_fileno(pcapConf[i].handle); tmp = virNetDevGetIndex(req->binding->portdevname, &ifindex);
threadkey = g_strdup(req->threadkey);
worker = virThreadPoolNewFull(1, 1, 0, virNWFilterDHCPDecodeWorker,
"dhcp-decode", NULL, req);
} }
tmp = virNetDevGetIndex(req->binding->portdevname, &ifindex);
threadkey = g_strdup(req->threadkey); /* let creator know how well we initialized */
worker = virThreadPoolNewFull(1, 1, 0, if (error || !threadkey || tmp < 0 || !worker || ifindex != req->ifindex) {
virNWFilterDHCPDecodeWorker, virErrorPreserveLast(&req->threadError);
"dhcp-decode", req->threadStatus = THREAD_STATUS_FAIL;
NULL, } else {
req); req->threadStatus = THREAD_STATUS_OK;
}
virCondSignal(&req->threadStatusCond);
} }
/* let creator know how well we initialized */
if (error || !threadkey || tmp < 0 || !worker ||
ifindex != req->ifindex) {
virErrorPreserveLast(&req->threadError);
req->threadStatus = THREAD_STATUS_FAIL;
} else {
req->threadStatus = THREAD_STATUS_OK;
}
virCondSignal(&req->threadStatusCond);
virNWFilterSnoopReqUnlock(req);
if (req->threadStatus != THREAD_STATUS_OK) if (req->threadStatus != THREAD_STATUS_OK)
goto cleanup; goto cleanup;
@ -1362,12 +1292,10 @@ virNWFilterDHCPSnoopThread(void *req0)
tmp = -1; tmp = -1;
/* protect req->binding->portdevname */ /* protect req->binding->portdevname */
virNWFilterSnoopReqLock(req); VIR_WITH_MUTEX_LOCK_GUARD(&req->lock) {
if (req->binding->portdevname)
if (req->binding->portdevname) tmp = virNetDevValidateConfig(req->binding->portdevname, NULL, ifindex);
tmp = virNetDevValidateConfig(req->binding->portdevname, NULL, ifindex); }
virNWFilterSnoopReqUnlock(req);
if (tmp <= 0) { if (tmp <= 0) {
error = true; error = true;
@ -1378,20 +1306,17 @@ virNWFilterDHCPSnoopThread(void *req0)
g_clear_pointer(&pcapConf[i].handle, pcap_close); g_clear_pointer(&pcapConf[i].handle, pcap_close);
/* protect req->binding->portdevname */ /* protect req->binding->portdevname */
virNWFilterSnoopReqLock(req); VIR_WITH_MUTEX_LOCK_GUARD(&req->lock) {
virReportError(VIR_ERR_INTERNAL_ERROR,
virReportError(VIR_ERR_INTERNAL_ERROR, _("interface '%s' failing; reopening"),
_("interface '%s' failing; " req->binding->portdevname);
"reopening"), if (req->binding->portdevname)
req->binding->portdevname); pcapConf[i].handle =
if (req->binding->portdevname) virNWFilterSnoopDHCPOpen(req->binding->portdevname,
pcapConf[i].handle = &req->binding->mac,
virNWFilterSnoopDHCPOpen(req->binding->portdevname, pcapConf[i].filter,
&req->binding->mac, pcapConf[i].dir);
pcapConf[i].filter, }
pcapConf[i].dir);
virNWFilterSnoopReqUnlock(req);
if (!pcapConf[i].handle) { if (!pcapConf[i].handle) {
error = true; error = true;
@ -1448,16 +1373,14 @@ virNWFilterDHCPSnoopThread(void *req0)
/* protect IfNameToKey */ /* protect IfNameToKey */
VIR_WITH_MUTEX_LOCK_GUARD(&virNWFilterSnoopState.snoopLock) { VIR_WITH_MUTEX_LOCK_GUARD(&virNWFilterSnoopState.snoopLock) {
/* protect req->binding->portdevname & req->threadkey */ /* protect req->binding->portdevname & req->threadkey */
virNWFilterSnoopReqLock(req); VIR_WITH_MUTEX_LOCK_GUARD(&req->lock) {
virNWFilterSnoopCancel(&req->threadkey);
virNWFilterSnoopCancel(&req->threadkey); ignore_value(virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey,
req->binding->portdevname));
ignore_value(virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey, g_clear_pointer(&req->binding->portdevname, g_free);
req->binding->portdevname)); }
g_clear_pointer(&req->binding->portdevname, g_free);
virNWFilterSnoopReqUnlock(req);
} }
cleanup: cleanup:
@ -1563,7 +1486,7 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriver *techdriver,
} }
/* prevent thread from holding req */ /* prevent thread from holding req */
virNWFilterSnoopReqLock(req); virMutexLock(&req->lock);
if (virThreadCreateFull(&thread, false, virNWFilterDHCPSnoopThread, if (virThreadCreateFull(&thread, false, virNWFilterDHCPSnoopThread,
"dhcp-snoop", false, req) != 0) { "dhcp-snoop", false, req) != 0) {
@ -1604,7 +1527,7 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriver *techdriver,
goto exit_snoop_cancel; goto exit_snoop_cancel;
} }
virNWFilterSnoopReqUnlock(req); virMutexUnlock(&req->lock);
virMutexUnlock(&virNWFilterSnoopState.snoopLock); virMutexUnlock(&virNWFilterSnoopState.snoopLock);
@ -1615,7 +1538,7 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriver *techdriver,
exit_snoop_cancel: exit_snoop_cancel:
virNWFilterSnoopCancel(&req->threadkey); virNWFilterSnoopCancel(&req->threadkey);
exit_snoopreq_unlock: exit_snoopreq_unlock:
virNWFilterSnoopReqUnlock(req); virMutexUnlock(&req->lock);
exit_rem_ifnametokey: exit_rem_ifnametokey:
virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey, binding->portdevname); virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey, binding->portdevname);
exit_snoopunlock: exit_snoopunlock:
@ -1706,12 +1629,11 @@ virNWFilterSnoopPruneIter(const void *payload,
const void *data G_GNUC_UNUSED) const void *data G_GNUC_UNUSED)
{ {
virNWFilterSnoopReq *req = (virNWFilterSnoopReq *)payload; virNWFilterSnoopReq *req = (virNWFilterSnoopReq *)payload;
bool del_req;
/* clean up orphaned, expired leases */ /* clean up orphaned, expired leases */
/* protect req->threadkey */ /* protect req->threadkey */
virNWFilterSnoopReqLock(req); VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock);
if (!req->threadkey) if (!req->threadkey)
virNWFilterSnoopReqLeaseTimerRun(req); virNWFilterSnoopReqLeaseTimerRun(req);
@ -1719,11 +1641,7 @@ virNWFilterSnoopPruneIter(const void *payload,
/* /*
* have the entry removed if it has no leases and no one holds a ref * have the entry removed if it has no leases and no one holds a ref
*/ */
del_req = ((req->start == NULL) && (g_atomic_int_get(&req->refctr) == 0)); return ((req->start == NULL) && (g_atomic_int_get(&req->refctr) == 0));
virNWFilterSnoopReqUnlock(req);
return del_req;
} }
/* /*
@ -1740,12 +1658,11 @@ virNWFilterSnoopSaveIter(void *payload,
virNWFilterSnoopIPLease *ipl; virNWFilterSnoopIPLease *ipl;
/* protect req->start */ /* protect req->start */
virNWFilterSnoopReqLock(req); VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock);
for (ipl = req->start; ipl; ipl = ipl->next) for (ipl = req->start; ipl; ipl = ipl->next)
ignore_value(virNWFilterSnoopLeaseFileWrite(tfd, req->ifkey, ipl)); ignore_value(virNWFilterSnoopLeaseFileWrite(tfd, req->ifkey, ipl));
virNWFilterSnoopReqUnlock(req);
return 0; return 0;
} }
@ -1901,7 +1818,7 @@ virNWFilterSnoopRemAllReqIter(const void *payload,
virNWFilterSnoopReq *req = (virNWFilterSnoopReq *)payload; virNWFilterSnoopReq *req = (virNWFilterSnoopReq *)payload;
/* protect req->binding->portdevname */ /* protect req->binding->portdevname */
virNWFilterSnoopReqLock(req); VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock);
if (req->binding && req->binding->portdevname) { if (req->binding && req->binding->portdevname) {
ignore_value(virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey, ignore_value(virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey,
@ -1917,8 +1834,6 @@ virNWFilterSnoopRemAllReqIter(const void *payload,
g_clear_pointer(&req->binding->portdevname, g_free); g_clear_pointer(&req->binding->portdevname, g_free);
} }
virNWFilterSnoopReqUnlock(req);
/* removal will call virNWFilterSnoopCancel() */ /* removal will call virNWFilterSnoopCancel() */
return 1; return 1;
} }
@ -2000,14 +1915,12 @@ virNWFilterDHCPSnoopEnd(const char *ifname)
} }
/* protect req->binding->portdevname & req->threadkey */ /* protect req->binding->portdevname & req->threadkey */
virNWFilterSnoopReqLock(req); VIR_WITH_MUTEX_LOCK_GUARD(&req->lock) {
/* keep valid lease req; drop interface association */
virNWFilterSnoopCancel(&req->threadkey);
/* keep valid lease req; drop interface association */ g_clear_pointer(&req->binding->portdevname, g_free);
virNWFilterSnoopCancel(&req->threadkey); }
g_clear_pointer(&req->binding->portdevname, g_free);
virNWFilterSnoopReqUnlock(req);
virNWFilterSnoopReqPut(req); virNWFilterSnoopReqPut(req);
} else { /* free all of them */ } else { /* free all of them */