From patchwork Tue Feb 12 01:56:04 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Han Zhou X-Patchwork-Id: 1040322 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="SktrpiL/"; dkim-atps=neutral Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 43z5RJ0fSxz9s3l for ; Tue, 12 Feb 2019 12:58:36 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 816FADAA9; Tue, 12 Feb 2019 01:58:13 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 5A2C0D4EA for ; Tue, 12 Feb 2019 01:55:57 +0000 (UTC) X-Greylist: whitelisted by SQLgrey-1.7.6 Received: from mail-pl1-f194.google.com (mail-pl1-f194.google.com [209.85.214.194]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 3221B27B for ; Tue, 12 Feb 2019 01:55:56 +0000 (UTC) Received: by mail-pl1-f194.google.com with SMTP id g9so483045plo.3 for ; Mon, 11 Feb 2019 17:55:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id; bh=N+XzoU60INDwi8dHYS96j/GhN9Zhzd73nUyD48xY+/8=; b=SktrpiL/P55gSAgRmhOyJ4OQ/xsqIL9nhnXuVEuP3sHmRdFR6CLLUVE9deZicCpgEs KbeL8Zuzd3SneAFp8JSfWw5aqycELdtlCsUnkLBmepeAelahgoOJDlUQ+wyIwnURkZGn b2hJy/P0CENOFORGfz4vkECb+t0axDw+4r9KrWP0QLP0ihM9mZKud7fiyi0BxdltTUBX E6x9Zt82+1550doXmGQ2UQMxGU2YOgjcEjVVDZbSlgrtz9LmQPH9OxGIEWUEtbS3UUCb oeko6RkcOytqP63HYgaAWxzoiceP4sKuIkDuJEeVR1T03Pfdgny/UaSG/mTEHkvao3jf 8aPg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=N+XzoU60INDwi8dHYS96j/GhN9Zhzd73nUyD48xY+/8=; b=jPepMxqDgbG0xz5H9JwJjmreghzk/KNsZF1tPOEfKl+Cwv5+89Qc2zM0ZWAMAIhl5V JSWixM30SkKKC1maSdNaormzqtK8NkXtYapkSW8jDduqY5comhwRe9FI41nQNAhu8bvx f2qG+bVMhYzzT9NJbXQSuPsqYZtD9GJkd9DCf7GoKdrWmQzQdh5PcBZXb7IDcfgmW112 mvospgFHoL/fXIu7L1+cNnnEu8uV29NMGck0N/GiOEQ0GaCUTDR9Edqyw1T2zaHK/fZ3 wW3jEOEhlcWG/gjuw/CujX9z8yrLTznJeV7tnw904aLx3ZV8gEQI2cNhqp6Fyr9MPogc A9+A== X-Gm-Message-State: AHQUAub16E13Y0e1AcCjbHWyVCNudo/hFod8G1lrbht2VadGK1D/P/T0 g08thggAM92ttdt/TAyFFxEtk9Xh X-Google-Smtp-Source: AHgI3Iapwf+U3UJUSeWeHnWOSxaWjCB9absV7Vs+z6KNeoflqDa2bjS5u2mJSEYl7ReJ9nKozMP9Ug== X-Received: by 2002:a17:902:2468:: with SMTP id m37mr1438991plg.314.1549936555349; Mon, 11 Feb 2019 17:55:55 -0800 (PST) Received: from localhost.localdomain.localdomain ([216.113.160.77]) by smtp.gmail.com with ESMTPSA id w11sm14795397pgk.16.2019.02.11.17.55.54 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 11 Feb 2019 17:55:54 -0800 (PST) From: Han Zhou X-Google-Original-From: Han Zhou To: dev@openvswitch.org Date: Mon, 11 Feb 2019 17:56:04 -0800 Message-Id: <1549936564-42388-1-git-send-email-hzhou8@ebay.com> X-Mailer: git-send-email 2.1.0 X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, WEIRD_QUOTING autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Subject: [ovs-dev] [PATCH] monitor.c: Fix crash when monitor condition adds new columns. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org From: Han Zhou The OVSDB conditional monitor implementation allows many clients to share same copy of monitored data if the clients are sharing same tables and columns being monitored, while they can have different monitor conditions. In monitor conditions they can have different columns which can be different from the columns being monitored. So the struct ovsdb_monitor_table maintains the union of the all the columns being used in any conditions. The problem of the current implementation is that for each change set generated, it doesn't maintain any metadata for the number of columns for the data that has already populated in it. Instead, it always rely on the n_columns field of the struct ovsdb_monitor_table to manipulate the data. However, the n_columns in struct ovsdb_monitor_table can increase (e.g. when a client changes its condition which involves more columns). So it can result in that the existing rows in a change set with N columns being later processed as if it had more than N columns, typically, when the row is freed. This causes the ovsdb-server crashing (see an example of the backtrace). The patch fixes the problem by maintaining n_columns for each change set, and added a test case which fails without the fix. (gdb) bt at lib/ovsdb-data.c:1031 out>, mt=) at ovsdb/monitor.c:320 mt=0x1e7b940) at ovsdb/monitor.c:333 out>, transaction=) at ovsdb/monitor.c:527 initial=, cond_updated=cond_updated@entry=false, unflushed_=unflushed_@entry=0x20dae70, condition=, version=) at ovsdb/monitor.c:1156 (m=m@entry=0x20dae40, initial=initial@entry=false) at ovsdb/jsonrpc-server.c:1655 at ovsdb/jsonrpc-server.c:1729 ovsdb/jsonrpc-server.c:551 ovsdb/jsonrpc-server.c:586 ovsdb/jsonrpc-server.c:401 exiting=0x7ffdb947f76f, run_process=0x0, remotes=0x7ffdb947f7c0, unixctl=0x1e7a560, all_dbs=0x7ffdb947f800, jsonrpc=, config=0x7ffdb947f820) at ovsdb/ovsdb-server.c:209 Signed-off-by: Han Zhou --- ovsdb/monitor.c | 88 ++++++++++++++++++++++++++++++++------------------ tests/ovsdb-monitor.at | 68 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 125 insertions(+), 31 deletions(-) diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c index dd06e26..8909ae1 100644 --- a/ovsdb/monitor.c +++ b/ovsdb/monitor.c @@ -120,6 +120,12 @@ struct ovsdb_monitor_changes { struct hmap rows; int n_refs; uint64_t transaction; + + /* Save the mt->n_columns that is used when creating the changes. + * It can be different from the current mt->n_columns because + * mt->n_columns can be increased when there are condition changes + * from any of the clients sharing the dbmon. */ + size_t n_columns; }; /* A particular table being monitored. */ @@ -156,7 +162,8 @@ typedef struct json * const struct ovsdb_monitor_session_condition * condition, enum ovsdb_monitor_row_type row_type, const void *, - bool initial, unsigned long int *changed); + bool initial, unsigned long int *changed, + size_t n_columns); static void ovsdb_monitor_destroy(struct ovsdb_monitor *dbmon); static struct ovsdb_monitor_changes * ovsdb_monitor_table_add_changes( @@ -255,14 +262,15 @@ ovsdb_monitor_changes_row_find(const struct ovsdb_monitor_changes *changes, return NULL; } -/* Allocates an array of 'mt->n_columns' ovsdb_datums and initializes them as +/* Allocates an array of 'n_columns' ovsdb_datums and initializes them as * copies of the data in 'row' drawn from the columns represented by * mt->columns[]. Returns the array. * * If 'row' is NULL, returns NULL. */ static struct ovsdb_datum * clone_monitor_row_data(const struct ovsdb_monitor_table *mt, - const struct ovsdb_row *row) + const struct ovsdb_row *row, + size_t n_columns) { struct ovsdb_datum *data; size_t i; @@ -271,8 +279,8 @@ clone_monitor_row_data(const struct ovsdb_monitor_table *mt, return NULL; } - data = xmalloc(mt->n_columns * sizeof *data); - for (i = 0; i < mt->n_columns; i++) { + data = xmalloc(n_columns * sizeof *data); + for (i = 0; i < n_columns; i++) { const struct ovsdb_column *c = mt->columns[i].column; const struct ovsdb_datum *src = &row->fields[c->index]; struct ovsdb_datum *dst = &data[i]; @@ -283,16 +291,17 @@ clone_monitor_row_data(const struct ovsdb_monitor_table *mt, return data; } -/* Replaces the mt->n_columns ovsdb_datums in row[] by copies of the data from +/* Replaces the n_columns ovsdb_datums in row[] by copies of the data from * in 'row' drawn from the columns represented by mt->columns[]. */ static void update_monitor_row_data(const struct ovsdb_monitor_table *mt, const struct ovsdb_row *row, - struct ovsdb_datum *data) + struct ovsdb_datum *data, + size_t n_columns) { size_t i; - for (i = 0; i < mt->n_columns; i++) { + for (i = 0; i < n_columns; i++) { const struct ovsdb_column *c = mt->columns[i].column; const struct ovsdb_datum *src = &row->fields[c->index]; struct ovsdb_datum *dst = &data[i]; @@ -305,16 +314,17 @@ update_monitor_row_data(const struct ovsdb_monitor_table *mt, } } -/* Frees all of the mt->n_columns ovsdb_datums in data[], using the types taken +/* Frees all of the n_columns ovsdb_datums in data[], using the types taken * from mt->columns[], plus 'data' itself. */ static void free_monitor_row_data(const struct ovsdb_monitor_table *mt, - struct ovsdb_datum *data) + struct ovsdb_datum *data, + size_t n_columns) { if (data) { size_t i; - for (i = 0; i < mt->n_columns; i++) { + for (i = 0; i < n_columns; i++) { const struct ovsdb_column *c = mt->columns[i].column; ovsdb_datum_destroy(&data[i], &c->type); @@ -326,11 +336,12 @@ free_monitor_row_data(const struct ovsdb_monitor_table *mt, /* Frees 'row', which must have been created from 'mt'. */ static void ovsdb_monitor_row_destroy(const struct ovsdb_monitor_table *mt, - struct ovsdb_monitor_row *row) + struct ovsdb_monitor_row *row, + size_t n_columns) { if (row) { - free_monitor_row_data(mt, row->old); - free_monitor_row_data(mt, row->new); + free_monitor_row_data(mt, row->old, n_columns); + free_monitor_row_data(mt, row->new, n_columns); free(row); } } @@ -449,6 +460,7 @@ ovsdb_monitor_condition_add_columns(struct ovsdb_monitor *dbmon, ovsdb_condition_get_columns(condition, &n_columns); for (i = 0; i < n_columns; i++) { + //VLOG_WARN("dbmon %p, cond_add_column: %s, n_columns %"PRIuSIZE, dbmon, columns[i]->name, n_columns); ovsdb_monitor_add_column(dbmon, table, columns[i], OJMS_NONE, false); } @@ -492,6 +504,7 @@ ovsdb_monitor_table_add_changes(struct ovsdb_monitor_table *mt, changes->transaction = next_txn; changes->mt = mt; changes->n_refs = 1; + changes->n_columns = mt->n_columns; hmap_init(&changes->rows); hmap_insert(&mt->changes, &changes->hmap_node, hash_uint64(next_txn)); @@ -522,7 +535,9 @@ ovsdb_monitor_table_untrack_changes(struct ovsdb_monitor_table *mt, struct ovsdb_monitor_changes *changes = ovsdb_monitor_table_find_changes(mt, transaction); if (changes) { + //VLOG_WARN("untrack change: %"PRIu64", found changes %p, n_refs %d", transaction, changes, changes->n_refs); if (--changes->n_refs == 0) { + //VLOG_WARN("untrack change: %"PRIu64", destroy changes %p, n_refs %d", transaction, changes, changes->n_refs); hmap_remove(&mt->changes, &changes->hmap_node); ovsdb_monitor_changes_destroy(changes); } @@ -540,8 +555,10 @@ ovsdb_monitor_table_track_changes(struct ovsdb_monitor_table *mt, changes = ovsdb_monitor_table_find_changes(mt, transaction); if (changes) { changes->n_refs++; + //VLOG_WARN("track new change: %"PRIu64", found changes %p, n_refs %d", transaction, changes, changes->n_refs); } else { ovsdb_monitor_table_add_changes(mt, transaction); + //VLOG_WARN("track new change: %"PRIu64, transaction); } } @@ -552,7 +569,7 @@ ovsdb_monitor_changes_destroy(struct ovsdb_monitor_changes *changes) HMAP_FOR_EACH_SAFE (row, next, hmap_node, &changes->rows) { hmap_remove(&changes->rows, &row->hmap_node); - ovsdb_monitor_row_destroy(changes->mt, row); + ovsdb_monitor_row_destroy(changes->mt, row, changes->n_columns); } hmap_destroy(&changes->rows); free(changes); @@ -788,7 +805,8 @@ ovsdb_monitor_row_skip_update(const struct ovsdb_monitor_table *mt, const struct ovsdb_datum *old, const struct ovsdb_datum *new, enum ovsdb_monitor_selection type, - unsigned long int *changed) + unsigned long int *changed, + size_t n_columns) { if (!(mt->select & type)) { return true; @@ -798,8 +816,8 @@ ovsdb_monitor_row_skip_update(const struct ovsdb_monitor_table *mt, size_t i, n_changes; n_changes = 0; - memset(changed, 0, bitmap_n_bytes(mt->n_columns)); - for (i = 0; i < mt->n_columns; i++) { + memset(changed, 0, bitmap_n_bytes(n_columns)); + for (i = 0; i < n_columns; i++) { const struct ovsdb_column *c = mt->columns[i].column; size_t index = row_type == OVSDB_ROW ? c->index : i; if (!ovsdb_datum_equals(&old[index], &new[index], &c->type)) { @@ -825,14 +843,15 @@ ovsdb_monitor_row_skip_update(const struct ovsdb_monitor_table *mt, * going to be used as part of an "update" notification. * * 'changed' must be a scratch buffer for internal use that is at least - * bitmap_n_bytes(mt->n_columns) bytes long. */ + * bitmap_n_bytes(n_columns) bytes long. */ static struct json * ovsdb_monitor_compose_row_update( const struct ovsdb_monitor_table *mt, const struct ovsdb_monitor_session_condition *condition OVS_UNUSED, enum ovsdb_monitor_row_type row_type OVS_UNUSED, const void *_row, - bool initial, unsigned long int *changed) + bool initial, unsigned long int *changed, + size_t n_columns OVS_UNUSED) { const struct ovsdb_monitor_row *row = _row; enum ovsdb_monitor_selection type; @@ -843,7 +862,8 @@ ovsdb_monitor_compose_row_update( ovs_assert(row_type == OVSDB_MONITOR_ROW); type = ovsdb_monitor_row_update_type(initial, row->old, row->new); if (ovsdb_monitor_row_skip_update(mt, row_type, row->old, - row->new, type, changed)) { + row->new, type, changed, + mt->n_columns)) { return NULL; } @@ -891,14 +911,15 @@ ovsdb_monitor_compose_row_update( * false if it is going to be used as part of an "update2" notification. * * 'changed' must be a scratch buffer for internal use that is at least - * bitmap_n_bytes(mt->n_columns) bytes long. */ + * bitmap_n_bytes(n_columns) bytes long. */ static struct json * ovsdb_monitor_compose_row_update2( const struct ovsdb_monitor_table *mt, const struct ovsdb_monitor_session_condition *condition, enum ovsdb_monitor_row_type row_type, const void *_row, - bool initial, unsigned long int *changed) + bool initial, unsigned long int *changed, + size_t n_columns) { enum ovsdb_monitor_selection type; struct json *row_update2, *diff_json; @@ -914,7 +935,8 @@ ovsdb_monitor_compose_row_update2( type = ovsdb_monitor_row_update_type_condition(mt, condition, initial, row_type, old, new); - if (ovsdb_monitor_row_skip_update(mt, row_type, old, new, type, changed)) { + if (ovsdb_monitor_row_skip_update(mt, row_type, old, new, type, changed, + n_columns)) { return NULL; } @@ -1032,7 +1054,7 @@ ovsdb_monitor_compose_update( HMAP_FOR_EACH_SAFE (row, next, hmap_node, &changes->rows) { struct json *row_json; row_json = (*row_update)(mt, condition, OVSDB_MONITOR_ROW, row, - initial, changed); + initial, changed, changes->n_columns); if (row_json) { ovsdb_monitor_add_json_row(&json, mt->table->schema->name, &table_json, row_json, @@ -1076,7 +1098,8 @@ ovsdb_monitor_compose_cond_change_update( row_json = ovsdb_monitor_compose_row_update2(mt, condition, OVSDB_ROW, row, - false, changed); + false, changed, + mt->n_columns); if (row_json) { ovsdb_monitor_add_json_row(&json, mt->table->schema->name, &table_json, row_json, @@ -1235,8 +1258,8 @@ ovsdb_monitor_changes_update(const struct ovsdb_row *old, change = xzalloc(sizeof *change); hmap_insert(&changes->rows, &change->hmap_node, uuid_hash(uuid)); change->uuid = *uuid; - change->old = clone_monitor_row_data(mt, old); - change->new = clone_monitor_row_data(mt, new); + change->old = clone_monitor_row_data(mt, old, changes->n_columns); + change->new = clone_monitor_row_data(mt, new, changes->n_columns); } else { if (new) { if (!change->new) { @@ -1275,12 +1298,14 @@ ovsdb_monitor_changes_update(const struct ovsdb_row *old, * replication, the row carries the same UUID as the row * just deleted. */ - change->new = clone_monitor_row_data(mt, new); + change->new = clone_monitor_row_data(mt, new, + changes->n_columns); } else { - update_monitor_row_data(mt, new, change->new); + update_monitor_row_data(mt, new, change->new, + changes->n_columns); } } else { - free_monitor_row_data(mt, change->new); + free_monitor_row_data(mt, change->new, changes->n_columns); change->new = NULL; if (!change->old) { @@ -1589,6 +1614,7 @@ ovsdb_monitor_commit(struct ovsdb_monitor *m, const struct ovsdb_txn *txn) ovsdb_monitor_json_cache_flush(m); break; } + //VLOG_WARN("monitor commit: m_transactions: %"PRIu64, m->n_transactions); } void diff --git a/tests/ovsdb-monitor.at b/tests/ovsdb-monitor.at index dca7cad..f6e21d0 100644 --- a/tests/ovsdb-monitor.at +++ b/tests/ovsdb-monitor.at @@ -589,3 +589,71 @@ row,action,name,number,_version [[[["name","==","one"]]]], [[[false]]], [[[true]]]]) + + +AT_SETUP(monitor-cond-change with many sessions pending) +AT_KEYWORDS([ovsdb server monitor monitor-cond negative]) +ordinal_schema > schema +AT_CHECK([ovsdb-tool create db schema], [0], [stdout], [ignore]) + +AT_CAPTURE_FILE([ovsdb-server-log]) +AT_CHECK([ovsdb-server --detach --no-chdir --pidfile --remote=punix:socket --log-file="`pwd`"/ovsdb-server-log db >/dev/null 2>&1]) +on_exit 'kill `cat ovsdb-server.pid`' +for txn in m4_foreach([txn], [[[["ordinals", + {"op": "insert", + "table": "ordinals", + "row": {"number": 0, "name": "zero"}}, + {"op": "insert", + "table": "ordinals", + "row": {"number": 1, "name": "one"}}, + {"op": "insert", + "table": "ordinals", + "row": {"number": 2, "name": "two"}}]]]], ['txn' ]); do + AT_CHECK([ovsdb-client transact unix:socket "$txn"], [0], [ignore], [ignore]) +done + +# 1001 clients monitoring column "name" and with condition for "name" only. +# The clients are created in a way that the 991th client will request condition +# change, so that the chance is high that the condition change will be handled +# before some pending changes are freed. + +cond='[[["name","==","ten"]]]' +for i in `seq 1 990`; do + AT_CHECK([ovsdb-client -vjsonrpc --pidfile=ovsdb-client$i.pid --detach --no-chdir -d json monitor-cond --format=csv unix:socket ordinals $cond ordinals ["name"]], [0], [ignore], [ignore]) +done + +AT_CHECK([ovsdb-client -vjsonrpc --pidfile --detach --no-chdir -d json monitor-cond --format=csv unix:socket ordinals $cond ordinals ["name"] > output], + [0], [ignore], [ignore]) + +for i in `seq 991 1000`; do + AT_CHECK([ovsdb-client -vjsonrpc --pidfile=ovsdb-client$i.pid --detach --no-chdir -d json monitor-cond --format=csv unix:socket ordinals $cond ordinals ["name"]], [0], [ignore], [ignore]) +done + +for txn in m4_foreach([txn], [[[["ordinals", + {"op": "insert", + "table": "ordinals", + "row": {"number": 10, "name": "ten"}}]]]], ['txn' ]); do + AT_CHECK([ovsdb-client transact unix:socket "$txn"], [0], + [ignore], [ignore], [kill `cat server-pid client-pid`]) +done + +# Change the condition so that a new column "number" is added to monitor table. +cond='[[["number","==",1]]]' +AT_CHECK([ovs-appctl -t ovsdb-client ovsdb-client/cond_change ordinals $cond], [0], [ignore], [ignore]) + +# Give some time for the server to flush and free pending changes +# (to crash, when n_columns is not handled properly) +sleep 1 + +AT_CHECK([ovsdb-client transact unix:socket '[["ordinals"]]'], [0], + [ignore], [ignore]) +AT_CHECK([ovs-appctl -t ovsdb-server -e exit], [0], [ignore], [ignore]) +OVS_WAIT_UNTIL([test ! -e ovsdb-server.pid && test ! -e ovsdb-client.pid]) +AT_CHECK([$PYTHON $srcdir/ovsdb-monitor-sort.py < output | uuidfilt], [0], [[row,action,name +<0>,insert,"""ten""" + +row,action,name +<0>,delete, +<1>,insert,"""one""" +]], [ignore]) +AT_CLEANUP