linux-kernel - Re: [PATCH v1 1/9] drivers: hv: dxgkrnl: Driver initialization and creation of dxgadapter

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, 14 Jan 2022 06:40:53 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Iouri Tarassov <iourit@...ux.microsoft.com>
Cc:     kys@...rosoft.com, haiyangz@...rosoft.com, sthemmin@...rosoft.com,
        wei.liu@...nel.org, linux-hyperv@...r.kernel.org,
        linux-kernel@...r.kernel.org, spronovo@...rosoft.com
Subject: Re: [PATCH v1 1/9] drivers: hv: dxgkrnl: Driver initialization and
 creation of dxgadapter

On Thu, Jan 13, 2022 at 04:08:07PM -0800, Iouri Tarassov wrote:
> 
> On 1/12/2022 11:46 PM, Greg KH wrote:
> > On Wed, Jan 12, 2022 at 11:55:06AM -0800, Iouri Tarassov wrote:
> > > +	dev_dbg(dxgglobaldev, "%s: %x:%x %p %pUb\n",
> > > +		    __func__, adapter->luid.b, adapter->luid.a, hdev->channel,
> > > +		    &hdev->channel->offermsg.offer.if_instance);
> > 
> > When I see something like "global device pointer", that is a HUGE red
> > flag.
> > 
> > No driver should ever have anything that is static to the driver like
> > this, it should always be per-device.  Please use the correct device
> > model here, which does not include a global pointer, but rather unique
> > ones that are given to you by the driver core.  That way you are never
> > tied to only "one device per system" as that is a constraint that you
> > will have to fix eventually, might as well do it all correctly the first
> > time as it is not any extra effort to do so
> Hi Greg,
> 
> dxgglobaldev is a pointer to the global driver data. By design there is a
> single hyper-v VM bus and a single corresponding /dev/dxg device.

That's fine, but use the pointer that you create based on your bus
device, not some static pointer please.

> Virtual GPU adapters are present on the VM bus. /dev/dxg device is used
> to enumerate all virtual GPUs, which are accessible only via IOCTLs
> to /dev/dxg. dxgglobaldev has a list of all vGPU adapters and
> other global driver state. This follows the design on Windows where a single
> global object in dxgkrnl.sys driver is used to enumerate and access all
> GPU devices. This is also how the public D3DKMT interface to dxgkrnl is
> structured.

First off, remember this isn't Windows, let's not make the same mistakes
they have made there please :)

Secondly, this isn't the problem, the issue is that you have a
non-dynamic device here, which is not how Linux drivers should ever
work.  It's fine to have a "mux" device like this, but create it
properly, based on the device that the driver core gives you as you must
respect that lifetime, not the lifetime of a static pointer embedded in
a module.

thanks,

greg k-h