Re: [PATCH v3 1/3] xen/arm: Add memory overlap check for bootinfo.reserved_mem

[Julien Grall <julien@xxxxxxx>]

On 31/01/2023 02:25, Henry Wang wrote:
> Hi Julien,
>
> > -----Original Message-----
> > From: Julien Grall <julien@xxxxxxx>
> > Subject: Re: [PATCH v3 1/3] xen/arm: Add memory overlap check for
> > bootinfo.reserved_mem
> >
> > Hi Henry,
> >
> > > +{
> > > +    paddr_t bank_start = INVALID_PADDR, bank_end = 0;
> > > +    paddr_t region_end = region_start + region_size;
> > > +    unsigned int i, bank_num = meminfo->nr_banks;
> > > +
> > > +    for ( i = 0; i < bank_num; i++ )
> > > +    {
> > > +        bank_start = meminfo->bank[i].start;
> > > +        bank_end = bank_start + meminfo->bank[i].size;
> > > +
> > > +        if ( region_end <= bank_start || region_start >= bank_end )
> > ... it clearly shows how this check would be wrong when either the bank
> > or the region is at the end of the address space. You may say it doesn't
> > overlap when it could (e.g. when region_end < region_start).
> Here do you mean if the region is at the end of the addr space,
> "region_start + region_end" will overflow and cause
> region_end < region_start? If so...
Yes.

> > That said, unless we rework 'bank', we would not properly solve the
> > problem. But that's likely a bigger piece of work and not something I
> > would request.
> >
> > So for now, I would suggest to add a comment. Stefano, what do you think?
> ...I am not really sure if simply adding a comment here would help,
> because when the overflow happens, we are already doomed because
> of the messed-up device tree.
Not necessarily. This could happen if the region is right at the top of 
the address (e.g. finishing at 2^64 - 1). As the 'end' is exclusive, 
then it would be equal to 0.
I think this is less likely for arm64, but this could happen for 32-bit 
Arm as we will allow the admin to reduce paddr_t from 64-bit to 32-bit.
> Would adding a `BUG_ON(region_end < region_start)` make sense to you?
No for the reason I stated above.

> > > +            continue;
> > > +        else
> > > +        {
> > > +            printk("Region: [%#"PRIpaddr", %#"PRIpaddr"] overlapping with
> > bank[%u]: [%#"PRIpaddr", %#"PRIpaddr"]\n",
> >
> > ']' usually mean inclusive. But here, 'end' is exclusive. So you want '['.
> Oh, now I understand the misunderstanding in our communication in v1:
> I didn't know '[' means exclusive because I was educated to use ')' [1] so I
> thought you meant inclusive. Sorry for this.
No worries. That might be only me using [ and ) interchangeably :).

> To keep consistency, may I use ')' here? Because I think this is the current
> way in the code base, for example see:
> xen/include/xen/numa.h L99: [*start, *end)
> xen/drivers/passthrough/amd/iommu_acpi.c L177: overlap [%lx,%lx)
I am fine with that.

> > BTW, the same comments applies for the second patch.
> I will fix this patch and #2 in v4.
I am happy to deal with it on commit if you want.

Cheers,

--
Julien Grall