* [PULL 1/3] hw/9pfs: avoid 'path' copy in v9fs_walk()
2021-09-02 11:42 [PULL 0/3] 9p queue 2021-09-02 Christian Schoenebeck
@ 2021-09-02 11:42 ` Christian Schoenebeck
2021-09-02 11:42 ` [PULL 2/3] hw/9pfs: use g_autofree in v9fs_walk() where possible Christian Schoenebeck
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Christian Schoenebeck @ 2021-09-02 11:42 UTC (permalink / raw)
To: qemu-devel, Peter Maydell; +Cc: Greg Kurz
The v9fs_walk() function resolves all client submitted path nodes to the
local 'pathes' array. Using a separate string scalar variable 'path'
inside the background worker thread loop and copying that local 'path'
string scalar variable subsequently to the 'pathes' array (at the end of
each loop iteration) is not necessary.
Instead simply resolve each path directly to the 'pathes' array and
don't use the string scalar variable 'path' inside the fs worker thread
loop at all.
The only advantage of the 'path' scalar was that in case of an error
the respective 'pathes' element would not be filled. Right now this is
not an issue as the v9fs_walk() function returns as soon as any error
occurs.
Suggested-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Message-Id: <7dacbecf25b2c9b4a0ce12d689a8a535f09a31e3.1629208359.git.qemu_oss@crudebyte.com>
---
hw/9pfs/9p.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 2815257f42..4d642ab12a 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1787,7 +1787,8 @@ static void coroutine_fn v9fs_walk(void *opaque)
strcmp("..", wnames[name_idx].data))
{
err = s->ops->name_to_path(&s->ctx, &dpath,
- wnames[name_idx].data, &path);
+ wnames[name_idx].data,
+ &pathes[name_idx]);
if (err < 0) {
err = -errno;
break;
@@ -1796,14 +1797,13 @@ static void coroutine_fn v9fs_walk(void *opaque)
err = -EINTR;
break;
}
- err = s->ops->lstat(&s->ctx, &path, &stbuf);
+ err = s->ops->lstat(&s->ctx, &pathes[name_idx], &stbuf);
if (err < 0) {
err = -errno;
break;
}
stbufs[name_idx] = stbuf;
- v9fs_path_copy(&dpath, &path);
- v9fs_path_copy(&pathes[name_idx], &path);
+ v9fs_path_copy(&dpath, &pathes[name_idx]);
}
}
});
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PULL 2/3] hw/9pfs: use g_autofree in v9fs_walk() where possible
2021-09-02 11:42 [PULL 0/3] 9p queue 2021-09-02 Christian Schoenebeck
2021-09-02 11:42 ` [PULL 1/3] hw/9pfs: avoid 'path' copy in v9fs_walk() Christian Schoenebeck
@ 2021-09-02 11:42 ` Christian Schoenebeck
2021-09-02 11:42 ` [PULL 3/3] 9pfs: fix crash in v9fs_walk() Christian Schoenebeck
2021-09-03 12:08 ` [PULL 0/3] 9p queue 2021-09-02 Peter Maydell
3 siblings, 0 replies; 5+ messages in thread
From: Christian Schoenebeck @ 2021-09-02 11:42 UTC (permalink / raw)
To: qemu-devel, Peter Maydell; +Cc: Greg Kurz
Suggested-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Message-Id: <b51670d2a39399535a035f6bc77c3cbeed85edae.1629208359.git.qemu_oss@crudebyte.com>
---
hw/9pfs/9p.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 4d642ab12a..c857b31321 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1703,11 +1703,12 @@ static bool same_stat_id(const struct stat *a, const struct stat *b)
static void coroutine_fn v9fs_walk(void *opaque)
{
int name_idx;
- V9fsQID *qids = NULL;
+ g_autofree V9fsQID *qids = NULL;
int i, err = 0;
V9fsPath dpath, path, *pathes = NULL;
uint16_t nwnames;
- struct stat stbuf, fidst, *stbufs = NULL;
+ struct stat stbuf, fidst;
+ g_autofree struct stat *stbufs = NULL;
size_t offset = 7;
int32_t fid, newfid;
V9fsString *wnames = NULL;
@@ -1872,8 +1873,6 @@ out_nofid:
v9fs_path_free(&pathes[name_idx]);
}
g_free(wnames);
- g_free(qids);
- g_free(stbufs);
g_free(pathes);
}
}
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PULL 3/3] 9pfs: fix crash in v9fs_walk()
2021-09-02 11:42 [PULL 0/3] 9p queue 2021-09-02 Christian Schoenebeck
2021-09-02 11:42 ` [PULL 1/3] hw/9pfs: avoid 'path' copy in v9fs_walk() Christian Schoenebeck
2021-09-02 11:42 ` [PULL 2/3] hw/9pfs: use g_autofree in v9fs_walk() where possible Christian Schoenebeck
@ 2021-09-02 11:42 ` Christian Schoenebeck
2021-09-03 12:08 ` [PULL 0/3] 9p queue 2021-09-02 Peter Maydell
3 siblings, 0 replies; 5+ messages in thread
From: Christian Schoenebeck @ 2021-09-02 11:42 UTC (permalink / raw)
To: qemu-devel, Peter Maydell; +Cc: Greg Kurz
v9fs_walk() utilizes the v9fs_co_run_in_worker({...}) macro to run the
supplied fs driver code block on a background worker thread.
When either the 'Twalk' client request was interrupted or if the client
requested fid for that 'Twalk' request caused a stat error then that
fs driver code block was left by 'break' keyword, with the intention to
return from worker thread back to main thread as well:
v9fs_co_run_in_worker({
if (v9fs_request_cancelled(pdu)) {
err = -EINTR;
break;
}
err = s->ops->lstat(&s->ctx, &dpath, &fidst);
if (err < 0) {
err = -errno;
break;
}
...
});
However that 'break;' statement also skipped the v9fs_co_run_in_worker()
macro's final and mandatory
/* re-enter back to qemu thread */
qemu_coroutine_yield();
call and thus caused the rest of v9fs_walk() to be continued being
executed on the worker thread instead of main thread, eventually
leading to a crash in the transport virtio transport driver.
To fix this issue and to prevent the same error from happening again by
other users of v9fs_co_run_in_worker() in future, auto wrap the supplied
code block into its own
do { } while (0);
loop inside the 'v9fs_co_run_in_worker' macro definition.
Full discussion and backtrace:
https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg05209.html
https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg00174.html
Fixes: 8d6cb100731c4d28535adbf2a3c2d1f29be3fef4
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Cc: qemu-stable@nongnu.org
Reviewed-by: Greg Kurz <groug@kaod.org>
Message-Id: <E1mLTBg-0002Bh-2D@lizzy.crudebyte.com>
---
hw/9pfs/coth.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/9pfs/coth.h b/hw/9pfs/coth.h
index c51289903d..f83c7dda7b 100644
--- a/hw/9pfs/coth.h
+++ b/hw/9pfs/coth.h
@@ -51,7 +51,9 @@
*/ \
qemu_coroutine_yield(); \
qemu_bh_delete(co_bh); \
- code_block; \
+ do { \
+ code_block; \
+ } while (0); \
/* re-enter back to qemu thread */ \
qemu_coroutine_yield(); \
} while (0)
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PULL 0/3] 9p queue 2021-09-02
2021-09-02 11:42 [PULL 0/3] 9p queue 2021-09-02 Christian Schoenebeck
` (2 preceding siblings ...)
2021-09-02 11:42 ` [PULL 3/3] 9pfs: fix crash in v9fs_walk() Christian Schoenebeck
@ 2021-09-03 12:08 ` Peter Maydell
3 siblings, 0 replies; 5+ messages in thread
From: Peter Maydell @ 2021-09-03 12:08 UTC (permalink / raw)
To: Christian Schoenebeck; +Cc: QEMU Developers, Greg Kurz
On Thu, 2 Sept 2021 at 12:50, Christian Schoenebeck
<qemu_oss@crudebyte.com> wrote:
>
> The following changes since commit 59a89510b62ec23dbeab8b02fa4e3526e353d8b6:
>
> Merge remote-tracking branch 'remotes/stefanberger/tags/pull-tpm-2021-09-01-1' into staging (2021-09-02 08:51:31 +0100)
>
> are available in the Git repository at:
>
> https://github.com/cschoenebeck/qemu.git tags/pull-9p-20210902
>
> for you to fetch changes up to f83df00900816476cca41bb536e4d532b297d76e:
>
> 9pfs: fix crash in v9fs_walk() (2021-09-02 13:26:22 +0200)
>
> ----------------------------------------------------------------
> 9pfs: misc patches
>
> * Fix an occasional crash when handling 'Twalk' requests.
>
> * Two code cleanup patches.
>
Applied, thanks.
Please update the changelog at https://wiki.qemu.org/ChangeLog/6.2
for any user-visible changes.
-- PMM
^ permalink raw reply [flat|nested] 5+ messages in thread