Compaction thread supposed to not change anything in the database
it is working on, since the same data can be accessed by the main
thread at the same time. However, while converting database rows
to JSON objects, strings in the datum will be cloned using
json_clone(), which is a shallow copy, and that will change the
reference counter for the JSON string object. If both the main
thread and the compaction thread will clone/destroy the same object
at the same time we may end up with a broken reference counter
leading to a memory leak or use-after free.
Adding a new argument to the database to JSON conversion to prevent
use of shallow copies from the compaction thread. This way all
the database operations will be truly read-only avoiding the race.
'ovsdb_atom_to_json' and 'ovsdb_datum_to_json' are more widely used,
so creating separate variant for these functions instead of adding
a new argument, to avoid changing a lot of existing code.
Other solution might be to use atomic reference counters, but that
will require API/ABI break, because counter is exposed in public
headers. Also, we can not easily expose atomic functions, so we'll
need to un-inline reference counting with the associated performance
cost.
Fixes: 3cd2cbd684e0 ("ovsdb: Prepare snapshot JSON in a separate thread.")
Reported-at: https://bugzilla.redhat.com/2133431
Signed-off-by: Ilya Maximets <[email protected]>
---
lib/ovsdb-data.c | 56 ++++++++++++++++++++++++++++++++++++----------
lib/ovsdb-data.h | 2 ++
ovsdb/file.c | 34 ++++++++++++++++++++++------
ovsdb/file.h | 3 ++-
ovsdb/ovsdb-tool.c | 5 +++--
ovsdb/ovsdb.c | 7 ++++--
ovsdb/trigger.c | 2 +-
7 files changed, 84 insertions(+), 25 deletions(-)
diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c
index 183e75258..f18f74298 100644
--- a/lib/ovsdb-data.c
+++ b/lib/ovsdb-data.c
@@ -455,9 +455,15 @@ ovsdb_atom_from_json(union ovsdb_atom *atom,
/* Converts 'atom', of the specified 'type', to JSON format, and returns the
* JSON. The caller is responsible for freeing the returned JSON.
*
+ * If 'allow_shallow_copies' is false, deep copy of the string JSON object
+ * will be used. Useful when the same string object is accessed by multiple
+ * threads as deep copy will not change the reference counter of the original
+ * JSON string.
+ *
* Refer to RFC 7047 for the format of the JSON that this function produces. */
-struct json *
-ovsdb_atom_to_json(const union ovsdb_atom *atom, enum ovsdb_atomic_type type)
+static struct json *
+ovsdb_atom_to_json__(const union ovsdb_atom *atom, enum ovsdb_atomic_type type,
+ bool allow_shallow_copies)
{
switch (type) {
case OVSDB_TYPE_VOID:
@@ -473,7 +479,8 @@ ovsdb_atom_to_json(const union ovsdb_atom *atom, enum
ovsdb_atomic_type type)
return json_boolean_create(atom->boolean);
case OVSDB_TYPE_STRING:
- return json_clone(atom->s);
+ return allow_shallow_copies ? json_clone(atom->s)
+ : json_deep_clone(atom->s);
case OVSDB_TYPE_UUID:
return wrap_json("uuid", json_string_create_nocopy(
@@ -485,6 +492,19 @@ ovsdb_atom_to_json(const union ovsdb_atom *atom, enum
ovsdb_atomic_type type)
}
}
+struct json *
+ovsdb_atom_to_json(const union ovsdb_atom *atom, enum ovsdb_atomic_type type)
+{
+ return ovsdb_atom_to_json__(atom, type, true);
+}
+
+static struct json *
+ovsdb_atom_to_json_deep(const union ovsdb_atom *atom,
+ enum ovsdb_atomic_type type)
+{
+ return ovsdb_atom_to_json__(atom, type, false);
+}
+
static char *
ovsdb_atom_from_string__(union ovsdb_atom *atom,
union ovsdb_atom **range_end_atom,
@@ -1409,12 +1429,15 @@ ovsdb_unconstrained_datum_from_json(struct ovsdb_datum
*datum,
static struct json *
ovsdb_base_to_json(const union ovsdb_atom *atom,
const struct ovsdb_base_type *base,
- bool use_row_names)
+ bool use_row_names,
+ bool allow_shallow_copies)
{
if (!use_row_names
|| base->type != OVSDB_TYPE_UUID
|| !base->uuid.refTableName) {
- return ovsdb_atom_to_json(atom, base->type);
+ return allow_shallow_copies
+ ? ovsdb_atom_to_json(atom, base->type)
+ : ovsdb_atom_to_json_deep(atom, base->type);
} else {
return json_array_create_2(
json_string_create("named-uuid"),
@@ -1425,7 +1448,8 @@ ovsdb_base_to_json(const union ovsdb_atom *atom,
static struct json *
ovsdb_datum_to_json__(const struct ovsdb_datum *datum,
const struct ovsdb_type *type,
- bool use_row_names)
+ bool use_row_names,
+ bool allow_shallow_copies)
{
if (ovsdb_type_is_map(type)) {
struct json **elems;
@@ -1435,14 +1459,15 @@ ovsdb_datum_to_json__(const struct ovsdb_datum *datum,
for (i = 0; i < datum->n; i++) {
elems[i] = json_array_create_2(
ovsdb_base_to_json(&datum->keys[i], &type->key,
- use_row_names),
+ use_row_names, allow_shallow_copies),
ovsdb_base_to_json(&datum->values[i], &type->value,
- use_row_names));
+ use_row_names, allow_shallow_copies));
}
return wrap_json("map", json_array_create(elems, datum->n));
} else if (datum->n == 1) {
- return ovsdb_base_to_json(&datum->keys[0], &type->key, use_row_names);
+ return ovsdb_base_to_json(&datum->keys[0], &type->key,
+ use_row_names, allow_shallow_copies);
} else {
struct json **elems;
size_t i;
@@ -1450,7 +1475,7 @@ ovsdb_datum_to_json__(const struct ovsdb_datum *datum,
elems = xmalloc(datum->n * sizeof *elems);
for (i = 0; i < datum->n; i++) {
elems[i] = ovsdb_base_to_json(&datum->keys[i], &type->key,
- use_row_names);
+ use_row_names, allow_shallow_copies);
}
return wrap_json("set", json_array_create(elems, datum->n));
@@ -1467,14 +1492,21 @@ struct json *
ovsdb_datum_to_json(const struct ovsdb_datum *datum,
const struct ovsdb_type *type)
{
- return ovsdb_datum_to_json__(datum, type, false);
+ return ovsdb_datum_to_json__(datum, type, false, true);
+}
+
+struct json *
+ovsdb_datum_to_json_deep(const struct ovsdb_datum *datum,
+ const struct ovsdb_type *type)
+{
+ return ovsdb_datum_to_json__(datum, type, false, false);
}
struct json *
ovsdb_datum_to_json_with_row_names(const struct ovsdb_datum *datum,
const struct ovsdb_type *type)
{
- return ovsdb_datum_to_json__(datum, type, true);
+ return ovsdb_datum_to_json__(datum, type, true, true);
}
static const char *
diff --git a/lib/ovsdb-data.h b/lib/ovsdb-data.h
index dcb620513..f048a8cb0 100644
--- a/lib/ovsdb-data.h
+++ b/lib/ovsdb-data.h
@@ -195,6 +195,8 @@ ovsdb_unconstrained_datum_from_json(struct ovsdb_datum *,
OVS_WARN_UNUSED_RESULT;
struct json *ovsdb_datum_to_json(const struct ovsdb_datum *,
const struct ovsdb_type *);
+struct json *ovsdb_datum_to_json_deep(const struct ovsdb_datum *,
+ const struct ovsdb_type *);
char *ovsdb_datum_from_string(struct ovsdb_datum *,
const struct ovsdb_type *, const char *,
diff --git a/ovsdb/file.c b/ovsdb/file.c
index ca80c2823..fdc289ad1 100644
--- a/ovsdb/file.c
+++ b/ovsdb/file.c
@@ -52,7 +52,8 @@ static void ovsdb_file_txn_init(struct ovsdb_file_txn *);
static void ovsdb_file_txn_add_row(struct ovsdb_file_txn *,
const struct ovsdb_row *old,
const struct ovsdb_row *new,
- const unsigned long int *changed);
+ const unsigned long int *changed,
+ bool allow_shallow_copies);
/* If set to 'true', file transactions will contain difference between
* datums of old and new rows and not the whole new datum for the column. */
@@ -361,12 +362,19 @@ ovsdb_file_change_cb(const struct ovsdb_row *old,
void *ftxn_)
{
struct ovsdb_file_txn *ftxn = ftxn_;
- ovsdb_file_txn_add_row(ftxn, old, new, changed);
+ ovsdb_file_txn_add_row(ftxn, old, new, changed, true);
return true;
}
+/* Converts the database into transaction JSON representation.
+ * If 'allow_shallow_copies' is false, makes sure that all the JSON
+ * objects in the resulted transaction JSON are separately allocated
+ * objects and not shallow clones of JSON objects already existing
+ * in the database. Useful when multiple threads are working on the
+ * same database object. */
struct json *
-ovsdb_to_txn_json(const struct ovsdb *db, const char *comment)
+ovsdb_to_txn_json(const struct ovsdb *db, const char *comment,
+ bool allow_shallow_copies)
{
struct ovsdb_file_txn ftxn;
@@ -378,7 +386,8 @@ ovsdb_to_txn_json(const struct ovsdb *db, const char
*comment)
const struct ovsdb_row *row;
HMAP_FOR_EACH (row, hmap_node, &table->rows) {
- ovsdb_file_txn_add_row(&ftxn, NULL, row, NULL);
+ ovsdb_file_txn_add_row(&ftxn, NULL, row, NULL,
+ allow_shallow_copies);
}
}
@@ -426,7 +435,8 @@ static void
ovsdb_file_txn_add_row(struct ovsdb_file_txn *ftxn,
const struct ovsdb_row *old,
const struct ovsdb_row *new,
- const unsigned long int *changed)
+ const unsigned long int *changed,
+ bool allow_shallow_copies)
{
struct json *row;
@@ -451,10 +461,20 @@ ovsdb_file_txn_add_row(struct ovsdb_file_txn *ftxn,
if (old && use_column_diff) {
ovsdb_datum_diff(&datum, &old->fields[idx],
&new->fields[idx], type);
- column_json = ovsdb_datum_to_json(&datum, type);
+ if (allow_shallow_copies) {
+ column_json = ovsdb_datum_to_json(&datum, type);
+ } else {
+ column_json = ovsdb_datum_to_json_deep(&datum, type);
+ }
ovsdb_datum_destroy(&datum, type);
} else {
- column_json = ovsdb_datum_to_json(&new->fields[idx], type);
+ if (allow_shallow_copies) {
+ column_json = ovsdb_datum_to_json(
+ &new->fields[idx], type);
+ } else {
+ column_json = ovsdb_datum_to_json_deep(
+ &new->fields[idx], type);
+ }
}
if (!row) {
row = json_object_create();
diff --git a/ovsdb/file.h b/ovsdb/file.h
index be4f6ad27..ae90d4fe1 100644
--- a/ovsdb/file.h
+++ b/ovsdb/file.h
@@ -25,7 +25,8 @@ struct ovsdb_txn;
void ovsdb_file_column_diff_disable(void);
-struct json *ovsdb_to_txn_json(const struct ovsdb *, const char *comment);
+struct json *ovsdb_to_txn_json(const struct ovsdb *, const char *comment,
+ bool allow_shallow_copies);
struct json *ovsdb_file_txn_to_json(const struct ovsdb_txn *);
struct json *ovsdb_file_txn_annotate(struct json *, const char *comment);
struct ovsdb_error *ovsdb_file_txn_from_json(struct ovsdb *,
diff --git a/ovsdb/ovsdb-tool.c b/ovsdb/ovsdb-tool.c
index df2e373c3..60f353197 100644
--- a/ovsdb/ovsdb-tool.c
+++ b/ovsdb/ovsdb-tool.c
@@ -304,7 +304,7 @@ do_create_cluster(struct ovs_cmdl_context *ctx)
struct ovsdb *ovsdb = ovsdb_file_read(src_file_name, false);
char *comment = xasprintf("created from %s", src_file_name);
- data = ovsdb_to_txn_json(ovsdb, comment);
+ data = ovsdb_to_txn_json(ovsdb, comment, true);
free(comment);
schema = ovsdb_schema_clone(ovsdb->schema);
ovsdb_destroy(ovsdb);
@@ -359,7 +359,8 @@ write_standalone_db(const char *file_name, const char
*comment,
error = ovsdb_log_write_and_free(log, ovsdb_schema_to_json(db->schema));
if (!error) {
- error = ovsdb_log_write_and_free(log, ovsdb_to_txn_json(db, comment));
+ error = ovsdb_log_write_and_free(log,
+ ovsdb_to_txn_json(db, comment, true));
}
ovsdb_log_close(log);
diff --git a/ovsdb/ovsdb.c b/ovsdb/ovsdb.c
index 8cbefbe3d..1c011fab0 100644
--- a/ovsdb/ovsdb.c
+++ b/ovsdb/ovsdb.c
@@ -585,7 +585,9 @@ compaction_thread(void *aux)
struct json *data;
VLOG_DBG("%s: Compaction thread started.", state->db->name);
- data = ovsdb_to_txn_json(state->db, "compacting database online");
+ data = ovsdb_to_txn_json(state->db, "compacting database online",
+ /* Do not allow shallow copies to avoid races. */
+ false);
state->data = json_serialized_object_create(data);
json_destroy(data);
@@ -633,7 +635,8 @@ ovsdb_snapshot(struct ovsdb *db, bool trim_memory
OVS_UNUSED)
if (!applied_index) {
/* Parallel compaction is not supported for standalone databases. */
state = xzalloc(sizeof *state);
- state->data = ovsdb_to_txn_json(db, "compacting database online");
+ state->data = ovsdb_to_txn_json(db,
+ "compacting database online", true);
state->schema = ovsdb_schema_to_json(db->schema);
} else if (ovsdb_snapshot_ready(db)) {
xpthread_join(db->snap_state->thread, NULL);
diff --git a/ovsdb/trigger.c b/ovsdb/trigger.c
index 7d3003bca..01bb80e28 100644
--- a/ovsdb/trigger.c
+++ b/ovsdb/trigger.c
@@ -282,7 +282,7 @@ ovsdb_trigger_try(struct ovsdb_trigger *t, long long int
now)
/* Make the new copy into a transaction log record. */
struct json *txn_json = ovsdb_to_txn_json(
- newdb, "converted by ovsdb-server");
+ newdb, "converted by ovsdb-server", true);
/* Propose the change. */
t->progress = ovsdb_txn_propose_schema_change(
--
2.37.3
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
