[ovs-dev] [PATCH v11 8/8] treewide: Add `ovs_assert` to check for null pointers
This patch adds an assortment of `ovs_assert` statements to check for
null pointers. We use assertions since it should be impossible for any
of these pointers to be NULL.
Signed-off-by: James Raphael Tiovalen
Reviewed-by: Simon Horman
---
lib/dp-packet.c| 1 +
lib/dpctl.c| 4
lib/odp-execute.c | 4
lib/rtnetlink.c| 4 ++--
lib/shash.c| 2 +-
lib/sset.c | 2 ++
ovsdb/jsonrpc-server.c | 4
ovsdb/monitor.c| 3 +++
ovsdb/ovsdb-server.c | 2 ++
ovsdb/ovsdb.c | 8 +++-
ovsdb/query.c | 2 ++
ovsdb/transaction.c| 2 ++
utilities/ovs-vsctl.c | 3 +++
vtep/vtep-ctl.c| 5 -
14 files changed, 41 insertions(+), 5 deletions(-)
diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index ca29b1f90..059db48ba 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -171,6 +171,7 @@ dp_packet_new_with_headroom(size_t size, size_t headroom)
struct dp_packet *
dp_packet_clone(const struct dp_packet *buffer)
{
+ovs_assert(buffer);
return dp_packet_clone_with_headroom(buffer, 0);
}
diff --git a/lib/dpctl.c b/lib/dpctl.c
index 3ba40fa8f..77461e6a6 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -336,6 +336,8 @@ dpctl_add_if(int argc OVS_UNUSED, const char *argv[],
value = "";
}
+ovs_assert(key);
+
if (!strcmp(key, "type")) {
type = value;
} else if (!strcmp(key, "port_no")) {
@@ -454,6 +456,8 @@ dpctl_set_if(int argc, const char *argv[], struct
dpctl_params *dpctl_p)
value = "";
}
+ovs_assert(key);
+
if (!strcmp(key, "type")) {
if (strcmp(value, type)) {
dpctl_error(dpctl_p, 0,
diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index 5cf6fbec0..44b2cd360 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -147,6 +147,8 @@ odp_set_ipv4(struct dp_packet *packet, const struct
ovs_key_ipv4 *key,
uint8_t new_tos;
uint8_t new_ttl;
+ovs_assert(nh);
+
if (mask->ipv4_src) {
ip_src_nh = get_16aligned_be32(>ip_src);
new_ip_src = key->ipv4_src | (ip_src_nh & ~mask->ipv4_src);
@@ -276,6 +278,8 @@ set_arp(struct dp_packet *packet, const struct ovs_key_arp
*key,
{
struct arp_eth_header *arp = dp_packet_l3(packet);
+ovs_assert(arp);
+
if (!mask) {
arp->ar_op = key->arp_op;
arp->ar_sha = key->arp_sha;
diff --git a/lib/rtnetlink.c b/lib/rtnetlink.c
index f67352603..37078d00e 100644
--- a/lib/rtnetlink.c
+++ b/lib/rtnetlink.c
@@ -112,7 +112,7 @@ rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change
*change)
if (parsed) {
const struct ifinfomsg *ifinfo;
-ifinfo = ofpbuf_at(buf, NLMSG_HDRLEN, sizeof *ifinfo);
+ifinfo = ofpbuf_at_assert(buf, NLMSG_HDRLEN, sizeof *ifinfo);
/* Wireless events can be spammy and cause a
* lot of unnecessary churn and CPU load in
@@ -175,7 +175,7 @@ rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change
*change)
if (parsed) {
const struct ifaddrmsg *ifaddr;
-ifaddr = ofpbuf_at(buf, NLMSG_HDRLEN, sizeof *ifaddr);
+ifaddr = ofpbuf_at_assert(buf, NLMSG_HDRLEN, sizeof *ifaddr);
change->nlmsg_type = nlmsg->nlmsg_type;
change->if_index = ifaddr->ifa_index;
diff --git a/lib/shash.c b/lib/shash.c
index b72b5bf27..c712b3769 100644
--- a/lib/shash.c
+++ b/lib/shash.c
@@ -270,7 +270,7 @@ void *
shash_find_and_delete_assert(struct shash *sh, const char *name)
{
void *data = shash_find_and_delete(sh, name);
-ovs_assert(data != NULL);
+ovs_assert(data);
return data;
}
diff --git a/lib/sset.c b/lib/sset.c
index aa1790020..afdc4392c 100644
--- a/lib/sset.c
+++ b/lib/sset.c
@@ -20,6 +20,7 @@
#include "openvswitch/dynamic-string.h"
#include "hash.h"
+#include "util.h"
static uint32_t
hash_name__(const char *name, size_t length)
@@ -261,6 +262,7 @@ char *
sset_pop(struct sset *set)
{
const char *name = SSET_FIRST(set);
+ovs_assert(name);
char *copy = xstrdup(name);
sset_delete(set, SSET_NODE_FROM_NAME(name));
return copy;
diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
index 5361b3c76..a3ca48a7b 100644
--- a/ovsdb/jsonrpc-server.c
+++ b/ovsdb/jsonrpc-server.c
@@ -1131,6 +1131,8 @@ static void
ovsdb_jsonrpc_trigger_create(struct ovsdb_jsonrpc_session *s, struct ovsdb *db,
struct jsonrpc_msg *request)
{
+ovs_assert(db);
+
/* Check for duplicate ID. */
size_t hash = json_hash(request->id, 0);
struct ovsdb_jsonrpc_trigger *t
@@ -1391,6 +1393,8 @@ ovsdb_jsonrpc_monitor_create(struct ovsdb_jsonrpc_session
*s, struct ovsdb *db,
enum ovsdb_monitor_version version,
const struct json *request_id)
{
+
[ovs-dev] [PATCH v11 6/8] ovs-vsctl: Fix crash when routing is enabled
In the case where routing is enabled, the bridge member of the
`vsctl_port` structs is not populated. This can cause a crash if we
attempt to access it. This patch fixes the crash by checking if the
bridge member is valid before attempting to access it. In the
`check_conflicts` function, we print both the port name and the bridge
name if routing is disabled and we only print the port name if routing
is enabled.
Signed-off-by: James Raphael Tiovalen
Reviewed-by: Simon Horman
---
utilities/ovs-vsctl.c | 22 --
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
index b3c75f8ba..f55c2965a 100644
--- a/utilities/ovs-vsctl.c
+++ b/utilities/ovs-vsctl.c
@@ -888,14 +888,23 @@ check_conflicts(struct vsctl_context *vsctl_ctx, const
char *name,
port = shash_find_data(_ctx->ports, name);
if (port) {
-ctl_fatal("%s because a port named %s already exists on "
-"bridge %s", msg, name, port->bridge->name);
+if (port->bridge) {
+ctl_fatal("%s because a port named %s already exists on "
+ "bridge %s", msg, name, port->bridge->name);
+} else {
+ctl_fatal("%s because a port named %s already exists", msg, name);
+}
}
iface = shash_find_data(_ctx->ifaces, name);
if (iface) {
-ctl_fatal("%s because an interface named %s already exists "
-"on bridge %s", msg, name, iface->port->bridge->name);
+if (iface->port->bridge) {
+ctl_fatal("%s because an interface named %s already exists "
+ "on bridge %s", msg, name, iface->port->bridge->name);
+} else {
+ctl_fatal("%s because an interface named %s already exists", msg,
+ name);
+}
}
free(msg);
@@ -935,7 +944,7 @@ find_port(struct vsctl_context *vsctl_ctx, const char
*name, bool must_exist)
ovs_assert(vsctl_ctx->cache_valid);
port = shash_find_data(_ctx->ports, name);
-if (port && !strcmp(name, port->bridge->name)) {
+if (port && port->bridge && !strcmp(name, port->bridge->name)) {
port = NULL;
}
if (must_exist && !port) {
@@ -953,7 +962,8 @@ find_iface(struct vsctl_context *vsctl_ctx, const char
*name, bool must_exist)
ovs_assert(vsctl_ctx->cache_valid);
iface = shash_find_data(_ctx->ifaces, name);
-if (iface && !strcmp(name, iface->port->bridge->name)) {
+if (iface && iface->port->bridge &&
+!strcmp(name, iface->port->bridge->name)) {
iface = NULL;
}
if (must_exist && !iface) {
--
2.40.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v11 7/8] lib, ovsdb: Add various null pointer checks
This commit adds various null pointer checks to some files in the `lib`
and the `ovsdb` directories to fix several Coverity defects. These
changes are grouped together as they perform similar checks, returning
early or skipping some action if a null pointer is encountered.
Signed-off-by: James Raphael Tiovalen
Reviewed-by: Simon Horman
---
lib/dp-packet.c| 8
lib/shash.c| 7 ++-
ovsdb/jsonrpc-server.c | 2 +-
ovsdb/monitor.c| 7 ++-
ovsdb/ovsdb-client.c | 7 ++-
ovsdb/row.c| 4 +++-
6 files changed, 26 insertions(+), 9 deletions(-)
diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 445cb4761..ca29b1f90 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -353,7 +353,7 @@ void *
dp_packet_put_zeros(struct dp_packet *b, size_t size)
{
void *dst = dp_packet_put_uninit(b, size);
-memset(dst, 0, size);
+nullable_memset(dst, 0, size);
return dst;
}
@@ -364,7 +364,7 @@ void *
dp_packet_put(struct dp_packet *b, const void *p, size_t size)
{
void *dst = dp_packet_put_uninit(b, size);
-memcpy(dst, p, size);
+nullable_memcpy(dst, p, size);
return dst;
}
@@ -436,7 +436,7 @@ void *
dp_packet_push_zeros(struct dp_packet *b, size_t size)
{
void *dst = dp_packet_push_uninit(b, size);
-memset(dst, 0, size);
+nullable_memset(dst, 0, size);
return dst;
}
@@ -447,7 +447,7 @@ void *
dp_packet_push(struct dp_packet *b, const void *p, size_t size)
{
void *dst = dp_packet_push_uninit(b, size);
-memcpy(dst, p, size);
+nullable_memcpy(dst, p, size);
return dst;
}
diff --git a/lib/shash.c b/lib/shash.c
index 2bfc8eb50..b72b5bf27 100644
--- a/lib/shash.c
+++ b/lib/shash.c
@@ -205,8 +205,13 @@ shash_delete(struct shash *sh, struct shash_node *node)
char *
shash_steal(struct shash *sh, struct shash_node *node)
{
-char *name = node->name;
+char *name;
+if (!node) {
+return NULL;
+}
+
+name = node->name;
hmap_remove(>map, >node);
free(node);
return name;
diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
index 17868f5b7..5361b3c76 100644
--- a/ovsdb/jsonrpc-server.c
+++ b/ovsdb/jsonrpc-server.c
@@ -1038,7 +1038,7 @@ ovsdb_jsonrpc_session_got_request(struct
ovsdb_jsonrpc_session *s,
request->id);
} else if (!strcmp(request->method, "get_schema")) {
struct ovsdb *db = ovsdb_jsonrpc_lookup_db(s, request, );
-if (!reply) {
+if (db && !reply) {
reply = jsonrpc_create_reply(ovsdb_schema_to_json(db->schema),
request->id);
}
diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
index 6965d4b4a..eba4b492b 100644
--- a/ovsdb/monitor.c
+++ b/ovsdb/monitor.c
@@ -478,7 +478,9 @@ ovsdb_monitor_add_column(struct ovsdb_monitor *dbmon,
struct ovsdb_monitor_column *c;
mt = shash_find_data(>tables, table->schema->name);
-
+if (!mt) {
+return NULL;
+}
/* Check for column duplication. Return duplicated column name. */
if (mt->columns_index_map[column->index] != -1) {
return column->name;
@@ -789,6 +791,9 @@ ovsdb_monitor_table_condition_update(
struct ovsdb_monitor_table_condition *mtc =
shash_find_data(>tables, table->schema->name);
+if (!mtc) {
+return NULL;
+}
struct ovsdb_error *error;
struct ovsdb_condition cond = OVSDB_CONDITION_INITIALIZER();
diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c
index 46484630d..2b12907b6 100644
--- a/ovsdb/ovsdb-client.c
+++ b/ovsdb/ovsdb-client.c
@@ -1873,6 +1873,10 @@ do_dump(struct jsonrpc *rpc, const char *database,
n_tables = shash_count(>tables);
}
+if (!tables) {
+goto end;
+}
+
/* Construct transaction to retrieve entire database. */
transaction = json_array_create_1(json_string_create(database));
for (i = 0; i < n_tables; i++) {
@@ -1932,8 +1936,9 @@ do_dump(struct jsonrpc *rpc, const char *database,
}
jsonrpc_msg_destroy(reply);
-shash_destroy(_columns);
free(tables);
+end:
+shash_destroy(_columns);
ovsdb_schema_destroy(schema);
}
diff --git a/ovsdb/row.c b/ovsdb/row.c
index d7bfbdd36..9f87c832a 100644
--- a/ovsdb/row.c
+++ b/ovsdb/row.c
@@ -399,7 +399,9 @@ ovsdb_row_set_add_row(struct ovsdb_row_set *set, const
struct ovsdb_row *row)
set->rows = x2nrealloc(set->rows, >allocated_rows,
sizeof *set->rows);
}
-set->rows[set->n_rows++] = row;
+if (set->rows) {
+set->rows[set->n_rows++] = row;
+}
}
struct json *
--
2.40.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v11 5/8] file, monitor: Add null pointer assertions for old and new ovsdb_rows
This commit adds non-null pointer assertions in some code that performs
some decisions based on old and new input ovsdb_rows.
Signed-off-by: James Raphael Tiovalen
Reviewed-by: Simon Horman
---
ovsdb/file.c| 2 ++
ovsdb/monitor.c | 4 +++-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/ovsdb/file.c b/ovsdb/file.c
index 2d887e53e..b1d386e76 100644
--- a/ovsdb/file.c
+++ b/ovsdb/file.c
@@ -522,7 +522,9 @@ ovsdb_file_txn_add_row(struct ovsdb_file_txn *ftxn,
}
if (row) {
+ovs_assert(new || old);
struct ovsdb_table *table = new ? new->table : old->table;
+ovs_assert(table);
char uuid[UUID_LEN + 1];
if (table != ftxn->table) {
diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
index 3cdd03b20..6965d4b4a 100644
--- a/ovsdb/monitor.c
+++ b/ovsdb/monitor.c
@@ -1332,8 +1332,10 @@ ovsdb_monitor_changes_update(const struct ovsdb_row *old,
const struct ovsdb_monitor_table *mt,
struct ovsdb_monitor_change_set_for_table *mcst)
{
+ovs_assert(new || old);
const struct uuid *uuid = ovsdb_row_get_uuid(new ? new : old);
-struct ovsdb_monitor_row *change;
+ovs_assert(uuid);
+struct ovsdb_monitor_row *change = NULL;
change = ovsdb_monitor_changes_row_find(mcst, uuid);
if (!change) {
--
2.40.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v11 4/8] ovsdb: Assert and check return values of `ovsdb_table_schema_get_column`
This commit adds a few null pointer assertions and checks to some return
values of `ovsdb_table_schema_get_column`. If a null pointer is
encountered in these blocks, either the assertion will fail or the
control flow will now be redirected to alternative paths which will
output the appropriate error messages.
Signed-off-by: James Raphael Tiovalen
Reviewed-by: Simon Horman
---
ovsdb/condition.c| 5 -
ovsdb/ovsdb-client.c | 7 +--
ovsdb/ovsdb-util.c | 6 ++
3 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/ovsdb/condition.c b/ovsdb/condition.c
index d0016fa7f..a1a009bfc 100644
--- a/ovsdb/condition.c
+++ b/ovsdb/condition.c
@@ -47,7 +47,10 @@ ovsdb_clause_from_json(const struct ovsdb_table_schema *ts,
/* Column and arg fields are not being used with boolean functions.
* Use dummy values */
-clause->column = ovsdb_table_schema_get_column(ts, "_uuid");
+const struct ovsdb_column *uuid_column =
+ ovsdb_table_schema_get_column(ts, "_uuid");
+ovs_assert(uuid_column);
+clause->column = uuid_column;
clause->index = clause->column->index;
ovsdb_datum_init_default(>arg, >column->type);
return NULL;
diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c
index bae2c5f04..46484630d 100644
--- a/ovsdb/ovsdb-client.c
+++ b/ovsdb/ovsdb-client.c
@@ -1232,8 +1232,11 @@ parse_monitor_columns(char *arg, const char *server,
const char *database,
}
free(nodes);
-add_column(server, ovsdb_table_schema_get_column(table, "_version"),
- columns, columns_json);
+const struct ovsdb_column *version_column =
+ovsdb_table_schema_get_column(table, "_version");
+
+ovs_assert(version_column);
+add_column(server, version_column, columns, columns_json);
}
if (!initial || !insert || !delete || !modify) {
diff --git a/ovsdb/ovsdb-util.c b/ovsdb/ovsdb-util.c
index 303191dc8..d287a6be3 100644
--- a/ovsdb/ovsdb-util.c
+++ b/ovsdb/ovsdb-util.c
@@ -291,9 +291,15 @@ ovsdb_util_write_string_string_column(struct ovsdb_row
*row,
size_t i;
column = ovsdb_table_schema_get_column(row->table->schema, column_name);
+if (!column) {
+VLOG_ERR("No %s column present in the %s table",
+ column_name, row->table->schema->name);
+goto unwind;
+}
datum = ovsdb_util_get_datum(row, column_name, OVSDB_TYPE_STRING,
OVSDB_TYPE_STRING, UINT_MAX);
if (!datum) {
+unwind:
for (i = 0; i < n; i++) {
free(keys[i]);
free(values[i]);
--
2.40.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v11 3/8] shash, simap, smap: Add assertions to `*_count` functions
This commit adds assertions in the functions `shash_count`,
`simap_count`, and `smap_count` to ensure that the corresponding input
struct pointer is not NULL.
This ensures that if the return values of `shash_sort`, `simap_sort`,
or `smap_sort` are NULL, then the following for loops would not attempt
to access the pointer, which might result in segmentation faults or
undefined behavior.
Signed-off-by: James Raphael Tiovalen
Reviewed-by: Simon Horman
---
lib/shash.c | 2 ++
lib/simap.c | 2 ++
lib/smap.c | 1 +
3 files changed, 5 insertions(+)
diff --git a/lib/shash.c b/lib/shash.c
index a7b2c6458..2bfc8eb50 100644
--- a/lib/shash.c
+++ b/lib/shash.c
@@ -17,6 +17,7 @@
#include
#include "openvswitch/shash.h"
#include "hash.h"
+#include "util.h"
static struct shash_node *shash_find__(const struct shash *,
const char *name, size_t name_len,
@@ -100,6 +101,7 @@ shash_is_empty(const struct shash *shash)
size_t
shash_count(const struct shash *shash)
{
+ovs_assert(shash);
return hmap_count(>map);
}
diff --git a/lib/simap.c b/lib/simap.c
index 0ee08d74d..1c01d4ebe 100644
--- a/lib/simap.c
+++ b/lib/simap.c
@@ -17,6 +17,7 @@
#include
#include "simap.h"
#include "hash.h"
+#include "util.h"
static size_t hash_name(const char *, size_t length);
static struct simap_node *simap_find__(const struct simap *,
@@ -84,6 +85,7 @@ simap_is_empty(const struct simap *simap)
size_t
simap_count(const struct simap *simap)
{
+ovs_assert(simap);
return hmap_count(>map);
}
diff --git a/lib/smap.c b/lib/smap.c
index c1633e2a1..f8c7eaccb 100644
--- a/lib/smap.c
+++ b/lib/smap.c
@@ -300,6 +300,7 @@ smap_is_empty(const struct smap *smap)
size_t
smap_count(const struct smap *smap)
{
+ovs_assert(smap);
return hmap_count(>map);
}
--
2.40.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v11 1/8] lib: Add non-null assertions to some return values of `dp_packet_data`
This commit adds some `ovs_assert()` checks to some return values of
`dp_packet_data()` to ensure that they are not NULL and to prevent
null-pointer dereferences, which might lead to unwanted crashes. We use
assertions since it should be impossible for these calls to
`dp_packet_data()` to return NULL.
Signed-off-by: James Raphael Tiovalen
Reviewed-by: Simon Horman
---
lib/dp-packet.c | 15 ++-
lib/netdev-native-tnl.c | 17 +++--
lib/pcap-file.c | 4 +++-
3 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index ae8ab5800..445cb4761 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -183,9 +183,12 @@ dp_packet_clone_with_headroom(const struct dp_packet
*buffer, size_t headroom)
struct dp_packet *new_buffer;
uint32_t mark;
-new_buffer = dp_packet_clone_data_with_headroom(dp_packet_data(buffer),
- dp_packet_size(buffer),
- headroom);
+const void *data_dp = dp_packet_data(buffer);
+ovs_assert(data_dp);
+
+new_buffer = dp_packet_clone_data_with_headroom(data_dp,
+dp_packet_size(buffer),
+headroom);
/* Copy the following fields into the returned buffer: l2_pad_size,
* l2_5_ofs, l3_ofs, l4_ofs, cutlen, packet_type and md. */
memcpy(_buffer->l2_pad_size, >l2_pad_size,
@@ -322,8 +325,10 @@ dp_packet_shift(struct dp_packet *b, int delta)
: true);
if (delta != 0) {
-char *dst = (char *) dp_packet_data(b) + delta;
-memmove(dst, dp_packet_data(b), dp_packet_size(b));
+const void *data_dp = dp_packet_data(b);
+ovs_assert(data_dp);
+char *dst = (char *) data_dp + delta;
+memmove(dst, data_dp, dp_packet_size(b));
dp_packet_set_data(b, dst);
}
}
diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
index 9abdf5107..c1551aa35 100644
--- a/lib/netdev-native-tnl.c
+++ b/lib/netdev-native-tnl.c
@@ -43,6 +43,7 @@
#include "seq.h"
#include "unaligned.h"
#include "unixctl.h"
+#include "util.h"
#include "openvswitch/vlog.h"
VLOG_DEFINE_THIS_MODULE(native_tnl);
@@ -221,12 +222,13 @@ netdev_tnl_calc_udp_csum(struct udp_header *udp, struct
dp_packet *packet,
{
uint32_t csum;
-if (netdev_tnl_is_header_ipv6(dp_packet_data(packet))) {
-csum = packet_csum_pseudoheader6(netdev_tnl_ipv6_hdr(
- dp_packet_data(packet)));
+void *data_dp = dp_packet_data(packet);
+ovs_assert(data_dp);
+
+if (netdev_tnl_is_header_ipv6(data_dp)) {
+csum = packet_csum_pseudoheader6(netdev_tnl_ipv6_hdr(data_dp));
} else {
-csum = packet_csum_pseudoheader(netdev_tnl_ip_hdr(
-dp_packet_data(packet)));
+csum = packet_csum_pseudoheader(netdev_tnl_ip_hdr(data_dp));
}
csum = csum_continue(csum, udp, ip_tot_size);
@@ -425,7 +427,10 @@ netdev_gre_pop_header(struct dp_packet *packet)
struct flow_tnl *tnl = >tunnel;
int hlen = sizeof(struct eth_header) + 4;
-hlen += netdev_tnl_is_header_ipv6(dp_packet_data(packet)) ?
+const void *data_dp = dp_packet_data(packet);
+ovs_assert(data_dp);
+
+hlen += netdev_tnl_is_header_ipv6(data_dp) ?
IPV6_HEADER_LEN : IP_HEADER_LEN;
pkt_metadata_init_tnl(md);
diff --git a/lib/pcap-file.c b/lib/pcap-file.c
index 3ed7ea488..9f4e2e1e2 100644
--- a/lib/pcap-file.c
+++ b/lib/pcap-file.c
@@ -284,6 +284,8 @@ ovs_pcap_write(struct pcap_file *p_file, struct dp_packet
*buf)
struct timeval tv;
ovs_assert(dp_packet_is_eth(buf));
+const void *data_dp = dp_packet_data(buf);
+ovs_assert(data_dp);
xgettimeofday();
prh.ts_sec = tv.tv_sec;
@@ -291,7 +293,7 @@ ovs_pcap_write(struct pcap_file *p_file, struct dp_packet
*buf)
prh.incl_len = dp_packet_size(buf);
prh.orig_len = dp_packet_size(buf);
ignore(fwrite(, sizeof prh, 1, p_file->file));
-ignore(fwrite(dp_packet_data(buf), dp_packet_size(buf), 1, p_file->file));
+ignore(fwrite(data_dp, dp_packet_size(buf), 1, p_file->file));
fflush(p_file->file);
}
--
2.40.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v11 2/8] lib, ovs-vsctl: Add zero-initializations
This commit adds zero-initializations by changing `SFL_ALLOC` from
`malloc` to `xzalloc`, adding a `memset` call to `sflAlloc`,
initializing a `pollfd` struct variable with zeroes, and changing some
calls to `xmalloc` to `xzalloc`. This is to prevent potential data leaks
or undefined behavior from potentially uninitialized variables.
Some variables would always be initialized by either the code flow or
the compiler. Thus, some of the associated Coverity reports might be
false positives. That said, it is still considered best practice to
zero-initialize variables upfront just in case to ensure the overall
resilience and security of OVS, as long as they do not impact
performance-critical code. As a bonus, it would also make static
analyzer tools, such as Coverity, happy.
Signed-off-by: James Raphael Tiovalen
---
lib/latch-unix.c | 2 +-
lib/sflow_agent.c | 12 ++--
lib/sflow_api.h | 2 +-
utilities/ovs-vsctl.c | 5 ++---
4 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/lib/latch-unix.c b/lib/latch-unix.c
index f4d10c39a..c62bb024b 100644
--- a/lib/latch-unix.c
+++ b/lib/latch-unix.c
@@ -71,7 +71,7 @@ latch_set(struct latch *latch)
bool
latch_is_set(const struct latch *latch)
{
-struct pollfd pfd;
+struct pollfd pfd = {0};
int retval;
pfd.fd = latch->fds[0];
diff --git a/lib/sflow_agent.c b/lib/sflow_agent.c
index c95f654a5..b15758ae1 100644
--- a/lib/sflow_agent.c
+++ b/lib/sflow_agent.c
@@ -510,8 +510,16 @@ void sfl_agent_sysError(SFLAgent *agent, char *modName,
char *msg)
static void * sflAlloc(SFLAgent *agent, size_t bytes)
{
-if(agent->allocFn) return (*agent->allocFn)(agent->magic, agent, bytes);
-else return SFL_ALLOC(bytes);
+void *alloc;
+if (agent->allocFn) {
+alloc = (*agent->allocFn)(agent->magic, agent, bytes);
+ovs_assert(alloc);
+memset(alloc, 0, bytes);
+} else {
+alloc = SFL_ALLOC(bytes);
+ovs_assert(alloc);
+}
+return alloc;
}
static void sflFree(SFLAgent *agent, void *obj)
diff --git a/lib/sflow_api.h b/lib/sflow_api.h
index a0530b37a..eb23e2acd 100644
--- a/lib/sflow_api.h
+++ b/lib/sflow_api.h
@@ -337,7 +337,7 @@ void sfl_agent_sysError(SFLAgent *agent, char *modName,
char *msg);
u_int32_t sfl_receiver_samplePacketsSent(SFLReceiver *receiver);
-#define SFL_ALLOC malloc
+#define SFL_ALLOC xzalloc
#define SFL_FREE free
#endif /* SFLOW_API_H */
diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
index 2f5ac1a26..b3c75f8ba 100644
--- a/utilities/ovs-vsctl.c
+++ b/utilities/ovs-vsctl.c
@@ -575,7 +575,7 @@ add_bridge_to_cache(struct vsctl_context *vsctl_ctx,
struct ovsrec_bridge *br_cfg, const char *name,
struct vsctl_bridge *parent, int vlan)
{
-struct vsctl_bridge *br = xmalloc(sizeof *br);
+struct vsctl_bridge *br = xzalloc(sizeof *br);
br->br_cfg = br_cfg;
br->name = xstrdup(name);
ovs_list_init(>ports);
@@ -659,7 +659,7 @@ static struct vsctl_port *
add_port_to_cache(struct vsctl_context *vsctl_ctx, struct vsctl_bridge *parent,
struct ovsrec_port *port_cfg)
{
-struct vsctl_port *port;
+struct vsctl_port *port = xzalloc(sizeof *port);
if (port_cfg->tag
&& *port_cfg->tag >= 0 && *port_cfg->tag <= 4095) {
@@ -671,7 +671,6 @@ add_port_to_cache(struct vsctl_context *vsctl_ctx, struct
vsctl_bridge *parent,
}
}
-port = xmalloc(sizeof *port);
ovs_list_push_back(>ports, >ports_node);
ovs_list_init(>ifaces);
port->port_cfg = port_cfg;
--
2.40.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v11 0/8] treewide: Fix multiple Coverity defects
This cleanup patchset addresses several high and medium-impact Coverity defects. Unit tests have been successfully executed via `make check` and they successfully passed. The list of revisions so far: v2: Fix some apply-robot checkpatch errors and warnings. v3: Fix some apply-robot checkpatch errors and warnings. v4: - Fix some apply-robot checkpatch errors and warnings. - Fix github-robot build failure: linux clang test asan. v5: Improve commit message to better describe the intent of this patch. v6: - Convert some explicit null pointer checks to assertions since they are checking for impossible conditions. - Use `nullable_memset()` and `nullable_memcpy()` instead of manually performing null checks for `memset()` and `memcpy()`. v7: - Split the single commit into a patch series which consists of 8 patches to make it easier to review and help keep things organized. - Incorporated feedback from Aaron Conole. v8: Incorporated feedback from Simon Horman and Ilya Maximets. v9: Fix warning for patch 2/8 due to using `sizeof` on a void pointer. v10: Resolve merge conflict for patch 8/8. v11: Fix order of calling `ovs_assert` before `memset` in patch 2/8. James Raphael Tiovalen (8): lib: Add non-null assertions to some return values of `dp_packet_data` lib, ovs-vsctl: Add zero-initializations shash, simap, smap: Add assertions to `*_count` functions ovsdb: Assert and check return values of `ovsdb_table_schema_get_column` file, monitor: Add null pointer assertions for old and new ovsdb_rows ovs-vsctl: Fix crash when routing is enabled lib, ovsdb: Add various null pointer checks treewide: Add `ovs_assert` to check for null pointers lib/dp-packet.c | 24 +++- lib/dpctl.c | 4 lib/latch-unix.c| 2 +- lib/netdev-native-tnl.c | 17 +++-- lib/odp-execute.c | 4 lib/pcap-file.c | 4 +++- lib/rtnetlink.c | 4 ++-- lib/sflow_agent.c | 12 ++-- lib/sflow_api.h | 2 +- lib/shash.c | 11 +-- lib/simap.c | 2 ++ lib/smap.c | 1 + lib/sset.c | 2 ++ ovsdb/condition.c | 5 - ovsdb/file.c| 2 ++ ovsdb/jsonrpc-server.c | 6 +- ovsdb/monitor.c | 14 -- ovsdb/ovsdb-client.c| 14 +++--- ovsdb/ovsdb-server.c| 2 ++ ovsdb/ovsdb-util.c | 6 ++ ovsdb/ovsdb.c | 8 +++- ovsdb/query.c | 2 ++ ovsdb/row.c | 4 +++- ovsdb/transaction.c | 2 ++ utilities/ovs-vsctl.c | 30 +- vtep/vtep-ctl.c | 5 - 26 files changed, 146 insertions(+), 43 deletions(-) -- 2.40.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v10 2/8] lib, ovs-vsctl: Add zero-initializations
On Wed, May 24, 2023 at 12:20 AM Simon Horman wrote: > > On question about this. > > memset() is passed alloc before the call to ovs_assert(). > From the POV of the safety you are implementing, is that ok? Ah right, apologies for that. It seems that I overlooked the order. `ovs_assert` should be called before `memset` since calling `memset` on a null pointer results in undefined behavior. I will correct this in the next patch version. Best regards, James Raphael Tiovalen ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [ovs-dev v10] ofproto-dpif-upcall: fix push_dp_ops
push_dp_ops only handles delete ops errors but ignores the modify
ops results. It's better to handle all the dp operation errors in
a consistent way.
This patch prevents the inconsistency by considering modify failure
in revalidators.
To note, we cannot perform two state transitions and change ukey_state
into UKEY_EVICTED directly here, because, if we do so, the
sweep will remove the ukey alone and leave dp flow alive. Later, the
dump will retrieve the dp flow and might even recover it. This will
contribute the stats of this dp flow twice.
v8->v9: add testsuite and delete INCONSISTENT ukey at revalidate_sweep.
v9->v10: change the commit message and refine the test case.
Signed-off-by: Peng He
---
ofproto/ofproto-dpif-upcall.c | 48 --
tests/dpif-netdev.at | 49 +++
2 files changed, 84 insertions(+), 13 deletions(-)
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index cd57fdbd9..5a75d9444 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -258,6 +258,7 @@ enum ukey_state {
UKEY_CREATED = 0,
UKEY_VISIBLE, /* Ukey is in umap, datapath flow install is queued. */
UKEY_OPERATIONAL, /* Ukey is in umap, datapath flow is installed. */
+UKEY_INCONSISTENT, /* Ukey is in umap, datapath flow is inconsistent. */
UKEY_EVICTING, /* Ukey is in umap, datapath flow delete is queued. */
UKEY_EVICTED, /* Ukey is in umap, datapath flow is deleted. */
UKEY_DELETED, /* Ukey removed from umap, ukey free is deferred. */
@@ -1999,6 +2000,10 @@ transition_ukey_at(struct udpif_key *ukey, enum
ukey_state dst,
* UKEY_VISIBLE -> UKEY_EVICTED
* A handler attempts to install the flow, but the datapath rejects it.
* Consider that the datapath has already destroyed it.
+ * UKEY_OPERATIONAL -> UKEY_INCONSISTENT
+ * A revalidator modifies the flow with error returns.
+ * UKEY_INCONSISTENT -> UKEY_EVICTING
+ * A revalidator decides to evict the datapath flow.
* UKEY_OPERATIONAL -> UKEY_EVICTING
* A revalidator decides to evict the datapath flow.
* UKEY_EVICTING-> UKEY_EVICTED
@@ -2007,7 +2012,9 @@ transition_ukey_at(struct udpif_key *ukey, enum
ukey_state dst,
* A revalidator has removed the ukey from the umap and is deleting it.
*/
if (ukey->state == dst - 1 || (ukey->state == UKEY_VISIBLE &&
- dst < UKEY_DELETED)) {
+ dst < UKEY_DELETED) ||
+ (ukey->state == UKEY_OPERATIONAL &&
+ dst == UKEY_EVICTING)) {
ukey->state = dst;
} else {
struct ds ds = DS_EMPTY_INITIALIZER;
@@ -2472,26 +2479,31 @@ push_dp_ops(struct udpif *udpif, struct ukey_op *ops,
size_t n_ops)
for (i = 0; i < n_ops; i++) {
struct ukey_op *op = [i];
-struct dpif_flow_stats *push, *stats, push_buf;
-
-stats = op->dop.flow_del.stats;
-push = _buf;
-
-if (op->dop.type != DPIF_OP_FLOW_DEL) {
-/* Only deleted flows need their stats pushed. */
-continue;
-}
if (op->dop.error) {
-/* flow_del error, 'stats' is unusable. */
if (op->ukey) {
ovs_mutex_lock(>ukey->mutex);
-transition_ukey(op->ukey, UKEY_EVICTED);
+if (op->ukey->state == UKEY_EVICTING) {
+transition_ukey(op->ukey, UKEY_EVICTED);
+} else if (op->ukey->state == UKEY_OPERATIONAL) {
+/* Modify failed, ukey's state was UKEY_OPERATIONAL */
+transition_ukey(op->ukey, UKEY_INCONSISTENT);
+}
ovs_mutex_unlock(>ukey->mutex);
}
continue;
}
+if (op->dop.type != DPIF_OP_FLOW_DEL) {
+/* Only deleted flows need their stats pushed. */
+continue;
+}
+
+struct dpif_flow_stats *push, *stats, push_buf;
+
+stats = op->dop.flow_del.stats;
+push = _buf;
+
if (op->ukey) {
ovs_mutex_lock(>ukey->mutex);
transition_ukey(op->ukey, UKEY_EVICTED);
@@ -2839,6 +2851,15 @@ revalidate(struct revalidator *revalidator)
continue;
}
+if (ukey->state == UKEY_INCONSISTENT) {
+ukey->dump_seq = dump_seq;
+reval_op_init([n_ops++], UKEY_DELETE, udpif, ukey,
+ , _actions);
+ovs_mutex_unlock(>mutex);
+continue;
+}
+
+
if (ukey->state <= UKEY_OPERATIONAL) {
/* The flow is now confirmed to be in the datapath. */
transition_ukey(ukey, UKEY_OPERATIONAL);
@@ -2927,13 +2948,14 @@ revalidator_sweep__(struct revalidator *revalidator,
bool purge)
Re: [ovs-dev] [ovs-dev v9] ofproto-dpif-upcall: fix push_dp_ops
Recently I have identified the real root cause for this inconsistency between ukey and megaflow actions. I thus decided to withdraw this version and change its commits. Will send a v10. Please do not review this version. 0-day Robot 于2023年6月2日周五 23:19写道： > Bleep bloop. Greetings Peng He, I am a robot and I have tried out your > patch. > Thanks for your contribution. > > I encountered some error that I wasn't expecting. See the details below. > > > checkpatch: > ERROR: Author Peng He needs to sign off. > WARNING: Unexpected sign-offs from developers who are not authors or > co-authors or committers: Peng He > Lines checked: 207, Warnings: 1, Errors: 1 > > > Please check this out. If you feel there has been an error, please email > acon...@redhat.com > > Thanks, > 0-day Robot > -- hepeng ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 3/3] netdev-offload-dpdk: use flow transfer proxy mechanism
Hi Eli,
Thanks for reviewing this. Please see below.
On Tue, 21 Feb 2023, Eli Britstein wrote:
-Original Message-
From: Ivan Malov
Sent: Tuesday, 21 February 2023 2:41
To: ovs-dev@openvswitch.org
Cc: Ilya Maximets ; Eli Britstein ; Ori
Kam ; David Marchand
Subject: [PATCH v4 3/3] netdev-offload-dpdk: use flow transfer proxy
mechanism
External email: Use caution opening links or attachments
Manage "transfer" flows via the corresponding mechanism.
Doing so requires that the traffic source be specified explicitly, via the
corresponding pattern item.
Signed-off-by: Ivan Malov
---
lib/netdev-dpdk.c | 88 +++
lib/netdev-dpdk.h | 4 +-
lib/netdev-offload-dpdk.c | 55 +++-
3 files changed, 117 insertions(+), 30 deletions(-)
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 2cebc3cca..3a9c9d9a0
100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -434,6 +434,7 @@ enum dpdk_hw_ol_features {
struct netdev_dpdk {
PADDED_MEMBERS_CACHELINE_MARKER(CACHE_LINE_SIZE, cacheline0,
+dpdk_port_t flow_transfer_proxy_port_id;
This extra field here makes it overflow one cache line.
dpdk_port_t port_id;
/* If true, device was attached by rte_eth_dev_attach(). */ @@ -1183,6
+1184,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
uint32_t rx_chksm_offload_capa = RTE_ETH_RX_OFFLOAD_UDP_CKSUM |
RTE_ETH_RX_OFFLOAD_TCP_CKSUM |
RTE_ETH_RX_OFFLOAD_IPV4_CKSUM;
+int ret;
/*
* Full tunnel offload requires that tunnel ID metadata be @@ -1194,6
+1196,24 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
*/
dpdk_eth_dev_init_rx_metadata(dev);
+/*
+ * Managing "transfer" flows requires that the user communicate them
+ * via a port which has the privilege to control the embedded switch.
+ * For some vendors, all ports in a given switching domain have
+ * this privilege. For other vendors, it's only one port.
+ *
+ * Get the proxy port ID and remember it for later use.
+ */
+ret = rte_flow_pick_transfer_proxy(dev->port_id,
+ >flow_transfer_proxy_port_id,
NULL);
+if (ret != 0) {
+/*
+ * The PMD does not indicate the proxy port.
+ * Assume the proxy is unneeded.
+ */
+dev->flow_transfer_proxy_port_id = dev->port_id;
+}
+
rte_eth_dev_info_get(dev->port_id, );
if (strstr(info.driver_name, "vf") != NULL) { @@ -3981,8 +4001,10 @@
netdev_dpdk_detach(struct unixctl_conn *conn, int argc OVS_UNUSED,
const char *argv[], void *aux OVS_UNUSED) {
struct ds used_interfaces = DS_EMPTY_INITIALIZER;
+struct netdev_dpdk *dev_self = NULL;
struct rte_eth_dev_info dev_info;
dpdk_port_t sibling_port_id;
+struct netdev_dpdk *dev;
dpdk_port_t port_id;
bool used = false;
char *response;
@@ -4000,8 +4022,6 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int
argc OVS_UNUSED,
argv[1]);
RTE_ETH_FOREACH_DEV_SIBLING (sibling_port_id, port_id) {
-struct netdev_dpdk *dev;
-
LIST_FOR_EACH (dev, list_node, _list) {
if (dev->port_id != sibling_port_id) {
continue;
@@ -4021,6 +4041,25 @@ netdev_dpdk_detach(struct unixctl_conn *conn,
int argc OVS_UNUSED,
}
ds_destroy(_interfaces);
+/*
+ * The device being detached may happen to be a flow proxy port
+ * for another device (still attached). If so, do not allow to
+ * detach. Devices dependent on this one must be detached first.
+ */
+LIST_FOR_EACH (dev, list_node, _list) {
+if (dev->port_id == port_id) {
+dev_self = dev;
+} else if (dev->flow_transfer_proxy_port_id == port_id) {
+response = xasprintf("Device '%s' can not be detached (flow
proxy)",
+ argv[1]);
This is not acceptable.
When removing a port, we clean the offloads using
netdev_offload_dpdk_flow_flush().
It should be enhanced to check if the proxy port is detached, remove the
offloads of all the ports that used it.
There is a related patch proposed in [1].
[1]
http://patchwork.ozlabs.org/project/openvswitch/patch/20220905144603.3585105-1-el...@nvidia.com/
I hear you. But do you want me to first wait for patch [1] to be applied
and then send the proxy enhancement to be applied on top of it?
Or do you believe I can do something in parallel here?
As I can see, patch [1] has not been applied yet.
Is there any problem with it?
If I need to wait, then I believe I can revoke this [3/3]
patch from the series for the time being and send v5.
What's your opinion?
+goto error;
+}
+}
+
+/* Indicate that the device being detached no longer needs a flow proxy.
*/
+if (dev_self != NULL)
+dev_self->flow_transfer_proxy_port_id = port_id;
+
