Re: [RFC PATCH v9 07/26] RDMA/rxe: Use kzmalloc/kfree for mca

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Jan 27, 2022 at 03:37:36PM -0600, Bob Pearson wrote:
> diff --git a/drivers/infiniband/sw/rxe/rxe_mcast.c b/drivers/infiniband/sw/rxe/rxe_mcast.c
> index 9336295c4ee2..39f38ee665f2 100644
> +++ b/drivers/infiniband/sw/rxe/rxe_mcast.c
> @@ -36,6 +36,7 @@ static struct rxe_mcg *create_grp(struct rxe_dev *rxe,
>  	grp = rxe_alloc_locked(&rxe->mc_grp_pool);
>  	if (!grp)
>  		return ERR_PTR(-ENOMEM);
> +	rxe_add_ref(grp);

I have no idea what this ref is for, the grp already has a ref of 1.

You should put the ref incrs near the place that makes a copy of the
pointer. Every pointer should have a ref.

When rxe_alloc_locked() returns the ref is 1 and this ref logically
belongs to the caller

When the caller does rxe_add_key_locked() then the ref is moved into
the rbtree and is now owned by the rbtree

When the caller does rxe_drop_key_locked() then the ref is moved out
of the rbtree and is now owned again by the caller.

After this patch this is leaking the memory on the error unwind:

	err = rxe_mcast_add(rxe, mgid);
	if (unlikely(err)) {
		rxe_drop_key_locked(grp);
		rxe_drop_ref(grp);
		return ERR_PTR(err);
	}

>  	INIT_LIST_HEAD(&grp->qp_list);
>  	spin_lock_init(&grp->mcg_lock);
> @@ -85,12 +86,28 @@ static int rxe_mcast_add_grp_elem(struct rxe_dev *rxe, struct rxe_qp *qp,
>  			   struct rxe_mcg *grp)
>  {
>  	int err;
> -	struct rxe_mca *elem;
> +	struct rxe_mca *mca, *new_mca;
>  
> -	/* check to see of the qp is already a member of the group */
> +	/* check to see if the qp is already a member of the group */
>  	spin_lock_bh(&grp->mcg_lock);
> -	list_for_each_entry(elem, &grp->qp_list, qp_list) {
> -		if (elem->qp == qp) {
> +	list_for_each_entry(mca, &grp->qp_list, qp_list) {
> +		if (mca->qp == qp) {
> +			spin_unlock_bh(&grp->mcg_lock);
> +			return 0;
> +		}
> +	}
> +	spin_unlock_bh(&grp->mcg_lock);

This would all be much simpler and faster to change it so the qp has
the list head that stores the list of groups it is joined to.

This code never seems to need to go from a group back to the list of
qps.

Jason