Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Message-ID: <853a21e6479b44b10b8f6c9874124c82c13bed3c.camel@redhat.com>
Subject: Re: [EXT] 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>,
        Sunil Kovvuri Goutham <sgoutham@marvell.com>,
        Geethasowjanya Akula <gakula@marvell.com>,
        Subbaraya Sundeep Bhatta <sbhatta@marvell.com>,
        Hariprasad Kelam <hkelam@marvell.com>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "edumazet@google.com" <edumazet@google.com>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Linu Cherian <lcherian@marvell.com>,
        Jerin Jacob Kollanukkaran <jerinj@marvell.com>,
        "horms@kernel.org" <horms@kernel.org>,
        "wojciech.drewek@intel.com" <wojciech.drewek@intel.com>
Date:   Wed, 06 Dec 2023 21:14:18 +0100
In-Reply-To: <SJ0PR18MB5216F4E57D57AD4DF5013D15DB84A@SJ0PR18MB5216.namprd18.prod.outlook.com>
References: <20231204141956.3956942-1-sumang@marvell.com>
         <20231204141956.3956942-2-sumang@marvell.com>
         <f18fdc337f58317b85acb45a0ac694f9140fc022.camel@redhat.com>
         <SJ0PR18MB5216F4E57D57AD4DF5013D15DB84A@SJ0PR18MB5216.namprd18.prod.outlook.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 Wed, 2023-12-06 at 16:33 +0000, Suman Ghosh wrote:
> > 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>
> >=20
> > Note that v5 was already applied to net-next. But I still have a
> > relevant note, see below.
> >=20
> > > @@ -5797,3 +6127,337 @@ int
> > > rvu_mbox_handler_nix_bandprof_get_hwinfo(struct rvu *rvu, struct
> > > msg_req *re
> > >=20
> > > =C2=A0	return 0;
> > > =C2=A0}
> > > +
> > > +static struct nix_mcast_grp_elem
> > > *rvu_nix_mcast_find_grp_elem(struct
> > nix_mcast_grp *mcast_grp,
> > > +							    =20
> > > 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);
> >=20
> > 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.
> >=20
> > What prevents such scenario?
> >=20
> > _If_ every mcast group manipulation happens under the rtnl lock,
> > then
> > you could as well completely remove the confusing mcast_grp_lock.
> >=20
> > Cheers,
> >=20
> > Paolo
> [Suman] I added this lock because, these requests can come from some
> user-space application also. In that case, application will send a
> mailbox to kernel toad/del
> Multicast nodes. But I got your point and there is indeed chances of
> race. Let me think through it and push a fix. So, what process should
> be followed here? Are you going to revert the change? Or I can push a
> separate fix on net tree?

You can push a follow-up fix.

We could end-up reverting the patch only if the fix will take too long
to land here, and the issue will start hitting people.

Cheers,

Paolo