vhost: fix vhost user virtqueue not accessable
Checks
Commit Message
Log feature is disabled in vhost user, so that log address was invalid
when checking. Add feature bit check can skip useless address check.
Fixes: 04cfc7fdbfca ("vhost: translate incoming log address to gpa")
Signed-off-by: Marvin Liu <yong.liu@intel.com>
---
lib/librte_vhost/vhost_user.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
Comments
Hi Marvin,
On 10/25/19 6:20 PM, Marvin Liu wrote:
> Log feature is disabled in vhost user, so that log address was invalid
> when checking. Add feature bit check can skip useless address check.
>
Just so I understand, what conditions is the log address invalid?
> Fixes: 04cfc7fdbfca ("vhost: translate incoming log address to gpa")
>
> Signed-off-by: Marvin Liu <yong.liu@intel.com>
> ---
> lib/librte_vhost/vhost_user.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 61ef699ac..0407fdc29 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -741,13 +741,15 @@ translate_ring_addresses(struct virtio_net *dev, int vq_index)
> vq->last_avail_idx = vq->used->idx;
> }
>
> - vq->log_guest_addr =
> - translate_log_addr(dev, vq, addr->log_guest_addr);
> - if (vq->log_guest_addr == 0) {
> - RTE_LOG(DEBUG, VHOST_CONFIG,
> - "(%d) failed to map log_guest_addr .\n",
> - dev->vid);
> - return dev;
> + if (dev->features & (1ULL << VHOST_F_LOG_ALL)) {
> + vq->log_guest_addr =
> + translate_log_addr(dev, vq, addr->log_guest_addr);
VHOST_F_LOG_ALL is only negotiated once the migration has started (at least from
qemu's perspective).
That means that we will postponing the translation of the log address to the
vhost_user_set_vring_addr() call that follows the VHOST_F_LOG_ALL enabling. In
that call there are (at least) two things that could go wrong and lead to a
migration failure:
- If VHOST_USER_F_PROTOCOL_FEATURES is not enabled, the address won't be translated:
vhost_user:795
if ((vq->enabled && (dev->features &
(1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) ||
access_ok) {
dev = translate_ring_addresses(dev, msg->payload.addr.index);
if (!dev)
return RTE_VHOST_MSG_RESULT_ERR;
*pdev = dev;
}
- If the IOMMU is enabled and there's a miss, we would have to wait for the
IOTLB_UPDATE and during that time, there would be failed accesses to the (still
untranslated) log address.
> + if (vq->log_guest_addr == 0) {
> + RTE_LOG(DEBUG, VHOST_CONFIG,
> + "(%d) failed to map log_guest_addr .\n",
> + dev->vid);
> + return dev;
> + }
> }
> vq->access_ok = 1;
>
>
Thanks,
Adrian
> -----Original Message-----
> From: Adrian Moreno [mailto:amorenoz@redhat.com]
> Sent: Friday, October 25, 2019 8:21 PM
> To: Liu, Yong <yong.liu@intel.com>; maxime.coquelin@redhat.com; Bie, Tiwei
> <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH] vhost: fix vhost user virtqueue not accessable
>
> Hi Marvin,
>
> On 10/25/19 6:20 PM, Marvin Liu wrote:
> > Log feature is disabled in vhost user, so that log address was invalid
> > when checking. Add feature bit check can skip useless address check.
> >
> Just so I understand, what conditions is the log address invalid?
>
> > Fixes: 04cfc7fdbfca ("vhost: translate incoming log address to gpa")
> >
> > Signed-off-by: Marvin Liu <yong.liu@intel.com>
> > ---
> > lib/librte_vhost/vhost_user.c | 16 +++++++++-------
> > 1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/lib/librte_vhost/vhost_user.c
> b/lib/librte_vhost/vhost_user.c
> > index 61ef699ac..0407fdc29 100644
> > --- a/lib/librte_vhost/vhost_user.c
> > +++ b/lib/librte_vhost/vhost_user.c
> > @@ -741,13 +741,15 @@ translate_ring_addresses(struct virtio_net *dev,
> int vq_index)
> > vq->last_avail_idx = vq->used->idx;
> > }
> >
> > - vq->log_guest_addr =
> > - translate_log_addr(dev, vq, addr->log_guest_addr);
> > - if (vq->log_guest_addr == 0) {
> > - RTE_LOG(DEBUG, VHOST_CONFIG,
> > - "(%d) failed to map log_guest_addr .\n",
> > - dev->vid);
> > - return dev;
> > + if (dev->features & (1ULL << VHOST_F_LOG_ALL)) {
> > + vq->log_guest_addr =
> > + translate_log_addr(dev, vq, addr->log_guest_addr);
>
> VHOST_F_LOG_ALL is only negotiated once the migration has started (at least
> from
> qemu's perspective).
> That means that we will postponing the translation of the log address to
> the
> vhost_user_set_vring_addr() call that follows the VHOST_F_LOG_ALL enabling.
> In
> that call there are (at least) two things that could go wrong and lead to a
> migration failure:
> - If VHOST_USER_F_PROTOCOL_FEATURES is not enabled, the address won't be
> translated:
>
> vhost_user:795
> if ((vq->enabled && (dev->features &
> (1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) ||
> access_ok) {
> dev = translate_ring_addresses(dev, msg->payload.addr.index);
> if (!dev)
> return RTE_VHOST_MSG_RESULT_ERR;
>
> *pdev = dev;
> }
>
> - If the IOMMU is enabled and there's a miss, we would have to wait for the
> IOTLB_UPDATE and during that time, there would be failed accesses to the
> (still
> untranslated) log address.
>
>
Thanks, Adrian.
Log address can be zero when logging is not enabled.
How about add other criteria after translation? Log address will be translated anyway and not affect vq status.
vq->log_guest_addr =
translate_log_addr(dev, vq, addr->log_guest_addr);
- if (vq->log_guest_addr == 0) {
+ if (vq->log_guest_addr == 0 && addr->flags) {
RTE_LOG(DEBUG, VHOST_CONFIG,
"(%d) failed to map log_guest_addr .\n",
dev->vid);
return dev;
}
Meanwhile, log address of packed ring is fixed to zero. Is any special reason to do that?
Regards,
Marvin
>
> > + if (vq->log_guest_addr == 0) {
> > + RTE_LOG(DEBUG, VHOST_CONFIG,
> > + "(%d) failed to map log_guest_addr .\n",
> > + dev->vid);
> > + return dev;
> > + }
> > }
> > vq->access_ok = 1;
> >
> >
> Thanks,
> Adrian
On 10/28/19 3:13 AM, Liu, Yong wrote:
>
>
>> -----Original Message-----
>> From: Adrian Moreno [mailto:amorenoz@redhat.com]
>> Sent: Friday, October 25, 2019 8:21 PM
>> To: Liu, Yong <yong.liu@intel.com>; maxime.coquelin@redhat.com; Bie, Tiwei
>> <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH] vhost: fix vhost user virtqueue not accessable
>>
>> Hi Marvin,
>>
>> On 10/25/19 6:20 PM, Marvin Liu wrote:
>>> Log feature is disabled in vhost user, so that log address was invalid
>>> when checking. Add feature bit check can skip useless address check.
>>>
>> Just so I understand, what conditions is the log address invalid?
>>
>>> Fixes: 04cfc7fdbfca ("vhost: translate incoming log address to gpa")
>>>
>>> Signed-off-by: Marvin Liu <yong.liu@intel.com>
>>> ---
>>> lib/librte_vhost/vhost_user.c | 16 +++++++++-------
>>> 1 file changed, 9 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/lib/librte_vhost/vhost_user.c
>> b/lib/librte_vhost/vhost_user.c
>>> index 61ef699ac..0407fdc29 100644
>>> --- a/lib/librte_vhost/vhost_user.c
>>> +++ b/lib/librte_vhost/vhost_user.c
>>> @@ -741,13 +741,15 @@ translate_ring_addresses(struct virtio_net *dev,
>> int vq_index)
>>> vq->last_avail_idx = vq->used->idx;
>>> }
>>>
>>> - vq->log_guest_addr =
>>> - translate_log_addr(dev, vq, addr->log_guest_addr);
>>> - if (vq->log_guest_addr == 0) {
>>> - RTE_LOG(DEBUG, VHOST_CONFIG,
>>> - "(%d) failed to map log_guest_addr .\n",
>>> - dev->vid);
>>> - return dev;
>>> + if (dev->features & (1ULL << VHOST_F_LOG_ALL)) {
>>> + vq->log_guest_addr =
>>> + translate_log_addr(dev, vq, addr->log_guest_addr);
>>
>> VHOST_F_LOG_ALL is only negotiated once the migration has started (at least
>> from
>> qemu's perspective).
>> That means that we will postponing the translation of the log address to
>> the
>> vhost_user_set_vring_addr() call that follows the VHOST_F_LOG_ALL enabling.
>> In
>> that call there are (at least) two things that could go wrong and lead to a
>> migration failure:
>> - If VHOST_USER_F_PROTOCOL_FEATURES is not enabled, the address won't be
>> translated:
>>
>> vhost_user:795
>> if ((vq->enabled && (dev->features &
>> (1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) ||
>> access_ok) {
>> dev = translate_ring_addresses(dev, msg->payload.addr.index);
>> if (!dev)
>> return RTE_VHOST_MSG_RESULT_ERR;
>>
>> *pdev = dev;
>> }
>>
>> - If the IOMMU is enabled and there's a miss, we would have to wait for the
>> IOTLB_UPDATE and during that time, there would be failed accesses to the
>> (still
>> untranslated) log address.
>>
>>
>
> Thanks, Adrian.
> Log address can be zero when logging is not enabled.
> How about add other criteria after translation? Log address will be translated anyway and not affect vq status.
>
> vq->log_guest_addr =
> translate_log_addr(dev, vq, addr->log_guest_addr);
> - if (vq->log_guest_addr == 0) {
> + if (vq->log_guest_addr == 0 && addr->flags) {
> RTE_LOG(DEBUG, VHOST_CONFIG,
> "(%d) failed to map log_guest_addr .\n",
> dev->vid);
> return dev;
> }
>
That makes perfect sense IMHO. Thank you.
> Meanwhile, log address of packed ring is fixed to zero. Is any special reason to do that?
>
No idea to be honest.
Thanks.
Adrian
> Regards,
> Marvin
>
>>
>>> + if (vq->log_guest_addr == 0) {
>>> + RTE_LOG(DEBUG, VHOST_CONFIG,
>>> + "(%d) failed to map log_guest_addr .\n",
>>> + dev->vid);
>>> + return dev;
>>> + }
>>> }
>>> vq->access_ok = 1;
>>>
>>>
>> Thanks,
>> Adrian
@@ -741,13 +741,15 @@ translate_ring_addresses(struct virtio_net *dev, int vq_index)
vq->last_avail_idx = vq->used->idx;
}
- vq->log_guest_addr =
- translate_log_addr(dev, vq, addr->log_guest_addr);
- if (vq->log_guest_addr == 0) {
- RTE_LOG(DEBUG, VHOST_CONFIG,
- "(%d) failed to map log_guest_addr .\n",
- dev->vid);
- return dev;
+ if (dev->features & (1ULL << VHOST_F_LOG_ALL)) {
+ vq->log_guest_addr =
+ translate_log_addr(dev, vq, addr->log_guest_addr);
+ if (vq->log_guest_addr == 0) {
+ RTE_LOG(DEBUG, VHOST_CONFIG,
+ "(%d) failed to map log_guest_addr .\n",
+ dev->vid);
+ return dev;
+ }
}
vq->access_ok = 1;