block: Mark bdrv_skip_implicit_filters() and callers GRAPH_RDLOCK

This adds GRAPH_RDLOCK annotations to declare that callers of bdrv_skip_implicit_filters() need to hold a reader lock for the graph because it calls bdrv_filter_child(), which accesses bs->file/backing. Signed-off-by: Kevin Wolf <[email protected]> Message-ID: <[email protected]> Reviewed-by: Eric Blake <[email protected]> Signed-off-by: Kevin Wolf <[email protected]>
devos50 · Nov 7, 2023 · 430da83 · 430da83
1 parent 372b69f
commit 430da83
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 16 deletions.
diff --git a/block.c b/block.c
@@ -4778,6 +4778,8 @@ bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
         return 0;
     }
 
+    bdrv_graph_rdlock_main_loop();
+
     switch (qobject_type(value)) {
     case QTYPE_QNULL:
         assert(is_backing); /* The 'file' option does not allow a null value */
@@ -4787,17 +4789,16 @@ bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
         str = qstring_get_str(qobject_to(QString, value));
         new_child_bs = bdrv_lookup_bs(NULL, str, errp);
         if (new_child_bs == NULL) {
-            return -EINVAL;
+            ret = -EINVAL;
+            goto out_rdlock;
         }
 
-        bdrv_graph_rdlock_main_loop();
         has_child = bdrv_recurse_has_child(new_child_bs, bs);
-        bdrv_graph_rdunlock_main_loop();
-
         if (has_child) {
             error_setg(errp, "Making '%s' a %s child of '%s' would create a "
                        "cycle", str, child_name, bs->node_name);
-            return -EINVAL;
+            ret = -EINVAL;
+            goto out_rdlock;
         }
         break;
     default:
@@ -4809,18 +4810,21 @@ bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
     }
 
     if (old_child_bs == new_child_bs) {
-        return 0;
+        ret = 0;
+        goto out_rdlock;
     }
 
     if (old_child_bs) {
         if (bdrv_skip_implicit_filters(old_child_bs) == new_child_bs) {
-            return 0;
+            ret = 0;
+            goto out_rdlock;
         }
 
         if (old_child_bs->implicit) {
             error_setg(errp, "Cannot replace implicit %s child of %s",
                        child_name, bs->node_name);
-            return -EPERM;
+            ret = -EPERM;
+            goto out_rdlock;
         }
     }
 
@@ -4831,7 +4835,8 @@ bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
          */
         error_setg(errp, "'%s' is a %s filter node that does not support a "
                    "%s child", bs->node_name, bs->drv->format_name, child_name);
-        return -EINVAL;
+        ret = -EINVAL;
+        goto out_rdlock;
     }
 
     if (is_backing) {
@@ -4852,6 +4857,7 @@ bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
         aio_context_acquire(ctx);
     }
 
+    bdrv_graph_rdunlock_main_loop();
     bdrv_graph_wrlock(new_child_bs);
 
     ret = bdrv_set_file_or_backing_noperm(bs, new_child_bs, is_backing,
@@ -4870,6 +4876,10 @@ bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
     }
 
     return ret;
+
+out_rdlock:
+    bdrv_graph_rdunlock_main_loop();
+    return ret;
 }
 
 /*

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
@@ -206,6 +206,9 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
     BlockBackend *blk;
     int ret;
 
+    GLOBAL_STATE_CODE();
+    GRAPH_RDLOCK_GUARD_MAINLOOP();
+
     if (!strcmp(device, "all")) {
         ret = blk_commit_all();
     } else {

diff --git a/blockdev.c b/blockdev.c
@@ -1746,10 +1746,10 @@ static void drive_backup_action(DriveBackup *backup,
         assert(format);
         if (source) {
             /* Implicit filters should not appear in the filename */
-            BlockDriverState *explicit_backing =
-                bdrv_skip_implicit_filters(source);
+            BlockDriverState *explicit_backing;
 
             bdrv_graph_rdlock_main_loop();
+            explicit_backing = bdrv_skip_implicit_filters(source);
             bdrv_refresh_filename(explicit_backing);
             bdrv_graph_rdunlock_main_loop();
 
@@ -3108,16 +3108,18 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
         bdrv_img_create(arg->target, format,
                         NULL, NULL, NULL, size, flags, false, &local_err);
     } else {
-        /* Implicit filters should not appear in the filename */
-        BlockDriverState *explicit_backing =
-            bdrv_skip_implicit_filters(target_backing_bs);
+        BlockDriverState *explicit_backing;
 
         switch (arg->mode) {
         case NEW_IMAGE_MODE_EXISTING:
             break;
         case NEW_IMAGE_MODE_ABSOLUTE_PATHS:
-            /* create new image with backing file */
+            /*
+             * Create new image with backing file.
+             * Implicit filters should not appear in the filename.
+             */
             bdrv_graph_rdlock_main_loop();
+            explicit_backing = bdrv_skip_implicit_filters(target_backing_bs);
             bdrv_refresh_filename(explicit_backing);
             bdrv_graph_rdunlock_main_loop();
 

diff --git a/include/block/block_int-global-state.h b/include/block/block_int-global-state.h
@@ -277,7 +277,8 @@ BdrvDirtyBitmap *block_dirty_bitmap_remove(const char *node, const char *name,
                                            Error **errp);
 
 
-BlockDriverState *bdrv_skip_implicit_filters(BlockDriverState *bs);
+BlockDriverState * GRAPH_RDLOCK
+bdrv_skip_implicit_filters(BlockDriverState *bs);
 
 /**
  * bdrv_add_aio_context_notifier: