On 03/22/2017 10:43 AM, Michal Privoznik wrote:

> > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > src/network/bridge_driver.c | 21 +++++++++------------ > 1 file changed, 9 insertions(+), 12 deletions(-) > > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c > index a753cd78b..0ea8e2348 100644 > --- a/src/network/bridge_driver.c > +++ b/src/network/bridge_driver.c > @@ -470,6 +470,7 @@ networkUpdateState(virNetworkObjPtr obj, > { > virNetworkDriverStatePtr driver = opaque; > dnsmasqCapsPtr dnsmasq_caps = networkGetDnsmasqCaps(driver); > + char *macMapFile = NULL; > int ret = -1; > > virObjectLock(obj); > @@ -486,6 +487,13 @@ networkUpdateState(virNetworkObjPtr obj, > /* If bridge doesn't exist, then mark it inactive */ > if (!(obj->def->bridge && virNetDevExists(obj->def->bridge) == 1)) > obj->active = 0; > + > + if (!(macMapFile = networkMacMgrFileName(driver, obj->def->bridge))) > + goto cleanup; > + > + if (!(obj->macmap = virMacMapNew(macMapFile))) > + goto cleanup; > + > break; ... but from what I can understand, the only differences are: 1) the macMapFile used to be regenerated right after reading the radvdpidfile (which in most cases doesn't exist because I think most of the time that same duty is handled by dnsmasq instead), and now it is regenerated *just before* reading radvdpidfile.

2) it used to be regenerated for any network with a def->bridge (so it would also happen for "unmanaged" bridge networks, where libvirt just points to an already-existing bridge), and it is now regenerated only for networks where libvirt creates and destroys the bridge (and might have a dnsmasq instance running.

On 03/28/2017 05:08 AM, Michal Privoznik wrote: > On 03/28/2017 02:22 AM, Laine Stump wrote: >> On 03/22/2017 10:43 AM, Michal Privoznik wrote: > >> >>> >>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >>> --- >>> src/network/bridge_driver.c | 21 +++++++++------------ >>> 1 file changed, 9 insertions(+), 12 deletions(-) >>> >>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c >>> index a753cd78b..0ea8e2348 100644 >>> --- a/src/network/bridge_driver.c >>> +++ b/src/network/bridge_driver.c >>> @@ -470,6 +470,7 @@ networkUpdateState(virNetworkObjPtr obj, >>> { >>> virNetworkDriverStatePtr driver = opaque; >>> dnsmasqCapsPtr dnsmasq_caps = networkGetDnsmasqCaps(driver); >>> + char *macMapFile = NULL; >>> int ret = -1; >>> >>> virObjectLock(obj); >>> @@ -486,6 +487,13 @@ networkUpdateState(virNetworkObjPtr obj, >>> /* If bridge doesn't exist, then mark it inactive */ >>> if (!(obj->def->bridge && virNetDevExists(obj->def->bridge) >>> == 1)) >>> obj->active = 0; >>> + >>> + if (!(macMapFile = networkMacMgrFileName(driver, >>> obj->def->bridge))) >>> + goto cleanup; >>> + >>> + if (!(obj->macmap = virMacMapNew(macMapFile))) >>> + goto cleanup; >>> + >>> break; >> >> >> ... but from what I can understand, the only differences are: >> >> 1) the macMapFile used to be regenerated right after reading the >> radvdpidfile (which in most cases doesn't exist because I think most of >> the time that same duty is handled by dnsmasq instead), and now it is >> regenerated *just before* reading radvdpidfile. > > I don't think this is any problem. It's just an ordering issue. Those > two features are orthogonal. > >> >> 2) it used to be regenerated for any network with a def->bridge (so it >> would also happen for "unmanaged" bridge networks, where libvirt just >> points to an already-existing bridge), and it is now regenerated only >> for networks where libvirt creates and destroys the bridge (and might >> have a dnsmasq instance running. I was wrong about this part. Previously the call to create macMapFile we're talking about happened during network driver initialization for any *active* network that had IP addresses defined on the network (and it just happens that a libvirt network can only have IP addresses defined if we are managing the bridge and running dnsmasq). With this patch, it is created during network driver initialization for any network that has forward mode = nat/route/open/none (i.e. any network that libvirt is running dnsmasq for). It's no longer inside a check for networking being active/not, but anyway networkUpdateState() returns immediately when it's called if the network isn't already active anyway. So essentially the code is doing the same thing it previously did. > > Ah. Is that right? I agree that the code is a bit unclear, but the > following should be true: > > 1) when a network is being freshly started up, the virMacMap module is > created and stored in the network object if and only if dnsmasq is > spawned (because only then it's us who assigns IP addresses). > > 2) when an interface is allocated/freed from a network that has the > module, the $br.macs file is updated accordingly. The file is created on > the first interface allocation, and unlinked on network undef. > > 3) when the daemon restarts, networkUpdateState() is called for every > running network. And if the $br.macs file exists for given network, the > module is created (we are reconstructing the network objects after all) > and the file is parsed to restore the previous state. Note, that the > dnsmasq is not spawned again - it kept running while libvirtd was > restarting. > > Now, there are two problems that I see: > > a) if there is no interface added in the 2nd step and the libvirt daemon > is restarted, in the 3rd step the file does not exist and thus the code > thinks no virMacMap module was used for the network so it does not > create one. This is obviously a bug. > > b) if you want to have the module for your network, you need to shut it > down (and thus cut off your domains from the connectivity) and start > over again. It's very annoying. When implementing this, my reasoning was > that it's better to give no results than partial results, it's better to > not have no $br.macs file than have a file containing just newly > allocated interfaces. Well, I was wrong. I think people don't want to > destroy their network unless really necessary. More so if the network > continues running even when the daemon is killed. Yeah, I agree with that. > > Therefore, what I am suggesting here is to rework step 3) so that the > module is created more frequently. It would solve both problems I'm > describing. But I don't think you are creating it any more frequently than before. You're doing it at exactly the same times as previously. In the end, I don't have anything *against* this patch, I just don't think it's doing anything useful.

On 03/31/2017 07:20 AM, Michal Privoznik wrote: > On 03/30/2017 06:00 PM, Laine Stump wrote: >> On 03/28/2017 05:08 AM, Michal Privoznik wrote: >>> On 03/28/2017 02:22 AM, Laine Stump wrote: >>>> On 03/22/2017 10:43 AM, Michal Privoznik wrote: >>> >>>> >>>>> >>>>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >>>>> --- >>>>> src/network/bridge_driver.c | 21 +++++++++------------ >>>>> 1 file changed, 9 insertions(+), 12 deletions(-) >>>>> >>>>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c >>>>> index a753cd78b..0ea8e2348 100644 >>>>> --- a/src/network/bridge_driver.c >>>>> +++ b/src/network/bridge_driver.c >>>>> @@ -470,6 +470,7 @@ networkUpdateState(virNetworkObjPtr obj, >>>>> { >>>>> virNetworkDriverStatePtr driver = opaque; >>>>> dnsmasqCapsPtr dnsmasq_caps = networkGetDnsmasqCaps(driver); >>>>> + char *macMapFile = NULL; >>>>> int ret = -1; >>>>> >>>>> virObjectLock(obj); >>>>> @@ -486,6 +487,13 @@ networkUpdateState(virNetworkObjPtr obj, >>>>> /* If bridge doesn't exist, then mark it inactive */ >>>>> if (!(obj->def->bridge && virNetDevExists(obj->def->bridge) >>>>> == 1)) >>>>> obj->active = 0; >>>>> + >>>>> + if (!(macMapFile = networkMacMgrFileName(driver, >>>>> obj->def->bridge))) >>>>> + goto cleanup; >>>>> + >>>>> + if (!(obj->macmap = virMacMapNew(macMapFile))) >>>>> + goto cleanup; >>>>> + >>>>> break; >>>> >>>> >>>> ... but from what I can understand, the only differences are: >>>> >>>> 1) the macMapFile used to be regenerated right after reading the >>>> radvdpidfile (which in most cases doesn't exist because I think most of >>>> the time that same duty is handled by dnsmasq instead), and now it is >>>> regenerated *just before* reading radvdpidfile. >>> >>> I don't think this is any problem. It's just an ordering issue. Those >>> two features are orthogonal. >>> >>>> >>>> 2) it used to be regenerated for any network with a def->bridge (so it >>>> would also happen for "unmanaged" bridge networks, where libvirt just >>>> points to an already-existing bridge), and it is now regenerated only >>>> for networks where libvirt creates and destroys the bridge (and might >>>> have a dnsmasq instance running. >> >> >> I was wrong about this part. Previously the call to create macMapFile >> we're talking about happened during network driver initialization for >> any *active* network that had IP addresses defined on the network (and >> it just happens that a libvirt network can only have IP addresses >> defined if we are managing the bridge and running dnsmasq). >> >> With this patch, it is created during network driver initialization for >> any network that has forward mode = nat/route/open/none (i.e. any >> network that libvirt is running dnsmasq for). It's no longer inside a >> check for networking being active/not, but anyway networkUpdateState() >> returns immediately when it's called if the network isn't already active >> anyway. >> >> So essentially the code is doing the same thing it previously did. >> >>> >>> Ah. Is that right? I agree that the code is a bit unclear, but the >>> following should be true: >>> >>> 1) when a network is being freshly started up, the virMacMap module is >>> created and stored in the network object if and only if dnsmasq is >>> spawned (because only then it's us who assigns IP addresses). >>> >>> 2) when an interface is allocated/freed from a network that has the >>> module, the $br.macs file is updated accordingly. The file is created on >>> the first interface allocation, and unlinked on network undef. >>> >>> 3) when the daemon restarts, networkUpdateState() is called for every >>> running network. And if the $br.macs file exists for given network, the >>> module is created (we are reconstructing the network objects after all) >>> and the file is parsed to restore the previous state. Note, that the >>> dnsmasq is not spawned again - it kept running while libvirtd was >>> restarting. >>> >>> Now, there are two problems that I see: >>> >>> a) if there is no interface added in the 2nd step and the libvirt daemon >>> is restarted, in the 3rd step the file does not exist and thus the code >>> thinks no virMacMap module was used for the network so it does not >>> create one. This is obviously a bug. >>> >>> b) if you want to have the module for your network, you need to shut it >>> down (and thus cut off your domains from the connectivity) and start >>> over again. It's very annoying. When implementing this, my reasoning was >>> that it's better to give no results than partial results, it's better to >>> not have no $br.macs file than have a file containing just newly >>> allocated interfaces. Well, I was wrong. I think people don't want to >>> destroy their network unless really necessary. More so if the network >>> continues running even when the daemon is killed. >> >> Yeah, I agree with that. >> >>> >>> Therefore, what I am suggesting here is to rework step 3) so that the >>> module is created more frequently. It would solve both problems I'm >>> describing. >> >> But I don't think you are creating it any more frequently than before. >> You're doing it at exactly the same times as previously. >> >> In the end, I don't have anything *against* this patch, I just don't >> think it's doing anything useful. > > How come? It's creating the macMap module more frequently Oh wait, now I see the difference - previously it would only create a new macMap if macMapFile existed, and now it does it even if macMapFile doesn't exist. Is that the difference you're talking about?

If that is what you say makes it "more frequently" (I haven't had the time or spare brain cells to verify that, but I'll take your word) then ACK.

https://bugzilla.redhat.com/show_bug.cgi?id=1434882 Imagine the following scenario: 1) virsh net-start default 2) virsh start myFavouriteDomain 3) virsh net-destroy default 4) virsh destroy myFavouriteDomain (assuming myFavouriteDomain has an interface from default network) Regardless of how unlikely this scenario looks like, we should not crash. The problem is, on net-destroy in networkShutdownNetworkVirtual() the virMacMap module is unrefed, but the stale pointer is kept around. Thus when the domain destroy procedure comes in, networkReleaseActualDevice() and subsequently networkMacMgrDel() is called. This function sees the stale pointer and starts calling the virMacMap module APIs which work over freed memory. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/network/bridge_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 0ea8e2348..3fcdd702e 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2488,7 +2488,8 @@ networkShutdownNetworkVirtual(virNetworkDriverStatePtr driver, if (network->def->bandwidth) virNetDevBandwidthClear(network->def->bridge); - virObjectUnref(network->macmap); + if (!virObjectUnref(network->macmap)) + network->macmap = NULL;

if (network->radvdPid > 0) { char *radvdpidbase; -- 2.11.0 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list