* [PATCH v2 0/4] ata: libata-transport: fix some error handing
@ 2022-11-08 7:52 Yang Yingliang
2022-11-08 7:52 ` [PATCH v2 1/4] ata: libata-transport: fix double ata_host_put() in ata_tport_add() Yang Yingliang
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Yang Yingliang @ 2022-11-08 7:52 UTC (permalink / raw)
To: linux-ide; +Cc: damien.lemoal, yangyingliang
patch #1 fix a null-ptr-deref problem that caused by double put refcount.
patch #2 ~ #4 handle error case of transport_add_device(), if it's
ignored, it will lead kernel crash because of trying to delete not
added device in transport_remove_device().
v1 -> v2:
Update commit message that suggested by Damien.
Yang Yingliang (4):
ata: libata-transport: fix double ata_host_put() in ata_tport_add()
ata: libata-transport: fix error handling in ata_tport_add()
ata: libata-transport: fix error handling in ata_tlink_add()
ata: libata-transport: fix error handling in ata_tdev_add()
drivers/ata/libata-transport.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/4] ata: libata-transport: fix double ata_host_put() in ata_tport_add()
2022-11-08 7:52 [PATCH v2 0/4] ata: libata-transport: fix some error handing Yang Yingliang
@ 2022-11-08 7:52 ` Yang Yingliang
2022-11-08 7:52 ` [PATCH v2 2/4] ata: libata-transport: fix error handling " Yang Yingliang
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Yang Yingliang @ 2022-11-08 7:52 UTC (permalink / raw)
To: linux-ide; +Cc: damien.lemoal, yangyingliang
In the error path in ata_tport_add(), when calling put_device(),
ata_tport_release() is called, it will put the refcount of 'ap->host'.
And then ata_host_put() is called again, the refcount is decreased
to 0, ata_host_release() is called, all ports are freed and set to
null.
When unbinding the device after failure, ata_host_stop() is called
to release the resources, it leads a null-ptr-deref(), because all
the ports all freed and null.
Unable to handle kernel NULL pointer dereference at virtual address 0000000000000008
CPU: 7 PID: 18671 Comm: modprobe Kdump: loaded Tainted: G E 6.1.0-rc3+ #8
pstate: 80400009 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : ata_host_stop+0x3c/0x84 [libata]
lr : release_nodes+0x64/0xd0
Call trace:
ata_host_stop+0x3c/0x84 [libata]
release_nodes+0x64/0xd0
devres_release_all+0xbc/0x1b0
device_unbind_cleanup+0x20/0x70
really_probe+0x158/0x320
__driver_probe_device+0x84/0x120
driver_probe_device+0x44/0x120
__driver_attach+0xb4/0x220
bus_for_each_dev+0x78/0xdc
driver_attach+0x2c/0x40
bus_add_driver+0x184/0x240
driver_register+0x80/0x13c
__pci_register_driver+0x4c/0x60
ahci_pci_driver_init+0x30/0x1000 [ahci]
Fix this by removing redundant ata_host_put() in the error path.
Fixes: 2623c7a5f279 ("libata: add refcounting to ata_host")
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
drivers/ata/libata-transport.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index a7e9a75410a3..105da3ec5eaa 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -317,7 +317,6 @@ int ata_tport_add(struct device *parent,
tport_err:
transport_destroy_device(dev);
put_device(dev);
- ata_host_put(ap->host);
return error;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/4] ata: libata-transport: fix error handling in ata_tport_add()
2022-11-08 7:52 [PATCH v2 0/4] ata: libata-transport: fix some error handing Yang Yingliang
2022-11-08 7:52 ` [PATCH v2 1/4] ata: libata-transport: fix double ata_host_put() in ata_tport_add() Yang Yingliang
@ 2022-11-08 7:52 ` Yang Yingliang
2022-11-08 7:52 ` [PATCH v2 3/4] ata: libata-transport: fix error handling in ata_tlink_add() Yang Yingliang
2022-11-08 7:52 ` [PATCH v2 4/4] ata: libata-transport: fix error handling in ata_tdev_add() Yang Yingliang
3 siblings, 0 replies; 9+ messages in thread
From: Yang Yingliang @ 2022-11-08 7:52 UTC (permalink / raw)
To: linux-ide; +Cc: damien.lemoal, yangyingliang
In ata_tport_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.
Unable to handle kernel NULL pointer dereference at virtual address 00000000000000d0
CPU: 12 PID: 13605 Comm: rmmod Kdump: loaded Tainted: G W 6.1.0-rc3+ #8
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_tport_delete+0x34/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_tport_add().
Fixes: d9027470b886 ("[libata] Add ATA transport class")
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
drivers/ata/libata-transport.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index 105da3ec5eaa..ef53bdfbcbb2 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -301,7 +301,9 @@ int ata_tport_add(struct device *parent,
pm_runtime_enable(dev);
pm_runtime_forbid(dev);
- transport_add_device(dev);
+ error = transport_add_device(dev);
+ if (error)
+ goto tport_transport_add_err;
transport_configure_device(dev);
error = ata_tlink_add(&ap->link);
@@ -312,6 +314,7 @@ int ata_tport_add(struct device *parent,
tport_link_err:
transport_remove_device(dev);
+ tport_transport_add_err:
device_del(dev);
tport_err:
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 3/4] ata: libata-transport: fix error handling in ata_tlink_add()
2022-11-08 7:52 [PATCH v2 0/4] ata: libata-transport: fix some error handing Yang Yingliang
2022-11-08 7:52 ` [PATCH v2 1/4] ata: libata-transport: fix double ata_host_put() in ata_tport_add() Yang Yingliang
2022-11-08 7:52 ` [PATCH v2 2/4] ata: libata-transport: fix error handling " Yang Yingliang
@ 2022-11-08 7:52 ` Yang Yingliang
2022-11-08 7:52 ` [PATCH v2 4/4] ata: libata-transport: fix error handling in ata_tdev_add() Yang Yingliang
3 siblings, 0 replies; 9+ messages in thread
From: Yang Yingliang @ 2022-11-08 7:52 UTC (permalink / raw)
To: linux-ide; +Cc: damien.lemoal, yangyingliang
In ata_tlink_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.
Unable to handle kernel NULL pointer dereference at virtual address 00000000000000d0
CPU: 33 PID: 13850 Comm: rmmod Kdump: loaded Tainted: G W 6.1.0-rc3+ #12
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+0x88/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_tlink_add().
Fixes: d9027470b886 ("[libata] Add ATA transport class")
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
drivers/ata/libata-transport.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index ef53bdfbcbb2..aac9336e8153 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -458,7 +458,9 @@ int ata_tlink_add(struct ata_link *link)
goto tlink_err;
}
- transport_add_device(dev);
+ error = transport_add_device(dev);
+ if (error)
+ goto tlink_transport_err;
transport_configure_device(dev);
ata_for_each_dev(ata_dev, link, ALL) {
@@ -473,6 +475,7 @@ int ata_tlink_add(struct ata_link *link)
ata_tdev_delete(ata_dev);
}
transport_remove_device(dev);
+ tlink_transport_err:
device_del(dev);
tlink_err:
transport_destroy_device(dev);
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 4/4] ata: libata-transport: fix error handling in ata_tdev_add()
2022-11-08 7:52 [PATCH v2 0/4] ata: libata-transport: fix some error handing Yang Yingliang
` (2 preceding siblings ...)
2022-11-08 7:52 ` [PATCH v2 3/4] ata: libata-transport: fix error handling in ata_tlink_add() Yang Yingliang
@ 2022-11-08 7:52 ` Yang Yingliang
2022-11-08 8:03 ` Damien Le Moal
3 siblings, 1 reply; 9+ messages in thread
From: Yang Yingliang @ 2022-11-08 7:52 UTC (permalink / raw)
To: linux-ide; +Cc: damien.lemoal, yangyingliang
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.
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;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 4/4] ata: libata-transport: fix error handling in ata_tdev_add()
2022-11-08 7:52 ` [PATCH v2 4/4] ata: libata-transport: fix error handling in ata_tdev_add() Yang Yingliang
@ 2022-11-08 8:03 ` Damien Le Moal
2022-11-08 9:27 ` Yang Yingliang
0 siblings, 1 reply; 9+ messages in thread
From: Damien Le Moal @ 2022-11-08 8:03 UTC (permalink / raw)
To: Yang Yingliang, linux-ide
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
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.
>
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 4/4] ata: libata-transport: fix error handling in ata_tdev_add()
2022-11-08 8:03 ` Damien Le Moal
@ 2022-11-08 9:27 ` Yang Yingliang
2022-11-08 9:33 ` Damien Le Moal
0 siblings, 1 reply; 9+ messages in thread
From: Yang Yingliang @ 2022-11-08 9:27 UTC (permalink / raw)
To: Damien Le Moal, linux-ide, yangyingliang
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;
>> }
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 4/4] ata: libata-transport: fix error handling in ata_tdev_add()
2022-11-08 9:27 ` Yang Yingliang
@ 2022-11-08 9:33 ` Damien Le Moal
2022-11-08 10:49 ` Yang Yingliang
0 siblings, 1 reply; 9+ messages in thread
From: Damien Le Moal @ 2022-11-08 9:33 UTC (permalink / raw)
To: Yang Yingliang, linux-ide
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 4/4] ata: libata-transport: fix error handling in ata_tdev_add()
2022-11-08 9:33 ` Damien Le Moal
@ 2022-11-08 10:49 ` Yang Yingliang
0 siblings, 0 replies; 9+ messages in thread
From: Yang Yingliang @ 2022-11-08 10:49 UTC (permalink / raw)
To: Damien Le Moal, linux-ide; +Cc: yangyingliang
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;
>>>> }
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-11-08 10:49 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-08 7:52 [PATCH v2 0/4] ata: libata-transport: fix some error handing Yang Yingliang
2022-11-08 7:52 ` [PATCH v2 1/4] ata: libata-transport: fix double ata_host_put() in ata_tport_add() Yang Yingliang
2022-11-08 7:52 ` [PATCH v2 2/4] ata: libata-transport: fix error handling " Yang Yingliang
2022-11-08 7:52 ` [PATCH v2 3/4] ata: libata-transport: fix error handling in ata_tlink_add() Yang Yingliang
2022-11-08 7:52 ` [PATCH v2 4/4] ata: libata-transport: fix error handling in ata_tdev_add() Yang Yingliang
2022-11-08 8:03 ` Damien Le Moal
2022-11-08 9:27 ` Yang Yingliang
2022-11-08 9:33 ` Damien Le Moal
2022-11-08 10:49 ` Yang Yingliang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox