ufs: replace module globals with a caller-owned context

The provisioning loader kept its parsed state in module globals, which
made it impossible to re-run or unit test, and its error paths left
the globals dangling - a caller that continued after a failed load
acted on freed or stale data, and several body strings were leaked
outright. A context owned by the caller removes the shared state, ties
the provisioning data's lifetime to the flash flow that uses it, and
gives every error path a single cleanup to go through.

Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>

include/qdl.h

@@ -183,8 +183,10 @@ struct qud_device_desc {
 
 struct qud_device_desc *qud_list(unsigned int *devices_found);
 
+struct ufs_provisioning;
+
 int firehose_run(struct qdl_device *qdl, struct list_head *ops);
-int firehose_provision(struct qdl_device *qdl, bool skip_reset);
+int firehose_provision(struct qdl_device *qdl, struct ufs_provisioning *ufs, bool skip_reset);
 int firehose_read_buf(struct qdl_device *qdl, struct firehose_op *read_op, void *out_buf, size_t out_size);
 
 /* Block-level entry points used by the nbdkit plugin */

src/firehose.c

@@ -1662,7 +1662,7 @@ static int firehose_detect_and_configure(struct qdl_device *qdl,
 	return 0;
 }
 
-int firehose_provision(struct qdl_device *qdl, bool skip_reset)
+int firehose_provision(struct qdl_device *qdl, struct ufs_provisioning *ufs, bool skip_reset)
 {
 	int ret;
 
@@ -1670,7 +1670,7 @@ int firehose_provision(struct qdl_device *qdl, bool skip_reset)
 	if (ret)
 		return ret;
 
-	ret = ufs_provisioning_execute(qdl, firehose_apply_ufs_common,
+	ret = ufs_provisioning_execute(ufs, qdl, firehose_apply_ufs_common,
 				       firehose_apply_ufs_body,
 				       firehose_apply_ufs_epilogue);
 	if (!ret)

src/qdl.c

@@ -1226,6 +1226,7 @@ static int qdl_flash(int argc, char **argv)
 	enum qdl_storage_type storage_type = QDL_STORAGE_UFS;
 	struct sahara_image sahara_images[MAPPING_SZ] = {};
 	struct list_head firehose_ops = LIST_INIT(firehose_ops);
+	struct ufs_provisioning ufs;
 	char *incdir = NULL;
 	char *serial = NULL;
 	const char *vip_generate_dir = NULL;
@@ -1266,6 +1267,8 @@ static int qdl_flash(int argc, char **argv)
 		{0, 0, 0, 0}
 	};
 
+	ufs_provisioning_init(&ufs);
+
 	while ((opt = getopt_long(argc, argv, "dvi:lu:S:D:s:fcnt:T:Rh", options, NULL)) != -1) {
 		switch (opt) {
 		case 'd':
@@ -1435,7 +1438,7 @@ static int qdl_flash(int argc, char **argv)
 			if (storage_type != QDL_STORAGE_UFS)
 				errx(1, "attempting to load provisioning config when storage isn't \"ufs\"");
 
-			ret = ufs_load(argv[optind], qdl_finalize_provisioning);
+			ret = ufs_load(&ufs, argv[optind], qdl_finalize_provisioning);
 			if (ret < 0)
 				errx(1, "ufs_load %s failed", argv[optind]);
 			break;
@@ -1521,8 +1524,8 @@ static int qdl_flash(int argc, char **argv)
 	if (ret < 0)
 		goto out_cleanup;
 
-	if (ufs_need_provisioning())
-		ret = firehose_provision(qdl, skip_reset);
+	if (ufs_need_provisioning(&ufs))
+		ret = firehose_provision(qdl, &ufs, skip_reset);
 	else
 		ret = firehose_run(qdl, &firehose_ops);
 	if (ret < 0)
@@ -1542,6 +1545,8 @@ out_cleanup:
 
 	firehose_free_ops(&firehose_ops);
 
+	ufs_provisioning_cleanup(&ufs);
+
 	if (qdl) {
 		if (qdl->vip_data.state != VIP_DISABLED)
 			vip_transfer_deinit(qdl);

src/ufs.c

@@ -17,10 +17,6 @@
 #include "list.h"
 #include "patch.h"
 
-struct ufs_common *ufs_common_p;
-struct ufs_epilogue *ufs_epilogue_p;
-static struct list_head ufs_bodies = LIST_INIT(ufs_bodies);
-
 static const char notice_bconfigdescrlock[] = "\n"
 "Please pay attention that UFS provisioning is irreversible (OTP) operation unless parameter bConfigDescrLock = 0.\n"
 "In order to prevent unintentional device locking the tool has the following safety:\n\n"
@@ -30,12 +26,37 @@ static const char notice_bconfigdescrlock[] = "\n"
 "	and don't use command line parameter --finalize-provisioning.\n\n"
 "In case of mismatch between CL and XML provisioning is not performed.\n\n";
 
-bool ufs_need_provisioning(void)
+void ufs_provisioning_init(struct ufs_provisioning *ufs)
 {
-	return !!ufs_epilogue_p;
+	ufs->common = NULL;
+	ufs->epilogue = NULL;
+	list_init(&ufs->bodies);
 }
 
-struct ufs_common *ufs_parse_common_params(xmlNode *node, bool finalize_provisioning __unused)
+void ufs_provisioning_cleanup(struct ufs_provisioning *ufs)
+{
+	struct ufs_body *body;
+	struct ufs_body *tmp;
+
+	free(ufs->common);
+	ufs->common = NULL;
+
+	list_for_each_entry_safe(body, tmp, &ufs->bodies, node) {
+		list_del(&body->node);
+		free((void *)body->desc);
+		free(body);
+	}
+
+	free(ufs->epilogue);
+	ufs->epilogue = NULL;
+}
+
+bool ufs_need_provisioning(const struct ufs_provisioning *ufs)
+{
+	return !!ufs->epilogue;
+}
+
+static struct ufs_common *ufs_parse_common_params(xmlNode *node, bool finalize_provisioning __unused)
 {
 	struct ufs_common *result;
 	int errors;
@@ -71,7 +92,7 @@ struct ufs_common *ufs_parse_common_params(xmlNode *node, bool finalize_provisio
 	return result;
 }
 
-struct ufs_body *ufs_parse_body(xmlNode *node)
+static struct ufs_body *ufs_parse_body(xmlNode *node)
 {
 	struct ufs_body *result;
 	int errors;
@@ -99,7 +120,7 @@ struct ufs_body *ufs_parse_body(xmlNode *node)
 	return result;
 }
 
-struct ufs_epilogue *ufs_parse_epilogue(xmlNode *node)
+static struct ufs_epilogue *ufs_parse_epilogue(xmlNode *node)
 {
 	struct ufs_epilogue *result;
 	int errors = 0;
@@ -116,16 +137,15 @@ struct ufs_epilogue *ufs_parse_epilogue(xmlNode *node)
 	return result;
 }
 
-int ufs_load(const char *ufs_file, bool finalize_provisioning)
+int ufs_load(struct ufs_provisioning *ufs, const char *ufs_file, bool finalize_provisioning)
 {
 	xmlNode *node;
 	xmlNode *root;
 	xmlDoc *doc;
 	int retval = 0;
 	struct ufs_body *ufs_body_tmp;
-	struct ufs_body *ufs_body;
 
-	if (ufs_common_p) {
+	if (ufs->common) {
 		ux_err("Only one UFS provisioning XML allowed, \"%s\" ignored\n",
 		       ufs_file);
 		return -EEXIST;
@@ -150,9 +170,9 @@ int ufs_load(const char *ufs_file, bool finalize_provisioning)
 		}
 
 		if (xmlGetProp(node, (xmlChar *)"bNumberLU")) {
-			if (!ufs_common_p) {
-				ufs_common_p = ufs_parse_common_params(node,
-								       finalize_provisioning);
+			if (!ufs->common) {
+				ufs->common = ufs_parse_common_params(node,
+								      finalize_provisioning);
 			} else {
 				ux_err("multiple UFS common tags found in \"%s\"\n",
 				       ufs_file);
@@ -160,7 +180,7 @@ int ufs_load(const char *ufs_file, bool finalize_provisioning)
 				break;
 			}
 
-			if (!ufs_common_p) {
+			if (!ufs->common) {
 				ux_err("invalid UFS common tag found in \"%s\"\n",
 				       ufs_file);
 				retval = -EINVAL;
@@ -169,7 +189,7 @@ int ufs_load(const char *ufs_file, bool finalize_provisioning)
 		} else if (xmlGetProp(node, (xmlChar *)"LUNum")) {
 			ufs_body_tmp = ufs_parse_body(node);
 			if (ufs_body_tmp) {
-				list_append(&ufs_bodies, &ufs_body_tmp->node);
+				list_append(&ufs->bodies, &ufs_body_tmp->node);
 			} else {
 				ux_err("invalid UFS body tag found in \"%s\"\n",
 				       ufs_file);
@@ -177,9 +197,9 @@ int ufs_load(const char *ufs_file, bool finalize_provisioning)
 				break;
 			}
 		} else if (xmlGetProp(node, (xmlChar *)"commit")) {
-			if (!ufs_epilogue_p) {
-				ufs_epilogue_p = ufs_parse_epilogue(node);
-				if (ufs_epilogue_p)
+			if (!ufs->epilogue) {
+				ufs->epilogue = ufs_parse_epilogue(node);
+				if (ufs->epilogue)
 					continue;
 			} else {
 				ux_err("multiple UFS finalizing tags found in \"%s\"\n",
@@ -188,7 +208,7 @@ int ufs_load(const char *ufs_file, bool finalize_provisioning)
 				break;
 			}
 
-			if (!ufs_epilogue_p) {
+			if (!ufs->epilogue) {
 				ux_err("invalid UFS finalizing tag found in \"%s\"\n",
 				       ufs_file);
 				retval = -EINVAL;
@@ -204,33 +224,25 @@ int ufs_load(const char *ufs_file, bool finalize_provisioning)
 
 	xmlFreeDoc(doc);
 
-	if (!retval && (!ufs_common_p || list_empty(&ufs_bodies) || !ufs_epilogue_p)) {
+	if (!retval && (!ufs->common || list_empty(&ufs->bodies) || !ufs->epilogue)) {
 		ux_err("incomplete UFS provisioning information in \"%s\"\n", ufs_file);
 		retval = -EINVAL;
 	}
 
-	if (retval) {
-		if (ufs_common_p) {
-			free(ufs_common_p);
-		}
-		list_for_each_entry_safe(ufs_body, ufs_body_tmp, &ufs_bodies, node) {
-			free(ufs_body);
-		}
-		if (ufs_epilogue_p) {
-			free(ufs_epilogue_p);
-		}
-		return retval;
-	}
-	if (!finalize_provisioning != !ufs_common_p->bConfigDescrLock) {
+	if (!retval && !finalize_provisioning != !ufs->common->bConfigDescrLock) {
 		ux_err("UFS provisioning value bConfigDescrLock %d in file \"%s\" don't match command line parameter --finalize-provisioning %d\n",
-		       ufs_common_p->bConfigDescrLock, ufs_file, finalize_provisioning);
+		       ufs->common->bConfigDescrLock, ufs_file, finalize_provisioning);
 		ux_err(notice_bconfigdescrlock);
-		return -EINVAL;
+		retval = -EINVAL;
 	}
-	return 0;
+
+	if (retval)
+		ufs_provisioning_cleanup(ufs);
+
+	return retval;
 }
 
-int ufs_provisioning_execute(struct qdl_device *qdl,
+int ufs_provisioning_execute(struct ufs_provisioning *ufs, struct qdl_device *qdl,
 			     int (*apply_ufs_common)(struct qdl_device *, struct ufs_common*),
 	int (*apply_ufs_body)(struct qdl_device *, struct ufs_body*),
 	int (*apply_ufs_epilogue)(struct qdl_device *, struct ufs_epilogue*, bool))
@@ -238,7 +250,7 @@ int ufs_provisioning_execute(struct qdl_device *qdl,
 	int ret;
 	struct ufs_body *body;
 
-	if (ufs_common_p->bConfigDescrLock) {
+	if (ufs->common->bConfigDescrLock) {
 		int i;
 
 		ux_info("WARNING: irreversible provisioning will start in 5s");
@@ -251,28 +263,28 @@ int ufs_provisioning_execute(struct qdl_device *qdl,
 	}
 
 	// Just ask a target to check the XML w/o real provisioning
-	ret = apply_ufs_common(qdl, ufs_common_p);
+	ret = apply_ufs_common(qdl, ufs->common);
 	if (ret)
 		return ret;
-	list_for_each_entry(body, &ufs_bodies, node) {
+	list_for_each_entry(body, &ufs->bodies, node) {
 		ret = apply_ufs_body(qdl, body);
 		if (ret)
 			return ret;
 	}
-	ret = apply_ufs_epilogue(qdl, ufs_epilogue_p, false);
+	ret = apply_ufs_epilogue(qdl, ufs->epilogue, false);
 	if (ret) {
 		ux_err("UFS provisioning impossible, provisioning XML may be corrupted\n");
 		return ret;
 	}
 
 	// Real provisioning -- target didn't refuse a given XML
-	ret = apply_ufs_common(qdl, ufs_common_p);
+	ret = apply_ufs_common(qdl, ufs->common);
 	if (ret)
 		return ret;
-	list_for_each_entry(body, &ufs_bodies, node) {
+	list_for_each_entry(body, &ufs->bodies, node) {
 		ret = apply_ufs_body(qdl, body);
 		if (ret)
 			return ret;
 	}
-	return apply_ufs_epilogue(qdl, ufs_epilogue_p, true);
+	return apply_ufs_epilogue(qdl, ufs->epilogue, true);
 }

src/ufs.h

@@ -47,11 +47,26 @@ struct ufs_epilogue {
 	bool		commit;
 };
 
-int ufs_load(const char *ufs_file, bool finalize_provisioning);
-int ufs_provisioning_execute(struct qdl_device *qdl,
+/*
+ * Parsed UFS provisioning description. Owned by the caller (instead of the
+ * former module globals) so it can be loaded, inspected and freed without
+ * shared state - which also makes the loader independently testable.
+ * Initialise with ufs_provisioning_init() and release with
+ * ufs_provisioning_cleanup().
+ */
+struct ufs_provisioning {
+	struct ufs_common *common;
+	struct ufs_epilogue *epilogue;
+	struct list_head bodies;
+};
+
+void ufs_provisioning_init(struct ufs_provisioning *ufs);
+void ufs_provisioning_cleanup(struct ufs_provisioning *ufs);
+int ufs_load(struct ufs_provisioning *ufs, const char *ufs_file, bool finalize_provisioning);
+int ufs_provisioning_execute(struct ufs_provisioning *ufs, struct qdl_device *qdl,
 			     int (*apply_ufs_common)(struct qdl_device *qdl, struct ufs_common *ufs),
 			     int (*apply_ufs_body)(struct qdl_device *qdl, struct ufs_body *ufs),
 			     int (*apply_ufs_epilogue)(struct qdl_device *qdl, struct ufs_epilogue *ufs, bool commit));
-bool ufs_need_provisioning(void);
+bool ufs_need_provisioning(const struct ufs_provisioning *ufs);
 
 #endif