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

[Si-Wei Liu <si-wei.liu@oracle.com>]

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.

>
>> 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.

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
>>>>>