> -----Original Message----- > From: Honnappa Nagarahalli [mailto:honnappa.nagaraha...@arm.com] > Sent: Thursday, May 4, 2023 7:46 AM > To: Konstantin Ananyev <konstantin.v.anan...@yandex.ru>; wangyunjian > <wangyunj...@huawei.com> > Cc: dev@dpdk.org; luyicai <luyi...@huawei.com>; sta...@dpdk.org; nd > <n...@arm.com>; nd <n...@arm.com> > Subject: RE: [dpdk-dev] [PATCH v2] ring: fix use after free in ring release > > <snip> > > > > > > >>>> > > >>>> > > >>>> > > >>>>> 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: sta...@dpdk.org > > >>>>> > > >>>>> Suggested-by: Honnappa Nagarahalli > > <honnappa.nagaraha...@arm.com> > > >>>>> Signed-off-by: Yunjian Wang <wangyunj...@huawei.com> > > >>>>> --- > > >>>>> 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 looked at other libraries, stack library is the closest. Stack library > frees the > memzone outside the lock. I think we should keep it consistent. > I am fine to move the free outside the lock.
Thanks, Konstantin Ananyev and Honnappa Nagarahalli. I will update the patch in v3 and move the free outside the lock. > > > > > > > > >> 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 <konstantin.v.anan...@yandex.ru> > > >>>> > > >>>>> rte_mcfg_tailq_write_unlock(); > > >>>>> > > >>>>> rte_free(te); > > >>>>> -- > > >>>>> 2.33.0 > > >>>> > > >>> > > >
