Re: [PATCH for-next v6 8/8] RDMA/rxe: Add wait for completion to obj destruct

[Bob Pearson <rpearsonhpe@gmail.com>]

On 12/7/21 13:28, Jason Gunthorpe wrote:
> On Mon, Dec 06, 2021 at 03:12:43PM -0600, Bob Pearson wrote:
>> This patch adds code to wait until pending activity on RDMA objects has
>> completed before freeing or returning to rdma-core where the object may
>> be freed.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
>> v6
>>   Corrected incorrect comment before __rxe_fini()
>>   Added a #define for complete timeout value.
>>   Changed type of __rxe_fini to int to return value from wait_for_completion.
>>  drivers/infiniband/sw/rxe/rxe_comp.c  |  4 +-
>>  drivers/infiniband/sw/rxe/rxe_mcast.c |  4 ++
>>  drivers/infiniband/sw/rxe/rxe_mr.c    |  2 +
>>  drivers/infiniband/sw/rxe/rxe_mw.c    | 14 +++--
>>  drivers/infiniband/sw/rxe/rxe_pool.c  | 31 +++++++++-
>>  drivers/infiniband/sw/rxe/rxe_pool.h  |  4 ++
>>  drivers/infiniband/sw/rxe/rxe_recv.c  |  4 +-
>>  drivers/infiniband/sw/rxe/rxe_req.c   | 11 ++--
>>  drivers/infiniband/sw/rxe/rxe_resp.c  |  6 +-
>>  drivers/infiniband/sw/rxe/rxe_verbs.c | 84 ++++++++++++++++++++-------
>>  10 files changed, 126 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_comp.c b/drivers/infiniband/sw/rxe/rxe_comp.c
>> index f363fe3fa414..a2bb66f320fa 100644
>> +++ b/drivers/infiniband/sw/rxe/rxe_comp.c
>> @@ -562,7 +562,9 @@ int rxe_completer(void *arg)
>>  	enum comp_state state;
>>  	int ret = 0;
>>  
>> -	rxe_add_ref(qp);
>> +	/* check qp pointer still valid */
>> +	if (!rxe_add_ref(qp))
>> +		return -EAGAIN;
> 
> This doesn't make sense..
> 
> If this already has a pointer to QP then it must already have a ref
> and add_ref cannot fail.
Good point. My first reaction to using kref_get_unless_zero() which returns a
value was to go around and check all the values to make sure no calls failed.
But you are correct. All the main tasklets depend on qp still being around since
the tasklet struct is contained inside of qp. Further all the qp references have
to be freed and the ref from kref_init before qp is freed so this call is both
safe from failing and completely unnecessary in the first place.

Bob
> 
> The kref_get_unless_zero() is a special case for something like a
> container where it is possible for the container to hold a 0 ref item
> in it.
> 
> The scheme you have makes that impossible because the container lock
> is held around the kref == 0 and list_del, so you never need
> unless_zero.
> 
> The optimization is to not hold the lock around the kref_get, allow
> the container to have a 0 ref until the release reaches list_del, and
> then lock and list_del. The reader side then needs the unless_zero
> 
> But the ONLY place unless_zero should be used is in a situation like
> that, and we should never see other failable refcount tests without
> some other clear explanation why the qp pointer with a 0 ref is not
> freed. The above doesn't qualify.
> 
> Further 
> 
> +static inline bool __rxe_add_ref(struct rxe_pool_elem *elem)
> +{
> +       return kref_get_unless_zero(&elem->ref_cnt);
> +}
> 
> Just serves to defeat refcount debugging which checks that it is
> impossible for a 0 ref to be incr'd which is proof of a use after free
> when refcounts are implemetned properly.
> 
> So I don't know what this is all about here but it needs some fixing..
> 
> Jason
>