libvirtd crashes when creating network interface pools in 6.0.0-0ubuntu8.13

This is an updated lp-1892132-Add-phys_port_name-support-on-virPCIGetNetName.patch that uses g_steal_pointer(&firstEntryName); when assigning *netname.

Yeah, looking at the patch it's clear the NULL setting was accidentally (?) left off:

- if (physPortID) {
- if (firstEntryName) {
- /* we didn't match the provided phys_port_id, but this
- * is probably because phys_port_id isn't implemented
- * for this NIC driver, so just return the first
- * (probably only) netname we found.
- */
- *netname = firstEntryName;
- firstEntryName = NULL;
- ret = 0;
- } else {
- virReportError(VIR_ERR_INTERNAL_ERROR, ...

Okay, this is what appears to have happened.

During the backport of:

commit 5b1c525b1f3608156884aed0dc5e925306c1e260
Author: Dmytro Linkin <email address hidden>
Date: Thu Jan 21 14:15:22 2021 +0200
Subject: util: Add phys_port_name support on virPCIGetNetName
Link: https://github.com/libvirt/libvirt/commit/5b1c525b1f3608156884aed0dc5e925306c1e260

Another refactor commit seems to have been squashed together and included:

commit 77401d549c52ba2b11c2132a8ca41d73a4130c85
Author: Laine Stump <email address hidden>
Date: Mon Oct 26 22:04:31 2020 -0400
Subject: util: refactor function to simplify and remove label
Link: https://github.com/libvirt/libvirt/commit/77401d549c52ba2b11c2132a8ca41d73a4130c85

This refactor commit directly depends on DIR* being changed to g_autoptr in the below commit:

commit c0ae4919e386cda6e21d3ba022ee187e8b09793b
Author: Laine Stump <email address hidden>
Date: Sun Oct 25 17:50:51 2020 -0400
Subject: change DIR* int g_autoptr(DIR) where appropriate
Link: https://github.com/libvirt/libvirt/commit/c0ae4919e386cda6e21d3ba022ee187e8b09793b#diff-d44ea20881ddbf7bd7b9e4fa9d2a6454812e5109e94160e5f97e2d40ff93a9ca

I think squashing "util: refactor function to simplify and remove label" into "util: Add phys_port_name support on virPCIGetNetName" was probably a bad idea here, without also taking on "change DIR* int g_autoptr(DIR) where appropriate".

I'll try come up with a slightly more sensible backport, possibly keeping the initial implementation.

The attachment "lp-1892132-Add-phys_port_name-support-on-virPCIGetNetName.patch" seems to be a patch. If it isn't, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are a member of the ~ubuntu-reviewers, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issues please contact him.]

Okay, after reviewing the various patches and backports, Frode's backport was correct, it was just missing the "firstEntryName = NULL;" line.

Attached is a debdiff with the "firstEntryName = NULL;" line restored. Only Focal is affected, Hirsute and Impish are working correctly.

I have a test package building in a ppa, will make a testcase now.

Test packages are now available in:

https://launchpad.net/~mruffell/+archive/ubuntu/lp1943481-test

Please note these test packages are NOT SUPPORTED by Canonical, and are for TEST PURPOSES ONLY. ONLY install in a dedicated test environment.

Instructions to Install (on a Focal system):
1) sudo add-apt-repository ppa:mruffell/lp1943481-test
2) sudo apt update
3) sudo apt install libvirt-daemon-system libvirt-clients
4) sudo reboot
5) sudo apt-cache policy libvirt-daemon-system
Installed: 6.0.0-0ubuntu8.13+lp1943481v20210914b2

Patch applied, installed and rebooted. Guest VM comes up now.
Thank you!

libvirt-daemon-system:
  Installed: 6.0.0-0ubuntu8.13+lp1943481v20210914b2
  Candidate: 6.0.0-0ubuntu8.13+lp1943481v20210914b2
  Version table:
 *** 6.0.0-0ubuntu8.13+lp1943481v20210914b2 500
        500 http://ppa.launchpad.net/mruffell/lp1943481-test/ubuntu
focal/main amd64 Packages
        100 /var/lib/dpkg/status
     6.0.0-0ubuntu8.13 500
        500 http://us.archive.ubuntu.com/ubuntu focal-updates/main amd64
Packages
     6.0.0-0ubuntu8.3 500
        500 http://us.archive.ubuntu.com/ubuntu focal-security/main amd64
Packages
     6.0.0-0ubuntu8 500
        500 http://us.archive.ubuntu.com/ubuntu focal/main amd64 Packages

On Mon, Sep 13, 2021 at 8:05 PM Matthew Ruffell <email address hidden>
wrote:

> Test packages are now available in:
>
> https://launchpad.net/~mruffell/+archive/ubuntu/lp1943481-test
>
> Please note these test packages are NOT SUPPORTED by Canonical, and are
> for TEST PURPOSES ONLY. ONLY install in a dedicated test environment.
>
> Instructions to Install (on a Focal system):
> 1) sudo add-apt-repository ppa:mruffell/lp1943481-test
> 2) sudo apt update
> 3) sudo apt install libvirt-daemon-system libvirt-clients
> 4) sudo reboot
> 5) sudo apt-cache policy libvirt-daemon-system
> Installed: 6.0.0-0ubuntu8.13+lp1943481v20210914b2
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1943481
>
> Title:
> libvirtd crashes when creating network interface pools in
> 6.0.0-0ubuntu8.13
>
> Status in libvirt package in Ubuntu:
> New
> Status in libvirt source package in Focal:
> In Progress
>
> Bug description:
> The latest libvirtd (6.0.0-0ubuntu8.13) crashes when trying to bring
> up network pools with the stacktrace below. I tracked down the
> problem to the newly added patch (lp-1892132-Add-phys_port_name-
> support-on-virPCIGetNetName.patch). Assigning *netname =
> firstEntryName; ends up in memory corruption. Looking at the
> mainline, I changed it to the following:
>
> *netname = g_steal_pointer(&firstEntryName);
>
> or you can just do
>
> firstEntryName = NULL;
>
> Both will solve the problem.
>
>
> #0 __GI_raise (sig=sig@entry=6) at
> ../sysdeps/unix/sysv/linux/raise.c:50
> #1 0x00007f40e5d1c859 in __GI_abort () at abort.c:79
> #2 0x00007f40e5d873ee in __libc_message (action=action@entry=do_abort,
> fmt=fmt@entry=0x7f40e5eb1285 "%s\n") at ../sysdeps/posix/libc_fatal.c:155
> #3 0x00007f40e5d8f47c in malloc_printerr (str=str@entry=0x7f40e5eb35d0
> "free(): double free detected in tcache 2") at malloc.c:5347
> #4 0x00007f40e5d910ed in _int_free (av=0x7f40c8000020,
> p=0x7f40c80079e0, have_lock=0) at malloc.c:4201
> #5 0x00007f40e61a9a4f in virFree (ptrptr=0x7f40c8003b60) at
> ../../../src/util/viralloc.c:348
> #6 0x00007f40dd0cf8b1 in networkCreateInterfacePool
> (netdef=0x7f40840187f0) at ../../../src/network/bridge_driver.c:2849
> #7 0x00007f40dd0d799c in networkStartNetworkExternal
> (obj=0x7f408400f720) at ../../../src/network/bridge_driver.c:2938
> #8 networkStartNetwork (driver=driver@entry=0x7f408400a7a0,
...

Hi Paul,

Thank you very much for testing, we appreciate it.

I'll make sure we get this fix fast-tracked, and try get a new libvirt build into -proposed in a couple hours, and try go for a release to -updates in the next day or so.

Thanks,
Matthew

Thank you Matthew for the quick response and I see you already added Frode who was driving this change.

I have reviewed the PPA and the diff that it has right now.

Yes 77401d54 seems present and c0ae4919e would be needed for that as dependency.
The code still has the needed VIR_DIR_CLOSE so it is not also leaking.

Around firstEntryName where the current issue happens the formerly merged refactor had:
  *netname = g_steal_pointer(&firstEntryName);
But the backport was
  *netname = firstEntryName;
And now your proposed solution is to add:
  firstEntryName = NULL;

(all that on a g_autofree type)

Let us play this through:

So in the upstream code it does:
  firstEntryName = NULL; (init)
  firstEntryName = g_strdup(entry->d_name); (assign a value)
  *netname = g_steal_pointer(&firstEntryName); (transfer ownership, sets NULL)
  on return g_autofree frees firstEntryName (being NULL this is a no-op, but a fallback for
    other return paths)

In the current bad backport it does:
  firstEntryName = NULL; (init)
  firstEntryName = g_strdup(entry->d_name); (assign a value)
  *netname = firstEntryName; (direct take over, leaving firstEntryName as-is)
  on return g_autofree frees firstEntryName which still being used from netname causes the crash

The suggestion to set
  firstEntryName = NULL;
will prevent the auto-clean from freeing the value and thereby prevent the segfault.
But will there be any return paths where it would need to be freed because of a leak?
This happens late on the way out which is good.
Yeah - I agree there is no return path that would leak firstEntryName.
But for clarity and matching what eventually is upstream is there a reason not to use g_steal_pointer in the very same place. The includes are already in place other code in virpci.c already uses it.

So instead of
  *netname = firstEntryName;
  firstEntryName = NULL;
maybe
  *netname = g_steal_pointer(&firstEntryName);

Both should be fine, not stopping you.
I've also taken the chance to reference this bug in the "testcase that should be added to regression tests" list.

I also double checked if Hirsute is ok and I agree to comment 5 - Hirsute already uses g_steal_pointer in that place and thereby is fine.

Ok, discussed with Matthew - g_steal_pointer would indeed be ok there as well, but OTOH I've checked the g_steal_pointer in glib2 glib2.0/glib/gmem.h and it does nothing else than what the code does now.
Given that it is a regression-update we do not want to loose any time.

I've done a few more checks and it LGTM also we should not loose any time on it, so I'm sponsoring this into Focal-unapproved.

Only a focal issue, so I marked the general libvirt task as invalid

Sponsored, Matthew will add the updated Description (with the SRU template) any minute.

Hello Paul, or anyone else affected,

Accepted libvirt into focal-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/libvirt/6.0.0-0ubuntu8.14 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-focal to verification-done-focal. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-focal. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Hi Paul,

Could you please install the libvirt packages in -proposed and verify that it fixes the problem?

Instructions to Install (on a Focal system):
1) cat << EOF | sudo tee /etc/apt/sources.list.d/ubuntu-$(lsb_release -cs)-proposed.list
# Enable Ubuntu proposed archive
deb http://archive.ubuntu.com/ubuntu/ $(lsb_release -cs)-proposed main universe
EOF
2) sudo apt update
3) sudo apt install libvirt-daemon-system libvirt-clients
4) sudo reboot
5) sudo apt-cache policy libvirt-daemon-system
Installed: 6.0.0-0ubuntu8.14

You may also wish to remove the -proposed archive once you have libvirt installed, if this is a production machine.
6) sudo rm /etc/apt/sources.list.d/ubuntu-$(lsb_release -cs)-proposed.list

If the fixed package works as expected, we will mark the launchpad bug as verified and fast-track the release to -updates.

Thanks,
Matthew

Installed, rebooted and VM's started up.. Fix is working

apt-cache policy libvirt-daemon-system

libvirt-daemon-system:
  Installed: 6.0.0-0ubuntu8.14
  Candidate: 6.0.0-0ubuntu8.14
  Version table:
 *** 6.0.0-0ubuntu8.14 500
        500 http://archive.ubuntu.com/ubuntu focal-proposed/main amd64
Packages
        100 /var/lib/dpkg/status
     6.0.0-0ubuntu8.13+lp1943481v20210914b2 500
        500 http://ppa.launchpad.net/mruffell/lp1943481-test/ubuntu
focal/main amd64 Packages
     6.0.0-0ubuntu8.13 500
        500 http://us.archive.ubuntu.com/ubuntu focal-updates/main amd64
Packages
     6.0.0-0ubuntu8.3 500
        500 http://us.archive.ubuntu.com/ubuntu focal-security/main amd64
Packages
     6.0.0-0ubuntu8 500
        500 http://us.archive.ubuntu.com/ubuntu focal/main amd64 Packages

On Mon, Sep 13, 2021 at 11:55 PM Matthew Ruffell <email address hidden>
wrote:

> Hi Paul,
>
> Could you please install the libvirt packages in -proposed and verify
> that it fixes the problem?
>
> Instructions to Install (on a Focal system):
> 1) cat << EOF | sudo tee /etc/apt/sources.list.d/ubuntu-$(lsb_release
> -cs)-proposed.list
> # Enable Ubuntu proposed archive
> deb http://archive.ubuntu.com/ubuntu/ $(lsb_release -cs)-proposed main
> universe
> EOF
> 2) sudo apt update
> 3) sudo apt install libvirt-daemon-system libvirt-clients
> 4) sudo reboot
> 5) sudo apt-cache policy libvirt-daemon-system
> Installed: 6.0.0-0ubuntu8.14
>
> You may also wish to remove the -proposed archive once you have libvirt
> installed, if this is a production machine.
> 6) sudo rm /etc/apt/sources.list.d/ubuntu-$(lsb_release -cs)-proposed.list
>
> If the fixed package works as expected, we will mark the launchpad bug
> as verified and fast-track the release to -updates.
>
> Thanks,
> Matthew
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1943481
>
> Title:
> libvirtd crashes when creating network interface pools in
> 6.0.0-0ubuntu8.13
>
> Status in libvirt package in Ubuntu:
> Invalid
> Status in libvirt source package in Focal:
> Fix Committed
>
> Bug description:
> [Impact]
>
> A regression was introduced in libvirt 6.0.0-0ubuntu8.13 for Focal,
> that affects users who use SR-IOV to pass through VF devices to KVM
> guests.
>
> The problem was introduced in the recent lp-1892132-Add-
> phys_port_name-support-on-virPCIGetNetName.patch patch, which changes
> how virPCIGetNetName() fetches the name of the underlying VF device,
> so it can be used to send netlink commands.
>
> There is a fallback case where we record the name of the device at the
> beginning, and if we fail all other lookups, we simply return the
> beginning name.
>
> In libvirt 6.0.0-0ubuntu8.13, a line to drop the reference to
> firstEntryName was dropped incorrectly:
>
> - if (firstEntryName) {
> - *netname = firstEntryName;
> - firstEntryName = NULL;
> - ret = 0;
> + if (firstEntryName) {
> + *netname = firstEntryName;
> + ret = 0;
>
> This results in a double free, as netname and firstEnt...

Thank you Paul!

Early release has been requested, as this is a simple fix this seems ok with some additional testing.

As discussed I've thrown this into some general regression testing.
While it didn't catch the former issue it provides some general coverage and confidence avoiding that something totally unexpected slipped in.

I tested only x86 as that was the system I had ready, but nothing should be arch-specific
prep (x86_64) : Pass 20 F/S/N 0/0/0 - RC 0 (16 min 41390 lin)
migrate (x86_64) : Pass 280 F/S/N 0/0/0 - RC 0 (67 min 212868 lin)
cross (x86_64) : Pass 46 F/S/N 0/0/1 - RC 0 (52 min 72643 lin)
misc (x86_64) : Pass 73 F/S/N 0/0/0 - RC 0 (31 min 41778 lin)

Thereby I think this should now be ready to fix up the regression-update case.

The verification of the Stable Release Update for libvirt has completed successfully and the package is now being released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

This bug was fixed in the package libvirt - 6.0.0-0ubuntu8.14

---------------
libvirt (6.0.0-0ubuntu8.14) focal; urgency=medium

  * Fixup backport of "util: Add phys_port_name support on virPCIGetNetName"
    to include the incorrectly removed "firstEntryName = NULL;" line, which
    caused a regression bringing up network pools. (LP: #1943481)
    - d/p/u/lp-1892132-Add-phys_port_name-support-on-virPCIGetNetName.patch

 -- Matthew Ruffell <email address hidden> Tue, 14 Sep 2021 14:00:49 +1200