From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 152264CA9 for ; Mon, 11 Mar 2019 11:08:04 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Mar 2019 03:08:04 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.58,467,1544515200"; d="scan'208";a="200174871" Received: from irsmsx104.ger.corp.intel.com ([163.33.3.159]) by orsmga001.jf.intel.com with ESMTP; 11 Mar 2019 03:08:03 -0700 Received: from irsmsx111.ger.corp.intel.com (10.108.20.4) by IRSMSX104.ger.corp.intel.com (163.33.3.159) with Microsoft SMTP Server (TLS) id 14.3.408.0; Mon, 11 Mar 2019 10:08:01 +0000 Received: from irsmsx107.ger.corp.intel.com ([169.254.10.15]) by irsmsx111.ger.corp.intel.com ([169.254.2.6]) with mapi id 14.03.0415.000; Mon, 11 Mar 2019 10:08:01 +0000 From: "Burakov, Anatoly" To: Yongseok Koh CC: dpdk stable Thread-Topic: please check if patch 'eal: fix strdup usages in internal config' is applicable to LTS release 17.11.6 Thread-Index: AQHU1doOduqj5MJU2EaX838UqeqoxKYGOF4g Date: Mon, 11 Mar 2019 10:08:00 +0000 Message-ID: References: <20190308180901.31938-1-yskoh@mellanox.com> In-Reply-To: <20190308180901.31938-1-yskoh@mellanox.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNzQ2NmE4N2ItZWMzMS00YTkxLWE4YzgtMjk1OTc5YWVmNDA5IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiSmlcLzFnT0lLVXRvRUhQSmx0Ykt3clwvWW9POFZFZDB0NkpzYnRocVBUOWlxb1FMMWZEc2dLYVwvODJRb1Y4TmhqcCJ9 dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-stable] please check if patch 'eal: fix strdup usages in internal config' is applicable to LTS release 17.11.6 X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 11 Mar 2019 10:08:05 -0000 It's not a critical bugfix, so it can be left out of it is deemed too much = change for a stable release. However, it is indeed applicable to stable, an= d can be merged. Thanks, Anatoly > -----Original Message----- > From: Yongseok Koh [mailto:yskoh@mellanox.com] > Sent: Friday, March 8, 2019 6:09 PM > To: Burakov, Anatoly > Cc: dpdk stable > Subject: please check if patch 'eal: fix strdup usages in internal config= ' is > applicable to LTS release 17.11.6 >=20 > Hi, >=20 > Even though the patch doesn't have "Cc: stable@dpdk.org" tag or "Fixes:" > tag, the commit log says it fixes some issue. Tags might be mistakenly mi= ssed. > We don't want to miss any fixes for stable releases. That's why I'm sendi= ng > this email. >=20 > Please send backports if you think the patch should be merged to LTS rele= ase > 17.11.6 Or, let me know if you have any comments, say, need more time, or > it's worthless to backport it. And please send it to "stable@dpdk.org", b= ut > not "dev@dpdk.org". >=20 > FYI, branch 17.11 is located at tree: > git://dpdk.org/dpdk-stable >=20 > It'd be great if you could do that in one or two weeks. Also, please add = a > heading line like below before the commit log body: > [ backported from upstream commit xxx ] >=20 > Example: http://dpdk.org/browse/dpdk- > stable/commit/?h=3D16.07&id=3Dc4831394c7d1944d8ec27d52c22997f20d19718e >=20 > Also please mention the target LTS in the subject line, as we have more t= han > one at the same time, for example: >=20 > [PATCH 17.11] foo/bar: fix baz >=20 > With git send-email, this can be achieved by appending the parameter: >=20 > --subject-prefix=3D'17.11' >=20 >=20 > Thanks. >=20 > Yongseok >=20 > --- > From 66d9f61de0885bd07662a016542600fe139d4eed Mon Sep 17 00:00:00 > 2001 > From: Anatoly Burakov > Date: Thu, 10 Jan 2019 13:38:59 +0000 > Subject: [PATCH] eal: fix strdup usages in internal config >=20 > Currently, we use strdup in a few places to store command-line parameter > values for certain internal config values. There are several issues with = that. >=20 > First of all, they're never freed, so memory ends up leaking either after= EAL > exit, or when these command-line options are supplied multiple times. >=20 > Second of all, they're defined as `const char *`, so they > *cannot* be freed even if we wanted to. >=20 > Finally, strdup may return NULL, which will be stored in the config. For = most > fields, NULL is a valid value, but for the default prefix, the value is a= lways > expected to be valid. >=20 > To fix all of this, three things are done. First, we change the definitio= ns of > these values to `char *` as opposed to `const char *`. This does not brea= k the > ABI, and previous code assumes constness (which is more restrictive), so = it's > safe to do so. >=20 > Then, fix all usages of strdup to check return value, and add a cleanup > function that will free the memory occupied by these strings, as well as > freeing them before assigning a new value to prevent leaks when parameter > is specified multiple times. >=20 > And finally, add an internal API to query hugefile prefix, so that, absen= t of a > valid value, a default value will be returned, and also fix up all usages= of > hugefile prefix to use this API instead of accessing hugefile prefix dire= ctly. >=20 > Bugzilla ID: 108 >=20 > Signed-off-by: Anatoly Burakov > --- > lib/librte_eal/bsdapp/eal/eal.c | 19 ++++++++++-- > lib/librte_eal/common/eal_common_options.c | 25 ++++++++++++++-- > lib/librte_eal/common/eal_filesystem.h | 6 +++- > lib/librte_eal/common/eal_internal_cfg.h | 6 ++-- > lib/librte_eal/common/eal_options.h | 1 + > lib/librte_eal/linuxapp/eal/eal.c | 46 ++++++++++++++++++++++++= ---- > -- > lib/librte_eal/linuxapp/eal/eal_memory.c | 2 +- > 7 files changed, 87 insertions(+), 18 deletions(-) >=20 > diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/= eal.c > index c8e0da097..1ba9bd7cf 100644 > --- a/lib/librte_eal/bsdapp/eal/eal.c > +++ b/lib/librte_eal/bsdapp/eal/eal.c > @@ -117,7 +117,7 @@ eal_create_runtime_dir(void) >=20 > /* create prefix-specific subdirectory under DPDK runtime dir */ > ret =3D snprintf(runtime_dir, sizeof(runtime_dir), "%s/%s", > - tmp, internal_config.hugefile_prefix); > + tmp, eal_get_hugefile_prefix()); > if (ret < 0 || ret =3D=3D sizeof(runtime_dir)) { > RTE_LOG(ERR, EAL, "Error creating prefix-specific runtime > path name\n"); > return -1; > @@ -535,9 +535,21 @@ eal_parse_args(int argc, char **argv) >=20 > switch (opt) { > case OPT_MBUF_POOL_OPS_NAME_NUM: > - internal_config.user_mbuf_pool_ops_name =3D > - strdup(optarg); > + { > + char *ops_name =3D strdup(optarg); > + if (ops_name =3D=3D NULL) > + RTE_LOG(ERR, EAL, "Could not store mbuf > pool ops name\n"); > + else { > + /* free old ops name */ > + if > (internal_config.user_mbuf_pool_ops_name !=3D > + NULL) > + > free(internal_config.user_mbuf_pool_ops_name); > + > + internal_config.user_mbuf_pool_ops_name > =3D > + ops_name; > + } > break; > + } > case 'h': > eal_usage(prgname); > exit(EXIT_SUCCESS); > @@ -923,6 +935,7 @@ rte_eal_cleanup(void) { > rte_service_finalize(); > rte_mp_channel_cleanup(); > + eal_cleanup_config(&internal_config); > return 0; > } >=20 > diff --git a/lib/librte_eal/common/eal_common_options.c > b/lib/librte_eal/common/eal_common_options.c > index 6e3a83b98..a2d862b5f 100644 > --- a/lib/librte_eal/common/eal_common_options.c > +++ b/lib/librte_eal/common/eal_common_options.c > @@ -169,6 +169,14 @@ eal_option_device_parse(void) > return ret; > } >=20 > +const char * > +eal_get_hugefile_prefix(void) > +{ > + if (internal_config.hugefile_prefix !=3D NULL) > + return internal_config.hugefile_prefix; > + return HUGEFILE_PREFIX_DEFAULT; > +} > + > void > eal_reset_internal_config(struct internal_config *internal_cfg) { @@ -1= 77,7 > +185,7 @@ eal_reset_internal_config(struct internal_config *internal_cfg) > internal_cfg->memory =3D 0; > internal_cfg->force_nrank =3D 0; > internal_cfg->force_nchannel =3D 0; > - internal_cfg->hugefile_prefix =3D HUGEFILE_PREFIX_DEFAULT; > + internal_cfg->hugefile_prefix =3D NULL; > internal_cfg->hugepage_dir =3D NULL; > internal_cfg->force_sockets =3D 0; > /* zero out the NUMA config */ > @@ -1348,6 +1356,19 @@ eal_auto_detect_cores(struct rte_config *cfg) } >=20 > int > +eal_cleanup_config(struct internal_config *internal_cfg) { > + if (internal_cfg->hugefile_prefix !=3D NULL) > + free(internal_cfg->hugefile_prefix); > + if (internal_cfg->hugepage_dir !=3D NULL) > + free(internal_cfg->hugepage_dir); > + if (internal_cfg->user_mbuf_pool_ops_name !=3D NULL) > + free(internal_cfg->user_mbuf_pool_ops_name); > + > + return 0; > +} > + > +int > eal_adjust_config(struct internal_config *internal_cfg) { > int i; > @@ -1387,7 +1408,7 @@ eal_check_common_options(struct internal_config > *internal_cfg) > RTE_LOG(ERR, EAL, "Invalid process type specified\n"); > return -1; > } > - if (index(internal_cfg->hugefile_prefix, '%') !=3D NULL) { > + if (index(eal_get_hugefile_prefix(), '%') !=3D NULL) { > RTE_LOG(ERR, EAL, "Invalid char, '%%', in -- > "OPT_FILE_PREFIX" " > "option\n"); > return -1; > diff --git a/lib/librte_eal/common/eal_filesystem.h > b/lib/librte_eal/common/eal_filesystem.h > index 64a028db7..89a3added 100644 > --- a/lib/librte_eal/common/eal_filesystem.h > +++ b/lib/librte_eal/common/eal_filesystem.h > @@ -28,6 +28,10 @@ eal_create_runtime_dir(void); int > eal_clean_runtime_dir(void); >=20 > +/** Function to return hugefile prefix that's currently set up */ const > +char * eal_get_hugefile_prefix(void); > + > #define RUNTIME_CONFIG_FNAME "config" > static inline const char * > eal_runtime_config_path(void) > @@ -89,7 +93,7 @@ static inline const char * eal_get_hugefile_path(char > *buffer, size_t buflen, const char *hugedir, int f_id) { > snprintf(buffer, buflen, HUGEFILE_FMT, hugedir, > - internal_config.hugefile_prefix, f_id); > + eal_get_hugefile_prefix(), f_id); > buffer[buflen - 1] =3D '\0'; > return buffer; > } > diff --git a/lib/librte_eal/common/eal_internal_cfg.h > b/lib/librte_eal/common/eal_internal_cfg.h > index 98e314fef..60eaead8f 100644 > --- a/lib/librte_eal/common/eal_internal_cfg.h > +++ b/lib/librte_eal/common/eal_internal_cfg.h > @@ -66,9 +66,9 @@ struct internal_config { > volatile int syslog_facility; /**< facility passed to openlog() */ > /** default interrupt mode for VFIO */ > volatile enum rte_intr_mode vfio_intr_mode; > - const char *hugefile_prefix; /**< the base filename of hugetlbfs > files */ > - const char *hugepage_dir; /**< specific hugetlbfs directory to > use */ > - const char *user_mbuf_pool_ops_name; > + char *hugefile_prefix; /**< the base filename of hugetlbfs files *= / > + char *hugepage_dir; /**< specific hugetlbfs directory to use */ > + char *user_mbuf_pool_ops_name; > /**< user defined mbuf pool ops name */ > unsigned num_hugepage_sizes; /**< how many sizes on this > system */ > struct hugepage_info hugepage_info[MAX_HUGEPAGE_SIZES]; > diff --git a/lib/librte_eal/common/eal_options.h > b/lib/librte_eal/common/eal_options.h > index 1480c5d77..58ee9ae33 100644 > --- a/lib/librte_eal/common/eal_options.h > +++ b/lib/librte_eal/common/eal_options.h > @@ -77,6 +77,7 @@ int eal_parse_common_option(int opt, const char > *argv, > struct internal_config *conf); > int eal_option_device_parse(void); > int eal_adjust_config(struct internal_config *internal_cfg); > +int eal_cleanup_config(struct internal_config *internal_cfg); > int eal_check_common_options(struct internal_config *internal_cfg); voi= d > eal_common_usage(void); enum rte_proc_type_t > eal_proc_type_detect(void); diff --git a/lib/librte_eal/linuxapp/eal/eal.= c > b/lib/librte_eal/linuxapp/eal/eal.c > index 2d8d470b8..a386829f3 100644 > --- a/lib/librte_eal/linuxapp/eal/eal.c > +++ b/lib/librte_eal/linuxapp/eal/eal.c > @@ -125,7 +125,7 @@ eal_create_runtime_dir(void) >=20 > /* create prefix-specific subdirectory under DPDK runtime dir */ > ret =3D snprintf(runtime_dir, sizeof(runtime_dir), "%s/%s", > - tmp, internal_config.hugefile_prefix); > + tmp, eal_get_hugefile_prefix()); > if (ret < 0 || ret =3D=3D sizeof(runtime_dir)) { > RTE_LOG(ERR, EAL, "Error creating prefix-specific runtime > path name\n"); > return -1; > @@ -727,13 +727,31 @@ eal_parse_args(int argc, char **argv) > exit(EXIT_SUCCESS); >=20 > case OPT_HUGE_DIR_NUM: > - internal_config.hugepage_dir =3D strdup(optarg); > + { > + char *hdir =3D strdup(optarg); > + if (hdir =3D=3D NULL) > + RTE_LOG(ERR, EAL, "Could not store > hugepage directory\n"); > + else { > + /* free old hugepage dir */ > + if (internal_config.hugepage_dir !=3D NULL) > + free(internal_config.hugepage_dir); > + internal_config.hugepage_dir =3D hdir; > + } > break; > - > + } > case OPT_FILE_PREFIX_NUM: > - internal_config.hugefile_prefix =3D strdup(optarg); > + { > + char *prefix =3D strdup(optarg); > + if (prefix =3D=3D NULL) > + RTE_LOG(ERR, EAL, "Could not store file > prefix\n"); > + else { > + /* free old prefix */ > + if (internal_config.hugefile_prefix !=3D NULL) > + free(internal_config.hugefile_prefix); > + internal_config.hugefile_prefix =3D prefix; > + } > break; > - > + } > case OPT_SOCKET_MEM_NUM: > if (eal_parse_socket_arg(optarg, > internal_config.socket_mem) < 0) { > @@ -783,10 +801,21 @@ eal_parse_args(int argc, char **argv) > break; >=20 > case OPT_MBUF_POOL_OPS_NAME_NUM: > - internal_config.user_mbuf_pool_ops_name =3D > - strdup(optarg); > + { > + char *ops_name =3D strdup(optarg); > + if (ops_name =3D=3D NULL) > + RTE_LOG(ERR, EAL, "Could not store mbuf > pool ops name\n"); > + else { > + /* free old ops name */ > + if > (internal_config.user_mbuf_pool_ops_name !=3D > + NULL) > + > free(internal_config.user_mbuf_pool_ops_name); > + > + internal_config.user_mbuf_pool_ops_name > =3D > + ops_name; > + } > break; > - > + } > case OPT_MATCH_ALLOCATIONS_NUM: > internal_config.match_allocations =3D 1; > break; > @@ -1238,6 +1267,7 @@ rte_eal_cleanup(void) > rte_memseg_walk(mark_freeable, NULL); > rte_service_finalize(); > rte_mp_channel_cleanup(); > + eal_cleanup_config(&internal_config); > return 0; > } >=20 > diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c > b/lib/librte_eal/linuxapp/eal/eal_memory.c > index 7d922a965..1b96b576e 100644 > --- a/lib/librte_eal/linuxapp/eal/eal_memory.c > +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c > @@ -438,7 +438,7 @@ find_numasocket(struct hugepage_file *hugepg_tbl, > struct hugepage_info *hpi) > } >=20 > snprintf(hugedir_str, sizeof(hugedir_str), > - "%s/%s", hpi->hugedir, > internal_config.hugefile_prefix); > + "%s/%s", hpi->hugedir, eal_get_hugefile_prefix()); >=20 > /* parse numa map */ > while (fgets(buf, sizeof(buf), f) !=3D NULL) { > -- > 2.11.0