> -----Original Message----- > From: Honnappa Nagarahalli [mailto:honnappa.nagaraha...@arm.com] > Sent: Thursday, April 20, 2023 5:44 AM > To: wangyunjian <wangyunj...@huawei.com>; dev@dpdk.org > Cc: konstantin.v.anan...@yandex.ru; luyicai <luyi...@huawei.com>; > sta...@dpdk.org; nd <n...@arm.com>; nd <n...@arm.com> > Subject: RE: [dpdk-dev] [PATCH] ring: fix use after free in ring release > > <snip> > > > > > > > > -----Original Message----- > > > > From: Yunjian Wang <wangyunj...@huawei.com> > > > > Sent: Monday, April 17, 2023 8:12 AM > > > > To: dev@dpdk.org > > > > Cc: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>; > > > > konstantin.v.anan...@yandex.ru; luyi...@huawei.com; Yunjian Wang > > > > <wangyunj...@huawei.com>; sta...@dpdk.org > > > > Subject: [dpdk-dev] [PATCH] ring: fix use after free in ring > > > > release > > > > > > > > When using the ring to find out tailq entry, however it had been > > > > freed by rte_memzone_free function. This change prevents that from > > happening. > > > I am unable to follow the problem you are describing. > > > After the memzone for the ring is released, the contents of the > > > memzone are not being used. I understand that the variable 'r' is > > > being used, but that should not cause any issues. > > > > > > > > > > > Fixes: 4e32101f9b01 ("ring: support freeing") > > > > Cc: sta...@dpdk.org > > > > > > > > Signed-off-by: Yunjian Wang <wangyunj...@huawei.com> > > > > --- > > > > lib/ring/rte_ring.c | 11 +++++------ > > > > 1 file changed, 5 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/lib/ring/rte_ring.c b/lib/ring/rte_ring.c index > > > > 8ed455043d..17d2d7f8a8 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; > > > > - } > > > Why do we need to free the memzone later? > > > > 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. > Thanks, understood > > > > > Thanks, > > Yunjian > > > > > > > - > > > > ring_list = RTE_TAILQ_CAST(rte_ring_tailq.head, rte_ring_list); > > > > rte_mcfg_tailq_write_lock(); > > > > > > > > @@ -349,7 +344,7 @@ rte_ring_free(struct rte_ring *r) > > > > > > > > if (te == NULL) { > > > > rte_mcfg_tailq_write_unlock(); > > > > - return; > > > > + goto free_memzone; > We do not need this. If 'te == NULL' is true, then the ring was not found or > possibly already freed.
OK > > > > > } > > > > > > > > TAILQ_REMOVE(ring_list, te, next); @@ -357,6 +352,10 @@ > > > > rte_ring_free(struct rte_ring *r) > We should free the memzone here while holding the lock OK, You are right. I fix it on your suggestions. https://patchwork.dpdk.org/project/dpdk/patch/c23b1135e1b0676ef7d82969b39a21df992d418f.1681972694.git.wangyunj...@huawei.com/ Thanks, Yunjian > > > > > rte_mcfg_tailq_write_unlock(); > > > > > > > > rte_free(te); > > > > + > > > > +free_memzone: > > > > + if (rte_memzone_free(r->memzone) != 0) > > > > + RTE_LOG(ERR, RING, "Cannot free memory\n"); > > > > } > Should be moved up as mentioned above > > > > > > > > > /* dump the status of the ring on the console */ > > > > -- > > > > 2.33.0
