Message ID | E1mLTBg-0002Bh-2D@lizzy.crudebyte.com |
---|---|
State | New |
Headers | show |
Series | 9pfs: fix crash in v9fs_walk() | expand |
On Wed, 1 Sep 2021 18:15:10 +0200 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > 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> > 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)
On Mittwoch, 1. September 2021 18:47:21 CEST Greg Kurz wrote: > On Wed, 1 Sep 2021 18:15:10 +0200 > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > 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> Queued on 9p.next: https://github.com/cschoenebeck/qemu/commits/9p.next Thanks! I'll send out a PR tomorrow. > > 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) Best regards, Christian Schoenebeck
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)
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 --- hw/9pfs/coth.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)