* [PATCH] PCI: endpoint: Fix configfs group removal on driver teardown
@ 2025-06-17 1:07 Damien Le Moal
2025-06-20 3:04 ` Damien Le Moal
2025-06-23 10:00 ` Niklas Cassel
0 siblings, 2 replies; 6+ messages in thread
From: Damien Le Moal @ 2025-06-17 1:07 UTC (permalink / raw)
To: linux-pci, Manivannan Sadhasivam, Krzysztof Wilczyński,
Kishon Vijay Abraham I, Bjorn Helgaas
Cc: Niklas Cassel
An endpoint driver configfs attributes group is added to the
epf_group list of struct pci_epf_driver pci_epf_add_cfs() but not
removed from this list when the attribute group is unregistered with
pci_ep_cfs_remove_epf_group(). Add the missing list_del_init() call in
that function to correctly remove the attribute group from the driver
list.
Furthermore, doing a list_del() on the epf_group field of struct
pci_epf_driver in pci_epf_remove_cfs() is not correct as this field is a
list head, not a list entry, and triggers a KASAN warning:
==================================================================
BUG: KASAN: slab-use-after-free in pci_epf_remove_cfs+0x17c/0x198
Write of size 8 at addr ffff00010f4a0d80 by task rmmod/319
CPU: 3 UID: 0 PID: 319 Comm: rmmod Not tainted 6.16.0-rc2 #1 NONE
Hardware name: Radxa ROCK 5B (DT)
Call trace:
show_stack+0x2c/0x84 (C)
dump_stack_lvl+0x70/0x98
print_report+0x17c/0x538
kasan_report+0xb8/0x190
__asan_report_store8_noabort+0x20/0x2c
pci_epf_remove_cfs+0x17c/0x198
pci_epf_unregister_driver+0x18/0x30
nvmet_pci_epf_cleanup_module+0x24/0x30 [nvmet_pci_epf]
__arm64_sys_delete_module+0x264/0x424
invoke_syscall+0x70/0x260
el0_svc_common.constprop.0+0xac/0x230
do_el0_svc+0x40/0x58
el0_svc+0x48/0xdc
el0t_64_sync_handler+0x10c/0x138
el0t_64_sync+0x198/0x19c
...
Remove this list_del() call from pci_epf_remove_cfs() and replace it
with a warning if the epf_group list is not empty.
Fixes: ef1433f717a2 ("PCI: endpoint: Create configfs entry for each pci_epf_device_id table entry")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
drivers/pci/endpoint/pci-ep-cfs.c | 1 +
drivers/pci/endpoint/pci-epf-core.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
index d712c7a866d2..63876537e7dc 100644
--- a/drivers/pci/endpoint/pci-ep-cfs.c
+++ b/drivers/pci/endpoint/pci-ep-cfs.c
@@ -691,6 +691,7 @@ void pci_ep_cfs_remove_epf_group(struct config_group *group)
if (IS_ERR_OR_NULL(group))
return;
+ list_del_init(&group->group_entry);
configfs_unregister_default_group(group);
}
EXPORT_SYMBOL(pci_ep_cfs_remove_epf_group);
diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index 577a9e490115..167dc6ee63f7 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -338,7 +338,7 @@ static void pci_epf_remove_cfs(struct pci_epf_driver *driver)
mutex_lock(&pci_epf_mutex);
list_for_each_entry_safe(group, tmp, &driver->epf_group, group_entry)
pci_ep_cfs_remove_epf_group(group);
- list_del(&driver->epf_group);
+ WARN_ON(!list_empty(&driver->epf_group));
mutex_unlock(&pci_epf_mutex);
}
--
2.49.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: endpoint: Fix configfs group removal on driver teardown
2025-06-17 1:07 [PATCH] PCI: endpoint: Fix configfs group removal on driver teardown Damien Le Moal
@ 2025-06-20 3:04 ` Damien Le Moal
2025-06-23 10:00 ` Niklas Cassel
1 sibling, 0 replies; 6+ messages in thread
From: Damien Le Moal @ 2025-06-20 3:04 UTC (permalink / raw)
To: linux-pci, Manivannan Sadhasivam, Krzysztof Wilczyński,
Kishon Vijay Abraham I, Bjorn Helgaas
Cc: Niklas Cassel
On 6/17/25 10:07, Damien Le Moal wrote:
> An endpoint driver configfs attributes group is added to the
> epf_group list of struct pci_epf_driver pci_epf_add_cfs() but not
> removed from this list when the attribute group is unregistered with
> pci_ep_cfs_remove_epf_group(). Add the missing list_del_init() call in
> that function to correctly remove the attribute group from the driver
> list.
>
> Furthermore, doing a list_del() on the epf_group field of struct
> pci_epf_driver in pci_epf_remove_cfs() is not correct as this field is a
> list head, not a list entry, and triggers a KASAN warning:
Ping ?
>
> ==================================================================
> BUG: KASAN: slab-use-after-free in pci_epf_remove_cfs+0x17c/0x198
> Write of size 8 at addr ffff00010f4a0d80 by task rmmod/319
>
> CPU: 3 UID: 0 PID: 319 Comm: rmmod Not tainted 6.16.0-rc2 #1 NONE
> Hardware name: Radxa ROCK 5B (DT)
> Call trace:
> show_stack+0x2c/0x84 (C)
> dump_stack_lvl+0x70/0x98
> print_report+0x17c/0x538
> kasan_report+0xb8/0x190
> __asan_report_store8_noabort+0x20/0x2c
> pci_epf_remove_cfs+0x17c/0x198
> pci_epf_unregister_driver+0x18/0x30
> nvmet_pci_epf_cleanup_module+0x24/0x30 [nvmet_pci_epf]
> __arm64_sys_delete_module+0x264/0x424
> invoke_syscall+0x70/0x260
> el0_svc_common.constprop.0+0xac/0x230
> do_el0_svc+0x40/0x58
> el0_svc+0x48/0xdc
> el0t_64_sync_handler+0x10c/0x138
> el0t_64_sync+0x198/0x19c
> ...
>
> Remove this list_del() call from pci_epf_remove_cfs() and replace it
> with a warning if the epf_group list is not empty.
>
> Fixes: ef1433f717a2 ("PCI: endpoint: Create configfs entry for each pci_epf_device_id table entry")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
> drivers/pci/endpoint/pci-ep-cfs.c | 1 +
> drivers/pci/endpoint/pci-epf-core.c | 2 +-
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
> index d712c7a866d2..63876537e7dc 100644
> --- a/drivers/pci/endpoint/pci-ep-cfs.c
> +++ b/drivers/pci/endpoint/pci-ep-cfs.c
> @@ -691,6 +691,7 @@ void pci_ep_cfs_remove_epf_group(struct config_group *group)
> if (IS_ERR_OR_NULL(group))
> return;
>
> + list_del_init(&group->group_entry);
> configfs_unregister_default_group(group);
> }
> EXPORT_SYMBOL(pci_ep_cfs_remove_epf_group);
> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index 577a9e490115..167dc6ee63f7 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -338,7 +338,7 @@ static void pci_epf_remove_cfs(struct pci_epf_driver *driver)
> mutex_lock(&pci_epf_mutex);
> list_for_each_entry_safe(group, tmp, &driver->epf_group, group_entry)
> pci_ep_cfs_remove_epf_group(group);
> - list_del(&driver->epf_group);
> + WARN_ON(!list_empty(&driver->epf_group));
> mutex_unlock(&pci_epf_mutex);
> }
>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: endpoint: Fix configfs group removal on driver teardown
2025-06-17 1:07 [PATCH] PCI: endpoint: Fix configfs group removal on driver teardown Damien Le Moal
2025-06-20 3:04 ` Damien Le Moal
@ 2025-06-23 10:00 ` Niklas Cassel
2025-06-23 12:01 ` Damien Le Moal
1 sibling, 1 reply; 6+ messages in thread
From: Niklas Cassel @ 2025-06-23 10:00 UTC (permalink / raw)
To: Damien Le Moal
Cc: linux-pci, Manivannan Sadhasivam, Krzysztof Wilczyński,
Kishon Vijay Abraham I, Bjorn Helgaas
Hello Damien,
On Tue, Jun 17, 2025 at 10:07:37AM +0900, Damien Le Moal wrote:
> An endpoint driver configfs attributes group is added to the
> epf_group list of struct pci_epf_driver pci_epf_add_cfs() but not
> removed from this list when the attribute group is unregistered with
> pci_ep_cfs_remove_epf_group(). Add the missing list_del_init() call in
> that function to correctly remove the attribute group from the driver
> list.
This seems like a bug (bug #1).
>
> Furthermore, doing a list_del() on the epf_group field of struct
> pci_epf_driver in pci_epf_remove_cfs() is not correct as this field is a
> list head, not a list entry, and triggers a KASAN warning:
This seems like another bug (bug #2).
I do understand that both bugs were introduced by the same commit.
However, since it is two separate bugs, I personally think that they
deserve two separate patches (even if they will have the same Fixes tag).
>
> ==================================================================
> BUG: KASAN: slab-use-after-free in pci_epf_remove_cfs+0x17c/0x198
> Write of size 8 at addr ffff00010f4a0d80 by task rmmod/319
>
> CPU: 3 UID: 0 PID: 319 Comm: rmmod Not tainted 6.16.0-rc2 #1 NONE
> Hardware name: Radxa ROCK 5B (DT)
> Call trace:
> show_stack+0x2c/0x84 (C)
> dump_stack_lvl+0x70/0x98
> print_report+0x17c/0x538
> kasan_report+0xb8/0x190
> __asan_report_store8_noabort+0x20/0x2c
> pci_epf_remove_cfs+0x17c/0x198
> pci_epf_unregister_driver+0x18/0x30
> nvmet_pci_epf_cleanup_module+0x24/0x30 [nvmet_pci_epf]
> __arm64_sys_delete_module+0x264/0x424
> invoke_syscall+0x70/0x260
> el0_svc_common.constprop.0+0xac/0x230
> do_el0_svc+0x40/0x58
> el0_svc+0x48/0xdc
> el0t_64_sync_handler+0x10c/0x138
> el0t_64_sync+0x198/0x19c
This KASAN splat seems to belong to bug #2.
I think it is a litte bit confusing that you attach a KASAN
splat to a patch that fixes two different bugs.
Surely this KASAN bug can be fixes with only:
- list_del(&driver->epf_group);
+ WARN_ON(!list_empty(&driver->epf_group));
So I think it would make more sense if the patch that fixes the KASAN splat
includes only the changes that are needed to fix the KASAN splat, and then
for the other bug, create a different patch that will then have a clearer
commit message (because it will not have an unrelated KASAN splat in it).
Kind regards,
Niklas
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: endpoint: Fix configfs group removal on driver teardown
2025-06-23 10:00 ` Niklas Cassel
@ 2025-06-23 12:01 ` Damien Le Moal
2025-06-23 13:35 ` Niklas Cassel
0 siblings, 1 reply; 6+ messages in thread
From: Damien Le Moal @ 2025-06-23 12:01 UTC (permalink / raw)
To: Niklas Cassel
Cc: linux-pci, Manivannan Sadhasivam, Krzysztof Wilczyński,
Kishon Vijay Abraham I, Bjorn Helgaas
On 6/23/25 19:00, Niklas Cassel wrote:
> Hello Damien,
>
> On Tue, Jun 17, 2025 at 10:07:37AM +0900, Damien Le Moal wrote:
>> An endpoint driver configfs attributes group is added to the
>> epf_group list of struct pci_epf_driver pci_epf_add_cfs() but not
>> removed from this list when the attribute group is unregistered with
>> pci_ep_cfs_remove_epf_group(). Add the missing list_del_init() call in
>> that function to correctly remove the attribute group from the driver
>> list.
>
> This seems like a bug (bug #1).
>
>>
>> Furthermore, doing a list_del() on the epf_group field of struct
>> pci_epf_driver in pci_epf_remove_cfs() is not correct as this field is a
>> list head, not a list entry, and triggers a KASAN warning:
>
> This seems like another bug (bug #2).
>
>
> I do understand that both bugs were introduced by the same commit.
>
> However, since it is two separate bugs, I personally think that they
> deserve two separate patches (even if they will have the same Fixes tag).
>
>
>>
>> ==================================================================
>> BUG: KASAN: slab-use-after-free in pci_epf_remove_cfs+0x17c/0x198
>> Write of size 8 at addr ffff00010f4a0d80 by task rmmod/319
>>
>> CPU: 3 UID: 0 PID: 319 Comm: rmmod Not tainted 6.16.0-rc2 #1 NONE
>> Hardware name: Radxa ROCK 5B (DT)
>> Call trace:
>> show_stack+0x2c/0x84 (C)
>> dump_stack_lvl+0x70/0x98
>> print_report+0x17c/0x538
>> kasan_report+0xb8/0x190
>> __asan_report_store8_noabort+0x20/0x2c
>> pci_epf_remove_cfs+0x17c/0x198
>> pci_epf_unregister_driver+0x18/0x30
>> nvmet_pci_epf_cleanup_module+0x24/0x30 [nvmet_pci_epf]
>> __arm64_sys_delete_module+0x264/0x424
>> invoke_syscall+0x70/0x260
>> el0_svc_common.constprop.0+0xac/0x230
>> do_el0_svc+0x40/0x58
>> el0_svc+0x48/0xdc
>> el0t_64_sync_handler+0x10c/0x138
>> el0t_64_sync+0x198/0x19c
>
> This KASAN splat seems to belong to bug #2.
>
>
>
> I think it is a litte bit confusing that you attach a KASAN
> splat to a patch that fixes two different bugs.
>
> Surely this KASAN bug can be fixes with only:
>
> - list_del(&driver->epf_group);
> + WARN_ON(!list_empty(&driver->epf_group));
Yes, with that, you will not get the KASAN warning, but you will get the
WARN_ON() to trigger unless bug #1 is applied first. But if you apply #1 first,
then any testing done with that bug fix only will trigger a NULL pointer
de-reference on the list_del().
For this reason, I very much dislike the idea of splitting this patch into 2
different patches as the bugs are inter-dependent.
Unless Mani insists on a split, I will not do it.
> So I think it would make more sense if the patch that fixes the KASAN splat
> includes only the changes that are needed to fix the KASAN splat, and then
> for the other bug, create a different patch that will then have a clearer
> commit message (because it will not have an unrelated KASAN splat in it).
Again, no. I do not like this idea. I can improve the commit message to explain
that more clearly though.
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: endpoint: Fix configfs group removal on driver teardown
2025-06-23 12:01 ` Damien Le Moal
@ 2025-06-23 13:35 ` Niklas Cassel
2025-06-24 0:58 ` Damien Le Moal
0 siblings, 1 reply; 6+ messages in thread
From: Niklas Cassel @ 2025-06-23 13:35 UTC (permalink / raw)
To: Damien Le Moal
Cc: linux-pci, Manivannan Sadhasivam, Krzysztof Wilczyński,
Kishon Vijay Abraham I, Bjorn Helgaas
On Mon, Jun 23, 2025 at 09:01:53PM +0900, Damien Le Moal wrote:
> On 6/23/25 19:00, Niklas Cassel wrote:
> >
> > I think it is a litte bit confusing that you attach a KASAN
> > splat to a patch that fixes two different bugs.
> >
> > Surely this KASAN bug can be fixes with only:
> >
> > - list_del(&driver->epf_group);
> > + WARN_ON(!list_empty(&driver->epf_group));
>
> Yes, with that, you will not get the KASAN warning, but you will get the
> WARN_ON() to trigger unless bug #1 is applied first. But if you apply #1 first,
> then any testing done with that bug fix only will trigger a NULL pointer
> de-reference on the list_del().
Ok, let me rephrase :)
Surely this KASAN bug can be fixed with only:
- list_del(&driver->epf_group);
As the bug is that the code is trying to delete a list head, which is just
wrong.
A patch 2/2 could add a list_del() to pci_ep_cfs_remove_epf_group() and the
WARN_ON() to pci_epf_remove_cfs().
Considering that pci_ep_cfs_remove_epf_group() also frees the memory for
the group, I think it is more correct to use list_del() rather than
list_del_init(), as no one will be able to re-use this group after calling
pci_ep_cfs_remove_epf_group() on it.
Kind regards,
Niklas
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: endpoint: Fix configfs group removal on driver teardown
2025-06-23 13:35 ` Niklas Cassel
@ 2025-06-24 0:58 ` Damien Le Moal
0 siblings, 0 replies; 6+ messages in thread
From: Damien Le Moal @ 2025-06-24 0:58 UTC (permalink / raw)
To: Niklas Cassel
Cc: linux-pci, Manivannan Sadhasivam, Krzysztof Wilczyński,
Kishon Vijay Abraham I, Bjorn Helgaas
On 6/23/25 10:35 PM, Niklas Cassel wrote:
> On Mon, Jun 23, 2025 at 09:01:53PM +0900, Damien Le Moal wrote:
>> On 6/23/25 19:00, Niklas Cassel wrote:
>>>
>>> I think it is a litte bit confusing that you attach a KASAN
>>> splat to a patch that fixes two different bugs.
>>>
>>> Surely this KASAN bug can be fixes with only:
>>>
>>> - list_del(&driver->epf_group);
>>> + WARN_ON(!list_empty(&driver->epf_group));
>>
>> Yes, with that, you will not get the KASAN warning, but you will get the
>> WARN_ON() to trigger unless bug #1 is applied first. But if you apply #1 first,
>> then any testing done with that bug fix only will trigger a NULL pointer
>> de-reference on the list_del().
>
> Ok, let me rephrase :)
>
> Surely this KASAN bug can be fixed with only:
>
> - list_del(&driver->epf_group);
>
> As the bug is that the code is trying to delete a list head, which is just
> wrong.
>
>
> A patch 2/2 could add a list_del() to pci_ep_cfs_remove_epf_group() and the
> WARN_ON() to pci_epf_remove_cfs().
>
> Considering that pci_ep_cfs_remove_epf_group() also frees the memory for
> the group, I think it is more correct to use list_del() rather than
> list_del_init(), as no one will be able to re-use this group after calling
> pci_ep_cfs_remove_epf_group() on it.
OK. Sending v2.
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-06-24 1:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-17 1:07 [PATCH] PCI: endpoint: Fix configfs group removal on driver teardown Damien Le Moal
2025-06-20 3:04 ` Damien Le Moal
2025-06-23 10:00 ` Niklas Cassel
2025-06-23 12:01 ` Damien Le Moal
2025-06-23 13:35 ` Niklas Cassel
2025-06-24 0:58 ` Damien Le Moal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).