From patchwork Mon Jan 24 10:34:32 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adrian Moreno X-Patchwork-Id: 1583375 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: bilbo.ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=ZPJn7ag5; dkim-atps=neutral Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=2605:bc80:3010::137; helo=smtp4.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Received: from smtp4.osuosl.org (smtp4.osuosl.org [IPv6:2605:bc80:3010::137]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by bilbo.ozlabs.org (Postfix) with ESMTPS id 4Jj5wQ5b8Nz9t0k for ; Mon, 24 Jan 2022 21:35:18 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id B50C8409AD; Mon, 24 Jan 2022 10:35:14 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp4.osuosl.org ([127.0.0.1]) by localhost (smtp4.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ELr5kCexSxyq; Mon, 24 Jan 2022 10:35:12 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp4.osuosl.org (Postfix) with ESMTPS id 7DAB0409B0; Mon, 24 Jan 2022 10:35:11 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 3BA3EC0039; Mon, 24 Jan 2022 10:35:11 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp1.osuosl.org (smtp1.osuosl.org [IPv6:2605:bc80:3010::138]) by lists.linuxfoundation.org (Postfix) with ESMTP id D0E25C002F for ; Mon, 24 Jan 2022 10:35:09 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id B1FD983312 for ; Mon, 24 Jan 2022 10:35:09 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Authentication-Results: smtp1.osuosl.org (amavisd-new); dkim=pass (1024-bit key) header.d=redhat.com Received: from smtp1.osuosl.org ([127.0.0.1]) by localhost (smtp1.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ADMPbpMEW6W1 for ; Mon, 24 Jan 2022 10:35:07 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by smtp1.osuosl.org (Postfix) with ESMTPS id D5B6A82F57 for ; Mon, 24 Jan 2022 10:35:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1643020505; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=mYmAxP/F4n0GUyEeiE10wAwmWvgapMTpfgNZ+nM+jto=; b=ZPJn7ag5wUUAqOwwJfGspzzRVOv0jLgNJDkwGObmb6FVSUivV+MLT5XWOXnVKQW+PLUD5K y4QaRuv41mcPB+qSESUTU7Kl8YPij9UqWwJ7VWH5XXsiFOyvMkhaUo5K9MkFUsXzN7MIjB R69ji0hOg2+S05CIllJvvSDgHyHIIMA= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-516-3thkmZHTPYiMOQ80SO833A-1; Mon, 24 Jan 2022 05:35:00 -0500 X-MC-Unique: 3thkmZHTPYiMOQ80SO833A-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 74DDC185302D; Mon, 24 Jan 2022 10:34:59 +0000 (UTC) Received: from amorenoz.users.ipa.redhat.com (unknown [10.39.193.20]) by smtp.corp.redhat.com (Postfix) with ESMTP id 909BC7AB41; Mon, 24 Jan 2022 10:34:49 +0000 (UTC) From: Adrian Moreno To: dev@openvswitch.org Date: Mon, 24 Jan 2022 11:34:32 +0100 Message-Id: <20220124103445.2459400-1-amorenoz@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=amorenoz@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Cc: jakub@redhat.com, fweimer@redhat.com, dceara@redhat.com, i.maximets@ovn.org Subject: [ovs-dev] [PATCH 00/13] Fix undefined behavior in loop macros X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" When running builds with UBSan, some undefined behavior was detected in the iteration of common data data structures in OVS. Coincidentally, a bug was reported [1] whose root cause whas another, this time undetected, undefined behavior in the iteration macros. From both cases, we conclude that the way we're currently iterating the data structures is prone to errors and UB. This series is an attempt to rewrite those macros in a UB-safe manner. The core problem is that #define OBJECT_CONTAINING(POINTER, OBJECT, MEMBER) macro is being used on invalid POINTER values. In some cases we use NULL to compute the end-of-loop condition. In others, we allow it to point to non-contained objects (e.g: a non-contained stack allocated "struct ovs_list" as in [1]). In order to systematically solve this in all cases this series introduces a new set of macros that implement a multi-variable loop iteration. They declare a hidden iterator variable inside the loop, use to iterate and evaluate the loop condition and only compute its OBJECT_CONTAINING if it satisfies the loop condition. One consequence of this safety guard is that the pointer provided by the user is set to NULL after the loop (if not exited via "break;"). Apart from normal iteration, many OVS data structures have a _SAFE version of the loop which require the user to declare an extra variable to hold the next value of the iterator. The _SAFE version of the multi-variable iterators have the extra benefit of not requiring such extra variable. On relevant data structures, an initial patch rewrites the macros in a backwards compatible manner and a second patch modifies all the callers to remove the unneeded variable. The fist patch would be easy to backport and the second would make code cleaner for the master branch. Testing notes: In order to verify this series removes all the loop-related UB, I've tested it on top of Dumitru's series [2] (without patch 1/11, which can hide some still invalid use of OBJECT_CONTAINING). I've also verified no extra errors are reported through clang-analyzer. Limitations: The proposed approach benefits code readability, therefore the name of the iterator variable is derived from the name of the object pointer given by the caller. This means that in an unlikely but still possible case in which a caller wants to nest two loops with the same iterating pointer object, the inner loop iterator variable will hide the one declared in the outer loop. This limitation is easy to spot (the compiler will warn) and easy to work around (just declaring another object pointer variable). I found no such code in the ovs tree. Credits: The idea was discussed in [3] and proposed by Jakub Jelinek . [1] https://bugzilla.redhat.com/show_bug.cgi?id=2014942 [2] https://patchwork.ozlabs.org/project/openvswitch/list/?series=277900 [3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103964 Adrian Moreno (13): util: add multi-variable loop iterator macros util: add safe multi-variable iterators list: use multi-variable helpers for list loops list: ensure iterator is NULL after pop loop list: remove the next variable in safe loops hmap: use multi-variable helpers for hmap loops hmap: implement UB-safe hmap pop iterator hmap: remove the next variable in safe loops cmap: use multi-variable iterators hindex: use multi-variable iterators hindex: remove the next variable in safe loops rculist: use multi-variable helpers for loop macros vtep: use _SAFE iterator if freing the iterator include/openvswitch/hmap.h | 93 +++++++++++++++++++----------------- include/openvswitch/list.h | 70 +++++++++++++++------------ include/openvswitch/shash.h | 5 +- include/openvswitch/util.h | 78 ++++++++++++++++++++++++++++++ lib/cfm.c | 4 +- lib/classifier.c | 4 +- lib/cmap.h | 24 ++++++---- lib/conntrack.c | 8 ++-- lib/dns-resolve.c | 4 +- lib/dpif-netdev.c | 19 ++++---- lib/fat-rwlock.c | 4 +- lib/hindex.h | 40 ++++++++-------- lib/hmapx.c | 4 +- lib/hmapx.h | 5 +- lib/ipf.c | 22 ++++----- lib/json.c | 4 +- lib/lacp.c | 4 +- lib/lldp/lldpd-structs.c | 7 ++- lib/lldp/lldpd.c | 8 ++-- lib/mac-learning.c | 4 +- lib/mcast-snooping.c | 12 ++--- lib/namemap.c | 4 +- lib/netdev-afxdp.c | 4 +- lib/netdev-dpdk.c | 8 ++-- lib/netdev-linux.c | 4 +- lib/netdev-offload-tc.c | 4 +- lib/ofp-msgs.c | 8 ++-- lib/ovs-lldp.c | 12 ++--- lib/ovs-numa.h | 4 +- lib/ovsdb-cs.c | 12 ++--- lib/ovsdb-idl.c | 46 +++++++++--------- lib/ovsdb-map-op.c | 4 +- lib/ovsdb-set-op.c | 4 +- lib/pcap-file.c | 4 +- lib/perf-counter.c | 4 +- lib/poll-loop.c | 4 +- lib/rculist.h | 61 ++++++++++++----------- lib/seq.c | 8 ++-- lib/shash.c | 8 ++-- lib/simap.c | 4 +- lib/simap.h | 5 +- lib/smap.c | 4 +- lib/smap.h | 5 +- lib/stopwatch.c | 4 +- lib/tnl-ports.c | 16 +++---- lib/unixctl.c | 8 ++-- lib/vconn.c | 4 +- ofproto/bond.c | 6 +-- ofproto/connmgr.c | 28 +++++------ ofproto/in-band.c | 4 +- ofproto/netflow.c | 8 ++-- ofproto/ofproto-dpif-ipfix.c | 12 ++--- ofproto/ofproto-dpif-sflow.c | 4 +- ofproto/ofproto-dpif-trace.c | 4 +- ofproto/ofproto-dpif-xlate.c | 12 ++--- ofproto/ofproto-dpif.c | 28 +++++------ ofproto/ofproto.c | 16 +++---- ovsdb/condition.c | 8 ++-- ovsdb/jsonrpc-server.c | 40 ++++++++-------- ovsdb/monitor.c | 36 +++++++------- ovsdb/ovsdb-server.c | 11 ++--- ovsdb/ovsdb-tool.c | 7 ++- ovsdb/ovsdb.c | 4 +- ovsdb/query.c | 4 +- ovsdb/raft-private.c | 4 +- ovsdb/raft.c | 33 +++++++------ ovsdb/relay.c | 4 +- ovsdb/replication.c | 8 ++-- ovsdb/table.c | 4 +- ovsdb/transaction-forward.c | 10 ++-- ovsdb/transaction.c | 36 +++++++------- ovsdb/trigger.c | 4 +- tests/test-cmap.c | 4 +- tests/test-hindex.c | 4 +- tests/test-hmap.c | 4 +- tests/test-list.c | 10 ++-- utilities/ovs-ofctl.c | 4 +- utilities/ovs-vsctl.c | 12 ++--- vswitchd/bridge.c | 72 ++++++++++++++-------------- vtep/vtep-ctl.c | 20 ++++---- 80 files changed, 616 insertions(+), 525 deletions(-)