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
