Re: [PATCH v8 03/15] virt: coco: tsm: Complete PCI registration before publishing the device

[Alexey Kardashevskiy <aik@amd.com>]

On 5/11/26 03:16, Aneesh Kumar K.V (Arm) wrote:
> tsm_register() calls device_add() before initializing and validating the
> PCI operations. This makes the new TSM discoverable through the class
> lookup while PCI registration can still fail. This ordering was introduced
> by commit 3225f52cde56 ("PCI/TSM: Establish Secure Sessions and Link
> Encryption").
> 
> An existing link TSM can keep the PCI connect attribute visible during
> registration of another TSM. A concurrent write naming the new TSM can
> therefore reach it through:
> 
>    connect_store()
>      find_tsm_dev()
>        class_find_device()
>      is_link_tsm()
>      pci_tsm_connect()
> 
> If the new backend supplies both link and device security operations,
> pci_tsm_register() rejects it before taking pci_tsm_rwsem. The registration
> thread then follows:
> 
>    tsm_register()
>      tsm_register_pci()
>        pci_tsm_register() -> -EINVAL
>        tsm_dev->pci_ops = NULL
>      device_del()
>      put_tsm_dev() [cleanup guard]
> 
> If connect has already attached a PCI context, its temporary device
> reference does not protect the stored tsm_dev pointer after the call
> returns. Registration cleanup can then free a TSM still referenced by
> that context. The original tsm_register_pci_or_reset() error path also
> had this problem: device_unregister() removed the class device and dropped
> its reference without tearing down an attached PCI context.
> 
> Commit 37a73b0448ad ("tsm: Remove the device from lookup before PCI
> teardown") subsequently added the unlocked pci_ops reset shown above.
> That reset can also race with the connect path's accesses to pci_ops.
> 
> Complete PCI registration before device_add() publishes the class device.
> On PCI registration failure, the TSM remains undiscoverable. If
> device_add() fails instead, unregister the PCI operations to roll back
> the counters and PCI sysfs visibility before releasing the device.
> 
> With pci_ops initialized before device_add(), the class PCI attributes
> are created with the correct visibility, so remove the subsequent sysfs
> group update. Keep the PCI change notification after successful device
> publication.
> 
> Fixes: 3225f52cde56 ("PCI/TSM: Establish Secure Sessions and Link Encryption")
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
> ---
>   drivers/virt/coco/tsm-core.c | 16 ++++++++++------
>   1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/virt/coco/tsm-core.c b/drivers/virt/coco/tsm-core.c
> index f79135986102..e370806d8436 100644
> --- a/drivers/virt/coco/tsm-core.c
> +++ b/drivers/virt/coco/tsm-core.c
> @@ -71,9 +71,6 @@ static int tsm_register_pci(struct tsm_dev *tsm_dev, struct pci_tsm_ops *pci_ops
>   			"PCI/TSM registration failure: %d\n", rc);
>   		return rc;
>   	}
> -
> -	/* Notify TSM userspace that PCI/TSM operations are now possible */
> -	kobject_uevent(&tsm_dev->dev.kobj, KOBJ_CHANGE);
>   	return 0;
>   }
>   
> @@ -91,15 +88,22 @@ struct tsm_dev *tsm_register(struct device *parent, struct pci_tsm_ops *pci_ops)
>   	if (rc)
>   		return ERR_PTR(rc);
>   
> -	rc = device_add(dev);
> +	/* Complete PCI registration before exposing the TSM to class lookup. */
> +	rc = tsm_register_pci(tsm_dev, pci_ops);

I'd move the !pci_ops check from tsm_register_pci() here, to match the pci_tsm_unregister() below. Or just open code it.

>   	if (rc)
>   		return ERR_PTR(rc);
>   
> -	rc = tsm_register_pci(tsm_dev, pci_ops);

ah that's why a pointer became an int...


> +	rc = device_add(dev);
>   	if (rc) {
> -		device_del(dev);
> +		if (pci_ops)
> +			pci_tsm_unregister(tsm_dev);
>   		return ERR_PTR(rc);
>   	}
> +
> +	/* Notify TSM userspace that PCI/TSM operations are now possible. */
> +	if (pci_ops)
> +		kobject_uevent(&dev->kobj, KOBJ_CHANGE);

Why do we do this only when pci_ops!=NULL? Also, why do we even allow pci_ops==NULL?
[a minute later] ah 04/15 does just that. Make it first in the series so following patches get simpler?

01..04 can go first really.

Thanks,


> +
>   	return no_free_ptr(tsm_dev);
>   }
>   EXPORT_SYMBOL_GPL(tsm_register);

-- 
Alexey