Re: [net-next PATCH] octeontx2-af: Fix multicast/mirror group lock/unlock issue

[Simon Horman]

On Tue, Dec 12, 2023 at 02:45:58PM +0530, Suman Ghosh wrote:
> As per the existing implementation, there exists a race between finding
> a multicast/mirror group entry and deleting that entry. The group lock
> was taken and released independently by rvu_nix_mcast_find_grp_elem()
> function. Which is incorrect and group lock should be taken during the
> entire operation of group updation/deletion. This patch fixes the same.
> 
> Fixes: 51b2804c19cd ("octeontx2-af: Add new mbox to support multicast/mirror offload")
> Signed-off-by: Suman Ghosh <sumang@xxxxxxxxxxx>

...

> @@ -6306,6 +6310,13 @@ int rvu_mbox_handler_nix_mcast_grp_destroy(struct rvu *rvu,
>  		return err;
>  
>  	mcast_grp = &nix_hw->mcast_grp;
> +
> +	/* If AF is requesting for the deletion,
> +	 * then AF is already taking the lock
> +	 */
> +	if (!req->is_af)
> +		mutex_lock(&mcast_grp->mcast_grp_lock);
> +
>  	elem = rvu_nix_mcast_find_grp_elem(mcast_grp, req->mcast_grp_idx);
>  	if (!elem)

Hi Suman,

Does mcast_grp_lock need to be released here?
If so, I would suggest a goto label, say unlock_grp.

>  		return NIX_AF_ERR_INVALID_MCAST_GRP;
> @@ -6333,12 +6344,6 @@ int rvu_mbox_handler_nix_mcast_grp_destroy(struct rvu *rvu,
>  	mutex_unlock(&mcast->mce_lock);
>  
>  delete_grp:
> -	/* If AF is requesting for the deletion,
> -	 * then AF is already taking the lock
> -	 */
> -	if (!req->is_af)
> -		mutex_lock(&mcast_grp->mcast_grp_lock);
> -
>  	list_del(&elem->list);
>  	kfree(elem);
>  	mcast_grp->count--;
> @@ -6370,9 +6375,20 @@ int rvu_mbox_handler_nix_mcast_grp_update(struct rvu *rvu,
>  		return err;
>  
>  	mcast_grp = &nix_hw->mcast_grp;
> +
> +	/* If AF is requesting for the updation,
> +	 * then AF is already taking the lock
> +	 */
> +	if (!req->is_af)
> +		mutex_lock(&mcast_grp->mcast_grp_lock);
> +
>  	elem = rvu_nix_mcast_find_grp_elem(mcast_grp, req->mcast_grp_idx);
> -	if (!elem)
> +	if (!elem) {
> +		if (!req->is_af)
> +			mutex_unlock(&mcast_grp->mcast_grp_lock);
> +
>  		return NIX_AF_ERR_INVALID_MCAST_GRP;
> +	}
>  
>  	/* If any pcifunc matches the group's pcifunc, then we can
>  	 * delete the entire group.
> @@ -6383,8 +6399,11 @@ int rvu_mbox_handler_nix_mcast_grp_update(struct rvu *rvu,
>  				/* Delete group */
>  				dreq.hdr.pcifunc = elem->pcifunc;
>  				dreq.mcast_grp_idx = elem->mcast_grp_idx;
> -				dreq.is_af = req->is_af;
> +				dreq.is_af = 1;
>  				rvu_mbox_handler_nix_mcast_grp_destroy(rvu, &dreq, NULL);
> +				if (!req->is_af)
> +					mutex_unlock(&mcast_grp->mcast_grp_lock);
> +
>  				return 0;
>  			}
>  		}
> @@ -6467,5 +6486,8 @@ int rvu_mbox_handler_nix_mcast_grp_update(struct rvu *rvu,
>  
>  done:

I think it would be good to rename this label, say unlock_mce;

>  	mutex_unlock(&mcast->mce_lock);

Add a new label here, say unlock_grp;
And jump to this label whenever there is a need for the mutex_unlock() below.

> +	if (!req->is_af)
> +		mutex_unlock(&mcast_grp->mcast_grp_lock);
> +
>  	return ret;
>  }
> -- 
> 2.25.1
>