[Devel] [PATCH vz7] ploop: properly restore old delta kobject on replace error

Alexander Atanasov alexander.atanasov at virtuozzo.com
Mon May 22 18:19:03 MSK 2023


Current code removes the old_delta kobject before the new delta
is ready to be used in case there is an error while reading BAT,
the "too short BAT" error we have seen.
The new delta and its objects are destroyed properly but the original
delta kobject is never added back, so pdelta sysfs dir stay empty and
userspace tools get confused since they consult this files before
performing operations on the ploop device. At later point when ploop
device is destroyed a second kobject_del will be called and it can lead
to a crash or corrupted memory.

To fix this instead of deleting the delta early rename it with prefix
and move the deletion at the end, while on error rename back to original
name. This way if something goes wrong it will be visible in sysfs as
old_N, where N is the delta level.

Extract code to rename kobject into a function and cleanup its users.

Fixes:
commit 5f3ee110e6f4 ("ploop: Repopulate holes_bitmap on changing delta")

https://jira.vzint.dev/browse/PSBM-146797
Signed-off-by: Alexander Atanasov <alexander.atanasov at virtuozzo.com>
---
 drivers/block/ploop/dev.c | 47 ++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/drivers/block/ploop/dev.c b/drivers/block/ploop/dev.c
index 6eb22168b5fe..0ed63f9da1cb 100644
--- a/drivers/block/ploop/dev.c
+++ b/drivers/block/ploop/dev.c
@@ -3557,6 +3557,16 @@ static int ploop_add_delta(struct ploop_device * plo, unsigned long arg)
 	return err;
 }
 
+/*
+ * Have to implement own version of kobject_rename since it is GPL only symbol
+ */
+static int ploop_rename_delta(struct ploop_delta *delta, int level, char *pref)
+{
+	kobject_del(&delta->kobj);
+	return KOBJECT_ADD(&delta->kobj, &delta->plo->kobj,
+			  "%s%d", pref ? : "", level);
+}
+
 static int ploop_replace_delta(struct ploop_device * plo, unsigned long arg)
 {
 	int err;
@@ -3594,15 +3604,9 @@ static int ploop_replace_delta(struct ploop_device * plo, unsigned long arg)
 	if (err)
 		goto out_destroy;
 
-	kobject_del(&old_delta->kobj);
-
-	err = KOBJECT_ADD(&delta->kobj, kobject_get(&plo->kobj),
-			  "%d", delta->level);
-	/* _put below is a pair for _get for OLD delta */
-	kobject_put(&plo->kobj);
-
+	err = ploop_rename_delta(old_delta, old_delta->level, "old_");
 	if (err < 0) {
-		kobject_put(&plo->kobj);
+		PL_WARN(plo, "Failed to rename old delta kobj\n");
 		goto out_close;
 	}
 
@@ -3611,10 +3615,17 @@ static int ploop_replace_delta(struct ploop_device * plo, unsigned long arg)
 		err = delta->ops->replace_delta(delta);
 		if (err) {
 			ploop_relax(plo);
-			goto out_kobj_del;
+			goto out_close;
 		}
 	}
 
+	err = KOBJECT_ADD(&delta->kobj, kobject_get(&plo->kobj),
+			  "%d", delta->level);
+	if (err < 0) {
+		/* _put for failed _ADD */
+		kobject_put(&plo->kobj);
+		goto out_close;
+	}
 	ploop_map_destroy(&plo->map);
 	list_replace_init(&old_delta->list, &delta->list);
 	ploop_delta_list_changed(plo);
@@ -3631,13 +3642,15 @@ static int ploop_replace_delta(struct ploop_device * plo, unsigned long arg)
 
 	old_delta->ops->stop(old_delta);
 	old_delta->ops->destroy(old_delta);
+	kobject_del(&old_delta->kobj);
 	kobject_put(&old_delta->kobj);
+	kobject_put(&plo->kobj);
 	return 0;
 
-out_kobj_del:
-	kobject_del(&delta->kobj);
-	kobject_put(&plo->kobj);
 out_close:
+	err = ploop_rename_delta(old_delta, old_delta->level, NULL);
+	if (err < 0)
+		PL_ERR(plo, "Failed to restore old delta kobj name\n");
 	delta->ops->stop(delta);
 out_destroy:
 	delta->ops->destroy(delta);
@@ -3974,15 +3987,7 @@ static void rename_deltas(struct ploop_device * plo, int level)
 
 		if (delta->level < level)
 			continue;
-#if 0
-		/* Oops, kobject_rename() is not exported! */
-		sprintf(nname, "%d", delta->level);
-		err = kobject_rename(&delta->kobj, nname);
-#else
-		kobject_del(&delta->kobj);
-		err = KOBJECT_ADD(&delta->kobj, &plo->kobj,
-				  "%d", delta->level);
-#endif
+		err = ploop_rename_delta(delta, delta->level, NULL);
 		if (err)
 			PL_WARN(plo, "rename_deltas: %d %d %d\n", err, level, delta->level);
 	}
-- 
2.39.1



More information about the Devel mailing list