Re: [PULL 00/63] virtio,pci,pc: features,fixes

[Hanna Czenczek <hreitz@redhat.com>]

On 22.07.24 23:32, Richard Henderson wrote:
> On 7/22/24 10:16, Michael S. Tsirkin wrote:
>> A couple of fixes are outstanding, will merge later.
>>
>>
>> The following changes since commit 
>> a87a7c449e532130d4fa8faa391ff7e1f04ed660:
>>
>>    Merge tag 'pull-loongarch-20240719' 
>> ofhttps://gitlab.com/gaosong/qemu into staging (2024-07-19 16:28:28 
>> +1000)
>>
>> are available in the Git repository at:
>>
>>    https://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git 
>> tags/for_upstream
>>
>> for you to fetch changes up to 67d834362c55d6fca6504975bc34755606f17cf2:
>>
>>    virtio: Always reset vhost devices (2024-07-21 14:45:56 -0400)
>>
>> ----------------------------------------------------------------
>> virtio,pci,pc: features,fixes
>>
>> pci: Initial support for SPDM Responders
>> cxl: Add support for scan media, feature commands, device patrol scrub
>>      control, DDR5 ECS control, firmware updates
>> virtio: in-order support
>> virtio-net: support for SR-IOV emulation (note: known issues on s390,
>>                                            might get reverted if not 
>> fixed)
>> smbios: memory device size is now configurable per Machine
>> cpu: architecture agnostic code to support vCPU Hotplug
>>
>> Fixes, cleanups all over the place.
>>
>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>
> Fails ubsan testing:
>
> https://gitlab.com/qemu-project/qemu/-/jobs/7397450714
>
> ../publish/hw/net/virtio-net.c:3895:18: runtime error: member access 
> within null pointer of type 'struct vhost_net'

Honestly, I saw this piece of code, but concluded it already doesn’t 
make sense, so I assumed someone™ who wrote this would know why it’s 
been written this way, and I should rather not touch it.

Specifically, the problem is that get_vhost_net() can return a NULL 
pointer[1], which is fine, but virtio_net_get_vhost() never checks 
this.  I assumed this was written with intent (i.e. 
`(uintptr_t)&net->dev == (uintptr_t)net`, so that NULL remains NULL), 
because it’s so obvious that get_vhost_net() can happily return NULL 
under many circumstances, but maybe not.

The same theoretically applies to virtio_crypto_get_vhost(), although I 
don’t think that can ever be NULL in practice.

I’ll re-send the reset patch in a series with two patches that fix those 
two functions to check for NULL and explicitly return NULL if 
necessary.  In the meantime, it probably makes sense to drop it from 
this pull request.

Hanna

[1] For some reason, it uses integer 0 throughout to signify NULL. That 
was another reason that put me off touching this.