[v8,0/8] idxd 'struct device' lifetime handling fixes

Message

Dave Jiang March 25, 2021, 3:54 p.m. UTC

v8:
- Do not emit negative value for sysfs 'minor' attrib (Dan)
- Use sysfs_emit() to emit sysfs 'minor' attrib (Jason)
- Fix interation of unwind cleanup of various allocation. (DanC)

v7:
- Fix up the 'struct device' setup in char device code (Jason)
- Split out the char dev fixes (Jason)
- Split out the DMA dev fixes (Dan)
- Split out the each of the conf_dev fixes
- Split out removal of the pcim_* calls
- Split out removal of the devm_* calls
- Split out the fixes for interrupt config calls
- Reviewed by Dan.

v6:
- Fix char dev initialization issues (Jason)
- Fix other 'struct device' initialization issues.

v5:
- Rebased against 5.12-rc dmaengine/fixes
v4:
- fix up the life time of cdev creation/destruction (Jason)
- Tested with KASAN and other memory allocation leak detections. (Jason)

v3:
- Remove devm_* for irq request and cleanup related bits (Jason)
v2:
- Remove all devm_* alloc for idxd_device (Jason)
- Add kref dep for dma_dev (Jason)

Vinod,
The series fixes the various 'struct device' lifetime handling in the
idxd driver. The devm managed lifetime is incompatible with 'struct device'
objects that resides in the idxd context. Tested with
CONFIG_DEBUG_KOBJECT_RELEASE and address all issues from that.

Please consider for damengine/fixes for the 5.12-rc.

---

Dave Jiang (8):
      dmaengine: idxd: fix dma device lifetime
      dmaengine: idxd: cleanup pci interrupt vector allocation management
      dmaengine: idxd: removal of pcim managed mmio mapping
      dmaengine: idxd: fix idxd conf_dev 'struct device' lifetime
      dmaengine: idxd: fix wq conf_dev 'struct device' lifetime
      dmaengine: idxd: fix engine conf_dev lifetime
      dmaengine: idxd: fix group conf_dev lifetime
      dmaengine: idxd: fix cdev setup and free device lifetime issues


 drivers/dma/idxd/cdev.c   | 131 +++++++-----------
 drivers/dma/idxd/device.c |  20 +--
 drivers/dma/idxd/idxd.h   |  54 ++++++--
 drivers/dma/idxd/init.c   | 284 +++++++++++++++++++++++++++-----------
 drivers/dma/idxd/irq.c    |  10 +-
 drivers/dma/idxd/sysfs.c  | 241 ++++++++++++++++----------------
 6 files changed, 439 insertions(+), 301 deletions(-)

--

Comments

Jason Gunthorpe March 30, 2021, 5:45 p.m. UTC | #1

On Thu, Mar 25, 2021 at 08:54:31AM -0700, Dave Jiang wrote:

> Vinod,
> The series fixes the various 'struct device' lifetime handling in the
> idxd driver. The devm managed lifetime is incompatible with 'struct device'
> objects that resides in the idxd context. Tested with
> CONFIG_DEBUG_KOBJECT_RELEASE and address all issues from that.
> 
> Please consider for damengine/fixes for the 5.12-rc.

It seems like an improvement..

The flow around probe is still weird:

static int idxd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
{
	idxd = idxd_alloc(pdev);
        ^^ but we don't device_initialize() this

	rc = idxd_probe(idxd);
	if (rc)
		goto err;

 err:
 err_iomap:
	idxd_free(idxd);
 
static int idxd_probe(struct idxd_device *idxd)
{

 err_int:
	idxd_free(idxd);
	return rc;

So we call idxd_free twice on error unwinds, and that will
crash. Unify idxd_free() and idxd_conf_device_release() as
appropriate.

Confused why they are different, why are some of the kfrees missing
from the release function?

Call device_initialize in idxd_alloc() and make it so that the release
function works properly. Move all the error unwind put_device's to
idxd_pci_probe()

..

	idxd->id = idr_alloc(&idxd_idrs[idxd->type], idxd, 0, 0, GFP_KERNEL);

Nothing ever reads the idxd_idr, so this should be an ida

Jason

Dave Jiang March 30, 2021, 5:58 p.m. UTC | #2

On 3/30/2021 10:45 AM, Jason Gunthorpe wrote:
> On Thu, Mar 25, 2021 at 08:54:31AM -0700, Dave Jiang wrote:
>
>> Vinod,
>> The series fixes the various 'struct device' lifetime handling in the
>> idxd driver. The devm managed lifetime is incompatible with 'struct device'
>> objects that resides in the idxd context. Tested with
>> CONFIG_DEBUG_KOBJECT_RELEASE and address all issues from that.
>>
>> Please consider for damengine/fixes for the 5.12-rc.
> It seems like an improvement..
>
> The flow around probe is still weird:
>
> static int idxd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> {
> 	idxd = idxd_alloc(pdev);
>          ^^ but we don't device_initialize() this
>
> 	rc = idxd_probe(idxd);
> 	if (rc)
> 		goto err;
>
>   err:
>   err_iomap:
> 	idxd_free(idxd);
>   
> static int idxd_probe(struct idxd_device *idxd)
> {
>
>   err_int:
> 	idxd_free(idxd);
> 	return rc;
>
> So we call idxd_free twice on error unwinds, and that will
> crash. Unify idxd_free() and idxd_conf_device_release() as
> appropriate.
>
> Confused why they are different, why are some of the kfrees missing
> from the release function?
>
> Call device_initialize in idxd_alloc() and make it so that the release
> function works properly. Move all the error unwind put_device's to
> idxd_pci_probe()

I think understand where you are pulling this towards. I think we'll 
need to rework conf_dev initialization into the main initialization 
rather than doing it at the end. Let me go rework that.


>
> ..
>
> 	idxd->id = idr_alloc(&idxd_idrs[idxd->type], idxd, 0, 0, GFP_KERNEL);
>
> Nothing ever reads the idxd_idr, so this should be an ida

Yep Dan pointed that out the other day. I have a patch that fixes it but 
it's in the next patch series. I'll pull it out and into this one.


>
> Jason