The function reads the parents list, so it needs to hold the graph lock.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block.c b/block.c
index 7df8780d6e..a82389f742 100644
--- a/block.c
+++ b/block.c
@@ -5934,9 +5934,11 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
         backing_file_str = base->filename;
     }
 
+    bdrv_graph_rdlock_main_loop();
     QLIST_FOREACH(c, &top->parents, next_parent) {
         updated_children = g_slist_prepend(updated_children, c);
     }
+    bdrv_graph_rdunlock_main_loop();
 
     /*
      * It seems correct to pass detach_subchain=true here, but it triggers
-- 
2.41.0

On Thu, Aug 17, 2023 at 02:50:16PM +0200, Kevin Wolf wrote:
> The function reads the parents list, so it needs to hold the graph lock.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 7df8780d6e..a82389f742 100644
> --- a/block.c
> +++ b/block.c
> @@ -5934,9 +5934,11 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
>          backing_file_str = base->filename;
>      }
>  
> +    bdrv_graph_rdlock_main_loop();
>      QLIST_FOREACH(c, &top->parents, next_parent) {
>          updated_children = g_slist_prepend(updated_children, c);
>      }
> +    bdrv_graph_rdunlock_main_loop();

This is GLOBAL_STATE_CODE, so why take the read lock? I thought nothing
can modify the graph here. If it could, then stashing the parents in the
updated_children probably wouldn't be safe anyway.

>  
>      /*
>       * It seems correct to pass detach_subchain=true here, but it triggers
> -- 
> 2.41.0
> 

Am 22.08.2023 um 21:35 hat Stefan Hajnoczi geschrieben:
> On Thu, Aug 17, 2023 at 02:50:16PM +0200, Kevin Wolf wrote:
> > The function reads the parents list, so it needs to hold the graph lock.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/block.c b/block.c
> > index 7df8780d6e..a82389f742 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -5934,9 +5934,11 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
> >          backing_file_str = base->filename;
> >      }
> >  
> > +    bdrv_graph_rdlock_main_loop();
> >      QLIST_FOREACH(c, &top->parents, next_parent) {
> >          updated_children = g_slist_prepend(updated_children, c);
> >      }
> > +    bdrv_graph_rdunlock_main_loop();
> 
> This is GLOBAL_STATE_CODE, so why take the read lock? I thought nothing
> can modify the graph here. If it could, then stashing the parents in
> the updated_children probably wouldn't be safe anyway.

The only thing bdrv_graph_rdlock_main_loop() does is asserting that the
conditions for doing nothing are met (GLOBAL_STATE_CODE + non-coroutine
context) and providing the right TSA attributes to make the compiler
happy.

Kevin

Am 17/08/2023 um 14:50 schrieb Kevin Wolf:
> The function reads the parents list, so it needs to hold the graph lock.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>