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

[Hanna Czenczek]

On 23.07.24 12:45, Michael S. Tsirkin wrote:
> On Tue, Jul 23, 2024 at 12:18:48PM +0200, Hanna Czenczek wrote:
> > 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.
> drop what specifically?

My patch, "virtio: Always reset vhost devices".

Judging from the CI trace, the problem to me appears to be that not 
making get_vhost() depend on vhost_started lets ubsan find a way to have 
get_vhost_net() return NULL (which I don’t find surprising, that’s why 
we check get_vhost()’s return value for whether it’s NULL).  It then 
complains about the `&net->dev` in virtio_net_get_vhost(), because net 
is NULL, and it’s not entirely clear that `&net->dev == NULL`, too (it 
is, which is why I decided to leave that piece of code be, originally).

So dropping my patch should make CI succeed again.

Hanna