Re: [PATCH v8 02/15] virt: coco: tsm: Remove the device from lookup before PCI teardown

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

Alexey Kardashevskiy <aik@amd.com> writes:

> On 5/11/26 03:16, Aneesh Kumar K.V (Arm) wrote:
>> PCI connect looks up a TSM by ID under pci_tsm_rwsem. Previously,
>> tsm_unregister() released that lock after tearing down PCI state while
>> the TSM was still in the class lookup. 
>
> tsm_unregister() is not touching that lock, what do I miss?
>

pci_tsm_unregister(). It uses guard(rwsem_write)(&pci_tsm_rwsem), so the
lock is released when pci_tsm_unregister() returns. We previously called
device_unregister() only after that.

>
>> A racing connect could then
>> attach a new PCI context that the teardown would never see.
>> 
>> Remove the device from the class lookup first, then take the PCI write
>> lock to drain in-flight operations and destroy their contexts. Drop the
>> device reference only after PCI teardown completes.
>> 
>> 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 | 22 ++++++++++++++--------
>>   1 file changed, 14 insertions(+), 8 deletions(-)
>> 
>> diff --git a/drivers/virt/coco/tsm-core.c b/drivers/virt/coco/tsm-core.c
>> index e784993353d8..f79135986102 100644
>> --- a/drivers/virt/coco/tsm-core.c
>> +++ b/drivers/virt/coco/tsm-core.c
>> @@ -56,26 +56,25 @@ static struct tsm_dev *alloc_tsm_dev(struct device *parent)
>>   	return no_free_ptr(tsm_dev);
>>   }
>>   
>> -static struct tsm_dev *tsm_register_pci_or_reset(struct tsm_dev *tsm_dev,
>> -						 struct pci_tsm_ops *pci_ops)
>> +static int tsm_register_pci(struct tsm_dev *tsm_dev, struct pci_tsm_ops *pci_ops)
>
> Why changing a pointer to an int here as later tsm_register() converts
> it back to a pointer?
>

At the end of the series we have
static int tsm_register_pci(struct tsm_dev *tsm_dev, struct pci_tsm_ops *pci_ops)


>
> And I never understood what is _reset for here (so +1 for that) but
> now we have tsm_register_pci() and pci_tsm_register() which is weird.
> May be just open code it? Thanks,
>

How about rename that to 
__tsm_register(struct tsm_dev *tsm_dev, struct pci_tsm_ops *pci_ops)


-aneesh