From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id D7CDE378E; Mon, 4 Sep 2017 12:12:29 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Sep 2017 03:12:19 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,474,1498546800"; d="scan'208";a="896889268" Received: from smonroyx-mobl.ger.corp.intel.com (HELO [10.237.221.28]) ([10.237.221.28]) by FMSMGA003.fm.intel.com with ESMTP; 04 Sep 2017 03:12:18 -0700 To: Patrick MacArthur , dev@dpdk.org References: <20170804185357.6612-1-patrick@patrickmacarthur.net> Cc: stable@dpdk.org From: Sergio Gonzalez Monroy Message-ID: Date: Mon, 4 Sep 2017 11:12:17 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <20170804185357.6612-1-patrick@patrickmacarthur.net> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] eal: Copy raw strings taken from command line X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Sep 2017 10:12:30 -0000 On 04/08/2017 19:53, Patrick MacArthur wrote: > Normally, command line argument strings are considered immutable, but > SPDK [1] and urdma [2] construct argv arrays to pass to rte_eal_init(). > These strings are allocated using malloc() and freed after DPDK > initialization with free(). However, in the case of --file-prefix and > --huge-dir, DPDK takes the pointer to these strings in argv directly. If > a secondary process calls rte_eal_pci_probe() after rte_eal_init() > returns, as is done by SPDK, this causes a use-after-free error because > the strings have been freed by the calling code immediately after > rte_eal_init() returns. > > This problem was observed when running SPDK example programs as a > secondary process and causes the secondary processes to fail: > >> Starting DPDK 16.11.1 initialization... >> [ DPDK EAL parameters: identify -c 4 --file-prefix=spdk3260 --base-virtaddr=0x1000000000 --proc-type=auto ] >> EAL: Detected 40 lcore(s) >> EAL: Auto-detected process type: SECONDARY >> EAL: Probing VFIO support... >> EAL: VFIO support initialized >> EAL: PCI device 0000:81:00.0 on NUMA socket 1 >> EAL: probe driver: 8086:953 spdk_nvme >> EAL: cannot connect to primary process! >> EAL: Error - exiting with code: 1 >> Cause: Requested device 0000:81:00.0 cannot be used > Running strace shows that the file prefix has been zero'd out by the > time that the secondary process attempts to probe the NVMe device. > > The use-after-free errors can be easily detected with valgrind: > >> ==8489== Invalid read of size 1 >> ==8489== at 0x4C30D22: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) >> ==8489== by 0x58DB955: vfprintf (vfprintf.c:1637) >> ==8489== by 0x59A4685: __vsnprintf_chk (vsnprintf_chk.c:63) >> ==8489== by 0x59A45E7: __snprintf_chk (snprintf_chk.c:34) >> ==8489== by 0x1246AB: get_socket_path.constprop.0 (in /home/pmacarth/src/spdk/examples/nvme/identify/identify) >> ==8489== by 0x124B09: vfio_mp_sync_connect_to_primary (in /home/pmacarth/src/spdk/examples/nvme/identify/identify) >> ==8489== by 0x123BE4: vfio_get_group_fd.part.1 (in /home/pmacarth/src/spdk/examples/nvme/identify/identify) >> ==8489== by 0x124366: vfio_setup_device (in /home/pmacarth/src/spdk/examples/nvme/identify/identify) >> ==8489== by 0x126C8A: pci_vfio_map_resource (in /home/pmacarth/src/spdk/examples/nvme/identify/identify) >> ==8489== by 0x12B115: pci_probe_all_drivers.part.0 (in /home/pmacarth/src/spdk/examples/nvme/identify/identify) >> ==8489== by 0x12B596: rte_eal_pci_probe (in /home/pmacarth/src/spdk/examples/nvme/identify/identify) >> ==8489== by 0x11D5B5: spdk_pci_enumerate (pci.c:147) >> ==8489== Address 0x63f362e is 14 bytes inside a block of size 32 free'd >> ==8489== at 0x4C2ED5B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) >> ==8489== by 0x11E6FB: spdk_free_args (init.c:136) >> ==8489== by 0x11EBF5: spdk_env_init (init.c:309) >> ==8489== by 0x10D2AA: main (identify.c:976) >> ==8489== Block was alloc'd at >> ==8489== at 0x4C2DB2F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) >> ==8489== by 0x11E7D7: _sprintf_alloc (init.c:76) >> ==8489== by 0x11EA78: spdk_build_eal_cmdline (init.c:251) >> ==8489== by 0x11EA78: spdk_env_init (init.c:282) >> ==8489== by 0x10D2AA: main (identify.c:976) >> ==8489== > Fix this by using strdup() to create separate memory buffers for these > strings. Note that this patch will cause valgrind to report memory > leaks of these buffers as there is nowhere to free them. Using static > buffers is an option but would make these strings have a fixed maximum > length whereas there is currently no limit defined by the API. > > [1] http://spdk.io > [2] https://github.com/zrlio/urdma > > Fixes: af75078fece3 ("first public release") > Cc: stable@dpdk.org > > Signed-off-by: Patrick MacArthur > --- > lib/librte_eal/linuxapp/eal/eal.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c > index 48f12f44cfa4..529d2cebc84b 100644 > --- a/lib/librte_eal/linuxapp/eal/eal.c > +++ b/lib/librte_eal/linuxapp/eal/eal.c > @@ -569,11 +569,11 @@ eal_parse_args(int argc, char **argv) > break; > > case OPT_HUGE_DIR_NUM: > - internal_config.hugepage_dir = optarg; > + internal_config.hugepage_dir = strdup(optarg); > break; > > case OPT_FILE_PREFIX_NUM: > - internal_config.hugefile_prefix = optarg; > + internal_config.hugefile_prefix = strdup(optarg); > break; > > case OPT_SOCKET_MEM_NUM: Acked-by: Sergio Gonzalez Monroy