On Tue, Mar 19, 2024 at 6:06 AM Si-Wei Liu <si-wei....@oracle.com> wrote:
>
>
>
> On 3/17/2024 8:20 PM, Jason Wang wrote:
> > On Sat, Mar 16, 2024 at 2:33 AM Si-Wei Liu <si-wei....@oracle.com> wrote:
> >>
> >>
> >> On 3/14/2024 8:50 PM, Jason Wang wrote:
> >>> On Fri, Mar 15, 2024 at 5:39 AM Si-Wei Liu <si-wei....@oracle.com> wrote:
> >>>> There could be a mix of both vhost-user and vhost-kernel clients
> >>>> in the same QEMU process, where separate vhost loggers for the
> >>>> specific vhost type have to be used. Make the vhost logger per
> >>>> backend type, and have them properly reference counted.
> >>> It's better to describe what's the advantage of doing this.
> >> Yes, I can add that to the log. Although it's a niche use case, it was
> >> actually a long standing limitation / bug that vhost-user and
> >> vhost-kernel loggers can't co-exist per QEMU process, but today it's
> >> just silent failure that may be ended up with. This bug fix removes that
> >> implicit limitation in the code.
> > Ok.
> >
> >>>> Suggested-by: Michael S. Tsirkin <m...@redhat.com>
> >>>> Signed-off-by: Si-Wei Liu <si-wei....@oracle.com>
> >>>>
> >>>> ---
> >>>> v3->v4:
> >>>> - remove checking NULL return value from vhost_log_get
> >>>>
> >>>> v2->v3:
> >>>> - remove non-effective assertion that never be reached
> >>>> - do not return NULL from vhost_log_get()
> >>>> - add neccessary assertions to vhost_log_get()
> >>>> ---
> >>>> hw/virtio/vhost.c | 45 +++++++++++++++++++++++++++++++++------------
> >>>> 1 file changed, 33 insertions(+), 12 deletions(-)
> >>>>
> >>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >>>> index 2c9ac79..612f4db 100644
> >>>> --- a/hw/virtio/vhost.c
> >>>> +++ b/hw/virtio/vhost.c
> >>>> @@ -43,8 +43,8 @@
> >>>> do { } while (0)
> >>>> #endif
> >>>>
> >>>> -static struct vhost_log *vhost_log;
> >>>> -static struct vhost_log *vhost_log_shm;
> >>>> +static struct vhost_log *vhost_log[VHOST_BACKEND_TYPE_MAX];
> >>>> +static struct vhost_log *vhost_log_shm[VHOST_BACKEND_TYPE_MAX];
> >>>>
> >>>> /* Memslots used by backends that support private memslots (without
> >>>> an fd). */
> >>>> static unsigned int used_memslots;
> >>>> @@ -287,6 +287,10 @@ static int vhost_set_backend_type(struct vhost_dev
> >>>> *dev,
> >>>> r = -1;
> >>>> }
> >>>>
> >>>> + if (r == 0) {
> >>>> + assert(dev->vhost_ops->backend_type == backend_type);
> >>>> + }
> >>>> +
> >>> Under which condition could we hit this?
> >> Just in case some other function inadvertently corrupted this earlier,
> >> we have to capture discrepancy in the first place... On the other hand,
> >> it will be helpful for other vhost backend writers to diagnose day-one
> >> bug in the code. I feel just code comment here will not be
> >> sufficient/helpful.
> > See below.
> >
> >>> It seems not good to assert a local logic.
> >> It seems to me quite a few local asserts are in the same file already,
> >> vhost_save_backend_state,
> > For example it has assert for
> >
> > assert(!dev->started);
> >
> > which is not the logic of the function itself but require
> > vhost_dev_start() not to be called before.
> >
> > But it looks like this patch you assert the code just a few lines
> > above the assert itself?
> Yes, that was the intent - for e.g. xxx_ops may contain corrupted
> xxx_ops.backend_type already before coming to this
> vhost_set_backend_type() function. And we may capture this corrupted
> state by asserting the expected xxx_ops.backend_type (to be consistent
> with the backend_type passed in),
This can happen for all variables. Not sure why backend_ops is special.
> which needs be done in the first place
> when this discrepancy is detected. In practice I think there should be
> no harm to add this assert, but this will add warranted guarantee to the
> current code.
For example, such corruption can happen after the assert() so a TOCTOU issue.
Thanks
>
> Regards,
> -Siwei
>
> >
> > dev->vhost_ops = &xxx_ops;
> >
> > ...
> >
> > assert(dev->vhost_ops->backend_type == backend_type)
> >
> > ?
> >
> > Thanks
> >
> >> vhost_load_backend_state,
> >> vhost_virtqueue_mask, vhost_config_mask, just to name a few. Why local
> >> assert a problem?
> >>
> >> Thanks,
> >> -Siwei
> >>
> >>> Thanks
> >>>
>
