From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 8CCDBA04B5; Mon, 2 Dec 2019 17:19:29 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 6B5741BE82; Mon, 2 Dec 2019 17:19:29 +0100 (CET) Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.120]) by dpdk.org (Postfix) with ESMTP id 82BC61BE80 for ; Mon, 2 Dec 2019 17:19:28 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1575303568; 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: in-reply-to:in-reply-to:references:references; bh=87CZpXr5PM0O+eXxydapQbC3fejEnxWwyPmzpg+IVII=; b=JckaMydsV2rSq+xMGX5ezgu3Vp/3b6ikCgokx/Nydt6kPtU+sMQfI8+sDSLSwgCtw6vuN+ X7HB+js6V/+IxlI3//0oWA9RN+4TIwAFcMMr0/tbKP3w8Ugty9JK4kZT9jNsktgsNTqTag dwSn9xAxYa/RgNxtpYr0punZB2hXFao= Received: from mail-vs1-f69.google.com (mail-vs1-f69.google.com [209.85.217.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-214-fUeMnMkzMq6qIsyUX7SVKA-1; Mon, 02 Dec 2019 11:19:25 -0500 Received: by mail-vs1-f69.google.com with SMTP id e127so4641187vsc.16 for ; Mon, 02 Dec 2019 08:19:24 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=6WO4prY7d8IqL0kwxld5RAGoaJQSji9S0BhIBd2dEuE=; b=O4bRCfDOsxUlADNy3SvxjhHmI40Uw8VCyTAUZhSffrDcqO7/OaCjLvcCfFlPl/y0L/ 7ckL11cx1sdzmWK9yw6lOibv7ew47tPVqNTb/U7ZSb3PhzyzDMO7GVHy/KqAA6S0FGIs 1zmPjbfA7XFqkY+JeFhAHZ7HW/SnhhKsSytGGWJA105EJJFRNc4n9qNnB313BIpSeTyb Af6iydXvSOVWPOCfIgQwn74K8fWQw8/5XwGS7zwBI2mI97+zr3oW02bt7DwImImH0ap+ suiWAyl/5RtN4fO2xz+VIvDwrKIuQC4+2zy3gzuD3bjUbhsyTLl+Z1tDNa/fzZMfRB0w BKeA== X-Gm-Message-State: APjAAAV6rzg94qohbiA4W2471BQbDcFQWMZGSaDO00VGskK0OIkC2Jfp +E0+OVB2EF/p+zBY1h1Eo4ERLAaPT+xwJVcrWFMVOCuZ2FHEy3SefcYH+5+s4GsCm1uo8XQVIPT 59pA/lICtIpcnpShuYic= X-Received: by 2002:a67:f9cb:: with SMTP id c11mr9216296vsq.180.1575303564202; Mon, 02 Dec 2019 08:19:24 -0800 (PST) X-Google-Smtp-Source: APXvYqwvElnNH1PPO+y7M+qFR4v1PuptTFFKUqQ7DseG+acrXFZ+cH5VLtt2hZjdFloDEYb8mE3AfOCRT+uXJCsSTOY= X-Received: by 2002:a67:f9cb:: with SMTP id c11mr9216273vsq.180.1575303563734; Mon, 02 Dec 2019 08:19:23 -0800 (PST) MIME-Version: 1.0 References: <20191126145606.13626-1-aconole@redhat.com> In-Reply-To: <20191126145606.13626-1-aconole@redhat.com> From: David Marchand Date: Mon, 2 Dec 2019 17:19:12 +0100 Message-ID: To: Aaron Conole Cc: dev , Harry van Haaren , Bruce Richardson , Pavan Nikhilesh , Gage Eads , Thomas Monjalon X-MC-Unique: fUeMnMkzMq6qIsyUX7SVKA-1 X-Mimecast-Spam-Score: 0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [dpdk-dev] [PATCH] service: don't walk out of bounds when checking services 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Tue, Nov 26, 2019 at 3:56 PM Aaron Conole wrote: > > The service_valid call is used without properly bounds checking the > input parameter. Almost all instances of the service_valid call are > inside a for() loop that prevents excessive walks, but some of the > public APIs don't bounds check and will pass invalid arguments. > > Prevent this by using SERVICE_GET_OR_ERR_RET where it makes sense, > and adding a bounds check to one service_valid() use. > > Fixes: 8d39d3e237c2 ("service: fix race in service on app lcore function"= ) > Fixes: e9139a32f6e8 ("service: add function to run on app lcore") > Fixes: e30dd31847d2 ("service: add mechanism for quiescing") > Signed-off-by: Aaron Conole > --- > lib/librte_eal/common/rte_service.c | 23 +++++++++++------------ > 1 file changed, 11 insertions(+), 12 deletions(-) > > diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/= rte_service.c > index 79235c03f8..73de7bbade 100644 > --- a/lib/librte_eal/common/rte_service.c > +++ b/lib/librte_eal/common/rte_service.c > @@ -345,11 +345,12 @@ rte_service_runner_do_callback(struct rte_service_s= pec_impl *s, > > > static inline int32_t > -service_run(uint32_t i, struct core_state *cs, uint64_t service_mask) > +service_run(uint32_t i, struct core_state *cs, uint64_t service_mask, > + struct rte_service_spec_impl *s) > { > - if (!service_valid(i)) > - return -EINVAL; > - struct rte_service_spec_impl *s =3D &rte_services[i]; > + if (!s) > + SERVICE_VALID_GET_OR_ERR_RET(i, s, -EINVAL); > + No need to check the service if we ensure that the passed index is valid. See below. > if (s->comp_runstate !=3D RUNSTATE_RUNNING || > s->app_runstate !=3D RUNSTATE_RUNNING || > !(service_mask & (UINT64_C(1) << i))) { > @@ -383,7 +384,7 @@ rte_service_may_be_active(uint32_t id) > int32_t lcore_count =3D rte_service_lcore_list(ids, RTE_MAX_LCORE= ); > int i; > > - if (!service_valid(id)) > + if (id >=3D RTE_SERVICE_NUM_MAX || !service_valid(id)) > return -EINVAL; > > for (i =3D 0; i < lcore_count; i++) { > @@ -397,12 +398,10 @@ rte_service_may_be_active(uint32_t id) > int32_t > rte_service_run_iter_on_app_lcore(uint32_t id, uint32_t serialize_mt_uns= afe) > { > - /* run service on calling core, using all-ones as the service mas= k */ > - if (!service_valid(id)) > - return -EINVAL; > - > struct core_state *cs =3D &lcore_states[rte_lcore_id()]; > - struct rte_service_spec_impl *s =3D &rte_services[id]; > + struct rte_service_spec_impl *s; > + > + SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL); > > /* Atomically add this core to the mapped cores first, then exami= ne if > * we can run the service. This avoids a race condition between > @@ -418,7 +417,7 @@ rte_service_run_iter_on_app_lcore(uint32_t id, uint32= _t serialize_mt_unsafe) > return -EBUSY; > } > > - int ret =3D service_run(id, cs, UINT64_MAX); > + int ret =3D service_run(id, cs, UINT64_MAX, s); > > if (serialize_mt_unsafe) > rte_atomic32_dec(&s->num_mapped_cores); > @@ -439,7 +438,7 @@ rte_service_runner_func(void *arg) > > for (i =3D 0; i < RTE_SERVICE_NUM_MAX; i++) { > /* return value ignored as no change to code flow= */ if (!service_valid(idx)) continue; Plus, if we add this check here, thenall loops in this file are consistent. WDYT? --=20 David Marchand