I identified with the author’s reluctance to launch a debugger versus rebuilding with a quick n dirty print statement. But (as the author also seems to accept) this is short term gain for long term pain. The extra time to launch a debugger and mentally reset is easily exceeded by the time that would be spent not doing this and endlessly hacking away adding in more logging.
Shouldn't bus_register_notifier() take a notifier_fn_t (and int priority) directly and create its own struct notifier_block? Why does it expose its linked-list node through its interface?
I guess maybe that would make un-registration less ergonomic (assuming you have to handle duplicate entries in the list). But the peril of the bug described here seems worse.
Isn't the problem that both CONFIG_IBMVIO and CONFIG_PCI are defined?
I had to see what IBMVIO was, it looks like the IBM Virtual I/O Server. Although you actually fixed the bug, the other bug might be that IBMVIO is defined when it's "The VIOS is part of the PowerVM® Editions hardware feature"
QEMU (with its pseries machine type) doesn't actually run the PowerVM hypervisor, but rather it directly implements a very similar hypervisor-guest interface (based on the Power Architecture Platform Reference and Open Firmware) such that they are treated as the same hardware platform in Linux's eyes.
QEMU is also capable of emulating a variety of other PowerPC machine types, including PowerNV/OpenPOWER bare-metal systems, various Power Macs, RS/6000s etc.
Makes me wonder how many other lurking type confusion bugs there are. KASAN only found this one because it happened to produce an out-of-bounds access (I guess pci_dev is larger than whatever struct the pointer really pointed to); it can’t detect the type confusion itself.
What the author doesn't explain, but I find myself wondering, is how did this go unnoticed for so long? What conditions expose it? Obviously it's easy enough to trigger in QEMU, but is real hardware in that condition so rare?
I feel like I missed a bit of context for what launched this whole adventure. The adventure itself was interesting, though!
It seems you need at least: 1. A PowerPC machine (with IOMMU, so I guess it can't be too old). 2. A PCI VGA card. (Emulated, it seems.) 3. IBM VIO bus support compiled in. 4. A KASAN kernel booted to get told about the issue so clearly, otherwise the failure may be much more subtle.
So, yeah, that's quite the confluence of things... Do PowerPC folks usually run their VMs with VGA emulation for example?
EDIT: Actually never mind, I think that just having PCI bus support and VGA arbitration support compiled in may be sufficient, instead of actually having a VGA card (real or not)?
I think, from my quick reading of the code, that it's triggerable by having VGA arbitration enabled (which requires having PCI enabled) and having IBM VIO support compiled in, without having a VGA device.
In any case, QEMU will give you a VGA device when run with default options, so it's not that rare for developers to run VMs with a VGA card.
This particular bug was found by one of our excellent test engineers, I'm not sure what specifically he was doing that led him to hit this bug, beyond testing stuff with KASAN enabled on a real machine using the IBM PowerVM hypervisor.
I think this is a great example where modern languages are better.
- It shows that declaring statics is unsafe.
- It shows that taking a reference to a static is inherently unsafe.
- It shows that it actually helps if we distinguish passing something as mutable or non-mutable, and keep said mutability exclusive.
I find this code VERY opaque to read in terms of what it is that we are actually passing.
I'm also confused why the creation of the structs wasn't and still isn't wrapped in an `ifdef`. Now if I only have a VIO device there is still a lingering `fail_iommu_pci_bus_notifier` in the code at top level. I'm not familiar enough with how this is compiled, but it looks like it's part of the public interface, and as such would not be able to be compiled away?
12 comments
[ 2.8 ms ] story [ 61.9 ms ] threadI guess maybe that would make un-registration less ergonomic (assuming you have to handle duplicate entries in the list). But the peril of the bug described here seems worse.
I had to see what IBMVIO was, it looks like the IBM Virtual I/O Server. Although you actually fixed the bug, the other bug might be that IBMVIO is defined when it's "The VIOS is part of the PowerVM® Editions hardware feature"
Were you on an IBM power box?
https://www.ibm.com/docs/en/power8?topic=server-virtual-io-o...
I'm just curious, because the IFDEFs assume one-or-the-other as written. Is having both busses a valid configuration?
Thanks!
QEMU is also capable of emulating a variety of other PowerPC machine types, including PowerNV/OpenPOWER bare-metal systems, various Power Macs, RS/6000s etc.
I feel like I missed a bit of context for what launched this whole adventure. The adventure itself was interesting, though!
So, yeah, that's quite the confluence of things... Do PowerPC folks usually run their VMs with VGA emulation for example?
EDIT: Actually never mind, I think that just having PCI bus support and VGA arbitration support compiled in may be sufficient, instead of actually having a VGA card (real or not)?
In any case, QEMU will give you a VGA device when run with default options, so it's not that rare for developers to run VMs with a VGA card.
This particular bug was found by one of our excellent test engineers, I'm not sure what specifically he was doing that led him to hit this bug, beyond testing stuff with KASAN enabled on a real machine using the IBM PowerVM hypervisor.
- It shows that declaring statics is unsafe.
- It shows that taking a reference to a static is inherently unsafe.
- It shows that it actually helps if we distinguish passing something as mutable or non-mutable, and keep said mutability exclusive.
I find this code VERY opaque to read in terms of what it is that we are actually passing.
I'm also confused why the creation of the structs wasn't and still isn't wrapped in an `ifdef`. Now if I only have a VIO device there is still a lingering `fail_iommu_pci_bus_notifier` in the code at top level. I'm not familiar enough with how this is compiled, but it looks like it's part of the public interface, and as such would not be able to be compiled away?