| 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
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,
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(-)