Patchwork [1/2] 9p-synth: fix read-side critical sections

login
register
mail settings
Submitter Paolo Bonzini
Date Aug. 8, 2012, 11:55 a.m.
Message ID <1344426944-7638-2-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/175915/
State New
Headers show

Comments

Paolo Bonzini - Aug. 8, 2012, 11:55 a.m.
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.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/9pfs/virtio-9p-synth.c | 35 ++++++++++++++++++++---------------
 1 file modificato, 20 inserzioni(+), 15 rimozioni(-)
Harsh Prateek Bora - Aug. 13, 2012, 6:47 p.m.
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,
>

Patch

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,