[v11 0/2] misc: pci_endpoint_test: Fix overflow of bar_size.

[v11 0/2] misc: pci_endpoint_test: Fix overflow of bar_size.

[Hans Zhang]

1. Remove the "remainder" code.
2. Fix overflow of bar_size.

Hans Zhang (2):
  misc: pci_endpoint_test: Remove the "remainder" code
  misc: pci_endpoint_test: Fix overflow of bar_size

 drivers/misc/pci_endpoint_test.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)


base-commit: ccb98ccef0e543c2bd4ef1a72270461957f3d8d0
-- 
2.25.1

[v11 1/2] misc: pci_endpoint_test: Remove the "remainder" code

[Hans Zhang]

A BAR size is always a power of two. buf_size = min(SZ_1M, bar_size);
If the BAR size is <= 1MB, there will be 1 iteration, no remainder. If
the BAR size is > 1MB, there will be more than one iteration, but the
size will always be evenly divisible by 1MB, so no remainder.

Signed-off-by: Hans Zhang <18255117159@163.com>
Reviewed-by: Niklas Cassel <cassel@kernel.org>
---
Changes since v9-v10:
https://lore.kernel.org/linux-pci/20250108072504.1696532-2-18255117159@163.com

- Remove the remain variable declaration.
---
 drivers/misc/pci_endpoint_test.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 3aaaf47fa4ee..f78c7540c52c 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -280,7 +280,7 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
 static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
 				  enum pci_barno barno)
 {
-	int j, bar_size, buf_size, iters, remain;
+	int j, bar_size, buf_size, iters;
 	void *write_buf __free(kfree) = NULL;
 	void *read_buf __free(kfree) = NULL;
 	struct pci_dev *pdev = test->pdev;
@@ -313,12 +313,6 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
 						 write_buf, read_buf, buf_size))
 			return false;
 
-	remain = bar_size % buf_size;
-	if (remain)
-		if (pci_endpoint_test_bar_memcmp(test, barno, buf_size * iters,
-						 write_buf, read_buf, remain))
-			return false;
-
 	return true;
 }
 
-- 
2.25.1

[v11 2/2] misc: pci_endpoint_test: Fix overflow of bar_size

[Hans Zhang]

With 8GB BAR2, running pcitest -b 2 fails with "TEST FAILED".

The return value of the `pci_resource_len` interface is not an integer.
Using `pcitest` with an 8GB BAR2, the bar_size of integer type will
overflow.

Change the data type of bar_size from integer to resource_size_t, to fix
the above issue.

Signed-off-by: Hans Zhang <18255117159@163.com>
Suggested-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Niklas Cassel <cassel@kernel.org>
---
Changes since v10:
https://lore.kernel.org/linux-pci/20250108080951.1700230-3-18255117159@163.com/

- Replace do_div with the div_u64 API.

Changes since v8-v9:
https://lore.kernel.org/linux-pci/20250104151652.1652181-1-18255117159@163.com/

- Split the patch.

Changes since v4-v7:
https://lore.kernel.org/linux-pci/20250102120222.1403906-1-18255117159@163.com/

- Fix 32-bit OS warnings and errors.
- Fix undefined reference to `__udivmoddi4`

Changes since v3:
https://lore.kernel.org/linux-pci/20241221141009.27317-1-18255117159@163.com/

- The patch subject were modified.

Changes since v2:
https://lore.kernel.org/linux-pci/20241220075253.16791-1-18255117159@163.com/

- Fix "changes" part goes below the --- line
- The patch commit message were modified.

Changes since v1:
https://lore.kernel.org/linux-pci/20241217121220.19676-1-18255117159@163.com/

- The patch subject and commit message were modified.
---
 drivers/misc/pci_endpoint_test.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index f78c7540c52c..0f6291801078 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -280,10 +280,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
 static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
 				  enum pci_barno barno)
 {
-	int j, bar_size, buf_size, iters;
+	int j, buf_size, iters;
 	void *write_buf __free(kfree) = NULL;
 	void *read_buf __free(kfree) = NULL;
 	struct pci_dev *pdev = test->pdev;
+	resource_size_t bar_size;
 
 	if (!test->bar[barno])
 		return false;
@@ -307,7 +308,7 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
 	if (!read_buf)
 		return false;
 
-	iters = bar_size / buf_size;
+	iters = div_u64(bar_size, buf_size);
 	for (j = 0; j < iters; j++)
 		if (pci_endpoint_test_bar_memcmp(test, barno, buf_size * j,
 						 write_buf, read_buf, buf_size))
-- 
2.25.1

Re: [v11 0/2] misc: pci_endpoint_test: Fix overflow of bar_size.

[Krzysztof Wilczyński]

Hello,

> 1. Remove the "remainder" code.
> 2. Fix overflow of bar_size.

Applied to misc for v6.14, thank you!

	Krzysztof

Re: [v11 1/2] misc: pci_endpoint_test: Remove the "remainder" code

[Manivannan Sadhasivam]

On Thu, Jan 09, 2025 at 05:45:55PM +0800, Hans Zhang wrote:
> A BAR size is always a power of two. buf_size = min(SZ_1M, bar_size);
> If the BAR size is <= 1MB, there will be 1 iteration, no remainder. If
> the BAR size is > 1MB, there will be more than one iteration, but the
> size will always be evenly divisible by 1MB, so no remainder.
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> Reviewed-by: Niklas Cassel <cassel@kernel.org>
> ---
> Changes since v9-v10:
> https://lore.kernel.org/linux-pci/20250108072504.1696532-2-18255117159@163.com
> 
> - Remove the remain variable declaration.
> ---
>  drivers/misc/pci_endpoint_test.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 3aaaf47fa4ee..f78c7540c52c 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -280,7 +280,7 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
>  static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
>  				  enum pci_barno barno)
>  {
> -	int j, bar_size, buf_size, iters, remain;
> +	int j, bar_size, buf_size, iters;
>  	void *write_buf __free(kfree) = NULL;
>  	void *read_buf __free(kfree) = NULL;
>  	struct pci_dev *pdev = test->pdev;
> @@ -313,12 +313,6 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
>  						 write_buf, read_buf, buf_size))
>  			return false;
>  
> -	remain = bar_size % buf_size;
> -	if (remain)
> -		if (pci_endpoint_test_bar_memcmp(test, barno, buf_size * iters,
> -						 write_buf, read_buf, remain))
> -			return false;
> -
>  	return true;
>  }
>  
> -- 
> 2.25.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [v11 2/2] misc: pci_endpoint_test: Fix overflow of bar_size

[Manivannan Sadhasivam]

On Thu, Jan 09, 2025 at 05:45:56PM +0800, Hans Zhang wrote:
> With 8GB BAR2, running pcitest -b 2 fails with "TEST FAILED".
> 
> The return value of the `pci_resource_len` interface is not an integer.
> Using `pcitest` with an 8GB BAR2, the bar_size of integer type will
> overflow.
> 
> Change the data type of bar_size from integer to resource_size_t, to fix
> the above issue.
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>
> Suggested-by: Niklas Cassel <cassel@kernel.org>
> Reviewed-by: Niklas Cassel <cassel@kernel.org>
> ---
> Changes since v10:
> https://lore.kernel.org/linux-pci/20250108080951.1700230-3-18255117159@163.com/
> 
> - Replace do_div with the div_u64 API.
> 
> Changes since v8-v9:
> https://lore.kernel.org/linux-pci/20250104151652.1652181-1-18255117159@163.com/
> 
> - Split the patch.
> 
> Changes since v4-v7:
> https://lore.kernel.org/linux-pci/20250102120222.1403906-1-18255117159@163.com/
> 
> - Fix 32-bit OS warnings and errors.
> - Fix undefined reference to `__udivmoddi4`
> 
> Changes since v3:
> https://lore.kernel.org/linux-pci/20241221141009.27317-1-18255117159@163.com/
> 
> - The patch subject were modified.
> 
> Changes since v2:
> https://lore.kernel.org/linux-pci/20241220075253.16791-1-18255117159@163.com/
> 
> - Fix "changes" part goes below the --- line
> - The patch commit message were modified.
> 
> Changes since v1:
> https://lore.kernel.org/linux-pci/20241217121220.19676-1-18255117159@163.com/
> 
> - The patch subject and commit message were modified.
> ---
>  drivers/misc/pci_endpoint_test.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index f78c7540c52c..0f6291801078 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -280,10 +280,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
>  static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
>  				  enum pci_barno barno)
>  {
> -	int j, bar_size, buf_size, iters;
> +	int j, buf_size, iters;
>  	void *write_buf __free(kfree) = NULL;
>  	void *read_buf __free(kfree) = NULL;
>  	struct pci_dev *pdev = test->pdev;
> +	resource_size_t bar_size;
>  
>  	if (!test->bar[barno])
>  		return false;
> @@ -307,7 +308,7 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
>  	if (!read_buf)
>  		return false;
>  
> -	iters = bar_size / buf_size;
> +	iters = div_u64(bar_size, buf_size);

This change is worth mentioning in the commit message. With that,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [v11 2/2] misc: pci_endpoint_test: Fix overflow of bar_size

[Niklas Cassel]

Hello Hans,

On Wed, Jan 15, 2025 at 10:28:42PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Jan 09, 2025 at 05:45:56PM +0800, Hans Zhang wrote:
> > With 8GB BAR2, running pcitest -b 2 fails with "TEST FAILED".
> > 
> > The return value of the `pci_resource_len` interface is not an integer.
> > Using `pcitest` with an 8GB BAR2, the bar_size of integer type will
> > overflow.
> > 
> > Change the data type of bar_size from integer to resource_size_t, to fix
> > the above issue.
> > 
> > Signed-off-by: Hans Zhang <18255117159@163.com>
> > Suggested-by: Niklas Cassel <cassel@kernel.org>

I intended to stay silent, but considering that Mani gave additional
feedback:

- The Suggested-by tag should be in patch 1/2, not 2/2 :)
  (Patch 2/2 was 100% you.)

- No need to keep the change log all the way back since V1, usually
  people just keep the change log since the last revision.


Kind regards,
Niklas

Re: [v11 2/2] misc: pci_endpoint_test: Fix overflow of bar_size

[Krzysztof Wilczyński]

Hello,

> I intended to stay silent, but considering that Mani gave additional
> feedback:

I am glad you didn't as feedback is always appreciated. :)

> - The Suggested-by tag should be in patch 1/2, not 2/2 :)
>   (Patch 2/2 was 100% you.)

I moved the Suggested-by tag around the correct patch.

Thank you!

	Krzysztof

Re: [v11 2/2] misc: pci_endpoint_test: Fix overflow of bar_size

[Niklas Cassel]

Hello Krzysztof,

On Thu, Jan 16, 2025 at 11:03:00AM +0900, Krzysztof Wilczyński wrote:
> Hello,
> 
> > I intended to stay silent, but considering that Mani gave additional
> > feedback:
> 
> I am glad you didn't as feedback is always appreciated. :)
> 
> > - The Suggested-by tag should be in patch 1/2, not 2/2 :)
> >   (Patch 2/2 was 100% you.)
> 
> I moved the Suggested-by tag around the correct patch.
> 
> Thank you!
> 
> 	Krzysztof

Since you have already applied this series, with my comment fixed up,
could your perhaps add the following (or similar) to the commit message
in patch 2/2:


"
By changing the type to resource_size_t, which is a typedef to phys_addr_t,
which can be 64-bit in certain configurations (e.g. X86_PAE selects
PHYS_ADDR_T_64BIT), even when the compiler is 32-bit. Thus, we also need to
change the division to do_div(), to properly perform a 64-bit division when
the compiler is 32-bit.
"


in order to address Mani's comment.


Kind regards,
Niklas

Re: [v11 2/2] misc: pci_endpoint_test: Fix overflow of bar_size

[Niklas Cassel]

On Thu, Jan 16, 2025 at 10:36:18AM +0100, Niklas Cassel wrote:
> Hello Krzysztof,
> 
> On Thu, Jan 16, 2025 at 11:03:00AM +0900, Krzysztof Wilczyński wrote:
> > Hello,
> > 
> > > I intended to stay silent, but considering that Mani gave additional
> > > feedback:
> > 
> > I am glad you didn't as feedback is always appreciated. :)
> > 
> > > - The Suggested-by tag should be in patch 1/2, not 2/2 :)
> > >   (Patch 2/2 was 100% you.)
> > 
> > I moved the Suggested-by tag around the correct patch.
> > 
> > Thank you!
> > 
> > 	Krzysztof
> 
> Since you have already applied this series, with my comment fixed up,
> could your perhaps add the following (or similar) to the commit message
> in patch 2/2:
> 
> 
> "
> By changing the type to resource_size_t, which is a typedef to phys_addr_t,
> which can be 64-bit in certain configurations (e.g. X86_PAE selects
> PHYS_ADDR_T_64BIT), even when the compiler is 32-bit. Thus, we also need to
> change the division to do_div(), to properly perform a 64-bit division when
> the compiler is 32-bit.
> "

s/do_div()/div_u64()/


Kind regards,
Niklas

Re: [v11 2/2] misc: pci_endpoint_test: Fix overflow of bar_size

[Krzysztof Wilczyński]

Hello,

> > Since you have already applied this series, with my comment fixed up,
> > could your perhaps add the following (or similar) to the commit message
> > in patch 2/2:
> > 
> > 
> > "
> > By changing the type to resource_size_t, which is a typedef to phys_addr_t,
> > which can be 64-bit in certain configurations (e.g. X86_PAE selects
> > PHYS_ADDR_T_64BIT), even when the compiler is 32-bit. Thus, we also need to
> > change the division to do_div(), to properly perform a 64-bit division when
> > the compiler is 32-bit.
> > "
> 
> s/do_div()/div_u64()/

Will do.  Sounds good!

Thank you!

	Krzysztof

Re: [v11 2/2] misc: pci_endpoint_test: Fix overflow of bar_size

[Jon Hunter]

Hi Hans,

On 09/01/2025 09:45, Hans Zhang wrote:
> With 8GB BAR2, running pcitest -b 2 fails with "TEST FAILED".
> 
> The return value of the `pci_resource_len` interface is not an integer.
> Using `pcitest` with an 8GB BAR2, the bar_size of integer type will
> overflow.
> 
> Change the data type of bar_size from integer to resource_size_t, to fix
> the above issue.
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>
> Suggested-by: Niklas Cassel <cassel@kernel.org>
> Reviewed-by: Niklas Cassel <cassel@kernel.org>
> ---
> Changes since v10:
> https://lore.kernel.org/linux-pci/20250108080951.1700230-3-18255117159@163.com/
> 
> - Replace do_div with the div_u64 API.
> 
> Changes since v8-v9:
> https://lore.kernel.org/linux-pci/20250104151652.1652181-1-18255117159@163.com/
> 
> - Split the patch.
> 
> Changes since v4-v7:
> https://lore.kernel.org/linux-pci/20250102120222.1403906-1-18255117159@163.com/
> 
> - Fix 32-bit OS warnings and errors.
> - Fix undefined reference to `__udivmoddi4`
> 
> Changes since v3:
> https://lore.kernel.org/linux-pci/20241221141009.27317-1-18255117159@163.com/
> 
> - The patch subject were modified.
> 
> Changes since v2:
> https://lore.kernel.org/linux-pci/20241220075253.16791-1-18255117159@163.com/
> 
> - Fix "changes" part goes below the --- line
> - The patch commit message were modified.
> 
> Changes since v1:
> https://lore.kernel.org/linux-pci/20241217121220.19676-1-18255117159@163.com/
> 
> - The patch subject and commit message were modified.
> ---
>   drivers/misc/pci_endpoint_test.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index f78c7540c52c..0f6291801078 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -280,10 +280,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
>   static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
>   				  enum pci_barno barno)
>   {
> -	int j, bar_size, buf_size, iters;
> +	int j, buf_size, iters;
>   	void *write_buf __free(kfree) = NULL;
>   	void *read_buf __free(kfree) = NULL;
>   	struct pci_dev *pdev = test->pdev;
> +	resource_size_t bar_size;
>   
>   	if (!test->bar[barno])
>   		return false;
> @@ -307,7 +308,7 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
>   	if (!read_buf)
>   		return false;
>   
> -	iters = bar_size / buf_size;
> +	iters = div_u64(bar_size, buf_size);
>   	for (j = 0; j < iters; j++)
>   		if (pci_endpoint_test_bar_memcmp(test, barno, buf_size * j,
>   						 write_buf, read_buf, buf_size))


This change breaks building the kernel with GCC v7 and I see ...

ERROR: modpost: "__aeabi_uldivmod" [drivers/misc/pci_endpoint_test.ko] 
undefined!
ERROR: modpost: "__aeabi_ldivmod" [drivers/misc/pci_endpoint_test.ko] 
undefined!

I know that this is an old GCC version, but this is a farm builder and 
the kernel still indicates that GCC v5.1 is still supported [0].

Jon

[0] 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/process/changes.rst

-- 
nvpublic

Re: [v11 2/2] misc: pci_endpoint_test: Fix overflow of bar_size

[Bjorn Helgaas]

On Tue, Jan 21, 2025 at 05:46:43PM +0000, Jon Hunter wrote:
> On 09/01/2025 09:45, Hans Zhang wrote:
> > With 8GB BAR2, running pcitest -b 2 fails with "TEST FAILED".
> > 
> > The return value of the `pci_resource_len` interface is not an integer.
> > Using `pcitest` with an 8GB BAR2, the bar_size of integer type will
> > overflow.
> > 
> > Change the data type of bar_size from integer to resource_size_t, to fix
> > the above issue.
> > 
> > Signed-off-by: Hans Zhang <18255117159@163.com>
> > Suggested-by: Niklas Cassel <cassel@kernel.org>
> > Reviewed-by: Niklas Cassel <cassel@kernel.org>
> > ---
> > Changes since v10:
> > https://lore.kernel.org/linux-pci/20250108080951.1700230-3-18255117159@163.com/
> > 
> > - Replace do_div with the div_u64 API.
> > 
> > Changes since v8-v9:
> > https://lore.kernel.org/linux-pci/20250104151652.1652181-1-18255117159@163.com/
> > 
> > - Split the patch.
> > 
> > Changes since v4-v7:
> > https://lore.kernel.org/linux-pci/20250102120222.1403906-1-18255117159@163.com/
> > 
> > - Fix 32-bit OS warnings and errors.
> > - Fix undefined reference to `__udivmoddi4`
> > 
> > Changes since v3:
> > https://lore.kernel.org/linux-pci/20241221141009.27317-1-18255117159@163.com/
> > 
> > - The patch subject were modified.
> > 
> > Changes since v2:
> > https://lore.kernel.org/linux-pci/20241220075253.16791-1-18255117159@163.com/
> > 
> > - Fix "changes" part goes below the --- line
> > - The patch commit message were modified.
> > 
> > Changes since v1:
> > https://lore.kernel.org/linux-pci/20241217121220.19676-1-18255117159@163.com/
> > 
> > - The patch subject and commit message were modified.
> > ---
> >   drivers/misc/pci_endpoint_test.c | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> > index f78c7540c52c..0f6291801078 100644
> > --- a/drivers/misc/pci_endpoint_test.c
> > +++ b/drivers/misc/pci_endpoint_test.c
> > @@ -280,10 +280,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
> >   static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
> >   				  enum pci_barno barno)
> >   {
> > -	int j, bar_size, buf_size, iters;
> > +	int j, buf_size, iters;
> >   	void *write_buf __free(kfree) = NULL;
> >   	void *read_buf __free(kfree) = NULL;
> >   	struct pci_dev *pdev = test->pdev;
> > +	resource_size_t bar_size;
> >   	if (!test->bar[barno])
> >   		return false;
> > @@ -307,7 +308,7 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
> >   	if (!read_buf)
> >   		return false;
> > -	iters = bar_size / buf_size;
> > +	iters = div_u64(bar_size, buf_size);
> >   	for (j = 0; j < iters; j++)
> >   		if (pci_endpoint_test_bar_memcmp(test, barno, buf_size * j,
> >   						 write_buf, read_buf, buf_size))
> 
> 
> This change breaks building the kernel with GCC v7 and I see ...
> 
> ERROR: modpost: "__aeabi_uldivmod" [drivers/misc/pci_endpoint_test.ko]
> undefined!
> ERROR: modpost: "__aeabi_ldivmod" [drivers/misc/pci_endpoint_test.ko]
> undefined!
> 
> I know that this is an old GCC version, but this is a farm builder and the
> kernel still indicates that GCC v5.1 is still supported [0].
> 
> Jon
> 
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/process/changes.rst

Dropping from pci/endpoint-test for now until this gets sorted out.

For reference, the current iteration (the one I'm dropping) is below:

commit c6e73776c0c4 ("misc: pci_endpoint_test: Prevent bar_size overflow")
Author: Hans Zhang <18255117159@163.com>
Date:   Thu Jan 9 17:45:56 2025 +0800

    misc: pci_endpoint_test: Prevent bar_size overflow
    
    Running 'pcitest -b 2' fails with "TEST FAILED" when the BAR2 size
    exceeds 4GB.
    
    The return value of the pci_resource_len() macro can be larger than that
    of a signed integer type. As such, for example, when using 'pcitest'
    with an 8GB BAR2, the bar_size of the integer type will overflow.
    
    Change bar_size from integer to resource_size_t to prevent integer
    overflow for large BAR sizes with 32-bit compilers.  Similarly, use
    div_u64() to perform 64-bit division in that situation.
    
    Link: https://lore.kernel.org/r/20250109094556.1724663-3-18255117159@163.com
    Signed-off-by: Hans Zhang <18255117159@163.com>
    [kwilczynski: commit log]
    Signed-off-by: Krzysztof Wilczyński <kwilczynski@kernel.org>
    [bhelgaas: pick up commit log update from
    https://lore.kernel.org/r/Z4jTEkznMUcApzbe@ryzen]
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Reviewed-by: Niklas Cassel <cassel@kernel.org>
    Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>


diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index be0c7c870e6a..abe6c8f03fca 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -283,10 +283,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
 static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
 				  enum pci_barno barno)
 {
-	int j, bar_size, buf_size, iters;
+	int j, buf_size, iters;
 	void *write_buf __free(kfree) = NULL;
 	void *read_buf __free(kfree) = NULL;
 	struct pci_dev *pdev = test->pdev;
+	resource_size_t bar_size;
 
 	if (!test->bar[barno])
 		return false;
@@ -310,7 +311,7 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
 	if (!read_buf)
 		return false;
 
-	iters = bar_size / buf_size;
+	iters = div_u64(bar_size, buf_size);
 	for (j = 0; j < iters; j++)
 		if (pci_endpoint_test_bar_memcmp(test, barno, buf_size * j,
 						 write_buf, read_buf, buf_size))

Re: [v11 2/2] misc: pci_endpoint_test: Fix overflow of bar_size

[Niklas Cassel]

On 21 January 2025 18:46:43 CET, Jon Hunter <jonathanh@nvidia.com> wrote:
>Hi Hans,
>
>On 09/01/2025 09:45, Hans Zhang wrote:
>> With 8GB BAR2, running pcitest -b 2 fails with "TEST FAILED".
>> 
>> The return value of the `pci_resource_len` interface is not an integer.
>> Using `pcitest` with an 8GB BAR2, the bar_size of integer type will
>> overflow.
>> 
>> Change the data type of bar_size from integer to resource_size_t, to fix
>> the above issue.
>> 
>> Signed-off-by: Hans Zhang <18255117159@163.com>
>> Suggested-by: Niklas Cassel <cassel@kernel.org>
>> Reviewed-by: Niklas Cassel <cassel@kernel.org>
>> ---
>> Changes since v10:
>> https://lore.kernel.org/linux-pci/20250108080951.1700230-3-18255117159@163.com/
>> 
>> - Replace do_div with the div_u64 API.
>> 
>> Changes since v8-v9:
>> https://lore.kernel.org/linux-pci/20250104151652.1652181-1-18255117159@163.com/
>> 
>> - Split the patch.
>> 
>> Changes since v4-v7:
>> https://lore.kernel.org/linux-pci/20250102120222.1403906-1-18255117159@163.com/
>> 
>> - Fix 32-bit OS warnings and errors.
>> - Fix undefined reference to `__udivmoddi4`
>> 
>> Changes since v3:
>> https://lore.kernel.org/linux-pci/20241221141009.27317-1-18255117159@163.com/
>> 
>> - The patch subject were modified.
>> 
>> Changes since v2:
>> https://lore.kernel.org/linux-pci/20241220075253.16791-1-18255117159@163.com/
>> 
>> - Fix "changes" part goes below the --- line
>> - The patch commit message were modified.
>> 
>> Changes since v1:
>> https://lore.kernel.org/linux-pci/20241217121220.19676-1-18255117159@163.com/
>> 
>> - The patch subject and commit message were modified.
>> ---
>>   drivers/misc/pci_endpoint_test.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
>> index f78c7540c52c..0f6291801078 100644
>> --- a/drivers/misc/pci_endpoint_test.c
>> +++ b/drivers/misc/pci_endpoint_test.c
>> @@ -280,10 +280,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
>>   static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
>>   				  enum pci_barno barno)
>>   {
>> -	int j, bar_size, buf_size, iters;
>> +	int j, buf_size, iters;
>>   	void *write_buf __free(kfree) = NULL;
>>   	void *read_buf __free(kfree) = NULL;
>>   	struct pci_dev *pdev = test->pdev;
>> +	resource_size_t bar_size;
>>     	if (!test->bar[barno])
>>   		return false;
>> @@ -307,7 +308,7 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
>>   	if (!read_buf)
>>   		return false;
>>   -	iters = bar_size / buf_size;
>> +	iters = div_u64(bar_size, buf_size);
>>   	for (j = 0; j < iters; j++)
>>   		if (pci_endpoint_test_bar_memcmp(test, barno, buf_size * j,
>>   						 write_buf, read_buf, buf_size))
>
>
>This change breaks building the kernel with GCC v7 and I see ...
>
>ERROR: modpost: "__aeabi_uldivmod" [drivers/misc/pci_endpoint_test.ko] undefined!
>ERROR: modpost: "__aeabi_ldivmod" [drivers/misc/pci_endpoint_test.ko] undefined!
>
>I know that this is an old GCC version, but this is a farm builder and the kernel still indicates that GCC v5.1 is still supported [0].
>

Hmm.. div_u64() should work on gcc 5.1.

Out of curiosity, is there any difference if you add:

#include <linux/math64.h>
to
drivers/misc/pci_endpoint_test.c ?


Kind regards,
Niklas

Re: [v11 2/2] misc: pci_endpoint_test: Fix overflow of bar_size

[Jon Hunter]

On 21/01/2025 20:31, Niklas Cassel wrote:
> 
> 
> On 21 January 2025 18:46:43 CET, Jon Hunter <jonathanh@nvidia.com> wrote:
>> Hi Hans,
>>
>> On 09/01/2025 09:45, Hans Zhang wrote:
>>> With 8GB BAR2, running pcitest -b 2 fails with "TEST FAILED".
>>>
>>> The return value of the `pci_resource_len` interface is not an integer.
>>> Using `pcitest` with an 8GB BAR2, the bar_size of integer type will
>>> overflow.
>>>
>>> Change the data type of bar_size from integer to resource_size_t, to fix
>>> the above issue.
>>>
>>> Signed-off-by: Hans Zhang <18255117159@163.com>
>>> Suggested-by: Niklas Cassel <cassel@kernel.org>
>>> Reviewed-by: Niklas Cassel <cassel@kernel.org>
>>> ---
>>> Changes since v10:
>>> https://lore.kernel.org/linux-pci/20250108080951.1700230-3-18255117159@163.com/
>>>
>>> - Replace do_div with the div_u64 API.
>>>
>>> Changes since v8-v9:
>>> https://lore.kernel.org/linux-pci/20250104151652.1652181-1-18255117159@163.com/
>>>
>>> - Split the patch.
>>>
>>> Changes since v4-v7:
>>> https://lore.kernel.org/linux-pci/20250102120222.1403906-1-18255117159@163.com/
>>>
>>> - Fix 32-bit OS warnings and errors.
>>> - Fix undefined reference to `__udivmoddi4`
>>>
>>> Changes since v3:
>>> https://lore.kernel.org/linux-pci/20241221141009.27317-1-18255117159@163.com/
>>>
>>> - The patch subject were modified.
>>>
>>> Changes since v2:
>>> https://lore.kernel.org/linux-pci/20241220075253.16791-1-18255117159@163.com/
>>>
>>> - Fix "changes" part goes below the --- line
>>> - The patch commit message were modified.
>>>
>>> Changes since v1:
>>> https://lore.kernel.org/linux-pci/20241217121220.19676-1-18255117159@163.com/
>>>
>>> - The patch subject and commit message were modified.
>>> ---
>>>    drivers/misc/pci_endpoint_test.c | 5 +++--
>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
>>> index f78c7540c52c..0f6291801078 100644
>>> --- a/drivers/misc/pci_endpoint_test.c
>>> +++ b/drivers/misc/pci_endpoint_test.c
>>> @@ -280,10 +280,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
>>>    static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
>>>    				  enum pci_barno barno)
>>>    {
>>> -	int j, bar_size, buf_size, iters;
>>> +	int j, buf_size, iters;
>>>    	void *write_buf __free(kfree) = NULL;
>>>    	void *read_buf __free(kfree) = NULL;
>>>    	struct pci_dev *pdev = test->pdev;
>>> +	resource_size_t bar_size;
>>>      	if (!test->bar[barno])
>>>    		return false;
>>> @@ -307,7 +308,7 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
>>>    	if (!read_buf)
>>>    		return false;
>>>    -	iters = bar_size / buf_size;
>>> +	iters = div_u64(bar_size, buf_size);
>>>    	for (j = 0; j < iters; j++)
>>>    		if (pci_endpoint_test_bar_memcmp(test, barno, buf_size * j,
>>>    						 write_buf, read_buf, buf_size))
>>
>>
>> This change breaks building the kernel with GCC v7 and I see ...
>>
>> ERROR: modpost: "__aeabi_uldivmod" [drivers/misc/pci_endpoint_test.ko] undefined!
>> ERROR: modpost: "__aeabi_ldivmod" [drivers/misc/pci_endpoint_test.ko] undefined!
>>
>> I know that this is an old GCC version, but this is a farm builder and the kernel still indicates that GCC v5.1 is still supported [0].
>>
> 
> Hmm.. div_u64() should work on gcc 5.1.
> 
> Out of curiosity, is there any difference if you add:
> 
> #include <linux/math64.h>
> to
> drivers/misc/pci_endpoint_test.c ?


I tried the above, but I am still seeing the same problem. So 
unfortunately, that did not fix it.

Cheers
Jon

-- 
nvpublic

Re: [v11 2/2] misc: pci_endpoint_test: Fix overflow of bar_size

[Niklas Cassel]

On Tue, Jan 21, 2025 at 05:46:43PM +0000, Jon Hunter wrote:
> > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> > index f78c7540c52c..0f6291801078 100644
> > --- a/drivers/misc/pci_endpoint_test.c
> > +++ b/drivers/misc/pci_endpoint_test.c
> > @@ -280,10 +280,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
> >   static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
> >   				  enum pci_barno barno)
> >   {
> > -	int j, bar_size, buf_size, iters;
> > +	int j, buf_size, iters;
> >   	void *write_buf __free(kfree) = NULL;
> >   	void *read_buf __free(kfree) = NULL;
> >   	struct pci_dev *pdev = test->pdev;
> > +	resource_size_t bar_size;
> >   	if (!test->bar[barno])
> >   		return false;
> > @@ -307,7 +308,7 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
> >   	if (!read_buf)
> >   		return false;
> > -	iters = bar_size / buf_size;
> > +	iters = div_u64(bar_size, buf_size);
> >   	for (j = 0; j < iters; j++)
> >   		if (pci_endpoint_test_bar_memcmp(test, barno, buf_size * j,
> >   						 write_buf, read_buf, buf_size))
> 
> 
> This change breaks building the kernel with GCC v7 and I see ...
> 
> ERROR: modpost: "__aeabi_uldivmod" [drivers/misc/pci_endpoint_test.ko]
> undefined!
> ERROR: modpost: "__aeabi_ldivmod" [drivers/misc/pci_endpoint_test.ko]
> undefined!
> 
> I know that this is an old GCC version, but this is a farm builder and the
> kernel still indicates that GCC v5.1 is still supported [0].

Arnd,

do you have any idea what is going on here?

I'm a bit puzzled, since looking at other reports of this error,
e.g.:
https://lore.kernel.org/all/20241018151016.3496613-1-arnd@kernel.org/

using div_u64() is usually the solution for this problem (for things that
are not performance critical), not the cause of it... any ideas?


Kind regards,
Niklas

Re: [v11 2/2] misc: pci_endpoint_test: Fix overflow of bar_size

[Arnd Bergmann]

On Wed, Jan 22, 2025, at 11:07, Niklas Cassel wrote:
> On Tue, Jan 21, 2025 at 05:46:43PM +0000, Jon Hunter wrote:
>> > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/>> 
>> 
>> This change breaks building the kernel with GCC v7 and I see ...
>> 
>> ERROR: modpost: "__aeabi_uldivmod" [drivers/misc/pci_endpoint_test.ko]
>> undefined!
>> ERROR: modpost: "__aeabi_ldivmod" [drivers/misc/pci_endpoint_test.ko]
>> undefined!
>> 
>> I know that this is an old GCC version, but this is a farm builder and the
>> kernel still indicates that GCC v5.1 is still supported [0].
>
> do you have any idea what is going on here?
>
> I'm a bit puzzled, since looking at other reports of this error,
> e.g.:
> https://lore.kernel.org/all/20241018151016.3496613-1-arnd@kernel.org/
>
> using div_u64() is usually the solution for this problem (for things that
> are not performance critical), not the cause of it... any ideas?

I have tried to look at the email thread, but not tried to reproduce
it yet. I have two ideas about what might be happening:

a) something causes a /different/ division to call into
   __aeabi_uldivmod(), not the one from div_u64().

b) the compiler notices one of the arguments to div_u64() being
   constant in some cases and splits the calling function into
   two special cases, for both the constant and non-constant
   cases. This sometimes confuses the __builtin_constant_p()
   in do_div() that decides to fall back to a 32-bit division.

Try looking at the .s file when you run 
'make drivers/misc/pci_endpoint_test.s' to see where exactly it
calls the two division functions, maybe you see the problem then,
otherwise I can try to reproduce it here.

    Arnd

Re: [v11 2/2] misc: pci_endpoint_test: Fix overflow of bar_size

[Jon Hunter]

On 22/01/2025 10:26, Arnd Bergmann wrote:
> On Wed, Jan 22, 2025, at 11:07, Niklas Cassel wrote:
>> On Tue, Jan 21, 2025 at 05:46:43PM +0000, Jon Hunter wrote:
>>>> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/>>
>>>
>>> This change breaks building the kernel with GCC v7 and I see ...
>>>
>>> ERROR: modpost: "__aeabi_uldivmod" [drivers/misc/pci_endpoint_test.ko]
>>> undefined!
>>> ERROR: modpost: "__aeabi_ldivmod" [drivers/misc/pci_endpoint_test.ko]
>>> undefined!
>>>
>>> I know that this is an old GCC version, but this is a farm builder and the
>>> kernel still indicates that GCC v5.1 is still supported [0].
>>
>> do you have any idea what is going on here?
>>
>> I'm a bit puzzled, since looking at other reports of this error,
>> e.g.:
>> https://lore.kernel.org/all/20241018151016.3496613-1-arnd@kernel.org/
>>
>> using div_u64() is usually the solution for this problem (for things that
>> are not performance critical), not the cause of it... any ideas?
> 
> I have tried to look at the email thread, but not tried to reproduce
> it yet. I have two ideas about what might be happening:
> 
> a) something causes a /different/ division to call into
>     __aeabi_uldivmod(), not the one from div_u64().
> 
> b) the compiler notices one of the arguments to div_u64() being
>     constant in some cases and splits the calling function into
>     two special cases, for both the constant and non-constant
>     cases. This sometimes confuses the __builtin_constant_p()
>     in do_div() that decides to fall back to a 32-bit division.
> 
> Try looking at the .s file when you run
> 'make drivers/misc/pci_endpoint_test.s' to see where exactly it
> calls the two division functions, maybe you see the problem then,
> otherwise I can try to reproduce it here.


FWIW I have encountered similar problems with this before with this 
compiler ...

https://lore.kernel.org/linux-tegra/7733a4ca-330b-4127-af12-33f376fbbc47@nvidia.com/

Jon

-- 
nvpublic

Re: [v11 2/2] misc: pci_endpoint_test: Fix overflow of bar_size

[Niklas Cassel]

On Wed, Jan 22, 2025 at 11:18:44AM +0000, Jon Hunter wrote:
> 
> On 22/01/2025 10:26, Arnd Bergmann wrote:
> > On Wed, Jan 22, 2025, at 11:07, Niklas Cassel wrote:
> > > On Tue, Jan 21, 2025 at 05:46:43PM +0000, Jon Hunter wrote:
> > > > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/>>
> > > > 
> > > > This change breaks building the kernel with GCC v7 and I see ...
> > > > 
> > > > ERROR: modpost: "__aeabi_uldivmod" [drivers/misc/pci_endpoint_test.ko]
> > > > undefined!
> > > > ERROR: modpost: "__aeabi_ldivmod" [drivers/misc/pci_endpoint_test.ko]
> > > > undefined!
> > > > 
> > > > I know that this is an old GCC version, but this is a farm builder and the
> > > > kernel still indicates that GCC v5.1 is still supported [0].
> > > 
> > > do you have any idea what is going on here?
> > > 
> > > I'm a bit puzzled, since looking at other reports of this error,
> > > e.g.:
> > > https://lore.kernel.org/all/20241018151016.3496613-1-arnd@kernel.org/
> > > 
> > > using div_u64() is usually the solution for this problem (for things that
> > > are not performance critical), not the cause of it... any ideas?
> > 
> > I have tried to look at the email thread, but not tried to reproduce
> > it yet. I have two ideas about what might be happening:
> > 
> > a) something causes a /different/ division to call into
> >     __aeabi_uldivmod(), not the one from div_u64().
> > 
> > b) the compiler notices one of the arguments to div_u64() being
> >     constant in some cases and splits the calling function into
> >     two special cases, for both the constant and non-constant
> >     cases. This sometimes confuses the __builtin_constant_p()
> >     in do_div() that decides to fall back to a 32-bit division.
> > 
> > Try looking at the .s file when you run
> > 'make drivers/misc/pci_endpoint_test.s' to see where exactly it
> > calls the two division functions, maybe you see the problem then,
> > otherwise I can try to reproduce it here.
> 
> 
> FWIW I have encountered similar problems with this before with this compiler
> ...
> 
> https://lore.kernel.org/linux-tegra/7733a4ca-330b-4127-af12-33f376fbbc47@nvidia.com/

I had no errors when building this patch using:
gcc (Debian 7.4.0-6) 7.4.0
on i386.

neither with CONFIG_PHYS_ADDR_T_64BIT=y (CONFIG_X86_PAE=y)
nor without it set.

So perhaps this is a bug in gcc 7.x on arm32 or in the arm32
implementation of div_u64() ?


Kind regards,
Niklas