Re: [PATCH v5 2/3] vfio/ism: Implement vfio_pci driver for ISM devices

[Julian Ruess]

On Tue Mar 17, 2026 at 10:16 PM CET, Niklas Schnelle wrote:
> On Tue, 2026-03-17 at 11:43 -0700, Farhan Ali wrote:
>> <..snip..>
>> 
>> On 3/17/2026 5:58 AM, Julian Ruess wrote:
>> > static int ism_vfio_pci_probe(struct pci_dev *pdev,
>> > +			      const struct pci_device_id *id)
>> > +{
>> > +	struct ism_vfio_pci_core_device *ivpcd;
>> > +	struct zpci_dev *zdev = to_zpci(pdev);
>> > +	char cache_name[20];
>> > +	int ret;
>> > +
>> > +	ivpcd = vfio_alloc_device(ism_vfio_pci_core_device, core_device.vdev,
>> > +				  &pdev->dev, &ism_pci_ops);
>> > +	if (IS_ERR(ivpcd))
>> > +		return PTR_ERR(ivpcd);
>> > +
>> > +	snprintf(cache_name, sizeof(cache_name), "ism_sb_fid_%08x", zdev->fid);
>> > +	ivpcd->store_block_cache =
>> > +		kmem_cache_create(cache_name, zdev->maxstbl, 0, 0, NULL);
>> > +	if (!ivpcd->store_block_cache) {
>> > +		vfio_put_device(&ivpcd->core_device.vdev);
>> > +		return -ENOMEM;
>> > +	}
>> > +
>> > +	dev_set_drvdata(&pdev->dev, &ivpcd->core_device);
>> > +	ret = vfio_pci_core_register_device(&ivpcd->core_device);
>> > +	if (ret) {
>> > +		kmem_cache_destroy(ivpcd->store_block_cache);
>> > +		vfio_put_device(&ivpcd->core_device.vdev);
>> > +	}
>> > +
>> > +	return ret;
>> > +}
>> > +
>> > +static void ism_vfio_pci_remove(struct pci_dev *pdev)
>> > +{
>> > +	struct vfio_pci_core_device *core_device;
>> > +	struct ism_vfio_pci_core_device *ivpcd;
>> > +
>> > +	core_device = dev_get_drvdata(&pdev->dev);
>> > +	ivpcd = container_of(core_device, struct ism_vfio_pci_core_device,
>> > +			     core_device);
>> > +
>> > +	vfio_pci_core_unregister_device(&ivpcd->core_device);
>> > +	vfio_put_device(&ivpcd->core_device.vdev);
>> > +
>> > +	kmem_cache_destroy(ivpcd->store_block_cache);
>> 
>> I think the kmem_cache_destroy() should be done before we do 
>> vfio_put_device() (or maybe even before 
>> vfio_pci_core_unregister_device()) to avoid use after free similar to 
>> ism_vfio_pci_probe(). Sorry I missed this earlier.
>> 
>> Thanks
>> 
>> Farhan
>> 
>
> Good find! Wouldn't it be even cleaner to provide vfio/ism specific
> .init and .release functions in ism_pci_ops that do the
> kmem_cache_create() / kmem_cache_destroy() as part of init / release?
> It seems this is done for e.g. Xe with xe_vfio_pci_init_dev()
> and xe_vfio_pci_release_dev(). In fact wouldn't that be even necessary
> to correctly handle the case where the above vfio_put_device() isn't
> the last reference?
>
> Thanks,
> Niklas

Thanks guys! Yes, I think we should introduce custom .init and .release functions. Will change that in the next version.