Re: [PATCH v3] switchtec: Fix stdev_release crash after suprise device loss.

[Daniel Stodden <dns@arista.com>]

Hi.

> On Nov 20, 2023, at 1:25 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> On Mon, Nov 13, 2023 at 01:21:50PM -0800, Daniel Stodden wrote:
>> A pci device hot removal may occur while stdev->cdev is held open. The
>> call to stdev_release is then delivered during close or exit, at a
>> point way past switchtec_pci_remove. Otherwise the last ref would
>> vanish with the trailing put_device, just before return.
>> 
>> At that later point in time, the device layer has alreay removed
>> stdev->mrpc_mmio map. Also, the stdev->pdev reference was not a
> 
> I guess this should say the "stdev->mmio_mrpc" (not "mrpc_mmio")?

Eww. My fault. Could you still correct that?

> 
>> counted one. Therefore, in dma mode, the iowrite32 in stdev_release
>> will cause a fatal page fault, and the subsequent dma_free_coherent,
>> if reached, would pass a stale &stdev->pdev->dev pointer.
>> 
>> Fixed by moving mrpc dma shutdown into switchtec_pci_remove, after
>> stdev_kill. Counting the stdev->pdev ref is now optional, but may
>> prevent future accidents.
>> 
>> Signed-off-by: Daniel Stodden <dns@arista.com>
>> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
> 
> Thanks, applied to "switchtec" for v6.8

Thank you.

Daniel

>> ---
>> drivers/pci/switch/switchtec.c | 24 ++++++++++++++++--------
>> 1 file changed, 16 insertions(+), 8 deletions(-)
>> 
>> diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
>> index e69cac84b605..d8718acdb85f 100644
>> --- a/drivers/pci/switch/switchtec.c
>> +++ b/drivers/pci/switch/switchtec.c
>> @@ -1251,13 +1251,6 @@ static void stdev_release(struct device *dev)
>> {
>> struct switchtec_dev *stdev = to_stdev(dev);
>> 
>> - if (stdev->dma_mrpc) {
>> - iowrite32(0, &stdev->mmio_mrpc->dma_en);
>> - flush_wc_buf(stdev);
>> - writeq(0, &stdev->mmio_mrpc->dma_addr);
>> - dma_free_coherent(&stdev->pdev->dev, sizeof(*stdev->dma_mrpc),
>> - stdev->dma_mrpc, stdev->dma_mrpc_dma_addr);
>> - }
>> kfree(stdev);
>> }
>> 
>> @@ -1301,7 +1294,7 @@ static struct switchtec_dev *stdev_create(struct pci_dev *pdev)
>> return ERR_PTR(-ENOMEM);
>> 
>> stdev->alive = true;
>> - stdev->pdev = pdev;
>> + stdev->pdev = pci_dev_get(pdev);
>> INIT_LIST_HEAD(&stdev->mrpc_queue);
>> mutex_init(&stdev->mrpc_mutex);
>> stdev->mrpc_busy = 0;
>> @@ -1587,6 +1580,18 @@ static int switchtec_init_pci(struct switchtec_dev *stdev,
>> return 0;
>> }
>> 
>> +static void switchtec_exit_pci(struct switchtec_dev *stdev)
>> +{
>> + if (stdev->dma_mrpc) {
>> + iowrite32(0, &stdev->mmio_mrpc->dma_en);
>> + flush_wc_buf(stdev);
>> + writeq(0, &stdev->mmio_mrpc->dma_addr);
>> + dma_free_coherent(&stdev->pdev->dev, sizeof(*stdev->dma_mrpc),
>> +  stdev->dma_mrpc, stdev->dma_mrpc_dma_addr);
>> + stdev->dma_mrpc = NULL;
>> + }
>> +}
>> +
>> static int switchtec_pci_probe(struct pci_dev *pdev,
>>       const struct pci_device_id *id)
>> {
>> @@ -1646,6 +1651,9 @@ static void switchtec_pci_remove(struct pci_dev *pdev)
>> ida_simple_remove(&switchtec_minor_ida, MINOR(stdev->dev.devt));
>> dev_info(&stdev->dev, "unregistered.\n");
>> stdev_kill(stdev);
>> + switchtec_exit_pci(stdev);
>> + pci_dev_put(stdev->pdev);
>> + stdev->pdev = NULL;
>> put_device(&stdev->dev);
>> }
>> 
>> -- 
>> 2.41.0
>>