Re: [PATCH v2 4/4] ata: libata-transport: fix error handling in ata_tdev_add()

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 11/8/22 18:27, Yang Yingliang wrote:
> Hi,
> 
> On 2022/11/8 16:03, Damien Le Moal wrote:
>> On 11/8/22 16:52, Yang Yingliang wrote:
>>> In ata_tdev_add(), the return value of transport_add_device() is not
>>> checked. As a result, another error after that function call leads to
>>> a kernel crash (null-ptr-deref) because transport_remove_device() is
>>> called to remove a device that was not added.
>> This was the error pattern for the previous 2 cases, but not for this
>> one. For this case, transport_configure_device() would be called for a
> Did you mean transport_remove_device() ?
>> device that was not added, no ? so where does the backtrace below come
>> from ? It does not correspond to the code this patch is changing.
> Yes, it's correspond, ata_tdev_delete() is inlined, so it doesn't show
> in the call traces.

I meant to say that the commit message explanation does not match at all
the cod echange. Totally unrelated explanation and functions mentioned.
Please fix that first and simply put the kdump trace you see if there is
an issue when calling transport_configure_device() after
transport_add_device() failed.

> 
> I make ata_tdev_delete() noinline and reproduced it again,
> ata_tdev_delete()
> is showed in the call trace:
> 
> [  140.120952][T13603] Call trace:
> [  140.124359][T13603]  device_del+0x48/0x3a0
> [  140.128713][T13603] attribute_container_class_device_del+0x28/0x40
> [  140.135226][T13603]  transport_remove_classdev+0x60/0x7c
> [  140.140783][T13603] attribute_container_device_trigger+0x118/0x120
> [  140.147289][T13603]  transport_remove_device+0x20/0x30
> [  140.152665][T13603]  ata_tdev_delete+0x24/0x50 [libata]
> [  140.158152][T13603]  ata_tlink_delete+0x40/0xa0 [libata]
> [  140.163724][T13603]  ata_tport_delete+0x2c/0x60 [libata]
> [  140.169294][T13603]  ata_port_detach+0x148/0x1b0 [libata]
> [  140.174951][T13603]  ata_pci_remove_one+0x50/0x80 [libata]
> [  140.180689][T13603]  ahci_remove_one+0x4c/0x8c [ahci]
> 
> Thanks,
> Yang
>>
>>> Unable to handle kernel NULL pointer dereference at virtual address
>>> 00000000000000d0
>>> CPU: 61 PID: 13969 Comm: rmmod Kdump: loaded Tainted: G       
>>> W          6.1.0-rc3+ #13
>>> pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>>> pc : device_del+0x48/0x39c
>>> lr : device_del+0x44/0x39c
>>> Call trace:
>>>   device_del+0x48/0x39c
>>>   attribute_container_class_device_del+0x28/0x40
>>>   transport_remove_classdev+0x60/0x7c
>>>   attribute_container_device_trigger+0x118/0x120
>>>   transport_remove_device+0x20/0x30
>>>   ata_tlink_delete+0x4c/0xb0 [libata]
>>>   ata_tport_delete+0x2c/0x60 [libata]
>>>   ata_port_detach+0x148/0x1b0 [libata]
>>>   ata_pci_remove_one+0x50/0x80 [libata]
>>>   ahci_remove_one+0x4c/0x8c [ahci]
>>>
>>> Fix this by checking and handling return value of transport_add_device()
>>> in ata_tdev_add().
>>>
>>> Fixes: d9027470b886 ("[libata] Add ATA transport class")
>>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
>>> ---
>>>   drivers/ata/libata-transport.c | 8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/ata/libata-transport.c
>>> b/drivers/ata/libata-transport.c
>>> index aac9336e8153..e4fb9d1b9b39 100644
>>> --- a/drivers/ata/libata-transport.c
>>> +++ b/drivers/ata/libata-transport.c
>>> @@ -713,7 +713,13 @@ static int ata_tdev_add(struct ata_device *ata_dev)
>>>           return error;
>>>       }
>>>   -    transport_add_device(dev);
>>> +    error = transport_add_device(dev);
>>> +    if (error) {
>>> +        device_del(dev);
>>> +        ata_tdev_free(ata_dev);
>>> +        return error;
>>> +    }
>>> +
>>>       transport_configure_device(dev);
>>>       return 0;
>>>   }

-- 
Damien Le Moal
Western Digital Research