[Qemu-devel] [PATCH] 9pfs: fix offset error in v9fs_xattr_read()

[Qemu-devel] [PATCH] 9pfs: fix offset error in v9fs_xattr_read()

[Greg Kurz]

The current code tries to copy `read_count' bytes starting at offset
`offset' from a `read_count`-sized iovec. This causes v9fs_pack() to
fail with ENOBUFS.

Since the PDU iovec is already partially filled with `offset' bytes,
let's skip them when creating `qiov_full' and have v9fs_pack() to
copy the whole of it. Moreover, this is consistent with the other
places where v9fs_init_qiov_from_pdu() is called.

This fixes commit "bcb8998fac16 9pfs: call v9fs_init_qiov_from_pdu
before v9fs_pack".

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index fa58877570f6..f0eef1a3ef53 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1685,8 +1685,8 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
     }
     offset += err;
 
-    v9fs_init_qiov_from_pdu(&qiov_full, pdu, 0, read_count, false);
-    err = v9fs_pack(qiov_full.iov, qiov_full.niov, offset,
+    v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, read_count, false);
+    err = v9fs_pack(qiov_full.iov, qiov_full.niov, 0,
                     ((char *)fidp->fs.xattr.value) + off,
                     read_count);
     qemu_iovec_destroy(&qiov_full);

Re: [Qemu-devel] [PATCH] 9pfs: fix offset error in v9fs_xattr_read()

[Stefano Stabellini]

On Sat, 21 Jan 2017, Greg Kurz wrote:
> The current code tries to copy `read_count' bytes starting at offset
> `offset' from a `read_count`-sized iovec. This causes v9fs_pack() to
> fail with ENOBUFS.
> 
> Since the PDU iovec is already partially filled with `offset' bytes,
> let's skip them when creating `qiov_full' and have v9fs_pack() to
> copy the whole of it. Moreover, this is consistent with the other
> places where v9fs_init_qiov_from_pdu() is called.
> 
> This fixes commit "bcb8998fac16 9pfs: call v9fs_init_qiov_from_pdu
> before v9fs_pack".

Sorry about that!


> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index fa58877570f6..f0eef1a3ef53 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1685,8 +1685,8 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
>      }
>      offset += err;
>  
> -    v9fs_init_qiov_from_pdu(&qiov_full, pdu, 0, read_count, false);
> -    err = v9fs_pack(qiov_full.iov, qiov_full.niov, offset,
> +    v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, read_count, false);

v9fs_init_qiov_from_pdu calls init_in_iov_from_pdu passing read_count as
size argument. offset is not passed to init_in_iov_from_pdu, it is only
used to initialized qiov_full.

In other words, don't we need to:

    v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, read_count + offset, false);

To make sure that qiov_full has "read_count + offset" bytes in it, and
qiov_full is initialized skipping the first "offset" bytes?


> +    err = v9fs_pack(qiov_full.iov, qiov_full.niov, 0,
>                      ((char *)fidp->fs.xattr.value) + off,
>                      read_count);
>      qemu_iovec_destroy(&qiov_full);
> 

Re: [Qemu-devel] [PATCH] 9pfs: fix offset error in v9fs_xattr_read()

[Greg Kurz]

On Mon, 23 Jan 2017 12:20:57 -0800 (PST)
Stefano Stabellini <sstabellini@kernel.org> wrote:

> On Sat, 21 Jan 2017, Greg Kurz wrote:
> > The current code tries to copy `read_count' bytes starting at offset
> > `offset' from a `read_count`-sized iovec. This causes v9fs_pack() to
> > fail with ENOBUFS.
> > 
> > Since the PDU iovec is already partially filled with `offset' bytes,
> > let's skip them when creating `qiov_full' and have v9fs_pack() to
> > copy the whole of it. Moreover, this is consistent with the other
> > places where v9fs_init_qiov_from_pdu() is called.
> > 
> > This fixes commit "bcb8998fac16 9pfs: call v9fs_init_qiov_from_pdu
> > before v9fs_pack".  
> 
> Sorry about that!
> 
> 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/9pfs/9p.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index fa58877570f6..f0eef1a3ef53 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -1685,8 +1685,8 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
> >      }
> >      offset += err;
> >  
> > -    v9fs_init_qiov_from_pdu(&qiov_full, pdu, 0, read_count, false);
> > -    err = v9fs_pack(qiov_full.iov, qiov_full.niov, offset,
> > +    v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, read_count, false);  
> 
> v9fs_init_qiov_from_pdu calls init_in_iov_from_pdu passing read_count as
> size argument. offset is not passed to init_in_iov_from_pdu, it is only
> used to initialized qiov_full.
> 
> In other words, don't we need to:
> 
>     v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, read_count + offset, false);
> 
> To make sure that qiov_full has "read_count + offset" bytes in it, and
> qiov_full is initialized skipping the first "offset" bytes?
> 

If we do that then qemu_iovec_concat() will skip offset bytes and then copy
read_count + offset bytes or am I missing something ?

> 
> > +    err = v9fs_pack(qiov_full.iov, qiov_full.niov, 0,
> >                      ((char *)fidp->fs.xattr.value) + off,
> >                      read_count);
> >      qemu_iovec_destroy(&qiov_full);
> >   

Re: [Qemu-devel] [PATCH] 9pfs: fix offset error in v9fs_xattr_read()

[Stefano Stabellini]

On Tue, 24 Jan 2017, Greg Kurz wrote:
> On Mon, 23 Jan 2017 12:20:57 -0800 (PST)
> Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> > On Sat, 21 Jan 2017, Greg Kurz wrote:
> > > The current code tries to copy `read_count' bytes starting at offset
> > > `offset' from a `read_count`-sized iovec. This causes v9fs_pack() to
> > > fail with ENOBUFS.
> > > 
> > > Since the PDU iovec is already partially filled with `offset' bytes,
> > > let's skip them when creating `qiov_full' and have v9fs_pack() to
> > > copy the whole of it. Moreover, this is consistent with the other
> > > places where v9fs_init_qiov_from_pdu() is called.
> > > 
> > > This fixes commit "bcb8998fac16 9pfs: call v9fs_init_qiov_from_pdu
> > > before v9fs_pack".  
> > 
> > Sorry about that!
> > 
> > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > >  hw/9pfs/9p.c |    4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > index fa58877570f6..f0eef1a3ef53 100644
> > > --- a/hw/9pfs/9p.c
> > > +++ b/hw/9pfs/9p.c
> > > @@ -1685,8 +1685,8 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
> > >      }
> > >      offset += err;
> > >  
> > > -    v9fs_init_qiov_from_pdu(&qiov_full, pdu, 0, read_count, false);
> > > -    err = v9fs_pack(qiov_full.iov, qiov_full.niov, offset,
> > > +    v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, read_count, false);  
> > 
> > v9fs_init_qiov_from_pdu calls init_in_iov_from_pdu passing read_count as
> > size argument. offset is not passed to init_in_iov_from_pdu, it is only
> > used to initialized qiov_full.
> > 
> > In other words, don't we need to:
> > 
> >     v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, read_count + offset, false);
> > 
> > To make sure that qiov_full has "read_count + offset" bytes in it, and
> > qiov_full is initialized skipping the first "offset" bytes?
> > 
> 
> If we do that then qemu_iovec_concat() will skip offset bytes and then copy
> read_count + offset bytes or am I missing something ?

You are right, sorry. qemu_iovec_concat didn't do what I thought it
would. In that case, I think we need to change v9fs_init_qiov_from_pdu
(in addition to the changes already in this patch):

@@ -1659,12 +1659,14 @@ static void v9fs_init_qiov_from_pdu(QEMUIOVector *qiov, V9fsPDU *pdu,
     if (is_write) {
         pdu->s->transport->init_out_iov_from_pdu(pdu, &iov, &niov);
     } else {
-        pdu->s->transport->init_in_iov_from_pdu(pdu, &iov, &niov, size);
+        pdu->s->transport->init_in_iov_from_pdu(pdu, &iov, &niov, size + skip);
     }
 
The reason is the same as before: iov needs to have offset+read_count
bytes in it, for qemu_iovec_concat to skip offset and iov still have
read_count bytes left.


> > > +    err = v9fs_pack(qiov_full.iov, qiov_full.niov, 0,
> > >                      ((char *)fidp->fs.xattr.value) + off,
> > >                      read_count);
> > >      qemu_iovec_destroy(&qiov_full);
> > >   
> 
> 

Re: [Qemu-devel] [PATCH] 9pfs: fix offset error in v9fs_xattr_read()

[Greg Kurz]

On Tue, 24 Jan 2017 14:24:23 -0800 (PST)
Stefano Stabellini <sstabellini@kernel.org> wrote:

> On Tue, 24 Jan 2017, Greg Kurz wrote:
> > On Mon, 23 Jan 2017 12:20:57 -0800 (PST)
> > Stefano Stabellini <sstabellini@kernel.org> wrote:
> >   
> > > On Sat, 21 Jan 2017, Greg Kurz wrote:  
> > > > The current code tries to copy `read_count' bytes starting at offset
> > > > `offset' from a `read_count`-sized iovec. This causes v9fs_pack() to
> > > > fail with ENOBUFS.
> > > > 
> > > > Since the PDU iovec is already partially filled with `offset' bytes,
> > > > let's skip them when creating `qiov_full' and have v9fs_pack() to
> > > > copy the whole of it. Moreover, this is consistent with the other
> > > > places where v9fs_init_qiov_from_pdu() is called.
> > > > 
> > > > This fixes commit "bcb8998fac16 9pfs: call v9fs_init_qiov_from_pdu
> > > > before v9fs_pack".    
> > > 
> > > Sorry about that!
> > > 
> > >   
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > ---
> > > >  hw/9pfs/9p.c |    4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > > index fa58877570f6..f0eef1a3ef53 100644
> > > > --- a/hw/9pfs/9p.c
> > > > +++ b/hw/9pfs/9p.c
> > > > @@ -1685,8 +1685,8 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
> > > >      }
> > > >      offset += err;
> > > >  
> > > > -    v9fs_init_qiov_from_pdu(&qiov_full, pdu, 0, read_count, false);
> > > > -    err = v9fs_pack(qiov_full.iov, qiov_full.niov, offset,
> > > > +    v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, read_count, false);    
> > > 
> > > v9fs_init_qiov_from_pdu calls init_in_iov_from_pdu passing read_count as
> > > size argument. offset is not passed to init_in_iov_from_pdu, it is only
> > > used to initialized qiov_full.
> > > 
> > > In other words, don't we need to:
> > > 
> > >     v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, read_count + offset, false);
> > > 
> > > To make sure that qiov_full has "read_count + offset" bytes in it, and
> > > qiov_full is initialized skipping the first "offset" bytes?
> > >   
> > 
> > If we do that then qemu_iovec_concat() will skip offset bytes and then copy
> > read_count + offset bytes or am I missing something ?  
> 
> You are right, sorry. qemu_iovec_concat didn't do what I thought it
> would. In that case, I think we need to change v9fs_init_qiov_from_pdu
> (in addition to the changes already in this patch):
> 
> @@ -1659,12 +1659,14 @@ static void v9fs_init_qiov_from_pdu(QEMUIOVector *qiov, V9fsPDU *pdu,
>      if (is_write) {
>          pdu->s->transport->init_out_iov_from_pdu(pdu, &iov, &niov);
>      } else {
> -        pdu->s->transport->init_in_iov_from_pdu(pdu, &iov, &niov, size);
> +        pdu->s->transport->init_in_iov_from_pdu(pdu, &iov, &niov, size + skip);
>      }
>  
> The reason is the same as before: iov needs to have offset+read_count
> bytes in it, for qemu_iovec_concat to skip offset and iov still have
> read_count bytes left.
> 

Ah! I had missed that since virtio_init_in_iov_from_pdu doesn't use
the size argument, but I now understand :)

Thanks for the clarification.

> 
> > > > +    err = v9fs_pack(qiov_full.iov, qiov_full.niov, 0,
> > > >                      ((char *)fidp->fs.xattr.value) + off,
> > > >                      read_count);
> > > >      qemu_iovec_destroy(&qiov_full);
> > > >     
> > 
> >