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 BFFB0A04B5; Tue, 3 Dec 2019 16:12:38 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id E01982B8D; Tue, 3 Dec 2019 16:12:37 +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 366A3235 for ; Tue, 3 Dec 2019 16:12:36 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1575385955; 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=fGBlHi9C0R4icoR4TE9HrXv31bLZAaic9Pac2ypEEPI=; b=ca8NfVVn7i+YLwL88R4ktcvmpeAgnS899BGTIDUOyEgsrcMQkdM2LxPqWnG8C8JIEaswda 17OTkRjUQyfbLCoioBpYGWMHWW6++idjrv5nn3bylOtcCQCr+tsz3BRYfJkw3nSlaSI3WJ wk+HOKEpVjW6HcExg0UBz7sNhEEKrGk= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-160-qJccEyHePCW-UFxYBrbqHQ-1; Tue, 03 Dec 2019 10:10:13 -0500 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 82EB4800D4E; Tue, 3 Dec 2019 15:10:12 +0000 (UTC) Received: from dhcp-25.97.bos.redhat.com (ovpn-122-146.rdu2.redhat.com [10.10.122.146]) by smtp.corp.redhat.com (Postfix) with ESMTPS id CAEF85C3FD; Tue, 3 Dec 2019 15:10:06 +0000 (UTC) From: Aaron Conole To: David Marchand Cc: dev , Harry van Haaren , Bruce Richardson , Pavan Nikhilesh , Gage Eads , Thomas Monjalon References: <20191126145606.13626-1-aconole@redhat.com> Date: Tue, 03 Dec 2019 10:10:05 -0500 In-Reply-To: (David Marchand's message of "Mon, 2 Dec 2019 17:19:12 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-MC-Unique: qJccEyHePCW-UFxYBrbqHQ-1 X-Mimecast-Spam-Score: 0 Content-Type: text/plain 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" David Marchand writes: > 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_= spec_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. Okay. I will document that then ;) > >> 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_LCOR= E); >> 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_un= safe) >> { >> - /* run service on calling core, using all-ones as the service ma= sk */ >> - 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 exam= ine 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, uint3= 2_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 flo= w */ > > if (!service_valid(idx)) > continue; > > Plus, if we add this check here, thenall loops in this file are consisten= t. > WDYT? Agreed - it's better. Okay.