Re: [PATCH rdma-next v1 1/2] lib/scatterlist: Fix wrong update of orig_nents

Re: [PATCH rdma-next v1 1/2] lib/scatterlist: Fix wrong update of orig_nents

[Marek Szyprowski]

Hi Leon,

On 29.06.2021 10:40, Leon Romanovsky wrote:
> From: Maor Gottlieb <maorg@nvidia.com>
>
> 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 <maorg@nvidia.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>

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 = 0x96000004
   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 = 0x00000004
   CM = 0, WnR = 0
[00376f42a6e40454] address between user and kernel address ranges
Internal error: Oops: 96000004 [#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: 80000005 (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 52800004 540000a2 (f8656813)
---[ end trace 4ba4f0c9c48711a1 ]---
Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b

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 sg table after the current one.
> -		 * sg_size is then one less than alloc size, since the last
> -		 * element is the chain pointer.
>   		 */
>   		if (alloc_size > curr_max_ents) {
>   			next = sg_chain_ptr(&sgl[curr_max_ents - 1]);
>   			alloc_size = curr_max_ents;
> -			sg_size = alloc_size - 1;
> -		} else {
> -			sg_size = alloc_size;
> -			next = NULL;
>   		}
>   
> -		table->orig_nents -= sg_size;
> +		table->total_nents -= alloc_size;
>   		if (nents_first_chunk)
>   			nents_first_chunk = 0;
>   		else
> @@ -301,20 +294,11 @@ int __sg_alloc_table(struct sg_table *table, unsigned int nents,
>   		} else {
>   			sg = alloc_fn(alloc_size, gfp_mask);
>   		}
> -		if (unlikely(!sg)) {
> -			/*
> -			 * Adjust entry count to reflect that the last
> -			 * entry of the previous table won't be used for
> -			 * linkage.  Without this, sg_kfree() may get
> -			 * confused.
> -			 */
> -			if (prv)
> -				table->nents = ++table->orig_nents;
> -
> +		if (unlikely(!sg))
>   			return -ENOMEM;
> -		}
>   
>   		sg_init_table(sg, alloc_size);
> +		table->total_nents += alloc_size;
>   		table->nents = table->orig_nents += sg_size;
>   
>   		/*
> @@ -385,12 +369,11 @@ static struct scatterlist *get_next_sg(struct sg_table *table,
>   	if (!new_sg)
>   		return ERR_PTR(-ENOMEM);
>   	sg_init_table(new_sg, alloc_size);
> +	table->total_nents += alloc_size;
>   	if (cur) {
>   		__sg_chain(next_sg, new_sg);
> -		table->orig_nents += alloc_size - 1;
>   	} else {
>   		table->sgl = new_sg;
> -		table->orig_nents = alloc_size;
>   		table->nents = 0;
>   	}
>   	return new_sg;
> @@ -515,6 +498,7 @@ struct scatterlist *__sg_alloc_table_from_pages(struct sg_table *sgt,
>   		cur_page = j;
>   	}
>   	sgt->nents += added_nents;
> +	sgt->orig_nents = sgt->nents;
>   out:
>   	if (!left_pages)
>   		sg_mark_end(s);

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

_______________________________________________
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

[Leon Romanovsky]

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 <maorg@nvidia.com>
> >
> > 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 <maorg@nvidia.com>
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>

<...>

> 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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3549 bytes --]

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 <maorg@nvidia.com>
> >
> > 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 <maorg@nvidia.com>
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> 
> 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 = 0x96000004
>    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 = 0x00000004
>    CM = 0, WnR = 0
> [00376f42a6e40454] address between user and kernel address ranges
> Internal error: Oops: 96000004 [#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: 80000005 (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 52800004 540000a2 (f8656813)
> ---[ end trace 4ba4f0c9c48711a1 ]---
> Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
> 
> 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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 156 bytes --]

_______________________________________________
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

[Leon Romanovsky]

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 <maorg@nvidia.com>
> > >
> > > 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 <maorg@nvidia.com>
> > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > 
> > 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 = 0x96000004
> >    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 = 0x00000004
> >    CM = 0, WnR = 0
> > [00376f42a6e40454] address between user and kernel address ranges
> > Internal error: Oops: 96000004 [#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: 80000005 (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 52800004 540000a2 (f8656813)
> > ---[ end trace 4ba4f0c9c48711a1 ]---
> > Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
> > 
> > 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