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

Niklas Cassel
2025-01-07T11:33:45Z

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