Re: [Qemu-devel] 9pfs: unWRITAble dirs with random errors in guest

[Aneesh Kumar K.V]

"Aneesh Kumar K.V" <address@hidden> writes:

> Michael Tokarev <address@hidden> writes:
>
>> Rehashing an old thread again...
>>
>> 28.02.2013 21:25, M. Mohan Kumar wrote:
>>> Michael Tokarev <address@hidden> writes:
>>> 
>>> 
>>>> 28.02.2013 17:55, M. Mohan Kumar wrote:
>>>>> Michael Tokarev <address@hidden> writes:
>>>>>
>>>>> Hi,
>>>>>
>>>>> Please try mounting with -oversion=9p2000.L
>>>>>
>>>>> With qemu-1.4.0 and 9p2000.L, I could not recreate this issue. ie not
>>>>> getting Unknown error during directory listing.
>>>>
>>>> Yes, with 9p2000.L it works fine, both reported
>>>> issues are gone.
>>>>
>>>>> I am using Guest kernel 3.8.0-rc5+.
>>>>
>>>> But do you see my original error, with the default mount version?
>>>> (I'm using 3.2 guest kernel if that matters, and it is also 32bits,
>>>> which should not matter).
>>> Yes, with default mount option, i am getting same error.
>>
>> So, is there any intention to fix that for the default mount
>> options?  Or should the corresponding protocol be just marked
>> as broken and qemu to refuse its usage?  I think it is either
>> one or another, -- either fix or drop, that is, keeping
>> known-broken behavor isn't exactly the right thing.
>
> I am trying to reproduce this. I guess there is some part of config i am
> missing
>
> I have export path /tmp/ and is  exported via 
>
> -virtfs local,path=/tmp/,mount_tag=v_tmp,security_model=mapped-file
>
> There is not .virtfs_metadata directory  in /tmp/
>
> in /tmp/ i have test/test2 with
>
> ls -ald test/test2
> dr-xr-xr-x 2 kvaneesh kvaneesh 4096 May 20 15:20 test/test2
>
> Now i am mounting the same via 
> mount -t 9p -oversion=9p2000.u,ro v_tmp /mnt    
>
> address@hidden:~# ls -al /mnt/test/test2/
> total 8
> dr-xr-xr-x 2 virtfs virtfs 4096 May 20 15:20 .
> drwxr-xr-x 3 virtfs virtfs 4096 May 20 15:00 ..
> -r--r--r-- 1 virtfs virtfs    0 May 20 15:20 k
>
> Any idea what i am missing ?

With lots of help from Mohan, I was able to figure out the details. This
turned out to be an error in client code related to how we are handling
errors in zero copy request. If you want to try, I am attaching two
patches below. Qemu patch is strictly not needed, But should get you the
correct error.

commit 9b97cc13e5c4455612ae09d72d2ae963c487c202
Author: Aneesh Kumar K.V <address@hidden>
Date:   Mon May 20 22:55:22 2013 +0530

    net/9p: Handle error in zero copy request correctly for 9p2000.u
    
    For zero copy request, error will be encoded in the user space buffer.
    So copy the error code correctly using copy_from_user. Here we use the
    extra bytes we allocate for zero copy request. If total error details
    are more than P9_ZC_HDR_SZ - 7 bytes, we return -EFAULT. The patch also
    avoid a memory allocation in the error path.
    
    Signed-off-by: Aneesh Kumar K.V <address@hidden>

diff --git a/net/9p/client.c b/net/9p/client.c
index 5e94dab..01f1779 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -562,36 +562,19 @@ static int p9_check_zc_errors(struct p9_client *c, struct 
p9_req_t *req,
 
        if (!p9_is_proto_dotl(c)) {
                /* Error is reported in string format */
-               uint16_t len;
-               /* 7 = header size for RERROR, 2 is the size of string len; */
-               int inline_len = in_hdrlen - (7 + 2);
+               int len;
+               /* 7 = header size for RERROR; */
+               int inline_len = in_hdrlen - 7;
 
-               /* Read the size of error string */
-               err = p9pdu_readf(req->rc, c->proto_version, "w", &len);
-               if (err)
-                       goto out_err;
-
-               ename = kmalloc(len + 1, GFP_NOFS);
-               if (!ename) {
-                       err = -ENOMEM;
+               len =  req->rc->size - req->rc->offset;
+               if (len > (P9_ZC_HDR_SZ - 7)) {
+                       err = -EFAULT;
                        goto out_err;
                }
-               if (len <= inline_len) {
-                       /* We have error in protocol buffer itself */
-                       if (pdu_read(req->rc, ename, len)) {
-                               err = -EFAULT;
-                               goto out_free;
 
-                       }
-               } else {
-                       /*
-                        *  Part of the data is in user space buffer.
-                        */
-                       if (pdu_read(req->rc, ename, inline_len)) {
-                               err = -EFAULT;
-                               goto out_free;
-
-                       }
+               ename = &req->rc->sdata[req->rc->offset];
+               if (len > inline_len) {
+                       /* We have error in external buffer */
                        if (kern_buf) {
                                memcpy(ename + inline_len, uidata,
                                       len - inline_len);
@@ -600,19 +583,19 @@ static int p9_check_zc_errors(struct p9_client *c, struct 
p9_req_t *req,
                                                     uidata, len - inline_len);
                                if (err) {
                                        err = -EFAULT;
-                                       goto out_free;
+                                       goto out_err;
                                }
                        }
                }
-               ename[len] = 0;
-               if (p9_is_proto_dotu(c)) {
-                       /* For dotu we also have error code */
-                       err = p9pdu_readf(req->rc,
-                                         c->proto_version, "d", &ecode);
-                       if (err)
-                               goto out_free;
+               ename = NULL;
+               err = p9pdu_readf(req->rc, c->proto_version, "s?d",
+                                 &ename, &ecode);
+               if (err)
+                       goto out_err;
+
+               if (p9_is_proto_dotu(c))
                        err = -ecode;
-               }
+
                if (!err || !IS_ERR_VALUE(err)) {
                        err = p9_errstr2errno(ename, strlen(ename));
 
@@ -628,8 +611,6 @@ static int p9_check_zc_errors(struct p9_client *c, struct 
p9_req_t *req,
        }
        return err;
 
-out_free:
-       kfree(ename);
 out_err:
        p9_debug(P9_DEBUG_ERROR, "couldn't parse error%d\n", err);
        return err;

commit 04a886359d01c297adeaa2328de5d2598fe77e07
Author: Aneesh Kumar K.V <address@hidden>
Date:   Mon May 20 17:28:29 2013 +0530

    hw/9pfs: use O_NOFOLLOW for mapped readlink operation
    
    With mapped security models like mapped-xattr and mapped-file, we save the
    symlink target as file contents. Now if we ever expose a normal directory
    with mapped security model and find real symlinks in export path, never
    follow them and return proper error.
    
    Signed-off-by: Aneesh Kumar K.V <address@hidden>

diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index 6ece6f7..87aa75d 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -284,7 +284,7 @@ static ssize_t local_readlink(FsContext *fs_ctx, V9fsPath 
*fs_path,
     if ((fs_ctx->export_flags & V9FS_SM_MAPPED) ||
         (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE)) {
         int fd;
-        fd = open(rpath(fs_ctx, path, buffer), O_RDONLY);
+        fd = open(rpath(fs_ctx, path, buffer), O_RDONLY | O_NOFOLLOW);
         if (fd == -1) {
             return -1;
         }