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

[Dave Jiang <dave.jiang@intel.com>]

On 2/24/2021 11:00 AM, Jason Gunthorpe wrote:
> 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.

Ok.

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


Ok, I'll add the cleanup for that as part of the removing devm_* calls.

>
> Jason