| Message ID | 1710448055-11709-1-git-send-email-si-wei.liu@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/2] vhost: dirty log should be per backend type | expand |
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. > > 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? It seems not good to assert a local logic. Thanks
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. > >> 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. > 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, vhost_load_backend_state, vhost_virtqueue_mask, vhost_config_mask, just to name a few. Why local assert a problem? Thanks, -Siwei > Thanks >
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? 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 > > >
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), 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. 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 >>>
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. > 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 > >>> >
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 >>>>>
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 > >>>>> >
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); + } + return r; } @@ -319,16 +323,22 @@ static struct vhost_log *vhost_log_alloc(uint64_t size, bool share) return log; } -static struct vhost_log *vhost_log_get(uint64_t size, bool share) +static struct vhost_log *vhost_log_get(VhostBackendType backend_type, + uint64_t size, bool share) { - struct vhost_log *log = share ? vhost_log_shm : vhost_log; + struct vhost_log *log; + + assert(backend_type > VHOST_BACKEND_TYPE_NONE); + assert(backend_type < VHOST_BACKEND_TYPE_MAX); + + log = share ? vhost_log_shm[backend_type] : vhost_log[backend_type]; if (!log || log->size != size) { log = vhost_log_alloc(size, share); if (share) { - vhost_log_shm = log; + vhost_log_shm[backend_type] = log; } else { - vhost_log = log; + vhost_log[backend_type] = log; } } else { ++log->refcnt; @@ -340,11 +350,20 @@ static struct vhost_log *vhost_log_get(uint64_t size, bool share) static void vhost_log_put(struct vhost_dev *dev, bool sync) { struct vhost_log *log = dev->log; + VhostBackendType backend_type; if (!log) { return; } + assert(dev->vhost_ops); + backend_type = dev->vhost_ops->backend_type; + + if (backend_type == VHOST_BACKEND_TYPE_NONE || + backend_type >= VHOST_BACKEND_TYPE_MAX) { + return; + } + --log->refcnt; if (log->refcnt == 0) { /* Sync only the range covered by the old log */ @@ -352,13 +371,13 @@ static void vhost_log_put(struct vhost_dev *dev, bool sync) vhost_log_sync_range(dev, 0, dev->log_size * VHOST_LOG_CHUNK - 1); } - if (vhost_log == log) { + if (vhost_log[backend_type] == log) { g_free(log->log); - vhost_log = NULL; - } else if (vhost_log_shm == log) { + vhost_log[backend_type] = NULL; + } else if (vhost_log_shm[backend_type] == log) { qemu_memfd_free(log->log, log->size * sizeof(*(log->log)), log->fd); - vhost_log_shm = NULL; + vhost_log_shm[backend_type] = NULL; } g_free(log); @@ -376,7 +395,8 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev) static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size) { - struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev)); + struct vhost_log *log = vhost_log_get(dev->vhost_ops->backend_type, + size, vhost_dev_log_is_shared(dev)); uint64_t log_base = (uintptr_t)log->log; int r; @@ -2037,7 +2057,8 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings) uint64_t log_base; hdev->log_size = vhost_get_log_size(hdev); - hdev->log = vhost_log_get(hdev->log_size, + hdev->log = vhost_log_get(hdev->vhost_ops->backend_type, + hdev->log_size, vhost_dev_log_is_shared(hdev)); log_base = (uintptr_t)hdev->log->log; r = hdev->vhost_ops->vhost_set_log_base(hdev,
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. 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(-)