Re: [PATCH v4 1/2] vhost: dirty log should be per backend type

[Jason Wang]

On Thu, Mar 21, 2024 at 4:29 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 3/19/2024 8:25 PM, Jason Wang wrote:
> > On Tue, Mar 19, 2024 at 6:06 AM Si-Wei Liu <si-wei.liu@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.liu@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.liu@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 <mst@redhat.com>
> >>>>>> Signed-off-by: Si-Wei Liu <si-wei.liu@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.
> The assert is just checking the backend_type field only. The other op
> fields in backend_ops have similar assert within the op function itself
> also. For e.g. vhost_user_requires_shm_log() and a lot of other
> vhost_user ops have the following:
>
>      assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
>
> vhost_vdpa_vq_get_addr() and a lot of other vhost_vdpa ops have:
>
>      assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA);
>
> vhost_kernel ops has similar assertions as well.
>
> The reason why it has to be checked against here is now the callers of
> vhost_log_get(), would pass in dev->vhost_ops->backend_type to the API,
> which are unable to verify the validity of the backend_type by
> themselves. The vhost_log_get() has necessary asserts to make bound
> check for the vhost_log[] or vhost_log_shm[] array, but specific assert
> against the exact backend type in vhost_set_backend_type() will further
> harden the implementation in vhost_log_get() and other backend ops.

As discussed, those assertions are to make sure of the logic
dependencies of other functions. (The assignment of vhost_ops doesn't
happen in those functions).

>
> >
> >> 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.
> Sure, it's best effort only. As pointed out earlier, I think together
> with this, there are other similar asserts already in various backend
> ops, which could be helpful to nail down the earliest point or a
> specific range where things may go wrong in the first place.

Ok, I think I'd leave the decision to Michael.

Thanks

>
> Thanks,
> -Siwei
>
> >
> > 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
> >>>>>
>