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

[Yang Yingliang <yangyingliang@huawei.com>]

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 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;
>>   }