Re: [v8] misc: pci_endpoint_test: Fix overflow of bar_size

[Niklas Cassel <cassel@kernel.org>]

On Tue, Jan 07, 2025 at 07:27:24PM +0800, Hans Zhang wrote:
> 
> 
> On 2025/1/7 18:32, Niklas Cassel wrote:
> > > > > ---
> > > > >    drivers/misc/pci_endpoint_test.c | 12 +++++++++---
> > > > >    1 file changed, 9 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> > > > > index 3aaaf47fa4ee..50d4616119af 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, remain;
> > > > >    	void *write_buf __free(kfree) = NULL;
> > > > >    	void *read_buf __free(kfree) = NULL;
> > > > >    	struct pci_dev *pdev = test->pdev;
> > > > > +	int j, buf_size, iters, remain;
> > > > > +	resource_size_t bar_size;
> > > 
> > > Fix resource_size_t to u64 bar_size.
> > > u64 bar_size;
> > > 
> > > > >    	if (!test->bar[barno])
> > > > >    		return false;
> > > > > @@ -307,13 +308,18 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
> > > > >    	if (!read_buf)
> > > > >    		return false;
> > > > > -	iters = bar_size / buf_size;
> > > > > +	if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT)) {
> > > > > +		remain = do_div(bar_size, buf_size);
> > > > > +		iters = bar_size;
> > > > > +	} else {
> > > > > +		iters = bar_size / buf_size;
> > > > > +		remain = bar_size % buf_size;
> > > > > +	}
> > > 
> > > Removed IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT), Execute the following code.
> > > 
> > > remain = do_div(bar_size, buf_size);
> > > iters = bar_size;
> > 
> > Perhaps keep it as resource_size_t and then cast it to u64 in the do_div()
> > call?
> 
> 
> Hi Niklas,
> 
> resource_size_t bar_size;
> remain = do_div((u64)bar_size, buf_size);
> 
> It works for the arm platform.
> 
> arch/arm/include/asm/div64.h
> static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
> {
> 	register unsigned int __base      asm("r4") = base;
> 	register unsigned long long __n   asm("r0") = *n;
> 	register unsigned long long __res asm("r2");
> 	unsigned int __rem;
> 	asm(	__asmeq("%0", "r0")
> 		__asmeq("%1", "r2")
> 		__asmeq("%2", "r4")
> 		"bl	__do_div64"
> 		: "+r" (__n), "=r" (__res)
> 		: "r" (__base)
> 		: "ip", "lr", "cc");
> 	__rem = __n >> 32;
> 	*n = __res;
> 	return __rem;
> }
> #define __div64_32 __div64_32
> 
> #define do_div(n, base) __div64_32(&(n), base)
> 
> 
> For X86 platforms, do_div is a macro definition, and the first parameter
> does not define its type. If the macro definition is replaced directly, an
> error will be reported in the ubuntu20.04 release.

What is the error?

We don't need to use do_div().
The current code that does normal / and % works fine on both
32-bit and 64-bit if you just do:

 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, buf_size, iters, remain;
        void *write_buf __free(kfree) = NULL;
        void *read_buf __free(kfree) = NULL;
        struct pci_dev *pdev = test->pdev;
+       u64 bar_size;

No?


Kind regards,
Niklas