netdev - RE: [PATCH v3 04/20] i40e: Register a virtbus device to provide RDMA

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 13 Dec 2019 23:08:34 +0000
From:   "Saleem, Shiraz" <shiraz.saleem@...el.com>
To:     Greg KH <gregkh@...uxfoundation.org>,
        "Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        "nhorman@...hat.com" <nhorman@...hat.com>,
        "sassmann@...hat.com" <sassmann@...hat.com>,
        "jgg@...pe.ca" <jgg@...pe.ca>,
        "parav@...lanox.com" <parav@...lanox.com>,
        "Ismail, Mustafa" <mustafa.ismail@...el.com>
Subject: RE: [PATCH v3 04/20] i40e: Register a virtbus device to provide RDMA

> Subject: Re: [PATCH v3 04/20] i40e: Register a virtbus device to provide RDMA
> 

[....]

> >  /**
> > @@ -275,6 +281,27 @@ void i40e_client_update_msix_info(struct i40e_pf *pf)
> >  	cdev->lan_info.msix_entries =
> > &pf->msix_entries[pf->iwarp_base_vector];
> >  }
> >
> > +static int i40e_init_client_virtdev(struct i40e_pf *pf) {
> > +	struct i40e_info *ldev = &pf->cinst->lan_info;
> > +	struct pci_dev *pdev = pf->pdev;
> > +	struct virtbus_device *vdev;
> > +	int ret;
> > +
> > +	vdev = &ldev->vdev;
> > +	vdev->name = I40E_PEER_RDMA_NAME;
> > +	vdev->dev.parent = &pf->pdev->dev;
> 
> What a total and complete mess of a tangled web you just wove here.
> 
> Ok, so you pass in a single pointer, that then dereferences 3 pointers deep to find
> the pointer to the virtbus_device structure, but then you point the parent of that
> device, back at the original structure's sub-pointer's device itself.
> 
> WTF?

OK. This is convoluted. Passing a pointer to the i40e_info object should suffice. So something like,

+static int i40e_init_client_virtdev(struct i40e_info *ldev) {
+       struct pci_dev *pdev = ldev->pcidev;
+       struct virtbus_device *vdev = &ldev->vdev;
+       int ret;
+
+       vdev->name = I40E_PEER_RDMA_NAME;
+       vdev->dev.parent = &pdev->dev;
+       ret = virtbus_dev_register(vdev);
+       if (ret)
+               return ret;
+
+       return 0;
+}
+

> 
> And who owns the memory of this thing that is supposed to be dynamically
> controlled by something OUTSIDE of this driver?  Who created that thing 3
> pointers deep?  What happens when you leak the memory below (hint, you did),
> and who is supposed to clean it up if you need to properly clean it up if something
> bad happens?

The i40e_info object memory is tied to the PF driver.

The object hierarchy is,

i40e_pf: pointer to i40e_client_instance 
	----- i40e_client_instance: i40e_info
		----- i40e_info: virtbus_device

For each PF, there is a client_instance object allocated.
The i40e_info object is populated and the virtbus_device hanging off this object is registered.
In irdma probe(), we use the container_of macro to get to this i40e_info object from the
virtbus_device. It contains all the ops/info which RDMA driver needs from the PCI function driver.

The lifetime of the i40e_info object (and the virtbus device) is tied to the PF.
When PF goes away, virtbus_device is unregistered and the client_instance object memory
is freed.

> 
> > +
> > +	ret = virtbus_dev_register(vdev);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failure adding client virtbus dev %s %d\n",
> > +			I40E_PEER_RDMA_NAME, ret);
> 
> Again, the core should handle this, right?

Right. Will fix.

> 
> > +		return ret;
> 
> Did you just leak memory?

Thanks! Will fix.

> Also, what ever happened to my "YOU ALL MUST AGREE TO WORK TOGETHER"
> requirement between this group, and the other group trying to do the same thing?  I
> want to see signed-off-by from EVERYONE involved before we are going to
> consider this thing.
> 
We will have all parties cc'ed in the next submission. Would encourage folks to review
and hopefully we can get some consensus.