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:   Sat, 14 Dec 2019 09:37:53 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     "Saleem, Shiraz" <shiraz.saleem@...el.com>
Cc:     "Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
        "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

On Fri, Dec 13, 2019 at 11:08:34PM +0000, Saleem, Shiraz wrote:
> > 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.

What is a "PF"?

> The object hierarchy is,
> 
> i40e_pf: pointer to i40e_client_instance 
> 	----- i40e_client_instance: i40e_info
> 		----- i40e_info: virtbus_device

So you are 3 pointers deep to get a structure that is dynamically
controlled?  Why are those "3 pointers" not also represented in sysfs?
You have a heiarchy within the kernel that is not being represented that
way to userspace, why?

Hint, I think this is totally wrong, you need to rework this to be sane.

> For each PF, there is a client_instance object allocated.

Great, make it dynamic and in the device tree.

> The i40e_info object is populated and the virtbus_device hanging off this object is registered.

Great, make that dynamic and inthe device tree.

If you think this is too much, then your whole mess here is too much and
needs to be made a lot simpler.

> 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.

Ok, that's what you are supposed to do, but why walk back 3 levels?

> The lifetime of the i40e_info object (and the virtbus device) is tied to the PF.

Careful, these are dynamic structures, you don't get to fully "tie"
anything here.

> When PF goes away, virtbus_device is unregistered and the client_instance object memory
> is freed.

Hopefully, if no one else has a reference (hint, you never know this.)
If you are relying on this, then you are not using the driver model
correctly.

> > 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.

Then when you post your patches, do so to be obvious that is what you
are asking for, don't try to do a "here's a pull request to take!" type
request like you did here, that's not nice.

thanks,

greg k-h