[PATCH v2] misc: pci_endpoint_test: Handle BAR sizes larger than INT_MAX

[PATCH v2] misc: pci_endpoint_test: Handle BAR sizes larger than INT_MAX

[Niklas Cassel]

Running 'pcitest -b 0' fails with "TEST FAILED" when the BAR0 size
is e.g. 8 GB.

The return value of the pci_resource_len() macro can be larger than that
of a signed integer type. Thus, when using 'pcitest' with an 8 GB BAR,
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.

In order to handle 64-bit resource_type_t on 32-bit platforms, we would
have needed to use a function like div_u64() or similar. Instead, change
the code to use addition instead of division. This avoids the need for
div_u64() or similar, while also simplifying the code.

Co-developed-by: Hans Zhang <18255117159@163.com>
Signed-off-by: Hans Zhang <18255117159@163.com>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
Changes since v1:
-Add reason for why division was changed to addition in commit log.

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

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index d5ac71a49386..8e48a15100f1 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -272,9 +272,9 @@ static const u32 bar_test_pattern[] = {
 };
 
 static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
-					enum pci_barno barno, int offset,
-					void *write_buf, void *read_buf,
-					int size)
+					enum pci_barno barno,
+					resource_size_t offset, void *write_buf,
+					void *read_buf, int size)
 {
 	memset(write_buf, bar_test_pattern[barno], size);
 	memcpy_toio(test->bar[barno] + offset, write_buf, size);
@@ -287,10 +287,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
 static int pci_endpoint_test_bar(struct pci_endpoint_test *test,
 				  enum pci_barno barno)
 {
-	int j, bar_size, buf_size, iters;
+	resource_size_t bar_size, offset = 0;
 	void *write_buf __free(kfree) = NULL;
 	void *read_buf __free(kfree) = NULL;
 	struct pci_dev *pdev = test->pdev;
+	int buf_size;
 
 	if (!test->bar[barno])
 		return -ENOMEM;
@@ -314,11 +315,12 @@ static int pci_endpoint_test_bar(struct pci_endpoint_test *test,
 	if (!read_buf)
 		return -ENOMEM;
 
-	iters = 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))
+	while (offset < bar_size) {
+		if (pci_endpoint_test_bar_memcmp(test, barno, offset, write_buf,
+						 read_buf, buf_size))
 			return -EIO;
+		offset += buf_size;
+	}
 
 	return 0;
 }
-- 
2.48.1

Re: [PATCH v2] misc: pci_endpoint_test: Handle BAR sizes larger than INT_MAX

[Frank Li]

On Fri, Jan 24, 2025 at 10:33:01AM +0100, Niklas Cassel wrote:
> Running 'pcitest -b 0' fails with "TEST FAILED" when the BAR0 size
> is e.g. 8 GB.
>
> The return value of the pci_resource_len() macro can be larger than that
> of a signed integer type. Thus, when using 'pcitest' with an 8 GB BAR,
> 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.
>
> In order to handle 64-bit resource_type_t on 32-bit platforms, we would
> have needed to use a function like div_u64() or similar. Instead, change
> the code to use addition instead of division. This avoids the need for
> div_u64() or similar, while also simplifying the code.
>
> Co-developed-by: Hans Zhang <18255117159@163.com>
> Signed-off-by: Hans Zhang <18255117159@163.com>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
> Changes since v1:
> -Add reason for why division was changed to addition in commit log.
>
>  drivers/misc/pci_endpoint_test.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index d5ac71a49386..8e48a15100f1 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -272,9 +272,9 @@ static const u32 bar_test_pattern[] = {
>  };
>
>  static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
> -					enum pci_barno barno, int offset,
> -					void *write_buf, void *read_buf,
> -					int size)
> +					enum pci_barno barno,
> +					resource_size_t offset, void *write_buf,
> +					void *read_buf, int size)
>  {
>  	memset(write_buf, bar_test_pattern[barno], size);
>  	memcpy_toio(test->bar[barno] + offset, write_buf, size);
> @@ -287,10 +287,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
>  static int pci_endpoint_test_bar(struct pci_endpoint_test *test,
>  				  enum pci_barno barno)
>  {
> -	int j, bar_size, buf_size, iters;
> +	resource_size_t bar_size, offset = 0;
>  	void *write_buf __free(kfree) = NULL;
>  	void *read_buf __free(kfree) = NULL;
>  	struct pci_dev *pdev = test->pdev;
> +	int buf_size;
>
>  	if (!test->bar[barno])
>  		return -ENOMEM;
> @@ -314,11 +315,12 @@ static int pci_endpoint_test_bar(struct pci_endpoint_test *test,
>  	if (!read_buf)
>  		return -ENOMEM;
>
> -	iters = 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))
> +	while (offset < bar_size) {
> +		if (pci_endpoint_test_bar_memcmp(test, barno, offset, write_buf,
> +						 read_buf, buf_size))
>  			return -EIO;
> +		offset += buf_size;
> +	}
>
>  	return 0;
>  }
> --
> 2.48.1
>

Re: [PATCH v2] misc: pci_endpoint_test: Handle BAR sizes larger than INT_MAX

[Jon Hunter]

On 24/01/2025 09:33, Niklas Cassel wrote:
> Running 'pcitest -b 0' fails with "TEST FAILED" when the BAR0 size
> is e.g. 8 GB.
> 
> The return value of the pci_resource_len() macro can be larger than that
> of a signed integer type. Thus, when using 'pcitest' with an 8 GB BAR,
> 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.
> 
> In order to handle 64-bit resource_type_t on 32-bit platforms, we would
> have needed to use a function like div_u64() or similar. Instead, change
> the code to use addition instead of division. This avoids the need for
> div_u64() or similar, while also simplifying the code.
> 
> Co-developed-by: Hans Zhang <18255117159@163.com>
> Signed-off-by: Hans Zhang <18255117159@163.com>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
> Changes since v1:
> -Add reason for why division was changed to addition in commit log.
> 
>   drivers/misc/pci_endpoint_test.c | 18 ++++++++++--------
>   1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index d5ac71a49386..8e48a15100f1 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -272,9 +272,9 @@ static const u32 bar_test_pattern[] = {
>   };
>   
>   static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
> -					enum pci_barno barno, int offset,
> -					void *write_buf, void *read_buf,
> -					int size)
> +					enum pci_barno barno,
> +					resource_size_t offset, void *write_buf,
> +					void *read_buf, int size)
>   {
>   	memset(write_buf, bar_test_pattern[barno], size);
>   	memcpy_toio(test->bar[barno] + offset, write_buf, size);
> @@ -287,10 +287,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
>   static int pci_endpoint_test_bar(struct pci_endpoint_test *test,
>   				  enum pci_barno barno)
>   {
> -	int j, bar_size, buf_size, iters;
> +	resource_size_t bar_size, offset = 0;
>   	void *write_buf __free(kfree) = NULL;
>   	void *read_buf __free(kfree) = NULL;
>   	struct pci_dev *pdev = test->pdev;
> +	int buf_size;
>   
>   	if (!test->bar[barno])
>   		return -ENOMEM;
> @@ -314,11 +315,12 @@ static int pci_endpoint_test_bar(struct pci_endpoint_test *test,
>   	if (!read_buf)
>   		return -ENOMEM;
>   
> -	iters = 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))
> +	while (offset < bar_size) {
> +		if (pci_endpoint_test_bar_memcmp(test, barno, offset, write_buf,
> +						 read_buf, buf_size))
>   			return -EIO;
> +		offset += buf_size;
> +	}
>   
>   	return 0;
>   }


This builds fine for me! I have ran through our testsuite and so ...

Tested-by: Jon Hunter <jonathanh@nvidia.com>

Jon

-- 
nvpublic

Re: [PATCH v2] misc: pci_endpoint_test: Handle BAR sizes larger than INT_MAX

[Manivannan Sadhasivam]

On Fri, Jan 24, 2025 at 10:33:01AM +0100, Niklas Cassel wrote:
> Running 'pcitest -b 0' fails with "TEST FAILED" when the BAR0 size
> is e.g. 8 GB.
> 
> The return value of the pci_resource_len() macro can be larger than that
> of a signed integer type. Thus, when using 'pcitest' with an 8 GB BAR,
> 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.
> 
> In order to handle 64-bit resource_type_t on 32-bit platforms, we would
> have needed to use a function like div_u64() or similar. Instead, change
> the code to use addition instead of division. This avoids the need for
> div_u64() or similar, while also simplifying the code.
> 

Fixes tag?

> Co-developed-by: Hans Zhang <18255117159@163.com>
> Signed-off-by: Hans Zhang <18255117159@163.com>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

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

> ---
> Changes since v1:
> -Add reason for why division was changed to addition in commit log.

You removed the history of this patch that was present in v1. Since there is
no link to v1, you could've kept it here for the sake of completeness.

- Mani

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

Re: [PATCH v2] misc: pci_endpoint_test: Handle BAR sizes larger than INT_MAX

[Niklas Cassel]

On Sat, Feb 01, 2025 at 09:31:54PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Jan 24, 2025 at 10:33:01AM +0100, Niklas Cassel wrote:
> > Running 'pcitest -b 0' fails with "TEST FAILED" when the BAR0 size
> > is e.g. 8 GB.
> > 
> > The return value of the pci_resource_len() macro can be larger than that
> > of a signed integer type. Thus, when using 'pcitest' with an 8 GB BAR,
> > 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.
> > 
> > In order to handle 64-bit resource_type_t on 32-bit platforms, we would
> > have needed to use a function like div_u64() or similar. Instead, change
> > the code to use addition instead of division. This avoids the need for
> > div_u64() or similar, while also simplifying the code.
> > 
> 
> Fixes tag?

size has been of type int since:
2c156ac71c6b ("misc: Add host side PCI driver for PCI test function device")

But I think a better SHA1 to use as the Fixes tag would be:
cda370ec6d1f ("misc: pci_endpoint_test: Avoid using hard-coded BAR sizes")

Which has the one that started using pci_resource_len()
(while still keeping size as type int).

Perhaps this can be amended while applying?


> 
> > Co-developed-by: Hans Zhang <18255117159@163.com>
> > Signed-off-by: Hans Zhang <18255117159@163.com>
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> > ---
> > Changes since v1:
> > -Add reason for why division was changed to addition in commit log.
> 
> You removed the history of this patch that was present in v1. Since there is
> no link to v1, you could've kept it here for the sake of completeness.

Yes, because I made the commit log way more verbose, as requested by Frank,
so it now includes references to div_u64() even though we are not using it.


Kind regards,
Niklas

Re: [PATCH v2] misc: pci_endpoint_test: Handle BAR sizes larger than INT_MAX

[Manivannan Sadhasivam]

On Sat, Feb 01, 2025 at 06:39:01PM +0100, Niklas Cassel wrote:
> On Sat, Feb 01, 2025 at 09:31:54PM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Jan 24, 2025 at 10:33:01AM +0100, Niklas Cassel wrote:
> > > Running 'pcitest -b 0' fails with "TEST FAILED" when the BAR0 size
> > > is e.g. 8 GB.
> > > 
> > > The return value of the pci_resource_len() macro can be larger than that
> > > of a signed integer type. Thus, when using 'pcitest' with an 8 GB BAR,
> > > 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.
> > > 
> > > In order to handle 64-bit resource_type_t on 32-bit platforms, we would
> > > have needed to use a function like div_u64() or similar. Instead, change
> > > the code to use addition instead of division. This avoids the need for
> > > div_u64() or similar, while also simplifying the code.
> > > 
> > 
> > Fixes tag?
> 
> size has been of type int since:
> 2c156ac71c6b ("misc: Add host side PCI driver for PCI test function device")
> 
> But I think a better SHA1 to use as the Fixes tag would be:
> cda370ec6d1f ("misc: pci_endpoint_test: Avoid using hard-coded BAR sizes")
> 
> Which has the one that started using pci_resource_len()
> (while still keeping size as type int).
> 
> Perhaps this can be amended while applying?
> 

Sure.

- Mani

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

Re: [PATCH v2] misc: pci_endpoint_test: Handle BAR sizes larger than INT_MAX

[Manivannan Sadhasivam]

On Fri, Jan 24, 2025 at 10:33:01AM +0100, Niklas Cassel wrote:
> Running 'pcitest -b 0' fails with "TEST FAILED" when the BAR0 size
> is e.g. 8 GB.
> 
> The return value of the pci_resource_len() macro can be larger than that
> of a signed integer type. Thus, when using 'pcitest' with an 8 GB BAR,
> 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.
> 
> In order to handle 64-bit resource_type_t on 32-bit platforms, we would
> have needed to use a function like div_u64() or similar. Instead, change
> the code to use addition instead of division. This avoids the need for
> div_u64() or similar, while also simplifying the code.
> 
> Co-developed-by: Hans Zhang <18255117159@163.com>
> Signed-off-by: Hans Zhang <18255117159@163.com>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Added fixes tag and applied to pci/endpoint!

- Mani

> ---
> Changes since v1:
> -Add reason for why division was changed to addition in commit log.
> 
>  drivers/misc/pci_endpoint_test.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index d5ac71a49386..8e48a15100f1 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -272,9 +272,9 @@ static const u32 bar_test_pattern[] = {
>  };
>  
>  static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
> -					enum pci_barno barno, int offset,
> -					void *write_buf, void *read_buf,
> -					int size)
> +					enum pci_barno barno,
> +					resource_size_t offset, void *write_buf,
> +					void *read_buf, int size)
>  {
>  	memset(write_buf, bar_test_pattern[barno], size);
>  	memcpy_toio(test->bar[barno] + offset, write_buf, size);
> @@ -287,10 +287,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
>  static int pci_endpoint_test_bar(struct pci_endpoint_test *test,
>  				  enum pci_barno barno)
>  {
> -	int j, bar_size, buf_size, iters;
> +	resource_size_t bar_size, offset = 0;
>  	void *write_buf __free(kfree) = NULL;
>  	void *read_buf __free(kfree) = NULL;
>  	struct pci_dev *pdev = test->pdev;
> +	int buf_size;
>  
>  	if (!test->bar[barno])
>  		return -ENOMEM;
> @@ -314,11 +315,12 @@ static int pci_endpoint_test_bar(struct pci_endpoint_test *test,
>  	if (!read_buf)
>  		return -ENOMEM;
>  
> -	iters = 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))
> +	while (offset < bar_size) {
> +		if (pci_endpoint_test_bar_memcmp(test, barno, offset, write_buf,
> +						 read_buf, buf_size))
>  			return -EIO;
> +		offset += buf_size;
> +	}
>  
>  	return 0;
>  }
> -- 
> 2.48.1
> 
> 

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