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

[Yang Yingliang <yangyingliang@huawei.com>]

On 2022/11/8 17:33, Damien Le Moal wrote:
> 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.
This patch handled return value of transport_add_device() and go to 
error path,
and the previous case handle it in the same way, I am not sure what's 
the difference
in them.

The only difference is releasing resource in error path, this patch 
calls ata_tdev_free()
to free device, and the other two cases call put_device(), did you mean 
this part?

All three cases will call transport_configure_device() after 
transport_add_device() failed,
there is no error trace, the error happened while calling 
transport_remove_device().

If I missed something, please correct me.

Thanks,
Yang
>
>> 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;
>>>>    }