lkml.org 
[lkml]   [2023]   [Dec]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@marvell.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
>

\
 
 \ /
  Last update: 2023-12-12 12:16    [W:0.052 / U:0.924 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site