Re: [Qemu-devel] [PATCH 1/2] 9p-synth: fix read-side critical sections

[Harsh Bora <harsh@linux.vnet.ibm.com>]

On 08/08/2012 05:25 PM, Paolo Bonzini wrote:
> The read-side critical sections in 9p-synth currently only include the
> navigation of the list.  This is incorrect; it works for two reasons,
> first obviously because rcu_read_lock/unlock are still no-ops; second,
> because elements of the list are never deleted from the list (only added).
> In fact, only adding items is the reason why rcu_read_lock/unlock can
> be left as no-ops.
>
> If items were deleted, they could be reclaimed as soon as the read-side
> critical section ends.  So, the read-side critical section must include
> all _usage_ of the node we got from the list too.

Acked-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>

>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/9pfs/virtio-9p-synth.c | 35 ++++++++++++++++++++---------------
>   1 file modificato, 20 inserzioni(+), 15 rimozioni(-)
>
> diff --git a/hw/9pfs/virtio-9p-synth.c b/hw/9pfs/virtio-9p-synth.c
> index 92e0b09..a91ebe1 100644
> --- a/hw/9pfs/virtio-9p-synth.c
> +++ b/hw/9pfs/virtio-9p-synth.c
> @@ -237,14 +237,15 @@ static int v9fs_synth_get_dentry(V9fsSynthNode *dir, struct dirent *entry,
>           }
>           i++;
>       }
> -    rcu_read_unlock();
>       if (!node) {
>           /* end of directory */
>           *result = NULL;
> -        return 0;
> +        goto out;
>       }
>       v9fs_synth_direntry(node, entry, off);
>       *result = entry;
> +out:
> +    rcu_read_unlock();
>       return 0;
>   }
>
> @@ -466,6 +467,7 @@ static int v9fs_synth_name_to_path(FsContext *ctx, V9fsPath *dir_path,
>   {
>       V9fsSynthNode *node;
>       V9fsSynthNode *dir_node;
> +    int ret = 0;
>
>       /* "." and ".." are not allowed */
>       if (!strcmp(name, ".") || !strcmp(name, "..")) {
> @@ -473,34 +475,37 @@ static int v9fs_synth_name_to_path(FsContext *ctx, V9fsPath *dir_path,
>           return -1;
>
>       }
> +
> +    rcu_read_lock();
>       if (!dir_path) {
>           dir_node = &v9fs_synth_root;
>       } else {
>           dir_node = *(V9fsSynthNode **)dir_path->data;
>       }
> -    if (!strcmp(name, "/")) {
> -        node = dir_node;
> -        goto out;
> -    }
> -    /* search for the name in the childern */
> -    rcu_read_lock();
> -    QLIST_FOREACH(node, &dir_node->child, sibling) {
> -        if (!strcmp(node->name, name)) {
> -            break;
> +
> +    node = dir_node;
> +    if (strcmp(name, "/") != 0) {
> +        /* search for the name in the childern */
> +        QLIST_FOREACH(node, &dir_node->child, sibling) {
> +            if (!strcmp(node->name, name)) {
> +                break;
> +            }
>           }
>       }
> -    rcu_read_unlock();
>
>       if (!node) {
>           errno = ENOENT;
> -        return -1;
> +        ret = -1;
> +        goto err_out;
>       }
> -out:
> +
>       /* Copy the node pointer to fid */
>       target->data = g_malloc(sizeof(void *));
>       memcpy(target->data, &node, sizeof(void *));
>       target->size = sizeof(void *);
> -    return 0;
> +err_out:
> +    rcu_read_unlock();
> +    return ret;
>   }
>
>   static int v9fs_synth_renameat(FsContext *ctx, V9fsPath *olddir,
>