From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:51398) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QEpZ8-00060h-N3 for qemu-devel@nongnu.org; Tue, 26 Apr 2011 17:13:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QEpZ7-0003Dx-6a for qemu-devel@nongnu.org; Tue, 26 Apr 2011 17:13:02 -0400 Received: from e36.co.us.ibm.com ([32.97.110.154]:47277) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QEpZ6-0003Do-Td for qemu-devel@nongnu.org; Tue, 26 Apr 2011 17:13:01 -0400 Received: from d03relay02.boulder.ibm.com (d03relay02.boulder.ibm.com [9.17.195.227]) by e36.co.us.ibm.com (8.14.4/8.13.1) with ESMTP id p3QL7KaY029784 for ; Tue, 26 Apr 2011 15:07:20 -0600 Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d03relay02.boulder.ibm.com (8.13.8/8.13.8/NCO v9.1) with ESMTP id p3QLCojg159962 for ; Tue, 26 Apr 2011 15:12:52 -0600 Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1]) by d03av02.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p3QLCJqR029024 for ; Tue, 26 Apr 2011 15:12:19 -0600 Message-ID: <4DB7354D.4040005@linux.vnet.ibm.com> Date: Tue, 26 Apr 2011 14:12:45 -0700 From: Venkateswararao Jujjuri MIME-Version: 1.0 References: <1303836715-4401-1-git-send-email-sassan@sassan.me.uk> In-Reply-To: <1303836715-4401-1-git-send-email-sassan@sassan.me.uk> Content-Type: multipart/alternative; boundary="------------020706040108040500090706" Subject: Re: [Qemu-devel] [PATCH] Fix bug with virtio-9p fsync List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org, Stefan Hajnoczi This is a multi-part message in MIME format. --------------020706040108040500090706 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit On 04/26/2011 09:51 AM, Sassan Panahinejad wrote: > v9fs_fsync and possibly others break when asked to operate on a directory. > It does not check fid_type to see if it is operating on a directory and therefore accesses the wrong element of the fs union. > This error can result in guest applications failing (in my case it was dpkg). > This patch fixes the issue, although there may be other, similar bugs in virtio-9p. > > Signed-off-by: Sassan Panahinejad > --- > > Here I've implemented it as a function. If you'd prefer a macro or inline function then let me know. > > Thanks > Sassan Please put your commentary on the top. After "---" you should only expect code diff. > hw/virtio-9p.c | 43 +++++++++++++++++++++++++++++++++++++------ > 1 files changed, 37 insertions(+), 6 deletions(-) > > diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c > index 7e29535..26be0fc 100644 > --- a/hw/virtio-9p.c > +++ b/hw/virtio-9p.c > @@ -1860,6 +1860,18 @@ static void v9fs_post_do_fsync(V9fsState *s, V9fsPDU *pdu, int err) > complete_pdu(s, pdu, err); > } > > +static int v9fs_get_fd(V9fsFidState *fidp) > +{ > + switch (fidp->fid_type) { > + case P9_FID_FILE: > + return fidp->fs.fd; > + case P9_FID_DIR: > + return dirfd(fidp->fs.dir); > + default: > + return -1; > + } > +} > + > static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu) > { > int32_t fid; > @@ -1867,6 +1879,7 @@ static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu) > V9fsFidState *fidp; > int datasync; > int err; > + int fd; > > pdu_unmarshal(pdu, offset, "dd",&fid,&datasync); > fidp = lookup_fid(s, fid); > @@ -1875,7 +1888,11 @@ static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu) > v9fs_post_do_fsync(s, pdu, err); > return; > } > - err = v9fs_do_fsync(s, fidp->fs.fd, datasync); > + if ((fd = v9fs_get_fd(fidp))>= 0) { > + err = v9fs_do_fsync(s, fd, datasync); > + } else { > + err = -EINVAL; > + } > v9fs_post_do_fsync(s, pdu, err); > } > > @@ -2984,6 +3001,7 @@ static void v9fs_wstat(V9fsState *s, V9fsPDU *pdu) > int32_t fid; > V9fsWstatState *vs; > int err = 0; > + int fd; > > vs = qemu_malloc(sizeof(*vs)); > vs->pdu = pdu; > @@ -2999,7 +3017,10 @@ static void v9fs_wstat(V9fsState *s, V9fsPDU *pdu) > > /* do we need to sync the file? */ > if (donttouch_stat(&vs->v9stat)) { > - err = v9fs_do_fsync(s, vs->fidp->fs.fd, 0); > + if ((fd = v9fs_get_fd(vs->fidp))>= 0) { > + err = v9fs_do_fsync(s, fd, 0); > + } else > + err = -EINVAL; > v9fs_wstat_post_fsync(s, vs, err); > return; > } > @@ -3176,6 +3197,7 @@ static void v9fs_lock(V9fsState *s, V9fsPDU *pdu) > { > int32_t fid, err = 0; > V9fsLockState *vs; > + int fd; > > vs = qemu_mallocz(sizeof(*vs)); > vs->pdu = pdu; > @@ -3198,8 +3220,12 @@ static void v9fs_lock(V9fsState *s, V9fsPDU *pdu) > err = -ENOENT; > goto out; > } > - > - err = v9fs_do_fstat(s, vs->fidp->fs.fd,&vs->stbuf); > + if ((fd = v9fs_get_fd(vs->fidp))>= 0) { > + err = v9fs_do_fstat(s, fd,&vs->stbuf); > + } else { > + err = -EINVAL; > + goto out; > + } I think we need a different handle for lock and getlock. If the operation is on a dir we need to return EISDIR The /cmd/ parameter is F_GETLK, F_GETLK64, F_SETLK, F_SETLK64, F_SETLKW, or F_SETLKW64, and /fildes/ refers to a directory. Since the handling is different, I am not sure if the separate function makes any sense now. May be go back to your initial check for sync and for locks, if the fid is a dir type then EISDIR otherwise EINVAL. Thanks, JV > if (err< 0) { > err = -errno; > goto out; > @@ -3221,6 +3247,7 @@ static void v9fs_getlock(V9fsState *s, V9fsPDU *pdu) > { > int32_t fid, err = 0; > V9fsGetlockState *vs; > + int fd; > > vs = qemu_mallocz(sizeof(*vs)); > vs->pdu = pdu; > @@ -3236,8 +3263,12 @@ static void v9fs_getlock(V9fsState *s, V9fsPDU *pdu) > err = -ENOENT; > goto out; > } > - > - err = v9fs_do_fstat(s, vs->fidp->fs.fd,&vs->stbuf); > + if ((fd = v9fs_get_fd(vs->fidp))>= 0) { > + err = v9fs_do_fstat(s, fd,&vs->stbuf); > + } else { > + err = -EINVAL; > + goto out; > + } > if (err< 0) { > err = -errno; > goto out; --------------020706040108040500090706 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit On 04/26/2011 09:51 AM, Sassan Panahinejad wrote:
v9fs_fsync and possibly others break when asked to operate on a directory.
It does not check fid_type to see if it is operating on a directory and therefore accesses the wrong element of the fs union.
This error can result in guest applications failing (in my case it was dpkg).
This patch fixes the issue, although there may be other, similar bugs in virtio-9p.

Signed-off-by: Sassan Panahinejad <sassan@sassan.me.uk>
---

Here I've implemented it as a function. If you'd prefer a macro or inline function then let me know.

Thanks
Sassan

Please put your commentary on the top. After "---" you should only expect code diff.
 hw/virtio-9p.c |   43 +++++++++++++++++++++++++++++++++++++------
 1 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
index 7e29535..26be0fc 100644
--- a/hw/virtio-9p.c
+++ b/hw/virtio-9p.c
@@ -1860,6 +1860,18 @@ static void v9fs_post_do_fsync(V9fsState *s, V9fsPDU *pdu, int err)
     complete_pdu(s, pdu, err);
 }

+static int v9fs_get_fd(V9fsFidState *fidp) 
+{
+    switch (fidp->fid_type) {
+        case P9_FID_FILE: 
+            return fidp->fs.fd;
+        case P9_FID_DIR: 
+            return dirfd(fidp->fs.dir);
+        default:
+            return -1;
+    }
+}
+
 static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu)
 {
     int32_t fid;
@@ -1867,6 +1879,7 @@ static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu)
     V9fsFidState *fidp;
     int datasync;
     int err;
+    int fd;

     pdu_unmarshal(pdu, offset, "dd", &fid, &datasync);
     fidp = lookup_fid(s, fid);
@@ -1875,7 +1888,11 @@ static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu)
         v9fs_post_do_fsync(s, pdu, err);
         return;
     }
-    err = v9fs_do_fsync(s, fidp->fs.fd, datasync);
+    if ((fd = v9fs_get_fd(fidp)) >= 0) {
+        err = v9fs_do_fsync(s, fd, datasync);
+    } else {
+	err = -EINVAL;
+    }
     v9fs_post_do_fsync(s, pdu, err);
 }

@@ -2984,6 +3001,7 @@ static void v9fs_wstat(V9fsState *s, V9fsPDU *pdu)
     int32_t fid;
     V9fsWstatState *vs;
     int err = 0;
+    int fd;

     vs = qemu_malloc(sizeof(*vs));
     vs->pdu = pdu;
@@ -2999,7 +3017,10 @@ static void v9fs_wstat(V9fsState *s, V9fsPDU *pdu)

     /* do we need to sync the file? */
     if (donttouch_stat(&vs->v9stat)) {
-        err = v9fs_do_fsync(s, vs->fidp->fs.fd, 0);
+        if ((fd = v9fs_get_fd(vs->fidp)) >= 0) {
+            err = v9fs_do_fsync(s, fd, 0);
+        } else
+            err = -EINVAL;
         v9fs_wstat_post_fsync(s, vs, err);
         return;
     }
@@ -3176,6 +3197,7 @@ static void v9fs_lock(V9fsState *s, V9fsPDU *pdu)
 {
     int32_t fid, err = 0;
     V9fsLockState *vs;
+    int fd;

     vs = qemu_mallocz(sizeof(*vs));
     vs->pdu = pdu;
@@ -3198,8 +3220,12 @@ static void v9fs_lock(V9fsState *s, V9fsPDU *pdu)
         err = -ENOENT;
         goto out;
     }
-
-    err = v9fs_do_fstat(s, vs->fidp->fs.fd, &vs->stbuf);
+    if ((fd = v9fs_get_fd(vs->fidp)) >= 0) {
+        err = v9fs_do_fstat(s, fd, &vs->stbuf);
+    } else {
+        err = -EINVAL;
+        goto out;
+    }

I think we need a different handle for lock and getlock.
If the operation is on a dir we need to return
EISDIR 

The cmd parameter is F_GETLK, F_GETLK64, F_SETLK, F_SETLK64, F_SETLKW, or F_SETLKW64, and fildes refers to a directory.

Since the handling is different, I am not sure if the separate function makes any sense now.
May be go back to your initial check for sync and for locks, if the fid is a dir type then EISDIR
otherwise EINVAL.
Thanks,
JV


     if (err < 0) {
         err = -errno;
         goto out;
@@ -3221,6 +3247,7 @@ static void v9fs_getlock(V9fsState *s, V9fsPDU *pdu)
 {
     int32_t fid, err = 0;
     V9fsGetlockState *vs;
+    int fd;

     vs = qemu_mallocz(sizeof(*vs));
     vs->pdu = pdu;
@@ -3236,8 +3263,12 @@ static void v9fs_getlock(V9fsState *s, V9fsPDU *pdu)
         err = -ENOENT;
         goto out;
     }
-
-    err = v9fs_do_fstat(s, vs->fidp->fs.fd, &vs->stbuf);
+    if ((fd = v9fs_get_fd(vs->fidp)) >= 0) {
+        err = v9fs_do_fstat(s, fd, &vs->stbuf);
+    } else {
+        err = -EINVAL;
+        goto out;
+    }
     if (err < 0) {
         err = -errno;
         goto out;

--------------020706040108040500090706--