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 651C042A42 for ; Wed, 3 May 2023 01:06:28 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 582E042D13; Wed, 3 May 2023 01:06:28 +0200 (CEST) Received: from forward502c.mail.yandex.net (forward502c.mail.yandex.net [178.154.239.210]) by mails.dpdk.org (Postfix) with ESMTP id 8057640DDC; Wed, 3 May 2023 01:06:25 +0200 (CEST) Received: from mail-nwsmtp-smtp-production-main-29.myt.yp-c.yandex.net (mail-nwsmtp-smtp-production-main-29.myt.yp-c.yandex.net [IPv6:2a02:6b8:c12:4e8f:0:640:efc:0]) by forward502c.mail.yandex.net (Yandex) with ESMTP id B12065EDA1; Wed, 3 May 2023 02:06:24 +0300 (MSK) Received: by mail-nwsmtp-smtp-production-main-29.myt.yp-c.yandex.net (smtp/Yandex) with ESMTPSA id L6XWwRxBWGk0-rcwQdlf2; Wed, 03 May 2023 02:06:24 +0300 X-Yandex-Fwd: 1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yandex.ru; s=mail; t=1683068784; bh=3fuGbHTqcP3RPctHPikA/OkG6QNwuzZ5CglGW9IaKKI=; h=From:In-Reply-To:Cc:Date:References:To:Subject:Message-ID; b=MC0TQCzKdxOss/0tQWgftJFltjq1REUfjicccmm3d13ckPv5iouVItahJtCfEtTtx tnZhBd1lbd2QOfBxwoxOZtlfOrXTi6w7dcH99cetra0M93FQ4XTrLiTU6K31FFEFtu ReBjdgnWuVzVEM8yEPJJzPCgPbeOLsENimfL2woY= Authentication-Results: mail-nwsmtp-smtp-production-main-29.myt.yp-c.yandex.net; dkim=pass header.i=@yandex.ru Message-ID: <6215f9c3-db40-a0c1-edc6-d859f2920609@yandex.ru> Date: Wed, 3 May 2023 00:06:21 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0 Subject: Re: [dpdk-dev] [PATCH v2] ring: fix use after free in ring release Content-Language: en-US To: Honnappa Nagarahalli , "wangyunjian@huawei.com" Cc: "dev@dpdk.org" , "luyicai@huawei.com" , "stable@dpdk.org" , nd References: <1e024be7-14a7-1997-43a2-2d2571fc984d@yandex.ru> From: Konstantin Ananyev In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org 01/05/2023 20:06, Honnappa Nagarahalli пишет: > > >> -----Original Message----- >> From: Konstantin Ananyev >> Sent: Monday, May 1, 2023 7:32 AM >> To: wangyunjian@huawei.com >> Cc: dev@dpdk.org; Honnappa Nagarahalli >> ; konstantin.v.ananyev@yandex.ru; >> luyicai@huawei.com; stable@dpdk.org >> Subject: Re: [dpdk-dev] [PATCH v2] ring: fix use after free in ring release >> >> >> >>> After the memzone is freed, it is not removed from the 'rte_ring_tailq'. >>> If rte_ring_lookup is called at this time, it will cause a >>> use-after-free problem. This change prevents that from happening. >>> >>> Fixes: 4e32101f9b01 ("ring: support freeing") >>> Cc: stable@dpdk.org >>> >>> Suggested-by: Honnappa Nagarahalli >>> Signed-off-by: Yunjian Wang >>> --- >>> v2: update code suggested by Honnappa Nagarahalli >>> --- >>> lib/ring/rte_ring.c | 8 +++----- >>> 1 file changed, 3 insertions(+), 5 deletions(-) >>> >>> diff --git a/lib/ring/rte_ring.c b/lib/ring/rte_ring.c index >>> 8ed455043d..2755323b8a 100644 >>> --- a/lib/ring/rte_ring.c >>> +++ b/lib/ring/rte_ring.c >>> @@ -333,11 +333,6 @@ rte_ring_free(struct rte_ring *r) >>> return; >>> } >>> >>> - if (rte_memzone_free(r->memzone) != 0) { >>> - RTE_LOG(ERR, RING, "Cannot free memory\n"); >>> - return; >>> - } >>> - >>> ring_list = RTE_TAILQ_CAST(rte_ring_tailq.head, rte_ring_list); >>> rte_mcfg_tailq_write_lock(); >>> >>> @@ -354,6 +349,9 @@ rte_ring_free(struct rte_ring *r) >>> >>> TAILQ_REMOVE(ring_list, te, next); >>> >>> + if (rte_memzone_free(r->memzone) != 0) >>> + RTE_LOG(ERR, RING, "Cannot free memory\n"); >>> + >> >> I nit: I think it is a bit better to first release the lock and then free the >> memzone. > I think both of our suggestions are contradictory. Any reason why you want to free outside the locked region? Don't know what you mean by 'both suggestions' here. I think I gave only one - move memzone_free() after tailq_write_unlock(). To be more precise: 1) rte_mcfg_tailq_write_lock(); ... 2) TAILQ_REMOVE(...); 3) rte_mcfg_tailq_write_unlock(); 4) rte_memzone_free(r->memzone); As I remember, memzones are protected by their own lock (mlock), so we don't need to hold qlock to free a memzone, after ring was already removed from the ring_list. > > I thought, since it belongs to the ring being freed, it makes sense to free it while holding the lock to avoid any race conditions (though, I have not checked what those are). As I understand, it is ok with current design to grab mlock while holding qlock. So, there is nothing wrong with current patch, I just think that in that case it is excessive, and can be safely avoided. > >> Apart from that, LGTM. >> Acked-by: Konstantin Ananyev >> >>> rte_mcfg_tailq_write_unlock(); >>> >>> rte_free(te); >>> -- >>> 2.33.0 >> >