double free of map set

double free of map set

[Bob Pearson]

Somehow I lost the email so you could resend it that would help.

Thanks. Good catch. I think there is a cleaner and simpler way to fix this. All the errors occur when there is a failure in setting up new MRs which first try to delete the already allocated memory and then drop the object which cleans it up again. It would be simpler to just return an error and not call rxe_mr_free_map_set(). Then the error will lead to rxe_mr_cleanup() which will delete the memory just once.

Bob

Re: double free of map set

[lizhijian]

On 25/12/2021 03:29, Bob Pearson wrote:
> Somehow I lost the email so you could resend it that would help.
>
> Thanks. Good catch. I think there is a cleaner and simpler way to fix this. All the errors occur when there is a failure in setting up new MRs which first try to delete the already allocated memory and then drop the object which cleans it up again.

> It would be simpler to just return an error and not call rxe_mr_free_map_set(). Then the error will lead to rxe_mr_cleanup() which will delete the memory just once.
Not sure i have got you.
i think it's not a convention style that we don't release/cleanup allocated resources  in error path in a same function.
rxe_mr_alloc() will release the map set itself in error path, where it also doesn't set map_set to NULL. that may also
cause a double free.

I wonder if you suggested that:
:~/lkp/linux$ git diff
diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
index 53271df10e47..a08b4718feec 100644
--- a/drivers/infiniband/sw/rxe/rxe_mr.c
+++ b/drivers/infiniband/sw/rxe/rxe_mr.c
@@ -140,7 +140,6 @@ static int rxe_mr_alloc(struct rxe_mr *mr, int num_buf, int both)
         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;
                 }
         }


Thanks
Zhijian

>
> Bob

Re: double free of map set

[lizhijian]

Hi Bob

I just post a V2 by following your suggestions. PTAL.


Thanks
Zhijian


On 27/12/2021 10:53, Li Zhijian wrote:
>
>
> On 25/12/2021 03:29, Bob Pearson wrote:
>> Somehow I lost the email so you could resend it that would help.
>>
>> Thanks. Good catch. I think there is a cleaner and simpler way to fix this. All the errors occur when there is a failure in setting up new MRs which first try to delete the already allocated memory and then drop the object which cleans it up again.
>
>> It would be simpler to just return an error and not call rxe_mr_free_map_set(). Then the error will lead to rxe_mr_cleanup() which will delete the memory just once.
> Not sure i have got you.
> i think it's not a convention style that we don't release/cleanup allocated resources  in error path in a same function.
> rxe_mr_alloc() will release the map set itself in error path, where it also doesn't set map_set to NULL. that may also
> cause a double free.
>
> I wonder if you suggested that:
> :~/lkp/linux$ git diff
> diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
> index 53271df10e47..a08b4718feec 100644
> --- a/drivers/infiniband/sw/rxe/rxe_mr.c
> +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
> @@ -140,7 +140,6 @@ static int rxe_mr_alloc(struct rxe_mr *mr, int num_buf, int both)
>         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;
>                 }
>         }
>
>
> Thanks
> Zhijian
>
>>
>> Bob
>