vhost-user: avoid misaligned access

-----Original Message-----
From: Michael S. Tsirkin [mailto:[email protected]]
Sent: Saturday, March 17, 2018 2:20 AM
To: [email protected]
Cc: Zhoujian (jay) <[email protected]>
Subject: [PATCH] vhost-user: avoid misaligned access

We can't pass a pointer to memory field directly since it's within a packed structure, so isn't aligned.
Pass a pointer on stack and copy.

Fixes 30c4cc7 ("vhost: used_memslots refactoring")

Cc Jay Zhou <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---

I had to apply this to fix make check errors with clang. Pls review, test and ack.

Thanks!

 hw/virtio/vhost-user.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

hw/virtio/vhost-user.c index renamed from hw/virtio/vhost-user.c +0 -0

c12fdd9..a44ee7f 100644

hw/virtio/vhost-user.c +1 -0

@@ -396,6 +396,7 @@ static int vhost_user_set_mem_table_postcopy(struct
 vhost_dev *dev,
     bool reply_supported = virtio_has_feature(dev->protocol_features,
 
 VHOST_USER_PROTOCOL_F_REPLY_ACK);
     VhostUserMsg msg_reply;
+    VhostUserMemory memory = {};
     int region_i, msg_i;

     VhostUserMsg msg = {

hw/virtio/vhost-user.c +6 -2

@@ -407,10 +408,11 @@ static int vhost_user_set_mem_table_postcopy(struct
 vhost_dev *dev,
         msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
     }
 
-    if (vhost_user_prepare_msg(dev, &msg.payload.memory, fds) < 0) {
+    if (vhost_user_prepare_msg(dev, &memory, fds) < 0) {
         error_report("Failed preparing vhost-user memory table msg");
         return -1;
     }
+    msg.payload.memory = memory;
 
     fd_num = msg.payload.memory.nregions;

@@ -549,16 +551,19 @@ static int vhost_user_set_mem_table(struct vhost_dev
 *dev,
         .hdr.request = VHOST_USER_SET_MEM_TABLE,
         .hdr.flags = VHOST_USER_VERSION,
     };
+    VhostUserMemory memory = {};
 
     if (reply_supported) {
         msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
     }
 
-    if (vhost_user_prepare_msg(dev, &msg.payload.memory, fds) < 0) {
+    if (vhost_user_prepare_msg(dev, &memory, fds) < 0) {
         error_report("Failed preparing vhost-user memory table msg");
         return -1;
     }
 
+    msg.payload.memory = memory;
+
     fd_num = msg.payload.memory.nregions;
 

     if (!fd_num) {

hw/virtio/vhost-user.c +0 -1

 const VhostOps user_ops = {
--

MST

Quoted 84 lines

-----Original Message-----
From: Michael S. Tsirkin [mailto:[email protected]]
Sent: Saturday, March 17, 2018 2:20 AM
To: [email protected]
Cc: Zhoujian (jay) <[email protected]>
Subject: [PATCH] vhost-user: avoid misaligned access

We can't pass a pointer to memory field directly since it's within a packed structure, so isn't aligned.
Pass a pointer on stack and copy.

Fixes 30c4cc7 ("vhost: used_memslots refactoring")

Cc Jay Zhou <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---

I had to apply this to fix make check errors with clang. Pls review, test and ack.

Thanks!

 hw/virtio/vhost-user.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

hw/virtio/vhost-user.c index renamed from hw/virtio/vhost-user.c +0 -0

c12fdd9..a44ee7f 100644

hw/virtio/vhost-user.c +1 -0

@@ -396,6 +396,7 @@ static int vhost_user_set_mem_table_postcopy(struct
 vhost_dev *dev,
     bool reply_supported = virtio_has_feature(dev->protocol_features,
 
 VHOST_USER_PROTOCOL_F_REPLY_ACK);
     VhostUserMsg msg_reply;
+    VhostUserMemory memory = {};
     int region_i, msg_i;

     VhostUserMsg msg = {

hw/virtio/vhost-user.c +6 -2

@@ -407,10 +408,11 @@ static int vhost_user_set_mem_table_postcopy(struct
 vhost_dev *dev,
         msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
     }
 
-    if (vhost_user_prepare_msg(dev, &msg.payload.memory, fds) < 0) {
+    if (vhost_user_prepare_msg(dev, &memory, fds) < 0) {
         error_report("Failed preparing vhost-user memory table msg");
         return -1;
     }
+    msg.payload.memory = memory;
 
     fd_num = msg.payload.memory.nregions;

@@ -549,16 +551,19 @@ static int vhost_user_set_mem_table(struct vhost_dev
 *dev,
         .hdr.request = VHOST_USER_SET_MEM_TABLE,
         .hdr.flags = VHOST_USER_VERSION,
     };
+    VhostUserMemory memory = {};
 
     if (reply_supported) {
         msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
     }
 
-    if (vhost_user_prepare_msg(dev, &msg.payload.memory, fds) < 0) {
+    if (vhost_user_prepare_msg(dev, &memory, fds) < 0) {
         error_report("Failed preparing vhost-user memory table msg");
         return -1;
     }
 
+    msg.payload.memory = memory;
+
     fd_num = msg.payload.memory.nregions;
 

     if (!fd_num) {

hw/virtio/vhost-user.c +3 -0

@@ -1575,8 +1580,11 @@ static void vhost_user_set_used_memslots(struct
 vhost_dev *dev)  {
     int fds[VHOST_MEMORY_MAX_NREGIONS];
     VhostUserMsg msg;
+    VhostUserMemory memory = {};
+
+    vhost_user_prepare_msg(dev, &memory, fds);
 

hw/virtio/vhost-user.c static void vhost_user_set_used_memslots(struct vhost_dev *dev) · line 1588

Quoted 3 lines

hw/virtio/vhost-user.c +1 -1

-    vhost_user_prepare_msg(dev, &msg.payload.memory, fds);
+    msg.payload.memory = memory;
 }

Hi Michael, here should be like this:

static void vhost_user_set_used_memslots(struct vhost_dev *dev)

 {
     int fds[VHOST_MEMORY_MAX_NREGIONS];

hw/virtio/vhost-user.c +2 -2

-    VhostUserMsg msg;
+    VhostUserMemory memory = {};
 
-    vhost_user_prepare_msg(dev, &msg.payload.memory, fds);
+    vhost_user_prepare_msg(dev, &memory, fds);
 }

Regards,
Jay

Quoted 4 lines

hw/virtio/vhost-user.c +0 -1

 const VhostOps user_ops = {
--

MST

Quoted 84 lines · Zhoujian (jay)

Quoted 84 lines

-----Original Message-----
From: Michael S. Tsirkin [mailto:[email protected]]
Sent: Saturday, March 17, 2018 2:20 AM
To: [email protected]
Cc: Zhoujian (jay) <[email protected]>
Subject: [PATCH] vhost-user: avoid misaligned access

We can't pass a pointer to memory field directly since it's within a packed structure, so isn't aligned.
Pass a pointer on stack and copy.

Fixes 30c4cc7 ("vhost: used_memslots refactoring")

Cc Jay Zhou <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---

I had to apply this to fix make check errors with clang. Pls review, test and ack.

Thanks!

 hw/virtio/vhost-user.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

hw/virtio/vhost-user.c index renamed from hw/virtio/vhost-user.c +0 -0

c12fdd9..a44ee7f 100644

hw/virtio/vhost-user.c +1 -0

@@ -396,6 +396,7 @@ static int vhost_user_set_mem_table_postcopy(struct
 vhost_dev *dev,
     bool reply_supported = virtio_has_feature(dev->protocol_features,
 
 VHOST_USER_PROTOCOL_F_REPLY_ACK);
     VhostUserMsg msg_reply;
+    VhostUserMemory memory = {};
     int region_i, msg_i;

     VhostUserMsg msg = {

hw/virtio/vhost-user.c +6 -2

@@ -407,10 +408,11 @@ static int vhost_user_set_mem_table_postcopy(struct
 vhost_dev *dev,
         msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
     }
 
-    if (vhost_user_prepare_msg(dev, &msg.payload.memory, fds) < 0) {
+    if (vhost_user_prepare_msg(dev, &memory, fds) < 0) {
         error_report("Failed preparing vhost-user memory table msg");
         return -1;
     }
+    msg.payload.memory = memory;
 
     fd_num = msg.payload.memory.nregions;

@@ -549,16 +551,19 @@ static int vhost_user_set_mem_table(struct vhost_dev
 *dev,
         .hdr.request = VHOST_USER_SET_MEM_TABLE,
         .hdr.flags = VHOST_USER_VERSION,
     };
+    VhostUserMemory memory = {};
 
     if (reply_supported) {
         msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
     }
 
-    if (vhost_user_prepare_msg(dev, &msg.payload.memory, fds) < 0) {
+    if (vhost_user_prepare_msg(dev, &memory, fds) < 0) {
         error_report("Failed preparing vhost-user memory table msg");
         return -1;
     }
 
+    msg.payload.memory = memory;
+
     fd_num = msg.payload.memory.nregions;
 

     if (!fd_num) {

hw/virtio/vhost-user.c +3 -0

@@ -1575,8 +1580,11 @@ static void vhost_user_set_used_memslots(struct
 vhost_dev *dev)  {
     int fds[VHOST_MEMORY_MAX_NREGIONS];
     VhostUserMsg msg;
+    VhostUserMemory memory = {};
+
+    vhost_user_prepare_msg(dev, &memory, fds);
 

Quoted 3 lines

hw/virtio/vhost-user.c static void vhost_user_set_used_memslots(struct vhost_dev *dev) · line 1588

Quoted 3 lines

hw/virtio/vhost-user.c +1 -1

-    vhost_user_prepare_msg(dev, &msg.payload.memory, fds);
+    msg.payload.memory = memory;
 }  

FYI I think it's better to pass pointer to msg to
avoid alignment issues.

Quoted 14 lines

Hi Michael, here should be like this:

static void vhost_user_set_used_memslots(struct vhost_dev *dev)

 {
     int fds[VHOST_MEMORY_MAX_NREGIONS];

hw/virtio/vhost-user.c +2 -2

-    VhostUserMsg msg;
+    VhostUserMemory memory = {};
 
-    vhost_user_prepare_msg(dev, &msg.payload.memory, fds);
+    vhost_user_prepare_msg(dev, &memory, fds);
 }

Quoted 2 lines

Regards,
Jay

But what's the point of all this? The structure is discarded after being initialized. Doesn't look right to me.

Quoted 5 lines

Quoted 5 lines

hw/virtio/vhost-user.c +0 -1

 const VhostOps user_ops = {
--

MST

Quoted 110 lines · On Tue, 20 Mar 2018 04:36:48 +0200 "Michael S. Tsirkin" <[email protected]> wrote:

Quoted 84 lines · Zhoujian (jay)

Quoted 84 lines

-----Original Message-----
From: Michael S. Tsirkin [mailto:[email protected]]
Sent: Saturday, March 17, 2018 2:20 AM
To: [email protected]
Cc: Zhoujian (jay) <[email protected]>
Subject: [PATCH] vhost-user: avoid misaligned access

We can't pass a pointer to memory field directly since it's within a packed structure, so isn't aligned.
Pass a pointer on stack and copy.

Fixes 30c4cc7 ("vhost: used_memslots refactoring")

Cc Jay Zhou <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---

I had to apply this to fix make check errors with clang. Pls review, test and ack.

Thanks!

 hw/virtio/vhost-user.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

hw/virtio/vhost-user.c index renamed from hw/virtio/vhost-user.c +0 -0

c12fdd9..a44ee7f 100644

hw/virtio/vhost-user.c +1 -0

@@ -396,6 +396,7 @@ static int vhost_user_set_mem_table_postcopy(struct
 vhost_dev *dev,
     bool reply_supported = virtio_has_feature(dev->protocol_features,
 
 VHOST_USER_PROTOCOL_F_REPLY_ACK);
     VhostUserMsg msg_reply;
+    VhostUserMemory memory = {};
     int region_i, msg_i;

     VhostUserMsg msg = {

hw/virtio/vhost-user.c +6 -2

@@ -407,10 +408,11 @@ static int vhost_user_set_mem_table_postcopy(struct
 vhost_dev *dev,
         msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
     }
 
-    if (vhost_user_prepare_msg(dev, &msg.payload.memory, fds) < 0) {
+    if (vhost_user_prepare_msg(dev, &memory, fds) < 0) {
         error_report("Failed preparing vhost-user memory table msg");
         return -1;
     }
+    msg.payload.memory = memory;
 
     fd_num = msg.payload.memory.nregions;

@@ -549,16 +551,19 @@ static int vhost_user_set_mem_table(struct vhost_dev
 *dev,
         .hdr.request = VHOST_USER_SET_MEM_TABLE,
         .hdr.flags = VHOST_USER_VERSION,
     };
+    VhostUserMemory memory = {};
 
     if (reply_supported) {
         msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
     }
 
-    if (vhost_user_prepare_msg(dev, &msg.payload.memory, fds) < 0) {
+    if (vhost_user_prepare_msg(dev, &memory, fds) < 0) {
         error_report("Failed preparing vhost-user memory table msg");
         return -1;
     }
 
+    msg.payload.memory = memory;
+
     fd_num = msg.payload.memory.nregions;
 

     if (!fd_num) {

hw/virtio/vhost-user.c +3 -0

@@ -1575,8 +1580,11 @@ static void vhost_user_set_used_memslots(struct
 vhost_dev *dev)  {
     int fds[VHOST_MEMORY_MAX_NREGIONS];
     VhostUserMsg msg;
+    VhostUserMemory memory = {};
+
+    vhost_user_prepare_msg(dev, &memory, fds);
 

Quoted 3 lines

hw/virtio/vhost-user.c static void vhost_user_set_used_memslots(struct vhost_dev *dev) · line 1588

Quoted 3 lines

hw/virtio/vhost-user.c +1 -1

-    vhost_user_prepare_msg(dev, &msg.payload.memory, fds);
+    msg.payload.memory = memory;
 }  

FYI I think it's better to pass pointer to msg to
avoid alignment issues.

Quoted 16 lines

Hi Michael, here should be like this:

static void vhost_user_set_used_memslots(struct vhost_dev *dev)

 {
     int fds[VHOST_MEMORY_MAX_NREGIONS];

hw/virtio/vhost-user.c +2 -2

-    VhostUserMsg msg;
+    VhostUserMemory memory = {};
 
-    vhost_user_prepare_msg(dev, &msg.payload.memory, fds);
+    vhost_user_prepare_msg(dev, &memory, fds);
 }

Regards,
Jay

Quoted 2 lines

But what's the point of all this? The structure is discarded after being initialized. Doesn't look right to me.

I think it was my idea,
point is to share vhost_user_prepare_msg() between _set_used_memslots and _set_mem_table* instead of duplicating code, of cause at the cost of discarding results in vhost_user_set_used_memslots()

Quoted 6 lines

Quoted 5 lines

Quoted 5 lines

hw/virtio/vhost-user.c +0 -1

 const VhostOps user_ops = {
--

MST