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@xxxxxxxxx>
> Signed-off-by: Bob Pearson <rpearsonhpe@xxxxxxxxx>
> 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
