Re: [PATCH rdma-next v1 1/2] lib/scatterlist: Fix wrong update of orig_nents
On Wed, Jun 30, 2021 at 07:44:21PM +0200, Thierry Reding wrote:
> On Wed, Jun 30, 2021 at 01:12:26PM +0200, Marek Szyprowski wrote:
> > Hi Leon,
> >
> > On 29.06.2021 10:40, Leon Romanovsky wrote:
> > > From: Maor Gottlieb
> > >
> > > orig_nents should represent the number of entries with pages,
> > > but __sg_alloc_table_from_pages sets orig_nents as the number of
> > > total entries in the table. This is wrong when the API is used for
> > > dynamic allocation where not all the table entries are mapped with
> > > pages. It wasn't observed until now, since RDMA umem who uses this
> > > API in the dynamic form doesn't use orig_nents implicit or explicit
> > > by the scatterlist APIs.
> > >
> > > Fix it by:
> > > 1. Set orig_nents as number of entries with pages also in
> > > __sg_alloc_table_from_pages.
> > > 2. Add a new field total_nents to reflect the total number of entries
> > > in the table. This is required for the release flow (sg_free_table).
> > > This filed should be used internally only by scatterlist.
> > >
> > > Fixes: 07da1223ec93 ("lib/scatterlist: Add support in dynamic allocation
> > > of SG table from pages")
> > > Signed-off-by: Maor Gottlieb
> > > Signed-off-by: Leon Romanovsky
> >
> > This patch landed in linux-next 20210630 as commit a52724456928
> > ("lib/scatterlist: Fix wrong update of orig_nents"). It causes serious
> > regression in DMA-IOMMU integration, which can be observed for example
> > on ARM Juno board during boot:
> >
> > Unable to handle kernel paging request at virtual address 00376f42a6e40454
> > Mem abort info:
> > ESR = 0x9604
> > EC = 0x25: DABT (current EL), IL = 32 bits
> > SET = 0, FnV = 0
> > EA = 0, S1PTW = 0
> > FSC = 0x04: level 0 translation fault
> > Data abort info:
> > ISV = 0, ISS = 0x0004
> > CM = 0, WnR = 0
> > [00376f42a6e40454] address between user and kernel address ranges
> > Internal error: Oops: 9604 [#1] PREEMPT SMP
> > Modules linked in:
> > CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.13.0-next-20210630+ #3585
> > Hardware name: ARM Juno development board (r1) (DT)
> > pstate: 8005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
> > pc : __sg_free_table+0x60/0xa0
> > lr : __sg_free_table+0x7c/0xa0
> > ..
> > Call trace:
> > __sg_free_table+0x60/0xa0
> > sg_free_table+0x1c/0x28
> > iommu_dma_alloc+0xc8/0x388
> > dma_alloc_attrs+0xcc/0xf0
> > dmam_alloc_attrs+0x68/0xb8
> > sil24_port_start+0x60/0xe0
> > ata_host_start.part.32+0xbc/0x208
> > ata_host_activate+0x64/0x150
> > sil24_init_one+0x1e8/0x268
> > local_pci_probe+0x3c/0xa0
> > pci_device_probe+0x128/0x1c8
> > really_probe+0x138/0x2d0
> > __driver_probe_device+0x78/0xd8
> > driver_probe_device+0x40/0x110
> > __driver_attach+0xcc/0x118
> > bus_for_each_dev+0x68/0xc8
> > driver_attach+0x20/0x28
> > bus_add_driver+0x168/0x1f8
> > driver_register+0x60/0x110
> > __pci_register_driver+0x5c/0x68
> > sil24_pci_driver_init+0x20/0x28
> > do_one_initcall+0x84/0x450
> > kernel_init_freeable+0x31c/0x38c
> > kernel_init+0x20/0x120
> > ret_from_fork+0x10/0x18
> > Code: d37be885 6b01007f 5284 54a2 (f8656813)
> > ---[ end trace 4ba4f0c9c48711a1 ]---
> > Kernel panic - not syncing: Attempted to kill init! exitcode=0x000b
> >
> > It looks that some changes to the scatterlist structures are missing
> > outside of the lib/scatterlist.c.
> >
> > For now I would suggest to revert this change.
>
> I see a very similar crash on Tegra during the HDA driver's probe.
>
> Leon, let me know if you need a tester for a replacement patch.
With a great pleasure, I'll contact you offline when we prepare it.
For now, this patch will be dropped.
Thanks
>
> Thanks,
> Thierry
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH rdma-next v1 1/2] lib/scatterlist: Fix wrong update of orig_nents
On Wed, Jun 30, 2021 at 01:12:26PM +0200, Marek Szyprowski wrote:
> Hi Leon,
>
> On 29.06.2021 10:40, Leon Romanovsky wrote:
> > From: Maor Gottlieb
> >
> > orig_nents should represent the number of entries with pages,
> > but __sg_alloc_table_from_pages sets orig_nents as the number of
> > total entries in the table. This is wrong when the API is used for
> > dynamic allocation where not all the table entries are mapped with
> > pages. It wasn't observed until now, since RDMA umem who uses this
> > API in the dynamic form doesn't use orig_nents implicit or explicit
> > by the scatterlist APIs.
> >
> > Fix it by:
> > 1. Set orig_nents as number of entries with pages also in
> > __sg_alloc_table_from_pages.
> > 2. Add a new field total_nents to reflect the total number of entries
> > in the table. This is required for the release flow (sg_free_table).
> > This filed should be used internally only by scatterlist.
> >
> > Fixes: 07da1223ec93 ("lib/scatterlist: Add support in dynamic allocation of
> > SG table from pages")
> > Signed-off-by: Maor Gottlieb
> > Signed-off-by: Leon Romanovsky
>
> This patch landed in linux-next 20210630 as commit a52724456928
> ("lib/scatterlist: Fix wrong update of orig_nents"). It causes serious
> regression in DMA-IOMMU integration, which can be observed for example
> on ARM Juno board during boot:
>
> Unable to handle kernel paging request at virtual address 00376f42a6e40454
> Mem abort info:
> ESR = 0x9604
> EC = 0x25: DABT (current EL), IL = 32 bits
> SET = 0, FnV = 0
> EA = 0, S1PTW = 0
> FSC = 0x04: level 0 translation fault
> Data abort info:
> ISV = 0, ISS = 0x0004
> CM = 0, WnR = 0
> [00376f42a6e40454] address between user and kernel address ranges
> Internal error: Oops: 9604 [#1] PREEMPT SMP
> Modules linked in:
> CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.13.0-next-20210630+ #3585
> Hardware name: ARM Juno development board (r1) (DT)
> pstate: 8005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
> pc : __sg_free_table+0x60/0xa0
> lr : __sg_free_table+0x7c/0xa0
> ..
> Call trace:
> __sg_free_table+0x60/0xa0
> sg_free_table+0x1c/0x28
> iommu_dma_alloc+0xc8/0x388
> dma_alloc_attrs+0xcc/0xf0
> dmam_alloc_attrs+0x68/0xb8
> sil24_port_start+0x60/0xe0
> ata_host_start.part.32+0xbc/0x208
> ata_host_activate+0x64/0x150
> sil24_init_one+0x1e8/0x268
> local_pci_probe+0x3c/0xa0
> pci_device_probe+0x128/0x1c8
> really_probe+0x138/0x2d0
> __driver_probe_device+0x78/0xd8
> driver_probe_device+0x40/0x110
> __driver_attach+0xcc/0x118
> bus_for_each_dev+0x68/0xc8
> driver_attach+0x20/0x28
> bus_add_driver+0x168/0x1f8
> driver_register+0x60/0x110
> __pci_register_driver+0x5c/0x68
> sil24_pci_driver_init+0x20/0x28
> do_one_initcall+0x84/0x450
> kernel_init_freeable+0x31c/0x38c
> kernel_init+0x20/0x120
> ret_from_fork+0x10/0x18
> Code: d37be885 6b01007f 5284 54a2 (f8656813)
> ---[ end trace 4ba4f0c9c48711a1 ]---
> Kernel panic - not syncing: Attempted to kill init! exitcode=0x000b
>
> It looks that some changes to the scatterlist structures are missing
> outside of the lib/scatterlist.c.
>
> For now I would suggest to revert this change.
I see a very similar crash on Tegra during the HDA driver's probe.
Leon, let me know if you need a tester for a replacement patch.
Thanks,
Thierry
signature.asc
Description: PGP signature
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH rdma-next v1 1/2] lib/scatterlist: Fix wrong update of orig_nents
On Wed, Jun 30, 2021 at 01:12:26PM +0200, Marek Szyprowski wrote:
> Hi Leon,
>
> On 29.06.2021 10:40, Leon Romanovsky wrote:
> > From: Maor Gottlieb
> >
> > orig_nents should represent the number of entries with pages,
> > but __sg_alloc_table_from_pages sets orig_nents as the number of
> > total entries in the table. This is wrong when the API is used for
> > dynamic allocation where not all the table entries are mapped with
> > pages. It wasn't observed until now, since RDMA umem who uses this
> > API in the dynamic form doesn't use orig_nents implicit or explicit
> > by the scatterlist APIs.
> >
> > Fix it by:
> > 1. Set orig_nents as number of entries with pages also in
> > __sg_alloc_table_from_pages.
> > 2. Add a new field total_nents to reflect the total number of entries
> > in the table. This is required for the release flow (sg_free_table).
> > This filed should be used internally only by scatterlist.
> >
> > Fixes: 07da1223ec93 ("lib/scatterlist: Add support in dynamic allocation of
> > SG table from pages")
> > Signed-off-by: Maor Gottlieb
> > Signed-off-by: Leon Romanovsky
<...>
> For now I would suggest to revert this change.
Thanks for the report, we will drop this patch.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH rdma-next v1 1/2] lib/scatterlist: Fix wrong update of orig_nents
Hi Leon,
On 29.06.2021 10:40, Leon Romanovsky wrote:
> From: Maor Gottlieb
>
> orig_nents should represent the number of entries with pages,
> but __sg_alloc_table_from_pages sets orig_nents as the number of
> total entries in the table. This is wrong when the API is used for
> dynamic allocation where not all the table entries are mapped with
> pages. It wasn't observed until now, since RDMA umem who uses this
> API in the dynamic form doesn't use orig_nents implicit or explicit
> by the scatterlist APIs.
>
> Fix it by:
> 1. Set orig_nents as number of entries with pages also in
> __sg_alloc_table_from_pages.
> 2. Add a new field total_nents to reflect the total number of entries
> in the table. This is required for the release flow (sg_free_table).
> This filed should be used internally only by scatterlist.
>
> Fixes: 07da1223ec93 ("lib/scatterlist: Add support in dynamic allocation of
> SG table from pages")
> Signed-off-by: Maor Gottlieb
> Signed-off-by: Leon Romanovsky
This patch landed in linux-next 20210630 as commit a52724456928
("lib/scatterlist: Fix wrong update of orig_nents"). It causes serious
regression in DMA-IOMMU integration, which can be observed for example
on ARM Juno board during boot:
Unable to handle kernel paging request at virtual address 00376f42a6e40454
Mem abort info:
ESR = 0x9604
EC = 0x25: DABT (current EL), IL = 32 bits
SET = 0, FnV = 0
EA = 0, S1PTW = 0
FSC = 0x04: level 0 translation fault
Data abort info:
ISV = 0, ISS = 0x0004
CM = 0, WnR = 0
[00376f42a6e40454] address between user and kernel address ranges
Internal error: Oops: 9604 [#1] PREEMPT SMP
Modules linked in:
CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.13.0-next-20210630+ #3585
Hardware name: ARM Juno development board (r1) (DT)
pstate: 8005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
pc : __sg_free_table+0x60/0xa0
lr : __sg_free_table+0x7c/0xa0
..
Call trace:
__sg_free_table+0x60/0xa0
sg_free_table+0x1c/0x28
iommu_dma_alloc+0xc8/0x388
dma_alloc_attrs+0xcc/0xf0
dmam_alloc_attrs+0x68/0xb8
sil24_port_start+0x60/0xe0
ata_host_start.part.32+0xbc/0x208
ata_host_activate+0x64/0x150
sil24_init_one+0x1e8/0x268
local_pci_probe+0x3c/0xa0
pci_device_probe+0x128/0x1c8
really_probe+0x138/0x2d0
__driver_probe_device+0x78/0xd8
driver_probe_device+0x40/0x110
__driver_attach+0xcc/0x118
bus_for_each_dev+0x68/0xc8
driver_attach+0x20/0x28
bus_add_driver+0x168/0x1f8
driver_register+0x60/0x110
__pci_register_driver+0x5c/0x68
sil24_pci_driver_init+0x20/0x28
do_one_initcall+0x84/0x450
kernel_init_freeable+0x31c/0x38c
kernel_init+0x20/0x120
ret_from_fork+0x10/0x18
Code: d37be885 6b01007f 5284 54a2 (f8656813)
---[ end trace 4ba4f0c9c48711a1 ]---
Kernel panic - not syncing: Attempted to kill init! exitcode=0x000b
It looks that some changes to the scatterlist structures are missing
outside of the lib/scatterlist.c.
For now I would suggest to revert this change.
> ---
> include/linux/scatterlist.h | 8 ++--
> lib/scatterlist.c | 32
> 2 files changed, 14 insertions(+), 26 deletions(-)
>
> diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
> index 6f70572b2938..1c889141eb91 100644
> --- a/include/linux/scatterlist.h
> +++ b/include/linux/scatterlist.h
> @@ -35,8 +35,12 @@ struct scatterlist {
>
> struct sg_table {
> struct scatterlist *sgl;/* the list */
> - unsigned int nents; /* number of mapped entries */
> - unsigned int orig_nents;/* original size of list */
> + unsigned int nents; /* number of DMA mapped entries */
> + unsigned int orig_nents;/* number of CPU mapped entries */
> + /* The fields below should be used internally only by
> + * scatterlist implementation.
> + */
> + unsigned int total_nents; /* number of total entries in the table
> */
> };
>
> /*
> diff --git a/lib/scatterlist.c b/lib/scatterlist.c
> index a59778946404..6db70a1e7dd0 100644
> --- a/lib/scatterlist.c
> +++ b/lib/scatterlist.c
> @@ -192,33 +192,26 @@ static void sg_kfree(struct scatterlist *sg, unsigned
> int nents)
> void __sg_free_table(struct sg_table *table, unsigned int max_ents,
>unsigned int nents_first_chunk, sg_free_fn *free_fn)
> {
> - struct scatterlist *sgl, *next;
> + struct scatterlist *sgl, *next = NULL;
> unsigned curr_max_ents = nents_first_chunk ?: max_ents;
>
> if (unlikely(!table->sgl))
> return;
>
> sgl = table->sgl;
> - while (table->orig_nents) {
> - unsigned int alloc_size = table->orig_nents;
> - unsigned int sg_size;
> + while (table->total_nents) {
> + unsigned int alloc_size = table->total_nents;
>
> /*
>* If we have more than max_ents segments left,
>* then assign 'next' to the
