RE: [PATCH for-rc 1/4] IB/hfi1: Call xa_destroy before freeing dummy_netdev

["Wan, Kaike" <kaike.wan@intel.com>]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Thursday, April 01, 2021 8:33 AM
> To: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
> Cc: dledford@redhat.com; linux-rdma@vger.kernel.org; Wan, Kaike
> <kaike.wan@intel.com>; stable@vger.kernel.org
> Subject: Re: [PATCH for-rc 1/4] IB/hfi1: Call xa_destroy before freeing
> dummy_netdev
> 
> On Wed, Mar 31, 2021 at 03:36:14PM -0400, Dennis Dalessandro wrote:
> > On 3/29/2021 10:09 AM, Jason Gunthorpe wrote:
> > > On Mon, Mar 29, 2021 at 09:48:17AM -0400,
> dennis.dalessandro@cornelisnetworks.com wrote:
> > >
> > > > diff --git a/drivers/infiniband/hw/hfi1/netdev_rx.c
> > > > b/drivers/infiniband/hw/hfi1/netdev_rx.c
> > > > index 2c8bc02..cec02e8 100644
> > > > +++ b/drivers/infiniband/hw/hfi1/netdev_rx.c
> > > > @@ -372,7 +372,11 @@ int hfi1_netdev_alloc(struct hfi1_devdata *dd)
> > > >   void hfi1_netdev_free(struct hfi1_devdata *dd)
> > > >   {
> > > >   	if (dd->dummy_netdev) {
> > > > +		struct hfi1_netdev_priv *priv =
> > > > +			hfi1_netdev_priv(dd->dummy_netdev);
> > > > +
> > > >   		dd_dev_info(dd, "hfi1 netdev freed\n");
> > > > +		xa_destroy(&priv->dev_tbl);
> > > >   		kfree(dd->dummy_netdev);
> > > >   		dd->dummy_netdev = NULL;
> > >
> > > This is doing kfree() on a struct net_device?? Huh?
> > >
> > > You should have put this in your own struct and used container_of
> > > not co-oped netdev_priv, then free your own struct.
> > >
> > > It is a bit weird to see a xa_destroy like this, how did things get
> > > ot the point that no concurrent thread can see the xarray but there
> > > is still stuff stored in it?
> > >
> > > And it is weird this is storing two different types in it too, with
> > > no refcounting..
> >
> > We do rework this stuff in the other patch series.
> >
> > https://patchwork.kernel.org/project/linux-rdma/patch/1617026056-50483
> > -11-git-send-email-dennis.dalessandro@cornelisnetworks.com/
> >
> > If we fix it up in the for-next series, what should we do about stable?
> 
> Well, if you are fixing bugs then order it bug fixes first, but this is tagged for rc
> and you still need to explain what bug it is actually fixing.
> 
> xa_destroy is not required if the xarray is already empty, so the commit
> message at least needs to explain how we get to a point where it still has
> something in it.
[Wan, Kaike] Shouldn't xa_destroy() always be called during cleanup, just in case that something is left behind?
Check the following:
static void ib_device_release(struct device *device)
{
	....
	xa_destroy(&dev->compat_devs);
	xa_destroy(&dev->client_data);
	kfree_rcu(dev, rcu_head);
}

> 
> Jason