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

[Stefano Stabellini <sstabellini@kernel.org>]

On Mon, 6 Jan 2020, Greg Kurz wrote:
> On Mon, 06 Jan 2020 16:24:18 +0100
> Christian Schoenebeck <qemu_oss@crudebyte.com> 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.

I agree with you that handling requests queuing is complex and overkill
for this. As I don't have a better suggestion, I am going to go with
retaining the error if the allocated buf_size is less than P9_IOHDRSZ.