9pfs: fix crash in v9fs_walk()

[PATCH] 9pfs: fix crash in v9fs_walk()

Posted by Christian Schoenebeck 2 years, 7 months ago

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

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

Re: [SPAM] [PATCH] 9pfs: fix crash in v9fs_walk()

Posted by Greg Kurz 2 years, 7 months ago

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)

Re: [PATCH] 9pfs: fix crash in v9fs_walk()

Posted by Christian Schoenebeck 2 years, 7 months ago

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