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

[Alexander Atanasov]

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 move it at the end,
on error restore back original.

Extract code to rename kobject into a function and cleanup its users.
Since kobject_add can fail, get an extra reference on error so later
kobject_del/kobject_put would not destroy it unexpectedly. Object
should be intact except that it would not be visible in sysfs.

Fixes:
5f3ee110e6f4 (ploop: Repopulate holes_bitmap on changing delta, 2019-04-17)

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

v1->v2: Addressing review comments. Removed the rename to minimize
possibility of errors. added getting extra refs after failed add.

diff --git a/drivers/block/ploop/dev.c b/drivers/block/ploop/dev.c
index 6eb22168b5fe..2833865b087f 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,27 +3604,25 @@ 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);
-
-	if (err < 0) {
-		kobject_put(&plo->kobj);
-		goto out_close;
-	}
 
 	ploop_quiesce(plo);
 	if (delta->ops->replace_delta) {
 		err = delta->ops->replace_delta(delta);
 		if (err) {
 			ploop_relax(plo);
-			goto out_kobj_del;
+			goto out_close;
 		}
 	}
 
+	/* Remove old delta kobj to avoid name collision with the new one */
+	kobject_del(&old_delta->kobj);
+	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_kobj_restore;
+	}
 	ploop_map_destroy(&plo->map);
 	list_replace_init(&old_delta->list, &delta->list);
 	ploop_delta_list_changed(plo);
@@ -3632,11 +3640,19 @@ 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_put(&old_delta->kobj);
-	return 0;
-
-out_kobj_del:
-	kobject_del(&delta->kobj);
 	kobject_put(&plo->kobj);
+	return 0;
+out_kobj_restore:
+	/* we haven't dropped our plo->kobj ref just add back */
+	err = KOBJECT_ADD(&old_delta->kobj, &plo->kobj, "%d", old_delta->level);
+	if (err < 0) {
+		PL_ERR(plo, "Failed to restore old delta kobject\n");
+		/*
+		 * Get an extra ref to parent object as kobject_add does, so it
+		 * later kobject_del doesn't destory it
+		 */
+		kobject_get(&plo->kobj);
+	}
 out_close:
 	delta->ops->stop(delta);
 out_destroy:
@@ -3974,17 +3990,12 @@ 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
-		if (err)
+		err = ploop_rename_delta(delta, delta->level, NULL);
+		if (err) {
 			PL_WARN(plo, "rename_deltas: %d %d %d\n", err, level, delta->level);
+			/* Add failed - get an extra ref to parent to match later  _put */
+			kobject_get(&delta->plo->kobj);
+		}
 	}
 }
 
-- 
2.39.1