| Message ID | 20211228014406.1033444-1-lizhijian@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [v2] RDMA/rxe: Prevent from double freeing rxe_map_set | expand |
On Tue, Dec 28, 2021 at 9:39 AM Li Zhijian <lizhijian@cn.fujitsu.com> wrote: > > a same rxe_map_set could be freed twice: > rxe_reg_user_mr() > -> rxe_mr_init_user() > -> rxe_mr_free_map_set() # 1st > -> rxe_drop_ref() > ... > -> rxe_mr_cleanup() > -> rxe_mr_free_map_set() # 2nd > > By convention, we should cleanup/free resources in the error path in the > same function where the resources are alloted in. So rxe_mr_init_user() > doesn't need to free the map_set directly. > > In addition, we have to reset map_set to NULL inside rxe_mr_alloc() if needed > to prevent from map_set being double freed in rxe_mr_cleanup(). > > CC: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com> Thanks. Acked-by: Zhu Yanjun <zyjzyj2000@gmail.com> Zhu Yanjun > --- > V2: Fix it by a simpler way by following suggestion from Bob, > --- > drivers/infiniband/sw/rxe/rxe_mr.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c > index 25c78aade822..7c4cd19a9db2 100644 > --- a/drivers/infiniband/sw/rxe/rxe_mr.c > +++ b/drivers/infiniband/sw/rxe/rxe_mr.c > @@ -141,6 +141,7 @@ static int rxe_mr_alloc(struct rxe_mr *mr, int num_buf, int both) > ret = rxe_mr_alloc_map_set(num_map, &mr->next_map_set); > if (ret) { > rxe_mr_free_map_set(mr->num_map, mr->cur_map_set); > + mr->cur_map_set = NULL; > goto err_out; > } > } > @@ -214,7 +215,7 @@ int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova, > pr_warn("%s: Unable to get virtual address\n", > __func__); > err = -ENOMEM; > - goto err_cleanup_map; > + goto err_release_umem; > } > > buf->addr = (uintptr_t)vaddr; > @@ -237,8 +238,6 @@ int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova, > > return 0; > > -err_cleanup_map: > - rxe_mr_free_map_set(mr->num_map, mr->cur_map_set); > err_release_umem: > ib_umem_release(umem); > err_out: > -- > 2.31.1 > > >
On Tue, Dec 28, 2021 at 09:44:06AM +0800, Li Zhijian wrote: > a same rxe_map_set could be freed twice: > rxe_reg_user_mr() > -> rxe_mr_init_user() > -> rxe_mr_free_map_set() # 1st > -> rxe_drop_ref() > ... > -> rxe_mr_cleanup() > -> rxe_mr_free_map_set() # 2nd > > By convention, we should cleanup/free resources in the error path in the > same function where the resources are alloted in. So rxe_mr_init_user() > doesn't need to free the map_set directly. > > In addition, we have to reset map_set to NULL inside rxe_mr_alloc() if needed > to prevent from map_set being double freed in rxe_mr_cleanup(). > > CC: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com> > Acked-by: Zhu Yanjun <zyjzyj2000@gmail.com> > --- > V2: Fix it by a simpler way by following suggestion from Bob, > --- > drivers/infiniband/sw/rxe/rxe_mr.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) Applied to for-rc, but I changed it to keep the goto unwind as is normal. @@ -135,19 +135,19 @@ static int rxe_mr_alloc(struct rxe_mr *mr, int num_buf, int both) ret = rxe_mr_alloc_map_set(num_map, &mr->cur_map_set); if (ret) - goto err_out; + return -ENOMEM; if (both) { ret = rxe_mr_alloc_map_set(num_map, &mr->next_map_set); - if (ret) { - rxe_mr_free_map_set(mr->num_map, mr->cur_map_set); - goto err_out; - } + if (ret) + goto err_free; } return 0; -err_out: +err_free: + rxe_mr_free_map_set(mr->num_map, mr->cur_map_set); + mr->cur_map_set = NULL; return -ENOMEM; }
diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c index 25c78aade822..7c4cd19a9db2 100644 --- a/drivers/infiniband/sw/rxe/rxe_mr.c +++ b/drivers/infiniband/sw/rxe/rxe_mr.c @@ -141,6 +141,7 @@ static int rxe_mr_alloc(struct rxe_mr *mr, int num_buf, int both) ret = rxe_mr_alloc_map_set(num_map, &mr->next_map_set); if (ret) { rxe_mr_free_map_set(mr->num_map, mr->cur_map_set); + mr->cur_map_set = NULL; goto err_out; } } @@ -214,7 +215,7 @@ int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova, pr_warn("%s: Unable to get virtual address\n", __func__); err = -ENOMEM; - goto err_cleanup_map; + goto err_release_umem; } buf->addr = (uintptr_t)vaddr; @@ -237,8 +238,6 @@ int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova, return 0; -err_cleanup_map: - rxe_mr_free_map_set(mr->num_map, mr->cur_map_set); err_release_umem: ib_umem_release(umem); err_out:
a same rxe_map_set could be freed twice: rxe_reg_user_mr() -> rxe_mr_init_user() -> rxe_mr_free_map_set() # 1st -> rxe_drop_ref() ... -> rxe_mr_cleanup() -> rxe_mr_free_map_set() # 2nd By convention, we should cleanup/free resources in the error path in the same function where the resources are alloted in. So rxe_mr_init_user() doesn't need to free the map_set directly. In addition, we have to reset map_set to NULL inside rxe_mr_alloc() if needed to prevent from map_set being double freed in rxe_mr_cleanup(). CC: Jason Gunthorpe <jgg@nvidia.com> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com> --- V2: Fix it by a simpler way by following suggestion from Bob, --- drivers/infiniband/sw/rxe/rxe_mr.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)