Re: [PATCH] 9p: init_in_iov_from_pdu can truncate the size

[Greg Kurz]

On Mon, 06 Jan 2020 16:24:18 +0100
Christian Schoenebeck <address@hidden> wrote:

> On Montag, 6. Januar 2020 14:42:54 CET Greg Kurz wrote:
> > > diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> > > index 775e8ff766..68873c3f5f 100644
> > > --- a/hw/9pfs/virtio-9p-device.c
> > > +++ b/hw/9pfs/virtio-9p-device.c
> > > @@ -145,19 +145,15 @@ static ssize_t virtio_pdu_vunmarshal(V9fsPDU *pdu,
> > > size_t offset,> 
> > >  }
> > >  
> > >  static void virtio_init_in_iov_from_pdu(V9fsPDU *pdu, struct iovec
> > >  **piov,
> > > 
> > > -                                        unsigned int *pniov, size_t size)
> > > +                                        unsigned int *pniov, size_t
> > > *size)
> > > 
> > >  {
> > >  
> > >      V9fsState *s = pdu->s;
> > >      V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
> > >      VirtQueueElement *elem = v->elems[pdu->idx];
> > >      size_t buf_size = iov_size(elem->in_sg, elem->in_num);
> > > 
> > > -    if (buf_size < size) {
> > > -        VirtIODevice *vdev = VIRTIO_DEVICE(v);
> > > -
> > > -        virtio_error(vdev,
> > > -                     "VirtFS reply type %d needs %zu bytes, buffer has
> > > %zu", -                     pdu->id + 1, size, buf_size);
> > > +    if (buf_size < *size) {
> > > +        *size = buf_size;
> > > 
> > >      }
> > 
> > As suggested by Christian in some other mail, it could still make sense to
> > raise the error if there isn't even enough space to pack a 9p message
> > header.
> 
> Another option: Instead of handling this as a hard error (which they probably 
> try to avoid in their use case): putting the handler asleep for a while by 
> calling qemu_co_sleep_ns_wakeable() in this case. Then a bit later transport 
> would eventually have the required buffer size and handler could continue the 
> request without an error.
> 

Waiting for an arbitrary amount of time (how much?) and retrying doesn't give
any guarantee either that things will go smoothly. If anything, I'd rather have
the transport to wake up the request when more buffer space gets available.

> But this would require more care. For instance subsequent request handlers 
> would need to check if there was already an event handler asleep, and if so 
> it 
> would either need to wake it up or put itself asleep as well to prevent the 
> request order being processed by server being messed up.
> 

And so on... ie. we would need to handle a queue of sleeping requests IIUC.
Not really fan to go this way to address what looks like a corner case.

> Best regards,
> Christian Schoenebeck
> 
>