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 2640B42A57 for ; Thu, 4 May 2023 00:32:08 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1F59242C76; Thu, 4 May 2023 00:32:08 +0200 (CEST) Received: from forward502b.mail.yandex.net (forward502b.mail.yandex.net [178.154.239.146]) by mails.dpdk.org (Postfix) with ESMTP id 17C55400EF; Thu, 4 May 2023 00:32:06 +0200 (CEST) Received: from mail-nwsmtp-smtp-production-main-24.iva.yp-c.yandex.net (mail-nwsmtp-smtp-production-main-24.iva.yp-c.yandex.net [IPv6:2a02:6b8:c0c:779c:0:640:7d74:0]) by forward502b.mail.yandex.net (Yandex) with ESMTP id 5301A5F01A; Thu, 4 May 2023 01:32:05 +0300 (MSK) Received: by mail-nwsmtp-smtp-production-main-24.iva.yp-c.yandex.net (smtp/Yandex) with ESMTPSA id 2WXqnP8DfW20-6K1sqYYP; Thu, 04 May 2023 01:32:04 +0300 X-Yandex-Fwd: 1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yandex.ru; s=mail; t=1683153124; bh=Op1JzDyRQRuHVr7f/rUXWqgeDYZgrvO303VHR0i5bgA=; h=From:In-Reply-To:Cc:Date:References:To:Subject:Message-ID; b=VcBv9W294rhhQMi4o9UxswGAl3LEMTSwomfKOaJ+6nO8VRz25eF0YApMExxkXqjO/ gldEP/reIQpwKh90p3aMK6Y0wYhC1HdLQXsR+leKLB78jk9z5kzjQgnQez0NllKonP gwHQv0LJ+SWmH9RpkRaXZf++eCUKdBKUuPMQcoS4= Authentication-Results: mail-nwsmtp-smtp-production-main-24.iva.yp-c.yandex.net; dkim=pass header.i=@yandex.ru Message-ID: Date: Wed, 3 May 2023 23:32:02 +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> <6215f9c3-db40-a0c1-edc6-d859f2920609@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 03/05/2023 06:44, Honnappa Nagarahalli пишет: > > >>>> >>>> >>>> >>>>> 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 wrote 'both of our suggestions'. Essentially, in v1, freeing the memzone was outside of the lock. I suggested to move it inside and you are suggesting to move it inside. Ah ok, I missed v1 and your comments for it. As I said before, I don't think that we need to hold qlock here while calling mmezone_free(). Though I don't see any harm with it either. I'd personally would move memzone_free() after releasing qlock, but if you guys prefer to keep it as it is - I wouldn't insist. > >> 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 >>>> >>> >