v9fs_do_readdir_with_stat() and v9fs_do_readdir() stores as much data in
the buffer as can fit unless marshaling erorr occurs. However, after
commit 23a006d the behavior pdu_marshal was changed, and on error the
routine assumes that buffers are misconfigured and breaks communication.

Signed-off-by: Jan Dakinevich <jan.dakinevich@gmail.com>
---
 hw/9pfs/9p.c | 44 ++++++++++++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 12 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 1e38109..8e0b87e 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1679,6 +1679,16 @@ static void v9fs_init_qiov_from_pdu(QEMUIOVector *qiov, V9fsPDU *pdu,
     qemu_iovec_concat(qiov, &elem, skip, size);
 }
 
+static size_t v9fs_marshal_size(V9fsPDU *pdu)
+{
+    struct iovec *iov;
+    unsigned int niov;
+
+    pdu->s->transport->init_in_iov_from_pdu(pdu, &iov, &niov, 0);
+
+    return iov_size(iov, niov);
+}
+
 static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
                            uint64_t off, uint32_t max_count)
 {
@@ -1725,6 +1735,10 @@ static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu,
     off_t saved_dir_pos;
     struct dirent *dent;
 
+    /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
+    size_t offset = 11;
+    size_t marshal_size = v9fs_marshal_size(pdu);
+
     /* save the directory position */
     saved_dir_pos = v9fs_co_telldir(pdu, fidp);
     if (saved_dir_pos < 0) {
@@ -1752,18 +1766,23 @@ static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu,
         if (err < 0) {
             break;
         }
-        /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
-        len = pdu_marshal(pdu, 11 + count, "S", &v9stat);
 
-        v9fs_readdir_unlock(&fidp->fs.dir);
+        if (v9stat.size + 2 > MIN(marshal_size - offset, max_count - count)) {
+            v9fs_readdir_unlock(&fidp->fs.dir);
 
-        if ((len != (v9stat.size + 2)) || ((count + len) > max_count)) {
             /* Ran out of buffer. Set dir back to old position and return */
             v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
             v9fs_stat_free(&v9stat);
             v9fs_path_free(&path);
             return count;
         }
+
+        len = pdu_marshal(pdu, offset, "S", &v9stat);
+        BUG_ON(len != v9stat.size + 2);
+
+        v9fs_readdir_unlock(&fidp->fs.dir);
+
+        offset += len;
         count += len;
         v9fs_stat_free(&v9stat);
         v9fs_path_free(&path);
@@ -1884,6 +1903,10 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
     off_t saved_dir_pos;
     struct dirent *dent;
 
+    /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
+    size_t offset = 11;
+    size_t marshal_size = v9fs_marshal_size(pdu);
+
     /* save the directory position */
     saved_dir_pos = v9fs_co_telldir(pdu, fidp);
     if (saved_dir_pos < 0) {
@@ -1899,7 +1922,8 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
         }
         v9fs_string_init(&name);
         v9fs_string_sprintf(&name, "%s", dent->d_name);
-        if ((count + v9fs_readdir_data_size(&name)) > max_count) {
+        if (v9fs_readdir_data_size(&name) > MIN(marshal_size - offset,
+                                                max_count - count)) {
             v9fs_readdir_unlock(&fidp->fs.dir);
 
             /* Ran out of buffer. Set dir back to old position and return */
@@ -1918,18 +1942,14 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
         qid.type = 0;
         qid.version = 0;
 
-        /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
-        len = pdu_marshal(pdu, 11 + count, "Qqbs",
+        len = pdu_marshal(pdu, offset, "Qqbs",
                           &qid, dent->d_off,
                           dent->d_type, &name);
+        BUG_ON(len != v9fs_readdir_data_size(&name));
 
         v9fs_readdir_unlock(&fidp->fs.dir);
 
-        if (len < 0) {
-            v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
-            v9fs_string_free(&name);
-            return len;
-        }
+        offset += len;
         count += len;
         v9fs_string_free(&name);
         saved_dir_pos = dent->d_off;
-- 
2.1.4

On Thu, 14 Sep 2017 19:31:36 +0300
Jan Dakinevich <jan.dakinevich@gmail.com> wrote:

> v9fs_do_readdir_with_stat() and v9fs_do_readdir() stores as much data in
> the buffer as can fit unless marshaling erorr occurs. However, after
> commit 23a006d the behavior pdu_marshal was changed, and on error the
> routine assumes that buffers are misconfigured and breaks communication.
> 

I agree and I could easily reproduce with a linux guest using 9p2000.u, ie, the
v9fs_do_readdir_with_stat() case, but I couldn't find a way to break transport
with 9p2000.L. Do you have a reproducer for the latter ?

I ask because I would appreciate to see some more details in the changelog,
for the records.

> Signed-off-by: Jan Dakinevich <jan.dakinevich@gmail.com>
> ---

Anyway the patch looks fine, but I'll wait for your answer before pushing
to 9p-next.

Cheers,

--
Greg

>  hw/9pfs/9p.c | 44 ++++++++++++++++++++++++++++++++------------
>  1 file changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 1e38109..8e0b87e 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1679,6 +1679,16 @@ static void v9fs_init_qiov_from_pdu(QEMUIOVector *qiov, V9fsPDU *pdu,
>      qemu_iovec_concat(qiov, &elem, skip, size);
>  }
>  
> +static size_t v9fs_marshal_size(V9fsPDU *pdu)
> +{
> +    struct iovec *iov;
> +    unsigned int niov;
> +
> +    pdu->s->transport->init_in_iov_from_pdu(pdu, &iov, &niov, 0);
> +
> +    return iov_size(iov, niov);
> +}
> +
>  static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
>                             uint64_t off, uint32_t max_count)
>  {
> @@ -1725,6 +1735,10 @@ static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu,
>      off_t saved_dir_pos;
>      struct dirent *dent;
>  
> +    /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
> +    size_t offset = 11;
> +    size_t marshal_size = v9fs_marshal_size(pdu);
> +
>      /* save the directory position */
>      saved_dir_pos = v9fs_co_telldir(pdu, fidp);
>      if (saved_dir_pos < 0) {
> @@ -1752,18 +1766,23 @@ static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu,
>          if (err < 0) {
>              break;
>          }
> -        /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
> -        len = pdu_marshal(pdu, 11 + count, "S", &v9stat);
>  
> -        v9fs_readdir_unlock(&fidp->fs.dir);
> +        if (v9stat.size + 2 > MIN(marshal_size - offset, max_count - count)) {
> +            v9fs_readdir_unlock(&fidp->fs.dir);
>  
> -        if ((len != (v9stat.size + 2)) || ((count + len) > max_count)) {
>              /* Ran out of buffer. Set dir back to old position and return */
>              v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
>              v9fs_stat_free(&v9stat);
>              v9fs_path_free(&path);
>              return count;
>          }
> +
> +        len = pdu_marshal(pdu, offset, "S", &v9stat);
> +        BUG_ON(len != v9stat.size + 2);
> +
> +        v9fs_readdir_unlock(&fidp->fs.dir);
> +
> +        offset += len;
>          count += len;
>          v9fs_stat_free(&v9stat);
>          v9fs_path_free(&path);
> @@ -1884,6 +1903,10 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
>      off_t saved_dir_pos;
>      struct dirent *dent;
>  
> +    /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
> +    size_t offset = 11;
> +    size_t marshal_size = v9fs_marshal_size(pdu);
> +
>      /* save the directory position */
>      saved_dir_pos = v9fs_co_telldir(pdu, fidp);
>      if (saved_dir_pos < 0) {
> @@ -1899,7 +1922,8 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
>          }
>          v9fs_string_init(&name);
>          v9fs_string_sprintf(&name, "%s", dent->d_name);
> -        if ((count + v9fs_readdir_data_size(&name)) > max_count) {
> +        if (v9fs_readdir_data_size(&name) > MIN(marshal_size - offset,
> +                                                max_count - count)) {
>              v9fs_readdir_unlock(&fidp->fs.dir);
>  
>              /* Ran out of buffer. Set dir back to old position and return */
> @@ -1918,18 +1942,14 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
>          qid.type = 0;
>          qid.version = 0;
>  
> -        /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
> -        len = pdu_marshal(pdu, 11 + count, "Qqbs",
> +        len = pdu_marshal(pdu, offset, "Qqbs",
>                            &qid, dent->d_off,
>                            dent->d_type, &name);
> +        BUG_ON(len != v9fs_readdir_data_size(&name));
>  
>          v9fs_readdir_unlock(&fidp->fs.dir);
>  
> -        if (len < 0) {
> -            v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
> -            v9fs_string_free(&name);
> -            return len;
> -        }
> +        offset += len;
>          count += len;
>          v9fs_string_free(&name);
>          saved_dir_pos = dent->d_off;

Greg,

> I couldn't find a way to break transport
> with 9p2000.L. Do you have a reproducer for the latter ?

You're right! The problem is related only to 9p2000.u protocol. In this 
protocol to list a directory folowing chain is used: TOPEN->TREAD. 
9p2000.L uses for the same goal TLOPEN->TREADDIR.

As I just figured out TREADDIR never fails in marshaling stage because 
it calls v9fs_do_readdir which checks for availble buffer size 
(max_count) before marshaling.

I am preparing new patch being inspired by v9fs_do_readdir()'s behavior.

On 09/15/2017 05:56 PM, Greg Kurz wrote:
> On Thu, 14 Sep 2017 19:31:36 +0300
> Jan Dakinevich <jan.dakinevich@gmail.com> wrote:
> 
>> v9fs_do_readdir_with_stat() and v9fs_do_readdir() stores as much data in
>> the buffer as can fit unless marshaling erorr occurs. However, after
>> commit 23a006d the behavior pdu_marshal was changed, and on error the
>> routine assumes that buffers are misconfigured and breaks communication.
>>
> 
> I agree and I could easily reproduce with a linux guest using 9p2000.u, ie, the
> v9fs_do_readdir_with_stat() case, but I couldn't find a way to break transport
> with 9p2000.L. Do you have a reproducer for the latter ?
> 
> I ask because I would appreciate to see some more details in the changelog,
> for the records.
> 
>> Signed-off-by: Jan Dakinevich <jan.dakinevich@gmail.com>
>> ---
> 
> Anyway the patch looks fine, but I'll wait for your answer before pushing
> to 9p-next.
> 
> Cheers,
> 
> --
> Greg
> 
>>   hw/9pfs/9p.c | 44 ++++++++++++++++++++++++++++++++------------
>>   1 file changed, 32 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
>> index 1e38109..8e0b87e 100644
>> --- a/hw/9pfs/9p.c
>> +++ b/hw/9pfs/9p.c
>> @@ -1679,6 +1679,16 @@ static void v9fs_init_qiov_from_pdu(QEMUIOVector *qiov, V9fsPDU *pdu,
>>       qemu_iovec_concat(qiov, &elem, skip, size);
>>   }
>>   
>> +static size_t v9fs_marshal_size(V9fsPDU *pdu)
>> +{
>> +    struct iovec *iov;
>> +    unsigned int niov;
>> +
>> +    pdu->s->transport->init_in_iov_from_pdu(pdu, &iov, &niov, 0);
>> +
>> +    return iov_size(iov, niov);
>> +}
>> +
>>   static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
>>                              uint64_t off, uint32_t max_count)
>>   {
>> @@ -1725,6 +1735,10 @@ static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu,
>>       off_t saved_dir_pos;
>>       struct dirent *dent;
>>   
>> +    /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
>> +    size_t offset = 11;
>> +    size_t marshal_size = v9fs_marshal_size(pdu);
>> +
>>       /* save the directory position */
>>       saved_dir_pos = v9fs_co_telldir(pdu, fidp);
>>       if (saved_dir_pos < 0) {
>> @@ -1752,18 +1766,23 @@ static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu,
>>           if (err < 0) {
>>               break;
>>           }
>> -        /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
>> -        len = pdu_marshal(pdu, 11 + count, "S", &v9stat);
>>   
>> -        v9fs_readdir_unlock(&fidp->fs.dir);
>> +        if (v9stat.size + 2 > MIN(marshal_size - offset, max_count - count)) {
>> +            v9fs_readdir_unlock(&fidp->fs.dir);
>>   
>> -        if ((len != (v9stat.size + 2)) || ((count + len) > max_count)) {
>>               /* Ran out of buffer. Set dir back to old position and return */
>>               v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
>>               v9fs_stat_free(&v9stat);
>>               v9fs_path_free(&path);
>>               return count;
>>           }
>> +
>> +        len = pdu_marshal(pdu, offset, "S", &v9stat);
>> +        BUG_ON(len != v9stat.size + 2);
>> +
>> +        v9fs_readdir_unlock(&fidp->fs.dir);
>> +
>> +        offset += len;
>>           count += len;
>>           v9fs_stat_free(&v9stat);
>>           v9fs_path_free(&path);
>> @@ -1884,6 +1903,10 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
>>       off_t saved_dir_pos;
>>       struct dirent *dent;
>>   
>> +    /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
>> +    size_t offset = 11;
>> +    size_t marshal_size = v9fs_marshal_size(pdu);
>> +
>>       /* save the directory position */
>>       saved_dir_pos = v9fs_co_telldir(pdu, fidp);
>>       if (saved_dir_pos < 0) {
>> @@ -1899,7 +1922,8 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
>>           }
>>           v9fs_string_init(&name);
>>           v9fs_string_sprintf(&name, "%s", dent->d_name);
>> -        if ((count + v9fs_readdir_data_size(&name)) > max_count) {
>> +        if (v9fs_readdir_data_size(&name) > MIN(marshal_size - offset,
>> +                                                max_count - count)) {
>>               v9fs_readdir_unlock(&fidp->fs.dir);
>>   
>>               /* Ran out of buffer. Set dir back to old position and return */
>> @@ -1918,18 +1942,14 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
>>           qid.type = 0;
>>           qid.version = 0;
>>   
>> -        /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
>> -        len = pdu_marshal(pdu, 11 + count, "Qqbs",
>> +        len = pdu_marshal(pdu, offset, "Qqbs",
>>                             &qid, dent->d_off,
>>                             dent->d_type, &name);
>> +        BUG_ON(len != v9fs_readdir_data_size(&name));
>>   
>>           v9fs_readdir_unlock(&fidp->fs.dir);
>>   
>> -        if (len < 0) {
>> -            v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
>> -            v9fs_string_free(&name);
>> -            return len;
>> -        }
>> +        offset += len;
>>           count += len;
>>           v9fs_string_free(&name);
>>           saved_dir_pos = dent->d_off;
> 

-- 
Best regards,
Jan Dakinevich