Re: [Qemu-devel] [PATCH] vhost-user: avoid misaligned access

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Mar 20, 2018 at 08:05:38AM +0100, Igor Mammedov wrote:
> On Tue, 20 Mar 2018 04:36:48 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Mar 20, 2018 at 02:01:07AM +0000, Zhoujian (jay) wrote:
> > > 
> > >   
> > > > -----Original Message-----
> > > > From: Michael S. Tsirkin [mailto:mst@redhat.com]
> > > > Sent: Saturday, March 17, 2018 2:20 AM
> > > > To: qemu-devel@nongnu.org
> > > > Cc: Zhoujian (jay) <jianjay.zhou@huawei.com>
> > > > 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 <jianjay.zhou@huawei.com>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > > 
> > > > 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(-)
> > > > 
> > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index
> > > > c12fdd9..a44ee7f 100644
> > > > --- a/hw/virtio/vhost-user.c
> > > > +++ b/hw/virtio/vhost-user.c
> > > > @@ -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 = {
> > > > @@ -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) {
> > > > @@ -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);
> > > > 
> > > > -    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.
> > 
> > > Hi Michael, here should be like this:
> > > 
> > > 
> > > 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, &msg.payload.memory, fds);
> > > +    vhost_user_prepare_msg(dev, &memory, fds);
> > >  }
> > > 
> > > 
> > > Regards,
> > > Jay  
> > 
> > 
> > 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()

So vhost_user_prepare_msg has a side effect of setting a
global flag, that is why it's called here.
Pls add a comment both near vhost_user_prepare_msg and where
it's called.

> > 
> > > > 
> > > >  const VhostOps user_ops = {
> > > > --
> > > > MST  
> >