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

[Jason Gunthorpe <jgg@nvidia.com>]

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.

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