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:   Thu, 19 Dec 2019 09:46:14 +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 Wed, Dec 18, 2019 at 06:57:10PM +0000, Saleem, Shiraz wrote:
> > > > Subject: Re: [PATCH v3 04/20] i40e: Register a virtbus device to
> > > > provide RDMA
> [.....]
> 
> > > >
> > > > 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"?
> 
> physical function.
> 
> > 
> > > 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.
> >
> 
> I think we can decouple the virtbus_device object from i40e_info object.
> 
> Instead allocate a i40e_virtbus_device object which contains
> the virtbus_device and a pointer to i40e_info object for the
> RDMA driver to consume on probe(). Register it the virtbus, and provide
> a release callback to free up its memory.
> 
> Sending a patch snippet to hopefully make it clearer.

Yes, this looks a little bit more sane.

greg k-h