On Tue, Nov 24, 2020 at 4:16 AM Dumitru Ceara <[email protected]> wrote: > > It's possible that the IDL client processes multiple jsonrpc updates > in a single ovsdb_idl_run(). > > Considering the following updates processed in a single IDL run: > 1. Update row R1 from table A while R1 is also referenced by row R2 from > table B: > - this adds R1 to table A's track_list. > 2. Delete row R1 from table A while R1 is also referenced by row R2 from > table B: > - because row R2 still refers to row R1, this will create an orphan > R1. > - at this point R1 is still in table A's hmap.
Hi Dumitru, For my understanding, deleting R1 should result in an update of R2 if the reference is weak, or failure of the transaction if the reference is strong. If it is a weak reference, then the single ovsdb_idl_run() should handle both R1 deletion and the R2 updates which removes the reference to R1, so row R1 should be removed from A's hmap when R2 update is handled. Could you explain how the problem of the dangling pointer happens? > > When the IDL client calls ovsdb_idl_track_clear() after it has finished > processing the tracked changes, row R1 gets freed leaving a dangling > pointer in table A's hmap. > > To fix this we don't free rows in ovsdb_idl_track_clear() if they are > orphan and still referenced by other rows, i.e., the row's 'dst_arcs' > list is not empty. Later, when all arc sources (e.g., R2) are > deleted, the orphan R1 will be cleaned up as well. > > The only exception is when the whole contents of the IDL are flushed, > in ovsdb_idl_db_clear(), in which case it's safe to free all rows. > > Reported-by: Ilya Maximets <[email protected]> > Fixes: 72aeb243a52a ("ovsdb-idl: Tracking - preserve data for deleted rows.") It seems this commit wouldn't add any chance of use-after-free because it made the condition more strict before freeing the row, while not changing the logic of removing it from the hmap. If the use-after-free is a real problem, it should have existed before the above commit. Please correct me if I am wrong. Thanks, Han > Signed-off-by: Dumitru Ceara <[email protected]> > --- > lib/ovsdb-idl.c | 21 ++++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > index b282ce4..cdeb63b 100644 > --- a/lib/ovsdb-idl.c > +++ b/lib/ovsdb-idl.c > @@ -221,7 +221,7 @@ struct ovsdb_idl_db { > struct uuid last_id; > }; > > -static void ovsdb_idl_db_track_clear(struct ovsdb_idl_db *); > +static void ovsdb_idl_db_track_clear(struct ovsdb_idl_db *, bool flush_all); > static void ovsdb_idl_db_add_column(struct ovsdb_idl_db *, > const struct ovsdb_idl_column *); > static void ovsdb_idl_db_omit(struct ovsdb_idl_db *, > @@ -660,7 +660,7 @@ ovsdb_idl_db_clear(struct ovsdb_idl_db *db) > ovsdb_idl_row_destroy_postprocess(db); > > db->cond_seqno = 0; > - ovsdb_idl_db_track_clear(db); > + ovsdb_idl_db_track_clear(db, true); > > if (changed) { > db->change_seqno++; > @@ -1947,7 +1947,7 @@ ovsdb_idl_track_is_updated(const struct ovsdb_idl_row *row, > * loop when it is ready to do ovsdb_idl_run() again. > */ > static void > -ovsdb_idl_db_track_clear(struct ovsdb_idl_db *db) > +ovsdb_idl_db_track_clear(struct ovsdb_idl_db *db, bool flush_all) > { > size_t i; > > @@ -1981,7 +1981,18 @@ ovsdb_idl_db_track_clear(struct ovsdb_idl_db *db) > free(row->tracked_old_datum); > row->tracked_old_datum = NULL; > } > - free(row); > + > + /* Rows that were reused as orphan after being processed > + * for deletion are still in the table hmap and will be > + * cleaned up when their src arcs are removed. > + * > + * The exception is when 'destroy' is explicitly set to > + * 'true' which usually happens when the complete IDL > + * contents are being flushed. > + */ > + if (flush_all || ovs_list_is_empty(&row->dst_arcs)) { > + free(row); > + } > } > } > } > @@ -1996,7 +2007,7 @@ ovsdb_idl_db_track_clear(struct ovsdb_idl_db *db) > void > ovsdb_idl_track_clear(struct ovsdb_idl *idl) > { > - ovsdb_idl_db_track_clear(&idl->data); > + ovsdb_idl_db_track_clear(&idl->data, false); > } > > static void > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
