From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id B1B0CA00BE; Tue, 14 Jun 2022 11:40:58 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A022840DDD; Tue, 14 Jun 2022 11:40:58 +0200 (CEST) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by mails.dpdk.org (Postfix) with ESMTP id 35B5040C35 for ; Tue, 14 Jun 2022 11:40:57 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1655199657; x=1686735657; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=ed4iqCc73DIcHYjalNajabjU0FDDxaFDvKL1w99fQ6o=; b=Y+BEOPuH7FP9MvGQc+M4OznbeQSx6bNC1A2z21QG94MnZxmBKLn/rhw7 zS3XBDkw9Y/WIKaE+2nwFyiIkUJI4ZTPgekX6vrCGLCVtPUb2Z9Nw3EyL 6YsoaQg05xM8AX+BUgOaBnl2czghvuAW8vm484ZlgR6tyCLUxQuUEdy49 1CYE7hYWKkvfs5UvRMVYClPOkCkm1a1NMEUReArqsHmFOIKCbJznA0cry 4A9/T6RUrKvwu7aylEZzrB5D/Homj5rth/RsMf7B4TRtLYhFmyeIyIp1B jWLjzGiZEYviU68XIk3YqODtjhZqZub/bSlowa4+uQyH4X/KX8JDhwKEu A==; X-IronPort-AV: E=McAfee;i="6400,9594,10377"; a="259018045" X-IronPort-AV: E=Sophos;i="5.91,299,1647327600"; d="scan'208";a="259018045" Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Jun 2022 02:40:56 -0700 X-IronPort-AV: E=Sophos;i="5.91,299,1647327600"; d="scan'208";a="830316008" Received: from bricha3-mobl.ger.corp.intel.com ([10.55.133.106]) by fmsmga006-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 14 Jun 2022 02:40:54 -0700 Date: Tue, 14 Jun 2022 10:40:50 +0100 From: Bruce Richardson To: Stephen Hemminger Cc: dev@dpdk.org, david.marchand@redhat.com, ktraynor@redhat.com, jingjing.wu@intel.com, beilei.xing@intel.com Subject: Re: [PATCH 0/4] clean up zero-length arrays Message-ID: References: <20220602150834.643745-1-bruce.richardson@intel.com> <20220608082302.09de7deb@hermes.local> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20220608082302.09de7deb@hermes.local> X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Wed, Jun 08, 2022 at 08:23:02AM -0700, Stephen Hemminger wrote: > On Thu, 2 Jun 2022 16:08:30 +0100 > Bruce Richardson wrote: > > > This patchset adds a coccinelle script to clean-up zero-length > > arrays in structures. The final patches are the result of running > > that script on the DPDK repository. > > > > Bruce Richardson (4): > > cocci: add script for zero-length arrays in structs > > drivers: replace zero-length arrays with undimensioned ones > > lib: replace zero-length arrays with undimensioned ones > > app: examples: replace zero-length arrays with undimensioned ones > > > > app/test/test_table_tables.c | 2 +- > > devtools/cocci/zero_length_array.cocci | 21 +++++++++++++++ > > drivers/bus/dpaa/include/netcfg.h | 4 +-- > > drivers/bus/vmbus/rte_vmbus_reg.h | 4 +-- > > drivers/common/cnxk/roc_se.h | 2 +- > > drivers/common/dpaax/caamflib/desc/ipsec.h | 2 +- > > drivers/common/dpaax/dpaax_iova_table.h | 2 +- > > drivers/common/mlx5/mlx5_prm.h | 10 +++---- > > drivers/crypto/ipsec_mb/ipsec_mb_private.h | 4 +-- > > drivers/crypto/virtio/virtio_ring.h | 4 +-- > > drivers/crypto/virtio/virtqueue.h | 2 +- > > drivers/net/atlantic/hw_atl/hw_atl_utils.h | 2 +- > > drivers/net/cxgbe/clip_tbl.h | 2 +- > > drivers/net/cxgbe/l2t.h | 2 +- > > drivers/net/cxgbe/mps_tcam.h | 2 +- > > drivers/net/cxgbe/smt.h | 2 +- > > drivers/net/enic/base/vnic_devcmd.h | 2 +- > > drivers/net/hinic/hinic_pmd_tx.h | 2 +- > > drivers/net/mlx5/mlx5_tx.h | 2 +- > > drivers/net/nfp/nfpcore/nfp_nsp.h | 2 +- > > drivers/net/virtio/virtio_ring.h | 4 +-- > > drivers/net/virtio/virtio_user/vhost_kernel.c | 2 +- > > drivers/net/virtio/virtio_user/vhost_vdpa.c | 2 +- > > drivers/net/virtio/virtqueue.h | 2 +- > > drivers/regex/mlx5/mlx5_rxp.h | 4 +-- > > examples/ip_reassembly/main.c | 2 +- > > examples/ptpclient/ptpclient.c | 4 +-- > > lib/cryptodev/cryptodev_pmd.h | 2 +- > > lib/cryptodev/rte_cryptodev.h | 2 +- > > lib/eventdev/rte_event_timer_adapter.h | 2 +- > > lib/ip_frag/ip_reassembly.h | 2 +- > > lib/ipsec/sa.h | 2 +- > > lib/rib/rte_rib.c | 2 +- > > lib/rib/rte_rib6.c | 2 +- > > lib/table/rte_swx_table_learner.c | 4 +-- > > lib/table/rte_table_hash_key16.c | 4 +-- > > lib/table/rte_table_hash_key32.c | 4 +-- > > lib/table/rte_table_hash_key8.c | 4 +-- > > lib/vhost/rte_vhost.h | 4 +-- > > 40 files changed, 101 insertions(+), 54 deletions(-) > > create mode 100644 devtools/cocci/zero_length_array.cocci > > create mode 100644 lib/count_comments.py > > > > -- > > 2.34.1 > > > > Bruce, looking at this commit, it looks like the underlying cause > of the problem with iavf was it is using array size of one > when flex array should be used: > > commit b5b3ea803e4741ad6a46a38d8227c78226d9054d > Author: Kevin Traynor > Date: Fri Apr 17 16:43:35 2020 +0100 > > eal/x86: ignore gcc 10 stringop-overflow warnings > > stringop-overflow warns when it sees a possible overflow > in a string operation. > > In the rte_memcpy functions different branches are taken > depending on the size. stringop-overflow is raised for the > branches in the function where it sees the static size of the > src could be overflowed. > > However, in reality a correct size argument and in some cases > dynamic allocation would ensure that this does not happen. > > For example, in the case below for key, the correct path will be > chosen in rte_memcpy_generic at runtime based on the size argument > but as some paths in the function could lead to a cast to 32 bytes > a warning is raised. > > In function ‘_mm256_storeu_si256’, > inlined from ‘rte_memcpy_generic’ > at ../lib/librte_eal/common/include/arch/x86/rte_memcpy.h:315:2, > inlined from ‘iavf_configure_rss_key’ > at ../lib/librte_eal/common/include/arch/x86/rte_memcpy.h:869:10: > > /usr/lib/gcc/x86_64-redhat-linux/10/include/avxintrin.h:928:8: > warning: writing 32 bytes into a region of size 1 [-Wstringop-overflow=] > 928 | *__P = __A; > | ~~~~~^~~~~ > In file included > from ../drivers/net/iavf/../../common/iavf/iavf_prototype.h:10, > from ../drivers/net/iavf/iavf.h:9, > from ../drivers/net/iavf/iavf_vchnl.c:22: > > ../drivers/net/iavf/iavf_vchnl.c: > In function ‘iavf_configure_rss_key’: > > ../drivers/net/iavf/../../common/iavf/virtchnl.h:508:5: > note: at offset 0 to object ‘key’ with size 1 declared here > 508 | u8 key[1]; /* RSS hash key, packed bytes */ > | ^~~ > I would tend to agree with your assessment. It looks like the "u8 key[1]" value should probably be "u8 key[]", and also in the following structure in the file, "u8 lut[1]" should probably be "u8 lut[]". Adding maintainers for driver on CC Beilei, Jingjing, in "common/iavf/virtchnl.h", there are quite a number of values at the end of structs which are defined as arrays of size 1. We suspect that many of these are placeholder arrays which should be given as unsigned arrays. Is this assessment correct? /Bruce