Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Message-ID: <f18fdc337f58317b85acb45a0ac694f9140fc022.camel@redhat.com>
Subject: Re: [net-next PATCH v6 1/2] octeontx2-af: Add new mbox to support
 multicast/mirror offload
From:   Paolo Abeni <pabeni@redhat.com>
To:     Suman Ghosh <sumang@marvell.com>, sgoutham@marvell.com,
        gakula@marvell.com, sbhatta@marvell.com, hkelam@marvell.com,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        lcherian@marvell.com, jerinj@marvell.com, horms@kernel.org,
        wojciech.drewek@intel.com
Date:   Wed, 06 Dec 2023 11:58:32 +0100
In-Reply-To: <20231204141956.3956942-2-sumang@marvell.com>
References: <20231204141956.3956942-1-sumang@marvell.com>
         <20231204141956.3956942-2-sumang@marvell.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Mon, 2023-12-04 at 19:49 +0530, Suman Ghosh wrote:
> A new mailbox is added to support offloading of multicast/mirror
> functionality. The mailbox also supports dynamic updation of the
> multicast/mirror list.
>=20
> Signed-off-by: Suman Ghosh <sumang@marvell.com>
> Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
> Reviewed-by: Simon Horman <horms@kernel.org>

Note that v5 was already applied to net-next. But I still have a
relevant note, see below.

> @@ -5797,3 +6127,337 @@ int rvu_mbox_handler_nix_bandprof_get_hwinfo(stru=
ct rvu *rvu, struct msg_req *re
> =20
>  	return 0;
>  }
> +
> +static struct nix_mcast_grp_elem *rvu_nix_mcast_find_grp_elem(struct nix=
_mcast_grp *mcast_grp,
> +							      u32 mcast_grp_idx)
> +{
> +	struct nix_mcast_grp_elem *iter;
> +	bool is_found =3D false;
> +
> +	mutex_lock(&mcast_grp->mcast_grp_lock);
> +	list_for_each_entry(iter, &mcast_grp->mcast_grp_head, list) {
> +		if (iter->mcast_grp_idx =3D=3D mcast_grp_idx) {
> +			is_found =3D true;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&mcast_grp->mcast_grp_lock);

AFAICS, at this point another thread/CPU could kick-in and run
rvu_mbox_handler_nix_mcast_grp_destroy() up to completion, freeing
'iter' before it's later used by the current thread.

What prevents such scenario?

_If_ every mcast group manipulation happens under the rtnl lock, then
you could as well completely remove the confusing mcast_grp_lock.

Cheers,

Paolo