Re: [Qemu-devel] [PATCH v1 2/4] vhost-user: Shutdown vhost-user connection when wrong messages are passed

[Tetsuya Mukawa]

On 2015/06/15 22:40, Stefan Hajnoczi wrote:
> On Fri, May 29, 2015 at 01:42:28PM +0900, Tetsuya Mukawa wrote:
>> When wrong vhost-user message are passed, the connection should be
>> shutdown.
>>
>> Signed-off-by: Tetsuya Mukawa <address@hidden>
>> ---
>>  hw/virtio/vhost-user.c | 17 ++++++++++-------
>>  include/sysemu/char.h  |  7 +++++++
>>  qemu-char.c            | 15 +++++++++++++++
>>  3 files changed, 32 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>> index e7ab829..4d7e3ba 100644
>> --- a/hw/virtio/vhost-user.c
>> +++ b/hw/virtio/vhost-user.c
>> @@ -183,6 +183,7 @@ static int vhost_user_write(struct vhost_dev *dev, 
>> VhostUserMsg *msg,
>>  static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
>>          void *arg)
>>  {
>> +    CharDriverState *chr = dev->opaque;
>>      VhostUserMsg msg;
>>      VhostUserRequest msg_request;
>>      struct vhost_vring_file *file = 0;
>> @@ -237,7 +238,7 @@ static int vhost_user_call(struct vhost_dev *dev, 
>> unsigned long int request,
>>          if (!fd_num) {
>>              error_report("Failed initializing vhost-user memory map, "
>>                      "consider using -object memory-backend-file share=on");
>> -            return -1;
>> +            goto close;
>>          }
>>  
>>          msg.size = sizeof(m.memory.nregions);
>> @@ -281,7 +282,7 @@ static int vhost_user_call(struct vhost_dev *dev, 
>> unsigned long int request,
>>          break;
>>      default:
>>          error_report("vhost-user trying to send unhandled ioctl");
>> -        return -1;
>> +        goto close;
>>          break;
>>      }
>>  
>> @@ -297,32 +298,34 @@ static int vhost_user_call(struct vhost_dev *dev, 
>> unsigned long int request,
>>          if (msg_request != msg.request) {
>>              error_report("Received unexpected msg type."
>>                      " Expected %d received %d", msg_request, msg.request);
>> -            return -1;
>> +            goto close;
>>          }
>>  
>>          switch (msg_request) {
>>          case VHOST_USER_GET_FEATURES:
>>              if (msg.size != sizeof(m.u64)) {
>>                  error_report("Received bad msg size.");
>> -                return -1;
>> +                goto close;
>>              }
>>              *((__u64 *) arg) = msg.u64;
>>              break;
>>          case VHOST_USER_GET_VRING_BASE:
>>              if (msg.size != sizeof(m.state)) {
>>                  error_report("Received bad msg size.");
>> -                return -1;
>> +                goto close;
>>              }
>>              memcpy(arg, &msg.state, sizeof(struct vhost_vring_state));
>>              break;
>>          default:
>>              error_report("Received unexpected msg type.");
>> -            return -1;
>> -            break;
>>          }
>>      }
>>  
>>      return 0;
>> +
>> +close:
>> +    qemu_chr_shutdown(chr, SHUT_RDWR);
>> +    return -1;
>>  }
> Why is shutdown(2) necessary?  We're aborting the connection and don't
> expect to process any more data, so we could just close it.

Yes, you are right.
It's my fault and here is current wrong behavior of this patch.

1. socket is shutdown by QEMU.
2. Vhost-user backend detects it, because socket is shutdown.
3. Vhost-user backend close socket.
    (This behavior is came from my test program)
4. QEMU detects it, then start closing event handling.

Apparently, I needed to close socket by QEMU. So I've checked the
qemu-char code more.
And I've found 'tcp_chr_disconnect' is the function I need to call, also
I've found I may not be able to use 'tcp_chr_close' for my purpose,
because 'tcp_chr_close' closes not only accepted fd, but also listen fd.
In my case, I just want to close accepted fd.
Because of above, 'qemu_chr_delete' that calls 'tcp_chr_close' is not
for my purpose.

Unfortunately, there is no function like 'qemu_chr_disconnect' in qemu-char.
So I may need to add it instead of 'qemu_chr_shutdown' in next patches.
Is this correct direction?

>
>> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
>> index 832b7fe..d944ded 100644
>> --- a/include/sysemu/char.h
>> +++ b/include/sysemu/char.h
>> @@ -70,6 +70,7 @@ struct CharDriverState {
>>      IOReadHandler *chr_read;
>>      void *handler_opaque;
>>      void (*chr_close)(struct CharDriverState *chr);
>> +    void (*chr_shutdown)(struct CharDriverState *chr, int how);
>>      void (*chr_accept_input)(struct CharDriverState *chr);
>>      void (*chr_set_echo)(struct CharDriverState *chr, bool echo);
>>      void (*chr_set_fe_open)(struct CharDriverState *chr, int fe_open);
>> @@ -124,6 +125,12 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
>>   */
>>  CharDriverState *qemu_chr_new(const char *label, const char *filename,
>>                                void (*init)(struct CharDriverState *s));
>> +/**
>> + * @qemu_chr_shutdown:
>> + *
>> + * Shutdown a fd of character backend.
>> + */
>> +void qemu_chr_shutdown(CharDriverState *chr, int how);
>>  
>>  /**
>>   * @qemu_chr_delete:
>> diff --git a/qemu-char.c b/qemu-char.c
>> index d0c1564..2b28808 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -2839,6 +2839,13 @@ static void tcp_chr_disconnect(CharDriverState *chr)
>>      }
>>  }
>>  
>> +static void tcp_chr_shutdown(CharDriverState *chr, int how)
>> +{
>> +    TCPCharDriver *s = chr->opaque;
>> +
>> +    shutdown(s->fd, how);
>> +}
>> +
>>  static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void 
>> *opaque)
>>  {
>>      CharDriverState *chr = opaque;
>> @@ -3836,6 +3843,13 @@ void qemu_chr_fe_release(CharDriverState *s)
>>      s->avail_connections++;
>>  }
>>  
>> +void qemu_chr_shutdown(CharDriverState *chr, int how)
>> +{
>> +    if (chr->chr_shutdown) {
>> +        chr->chr_shutdown(chr, how);
>> +    }
>> +}
>> +
>>  void qemu_chr_delete(CharDriverState *chr)
>>  {
>>      QTAILQ_REMOVE(&chardevs, chr, next);
>> @@ -4154,6 +4168,7 @@ static CharDriverState 
>> *qmp_chardev_open_socket(ChardevSocket *sock,
>>      chr->chr_write = tcp_chr_write;
>>      chr->chr_sync_read = tcp_chr_sync_read;
>>      chr->chr_close = tcp_chr_close;
>> +    chr->chr_shutdown = tcp_chr_shutdown;
>>      chr->get_msgfds = tcp_get_msgfds;
>>      chr->set_msgfds = tcp_set_msgfds;
>>      chr->chr_add_client = tcp_chr_add_client;
> Please split this into a separate qemu-char.c patch.  I hesitate to add
> a shutdown(2) interface since it adds a new state that the rest of the
> qemu-char.c code doesn't know about.

As described above, I may need to add 'qemu_chr_disconnect'.
It will be the interface of closing accepted fd.

Regards,
Tetsuya