Re: [Qemu-devel] [PATCH] Fix bug with virtio-9p fsync

[Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 4554 bytes --]

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;


[-- Attachment #2: Type: text/html, Size: 5588 bytes --]