Patchwork Fix bug with virtio-9p fsync

login
register
mail settings
Submitter Sassan Panahinejad
Date April 26, 2011, 12:14 p.m.
Message ID <1303820065-25262-1-git-send-email-sassan@sassan.me.uk>
Download mbox | patch
Permalink /patch/92913/
State New
Headers show

Comments

Sassan Panahinejad - April 26, 2011, 12:14 p.m.
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>
---
 hw/virtio-9p.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
Stefan Hajnoczi - April 26, 2011, 12:58 p.m.
On Tue, Apr 26, 2011 at 1:14 PM, Sassan Panahinejad <sassan@sassan.me.uk> 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>
> ---
>  hw/virtio-9p.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
> index 7e29535..cc4fdc8 100644
> --- a/hw/virtio-9p.c
> +++ b/hw/virtio-9p.c
> @@ -1875,7 +1875,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 (fidp->fid_type == P9_FID_DIR) {
> +        err = v9fs_do_fsync(s, dirfd(fidp->fs.dir), datasync);
> +    } else {
> +        err = v9fs_do_fsync(s, fidp->fs.fd, datasync);
> +    }

What about P9_FID_XATTR, seems like we have the same issue there too?

wstat, lock, and getlock need closer auditing and perhaps fixing.

Stefan
Sassan Panahinejad - April 26, 2011, 1:29 p.m.
On 26 April 2011 13:58, Stefan Hajnoczi <stefanha@gmail.com> wrote:

> What about P9_FID_XATTR, seems like we have the same issue there too?
>
> wstat, lock, and getlock need closer auditing and perhaps fixing.
>
> Stefan
>

Sorry, forgot to hit reply-to-all.

Yes, it is probable that those functions will suffer from the same bug.
I will have to study XATTR and see how that will be affected. I don't know
whether it is possible for these functions to be called for XATTR, and if it
is then I do not know the proper way to handle it.
Perhaps we should have some function or macro to obtain the correct FD from
an fidp structure, which could be used for fsync, wstat, lock and getlock?

Sassan
jvrao - April 26, 2011, 2:25 p.m.
On 04/26/2011 06:29 AM, Sassan Panahinejad wrote:
> I will have to study XATTR and see how that will be affected. I don't 
> know whether it is possible for these functions to be called for 
> XATTR, and if it is then I do not know the proper way to handle it.
> Perhaps we should have some function or macro to obtain the correct FD 
> from an fidp structure, which could be used for fsync, wstat, lock and 
> getlock?
I agree; we need some level of macro for this. How about doing that as 
part of this patch itself?

Thanks,
JV

Patch

diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
index 7e29535..cc4fdc8 100644
--- a/hw/virtio-9p.c
+++ b/hw/virtio-9p.c
@@ -1875,7 +1875,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 (fidp->fid_type == P9_FID_DIR) {
+        err = v9fs_do_fsync(s, dirfd(fidp->fs.dir), datasync);
+    } else {
+        err = v9fs_do_fsync(s, fidp->fs.fd, datasync);
+    }
     v9fs_post_do_fsync(s, pdu, err);
 }