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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 12 Dec 2023 11:16:01 +0000
From: Simon Horman <horms@...nel.org>
To: Suman Ghosh <sumang@...vell.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, sgoutham@...vell.com, sbhatta@...vell.com,
	jerinj@...vell.com, gakula@...vell.com, hkelam@...vell.com,
	lcherian@...vell.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [net-next PATCH] octeontx2-af: Fix multicast/mirror group
 lock/unlock issue

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@...vell.com>

...

> @@ -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
>