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 <[email protected]>
---
Note: This is a follow up of
https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_netdev_net-2Dnext_c_51b2804c19cd&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=7si3Xn9Ly-Se1a655kvEPIYU0nQ9HPeN280sEUv5ROU&m=NjKPoTkYVlL5Dh4aSr3-dVo-AukiIperlvB0S4_Mqzkyl_VcYAAKrWhkGZE5Cx-p&s=AkBf0454Xm-0adqV0Os7ZE8peaCXtYyuNbCS5kit6Jk&e=
.../ethernet/marvell/octeontx2/af/rvu_nix.c | 58 +++++++++++++------
1 file changed, 40 insertions(+), 18 deletions(-)
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
index b01503acd520..0ab5626380c5 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
@@ -6142,14 +6142,12 @@ static struct nix_mcast_grp_elem *rvu_nix_mcast_find_grp_elem(struct nix_mcast_g
struct nix_mcast_grp_elem *iter;
bool is_found = false;
- mutex_lock(&mcast_grp->mcast_grp_lock);
list_for_each_entry(iter, &mcast_grp->mcast_grp_head, list) {
if (iter->mcast_grp_idx == mcast_grp_idx) {
is_found = true;
break;
}
}
- mutex_unlock(&mcast_grp->mcast_grp_lock);
if (is_found)
return iter;
@@ -6162,7 +6160,7 @@ int rvu_nix_mcast_get_mce_index(struct rvu *rvu, u16 pcifunc, u32 mcast_grp_idx)
struct nix_mcast_grp_elem *elem;
struct nix_mcast_grp *mcast_grp;
struct nix_hw *nix_hw;
- int blkaddr;
+ int blkaddr, ret;
blkaddr = rvu_get_blkaddr(rvu, BLKTYPE_NIX, pcifunc);
nix_hw = get_nix_hw(rvu->hw, blkaddr);
@@ -6170,11 +6168,15 @@ int rvu_nix_mcast_get_mce_index(struct rvu *rvu, u16 pcifunc, u32 mcast_grp_idx)
return NIX_AF_ERR_INVALID_NIXBLK;
mcast_grp = &nix_hw->mcast_grp;
+ mutex_lock(&mcast_grp->mcast_grp_lock);
elem = rvu_nix_mcast_find_grp_elem(mcast_grp, mcast_grp_idx);
if (!elem)
- return NIX_AF_ERR_INVALID_MCAST_GRP;
+ ret = NIX_AF_ERR_INVALID_MCAST_GRP;
+ else
+ ret = elem->mce_start_index;
- return elem->mce_start_index;
+ mutex_unlock(&mcast_grp->mcast_grp_lock);
+ return ret;
}
void rvu_nix_mcast_flr_free_entries(struct rvu *rvu, u16 pcifunc)
@@ -6238,7 +6240,7 @@ int rvu_nix_mcast_update_mcam_entry(struct rvu *rvu, u16 pcifunc,
struct nix_mcast_grp_elem *elem;
struct nix_mcast_grp *mcast_grp;
struct nix_hw *nix_hw;
- int blkaddr;
+ int blkaddr, ret = 0;
blkaddr = rvu_get_blkaddr(rvu, BLKTYPE_NIX, pcifunc);
nix_hw = get_nix_hw(rvu->hw, blkaddr);
@@ -6246,13 +6248,15 @@ int rvu_nix_mcast_update_mcam_entry(struct rvu *rvu, u16 pcifunc,
return NIX_AF_ERR_INVALID_NIXBLK;
mcast_grp = &nix_hw->mcast_grp;
+ mutex_lock(&mcast_grp->mcast_grp_lock);
elem = rvu_nix_mcast_find_grp_elem(mcast_grp, mcast_grp_idx);
if (!elem)
- return NIX_AF_ERR_INVALID_MCAST_GRP;
-
- elem->mcam_index = mcam_index;
+ ret = NIX_AF_ERR_INVALID_MCAST_GRP;
+ else
+ elem->mcam_index = mcam_index;
- return 0;
+ mutex_unlock(&mcast_grp->mcast_grp_lock);
+ return ret;
}
int rvu_mbox_handler_nix_mcast_grp_create(struct rvu *rvu,
@@ -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)
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:
mutex_unlock(&mcast->mce_lock);
+ if (!req->is_af)
+ mutex_unlock(&mcast_grp->mcast_grp_lock);
+
return ret;
}
--
2.25.1
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 <[email protected]>
...
> @@ -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
>
>
>> @@ -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.
[Suman] ack, will update in v2
>
>> 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;
[Suman] ack, will update in v2
>
>> 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.
[Suman] ack, will update in v2
>
>> + if (!req->is_af)
>> + mutex_unlock(&mcast_grp->mcast_grp_lock);
>> +
>> return ret;
>> }
>> --
>> 2.25.1
>>