linux-kernel - Re: [PATCH v2 13/14] RDMA/rxe: Rename kfree_rcu() to kvfree_rcu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 15 Mar 2023 14:05:19 -0500
From:   Bob Pearson <rpearsonhpe@...il.com>
To:     Joel Fernandes <joel@...lfernandes.org>
Cc:     Zhu Yanjun <zyjzyj2000@...il.com>, Jason Gunthorpe <jgg@...pe.ca>,
        Leon Romanovsky <leon@...nel.org>,
        Devesh Sharma <devesh.s.sharma@...cle.com>,
        "Paul E . McKenney" <paulmck@...nel.org>,
        Zhu Yanjun <yanjun.zhu@...ux.dev>, linux-rdma@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 13/14] RDMA/rxe: Rename kfree_rcu() to
 kvfree_rcu_mightsleep()

On 3/15/23 13:41, Joel Fernandes wrote:
> On Wed, Mar 15, 2023 at 2:38 PM Bob Pearson <rpearsonhpe@...il.com> wrote:
>>
>> On 3/15/23 13:19, Joel Fernandes (Google) wrote:
>>> The k[v]free_rcu() macro's single-argument form is deprecated.
>>> Therefore switch to the new k[v]free_rcu_mightsleep() variant. The goal
>>> is to avoid accidental use of the single-argument forms, which can
>>> introduce functionality bugs in atomic contexts and latency bugs in
>>> non-atomic contexts.
>>>
>>> There is no functionality change with this patch.
>>>
>>> Link: https://lore.kernel.org/rcu/20230201150815.409582-1-urezki@gmail.com
>>> Acked-by: Zhu Yanjun <zyjzyj2000@...il.com>
>>> Reviewed-by: Bob Pearson <rpearsonhpe@...il.com>
>>> Reviewed-by: Paul E. McKenney <paulmck@...nel.org>
>>> Fixes: 72a03627443d ("RDMA/rxe: Remove rxe_alloc()")
>>> Signed-off-by: Joel Fernandes (Google) <joel@...lfernandes.org>
>>> ---
>>>  drivers/infiniband/sw/rxe/rxe_mr.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
>>> index b10aa1580a64..ae3a100e18fb 100644
>>> --- a/drivers/infiniband/sw/rxe/rxe_mr.c
>>> +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
>>> @@ -731,7 +731,7 @@ int rxe_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata)
>>>               return -EINVAL;
>>>
>>>       rxe_cleanup(mr);
>>> -     kfree_rcu(mr);
>>> +     kfree_rcu_mightsleep(mr);
>>>       return 0;
>>>  }
>>>
>> Please replace kfree_rcu_mightsleep() by kfree(). There is no need to delay the kfree(mr).
> 
> I thought you said either was Ok, but yeah that's fine with me. I was
> trying to play it safe ;-). An extra GP may not hurt, but not having
> one when it is needed might. I will update it to just be kfree.

Thanks. The reason to not add the pause is that it really isn't required and will just make
people think that it is. With the current state of the driver the mr cleanup code will disable
looking up the mr from its rkey or lkey and then wait until all the references to the mr are dropped
before calling kfree. This will always work (unless a new bug is introduced) so no reason to
add the rcu delay.

Bob
> 
> <quote>
>>   rxe_cleanup(mr);
>> - kfree_rcu(mr);
>> + kfree_rcu_mightsleep(mr);
>>   return 0;
>>  }
>>
> 
> I would prefer just
> - kfree_rcu(mr);
> + kfree(mr);
> 
> but either one will work.
> Bob
> </quote>
> 
>  - Joel