Hi Mattias,
On 5/21/19 9:43 PM, Mattias Rönnblom wrote:
On 2019-05-17 14:22, Maxime Coquelin wrote:
Handling of fragmented virtio-net header and indirect descriptors
tables was implemented to fix CVE-2018-1059. It should not never
happen with healthy guests and so are already considered as
unlikely code path.
This patch moves these bits into non-inline dedicated functions
to reduce the I-cache pressure.
Signed-off-by: Maxime Coquelin <maxime.coque...@redhat.com>
---
lib/librte_vhost/vhost.c | 33 +++++++++++
lib/librte_vhost/vhost.h | 35 +-----------
lib/librte_vhost/virtio_net.c | 102 +++++++++++++++++++---------------
3 files changed, 91 insertions(+), 79 deletions(-)
diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index 4a54ad6bd1..8a4379bc13 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -201,6 +201,39 @@ __vhost_log_cache_write(struct virtio_net *dev,
struct vhost_virtqueue *vq,
}
+void *
+alloc_copy_ind_table(struct virtio_net *dev, struct vhost_virtqueue *vq,
This function should have a prefix.
This function is just moved from vhost.h to vhost.c, so not the purpose
of the patch.
But I agree your comment, I'll send a patch to add a prefix.
+ uint64_t desc_addr, uint64_t desc_len)
+{
+ void *idesc;
+ uint64_t src, dst;
+ uint64_t len, remain = desc_len;
+
+ idesc = rte_malloc(__func__, desc_len, 0);
+ if (unlikely(!idesc))
if (idesc == NULL)
Ditto, that is not the purpose of the patch that is just moving the
function.
I agree this is not matching the coding rules specified in the
documentation, though.
+ return NULL;
+
+ dst = (uint64_t)(uintptr_t)idesc;
+
+ while (remain) {
remain > 0
Ditto.
+ len = remain;
+ src = vhost_iova_to_vva(dev, vq, desc_addr, &len,
+ VHOST_ACCESS_RO);
+ if (unlikely(!src || !len)) {
+ rte_free(idesc);
+ return NULL;
+ }
+
+ rte_memcpy((void *)(uintptr_t)dst, (void *)(uintptr_t)src, len);
Just for my understanding: what difference does that (uintptr_t) cast do?
This is required to build 32bits (-Werror=int-to-pointer-cast)
+
+ remain -= len;
+ dst += len;
+ desc_addr += len;
+ }
+
+ return idesc;
+}
+
void
cleanup_vq(struct vhost_virtqueue *vq, int destroy)
{
diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
index 3ab7b4950f..ab26454e1c 100644
--- a/lib/librte_vhost/vhost.h
+++ b/lib/librte_vhost/vhost.h
@@ -488,6 +488,8 @@ void vhost_backend_cleanup(struct virtio_net *dev);
uint64_t __vhost_iova_to_vva(struct virtio_net *dev, struct
vhost_virtqueue *vq,
uint64_t iova, uint64_t *len, uint8_t perm);
+void *alloc_copy_ind_table(struct virtio_net *dev, struct
vhost_virtqueue *vq,
+ uint64_t desc_addr, uint64_t desc_len);
int vring_translate(struct virtio_net *dev, struct vhost_virtqueue
*vq);
void vring_invalidate(struct virtio_net *dev, struct vhost_virtqueue
*vq);
@@ -601,39 +603,6 @@ vhost_vring_call_packed(struct virtio_net *dev,
struct vhost_virtqueue *vq)
eventfd_write(vq->callfd, (eventfd_t)1);
}
-static __rte_always_inline void *
-alloc_copy_ind_table(struct virtio_net *dev, struct vhost_virtqueue *vq,
- uint64_t desc_addr, uint64_t desc_len)
-{
- void *idesc;
- uint64_t src, dst;
- uint64_t len, remain = desc_len;
-
- idesc = rte_malloc(__func__, desc_len, 0);
- if (unlikely(!idesc))
- return 0;
-
- dst = (uint64_t)(uintptr_t)idesc;
-
- while (remain) {
- len = remain;
- src = vhost_iova_to_vva(dev, vq, desc_addr, &len,
- VHOST_ACCESS_RO);
- if (unlikely(!src || !len)) {
- rte_free(idesc);
- return 0;
- }
-
- rte_memcpy((void *)(uintptr_t)dst, (void *)(uintptr_t)src, len);
-
- remain -= len;
- dst += len;
- desc_addr += len;
- }
-
- return idesc;
-}
-
static __rte_always_inline void
free_ind_table(void *idesc)
{
diff --git a/lib/librte_vhost/virtio_net.c
b/lib/librte_vhost/virtio_net.c
index 35ae4992c2..494dd9957e 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -610,6 +610,35 @@ reserve_avail_buf_packed(struct virtio_net *dev,
struct vhost_virtqueue *vq,
return 0;
}
+static void
+copy_vnet_hdr_to_desc(struct virtio_net *dev, struct vhost_virtqueue
*vq,
__rte_noinline? Or you don't care about this function being inlined or not?
Right, I'll add it here and there in next revision.
I'll try to send a patch to fix the kind of style issues you reported.
If you want to do it that would be great, just let me know.
Thanks,
Maxime
