On Thu, Mar 21, 2024 at 4:29 AM Si-Wei Liu <si-wei....@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....@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. > 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 > >>>>> >