Re: [PATCH v2] dmaengine: idxd: Do not use devm for 'struct device' object allocation

From: Jason Gunthorpe <jgg@nvidia.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: Dave Jiang <dave.jiang@intel.com>, Vinod Koul <vkoul@kernel.org>,
	<dmaengine@vger.kernel.org>
Subject: Re: [PATCH v2] dmaengine: idxd: Do not use devm for 'struct device' object allocation
Date: Wed, 24 Feb 2021 14:00:08 -0400	[thread overview]
Message-ID: <20210224180008.GL4247@nvidia.com> (raw)
In-Reply-To: <CAPcyv4huU8qTjdB6X+cNcgr-zX6XHoVT7nE6q2Sr6Y0UoiUkCw@mail.gmail.com>

On Wed, Feb 24, 2021 at 09:49:39AM -0800, Dan Williams wrote:

> > > I'd say this is follow-on work post bug-fix for the current lifetime violation.
> > >
> > >> +       kfree(idxd->msix_entries);
> > >> +       kfree(idxd->irq_entries);
> > >> +       kfree(idxd->int_handles);
> > > These seem tied to the lifetime of the driver bind to the pci_device
> > > not the conf_dev, or are those lines blurred here?
> >
> > Jason asked those to be not allocated by devm_* so there wouldn't be any
> > inter-mixing of allocation methods and causing a mess.
> 
> Ok, it wasn't clear to me where the lines were drawn, so moving all to
> the lowest common denominator lifetime makes sense.

Usualy when I see things like this I ask why those are dedicated
allocations too?

It looks like msix_entries is usless, this could be a temp allocation
during probe and the vector # stored in the irq_entries.

int_handles might like to be a flex array, not sure

The other devm stuff looks questionable:

        rc = devm_request_threaded_irq(dev, msix->vector, idxd_irq_handler,
                                       idxd_misc_thread, 0, "idxd-misc",
                                       irq_entry);

So we pass irq_entry which is not devm managed memory to the irq, but
we don't remove the irq during the driver remove() function, so this
could use-after free.

However this is counter-acted by masking and synchronize_irq extra
code.

Looks to be a lot simpler/clearer to just skip devm and call release
irq instead of the other stuff.

Jason