MDEV-8043 innodb tablespace encryption

"use after free" bug, when a thread replaces space->crypt_data
and frees the old crypt_data object while it's being used
by another thread.

storage/innobase/fil/fil0crypt.cc

@@ -216,25 +216,31 @@ fil_space_create_crypt_data(
 }
 
 /******************************************************************
-Compare two crypt objects */
+Merge fil_space_crypt_t object */
 UNIV_INTERN
-int
-fil_space_crypt_compare(
+void
+fil_space_merge_crypt_data(
 /*====================*/
-	const fil_space_crypt_t* crypt_data1,/*!< in: Crypt data */
-	const fil_space_crypt_t* crypt_data2)/*!< in: Crypt data */
+	fil_space_crypt_t* dst,/*!< out: Crypt data */
+	const fil_space_crypt_t* src)/*!< in: Crypt data */
 {
-	ut_a(crypt_data1->type == CRYPT_SCHEME_UNENCRYPTED ||
-	     crypt_data1->type == CRYPT_SCHEME_1);
+	mutex_enter(&dst->mutex);
 
-	ut_a(crypt_data2->type == CRYPT_SCHEME_UNENCRYPTED ||
-	     crypt_data2->type == CRYPT_SCHEME_1);
+	/* validate that they are mergeable */
+	ut_a(src->type == CRYPT_SCHEME_UNENCRYPTED ||
+	     src->type == CRYPT_SCHEME_1);
+
+	ut_a(dst->type == CRYPT_SCHEME_UNENCRYPTED ||
+	     dst->type == CRYPT_SCHEME_1);
 
 	/* no support for changing iv (yet?) */
-	ut_a(memcmp(crypt_data1->iv, crypt_data2->iv,
-		    sizeof(crypt_data1->iv)) == 0);
+	ut_a(memcmp(src->iv, dst->iv, sizeof(src->iv)) == 0);
+
+	dst->type = src->type;
+	dst->min_key_version = src->min_key_version;
+	dst->keyserver_requests += src->keyserver_requests;
 
-	return 0;
+	mutex_exit(&dst->mutex);
 }
 
 /******************************************************************

storage/innobase/fil/fil0fil.cc

@@ -6862,7 +6862,7 @@ fil_space_set_crypt_data(
 	fil_space_crypt_t* crypt_data) /*!< in: crypt data */
 {
 	fil_space_t*	space;
-	fil_space_crypt_t* old_crypt_data = NULL;
+	fil_space_crypt_t* free_crypt_data = NULL;
 
 	ut_ad(fil_system);
 
@@ -6872,24 +6872,24 @@ fil_space_set_crypt_data(
 
 	if (space != NULL) {
 		if (space->crypt_data != NULL) {
-			ut_a(!fil_space_crypt_compare(crypt_data,
-						      space->crypt_data));
-			old_crypt_data = space->crypt_data;
+			fil_space_merge_crypt_data(space->crypt_data,
+						   crypt_data);
+			free_crypt_data = crypt_data;
+		} else {
+			space->crypt_data = crypt_data;
 		}
-
-		space->crypt_data = crypt_data;
 	} else {
 		/* there is a small risk that tablespace has been deleted */
-		old_crypt_data = crypt_data;
+		free_crypt_data = crypt_data;
 	}
 
 	mutex_exit(&fil_system->mutex);
 
-	if (old_crypt_data != NULL) {
-		/* first assign space->crypt_data
-		* then destroy old_crypt_data when no new references to
-		* it can be created.
+	if (free_crypt_data != NULL) {
+		/* there was already crypt data present and the new crypt
+		* data provided as argument to this function has been merged
+		* into that => free new crypt data
 		*/
-		fil_space_destroy_crypt_data(&old_crypt_data);
+		fil_space_destroy_crypt_data(&free_crypt_data);
 	}
 }

storage/innobase/include/fil0crypt.h

@@ -141,13 +141,12 @@ fil_space_set_crypt_data(
 	fil_space_crypt_t* crypt_data); /*!< in: crypt data to set */
 
 /*********************************************************************
-Compare crypt data*/
-UNIV_INTERN
-int
-fil_space_crypt_compare(
+Merge crypt data */
+void
+fil_space_merge_crypt_data(
 /*======================*/
-	const fil_space_crypt_t* crypt_data1,  /*!< in: crypt data */
-	const fil_space_crypt_t* crypt_data2); /*!< in: crypt data */
+	fil_space_crypt_t* dst_crypt_data,  /*!< in: crypt_data */
+	const fil_space_crypt_t* src_crypt_data); /*!< in: crypt data */
 
 /*********************************************************************
 Read crypt data from buffer page */

storage/xtradb/fil/fil0crypt.cc

@@ -216,25 +216,31 @@ fil_space_create_crypt_data(
 }
 
 /******************************************************************
-Compare two crypt objects */
+Merge fil_space_crypt_t object */
 UNIV_INTERN
-int
-fil_space_crypt_compare(
+void
+fil_space_merge_crypt_data(
 /*====================*/
-	const fil_space_crypt_t* crypt_data1,/*!< in: Crypt data */
-	const fil_space_crypt_t* crypt_data2)/*!< in: Crypt data */
+	fil_space_crypt_t* dst,/*!< out: Crypt data */
+	const fil_space_crypt_t* src)/*!< in: Crypt data */
 {
-	ut_a(crypt_data1->type == CRYPT_SCHEME_UNENCRYPTED ||
-	     crypt_data1->type == CRYPT_SCHEME_1);
+	mutex_enter(&dst->mutex);
 
-	ut_a(crypt_data2->type == CRYPT_SCHEME_UNENCRYPTED ||
-	     crypt_data2->type == CRYPT_SCHEME_1);
+	/* validate that they are mergeable */
+	ut_a(src->type == CRYPT_SCHEME_UNENCRYPTED ||
+	     src->type == CRYPT_SCHEME_1);
+
+	ut_a(dst->type == CRYPT_SCHEME_UNENCRYPTED ||
+	     dst->type == CRYPT_SCHEME_1);
 
 	/* no support for changing iv (yet?) */
-	ut_a(memcmp(crypt_data1->iv, crypt_data2->iv,
-		    sizeof(crypt_data1->iv)) == 0);
+	ut_a(memcmp(src->iv, dst->iv, sizeof(src->iv)) == 0);
+
+	dst->type = src->type;
+	dst->min_key_version = src->min_key_version;
+	dst->keyserver_requests += src->keyserver_requests;
 
-	return 0;
+	mutex_exit(&dst->mutex);
 }
 
 /******************************************************************

storage/xtradb/fil/fil0fil.cc

@@ -6989,7 +6989,7 @@ fil_space_set_crypt_data(
 	fil_space_crypt_t* crypt_data) /*!< in: crypt data */
 {
 	fil_space_t*	space;
-	fil_space_crypt_t* old_crypt_data = NULL;
+	fil_space_crypt_t* free_crypt_data = NULL;
 
 	ut_ad(fil_system);
 
@@ -6999,24 +6999,24 @@ fil_space_set_crypt_data(
 
 	if (space != NULL) {
 		if (space->crypt_data != NULL) {
-			ut_a(!fil_space_crypt_compare(crypt_data,
-						      space->crypt_data));
-			old_crypt_data = space->crypt_data;
+			fil_space_merge_crypt_data(space->crypt_data,
+						   crypt_data);
+			free_crypt_data = crypt_data;
+		} else {
+			space->crypt_data = crypt_data;
 		}
-
-		space->crypt_data = crypt_data;
 	} else {
 		/* there is a small risk that tablespace has been deleted */
-		old_crypt_data = crypt_data;
+		free_crypt_data = crypt_data;
 	}
 
 	mutex_exit(&fil_system->mutex);
 
-	if (old_crypt_data != NULL) {
-		/* first assign space->crypt_data
-		* then destroy old_crypt_data when no new references to
-		* it can be created.
+	if (free_crypt_data != NULL) {
+		/* there was already crypt data present and the new crypt
+		* data provided as argument to this function has been merged
+		* into that => free new crypt data
 		*/
-		fil_space_destroy_crypt_data(&old_crypt_data);
+		fil_space_destroy_crypt_data(&free_crypt_data);
 	}
 }

storage/xtradb/include/fil0crypt.h

@@ -141,13 +141,12 @@ fil_space_set_crypt_data(
 	fil_space_crypt_t* crypt_data); /*!< in: crypt data to set */
 
 /*********************************************************************
-Compare crypt data*/
-UNIV_INTERN
-int
-fil_space_crypt_compare(
+Merge crypt data */
+void
+fil_space_merge_crypt_data(
 /*======================*/
-	const fil_space_crypt_t* crypt_data1,  /*!< in: crypt data */
-	const fil_space_crypt_t* crypt_data2); /*!< in: crypt data */
+	fil_space_crypt_t* dst_crypt_data,  /*!< in: crypt_data */
+	const fil_space_crypt_t* src_crypt_data); /*!< in: crypt data */
 
 /*********************************************************************
 Read crypt data from buffer page */