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
