mir/ovs
2
0
Fork 0
mirror of https://github.com/openvswitch/ovs synced 2025-08-22 01:51:26 +00:00
Code Issues Releases Wiki Activity

ovsdb: Fix race for datum JSON string reference counter.

Browse Source 
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
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
This commit is contained in:
Ilya Maximets 2022-10-10 15:11:57 +02:00
parent ccd26e79e5
commit dc54104526
7 changed files with 84 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

56
lib/ovsdb-data.c
View File

@ -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 *

2
lib/ovsdb-data.h
View File

@ -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 *,

32
ovsdb/file.c
View File

@ -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);
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();

3
ovsdb/file.h
View File

@ -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 *,

5
ovsdb/ovsdb-tool.c
View File

@ -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);

7
ovsdb/ovsdb.c
View File

@ -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);

2
ovsdb/trigger.c
View File

@ -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(