Re: [PATCH 4/5] lib/ioremap: Ensure phys_addr actually corresponds to a physical address

[Sean Christopherson]

On Wed, Sep 12, 2018 at 05:39:14PM +0100, Will Deacon wrote:
> Hi Sean,
> 
> Thanks for looking at the patch.
> 
> On Wed, Sep 12, 2018 at 08:09:39AM -0700, Sean Christopherson wrote:
> > On Wed, Sep 12, 2018 at 11:26:13AM +0100, Will Deacon wrote:
> > > The current ioremap() code uses a phys_addr variable at each level of
> > > page table, which is confusingly offset by subtracting the base virtual
> > > address being mapped so that adding the current virtual address back on
> > > when iterating through the page table entries gives back the corresponding
> > > physical address.
> > > 
> > > This is fairly confusing and results in all users of phys_addr having to
> > > add the current virtual address back on. Instead, this patch just updates
> > > phys_addr when iterating over the page table entries, ensuring that it's
> > > always up-to-date and doesn't require explicit offsetting.
> > > 
> > > Cc: Chintan Pandya <cpandya@xxxxxxxxxxxxxx>
> > > Cc: Toshi Kani <toshi.kani@xxxxxxx>
> > > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > > Cc: Michal Hocko <mhocko@xxxxxxxx>
> > > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Will Deacon <will.deacon@xxxxxxx>
> > > ---
> > >  lib/ioremap.c | 28 ++++++++++++----------------
> > >  1 file changed, 12 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/lib/ioremap.c b/lib/ioremap.c
> > > index 6c72764af19c..fc834a59c90c 100644
> > > --- a/lib/ioremap.c
> > > +++ b/lib/ioremap.c
> > > @@ -101,19 +101,18 @@ static inline int ioremap_pmd_range(pud_t *pud, unsigned long addr,
> > >  	pmd_t *pmd;
> > >  	unsigned long next;
> > >  
> > > -	phys_addr -= addr;
> > >  	pmd = pmd_alloc(&init_mm, pud, addr);
> > >  	if (!pmd)
> > >  		return -ENOMEM;
> > >  	do {
> > >  		next = pmd_addr_end(addr, end);
> > >  
> > > -		if (ioremap_try_huge_pmd(pmd, addr, next, phys_addr + addr, prot))
> > > +		if (ioremap_try_huge_pmd(pmd, addr, next, phys_addr, prot))
> > >  			continue;
> > >  
> > > -		if (ioremap_pte_range(pmd, addr, next, phys_addr + addr, prot))
> > > +		if (ioremap_pte_range(pmd, addr, next, phys_addr, prot))
> > >  			return -ENOMEM;
> > > -	} while (pmd++, addr = next, addr != end);
> > > +	} while (pmd++, addr = next, phys_addr += PMD_SIZE, addr != end);
> > 
> > I think bumping phys_addr by PXX_SIZE is wrong if phys_addr and addr
> > start unaligned with respect to PXX_SIZE.  The addresses must be
> > PAGE_ALIGNED, which lets ioremap_pte_range() do a simple calculation,
> > but that doesn't hold true for the upper levels, i.e. phys_addr needs
> > to be adjusted using an algorithm similar to pxx_addr_end().
> > 
> > Using a 2mb page as an example (lower 32 bits only): 
> > 
> > pxx_size  = 0x00020000
> > pxx_mask  = 0xfffe0000
> > addr      = 0x1000
> > end       = 0x00040000
> > phys_addr = 0x1000
> > 
> > Loop 1:
> >    addr = 0x1000
> >    phys = 0x1000
> > 
> > Loop 2:
> >    addr = 0x20000
> >    phys = 0x21000
> 
> Yes, I think you're completely right, however I also don't think this
> can happen with the current code (and I've failed to trigger it in my
> testing). The virtual addresses allocated for VM_IOREMAP allocations
> are aligned to the order of the allocation, which means that the virtual
> address at the start of the mapping is aligned such that when we hit the
> end of a pXd, we know we've mapped the previous PXD_SIZE bytes.
> 
> Having said that, this is clearly a change from the current code and I
> haven't audited architectures other than arm64 (where IOREMAP_MAX_ORDER
> corresponds to the maximum size of our huge mappings), so it would be
> much better not to introduce this funny behaviour in a patch that aims
> to reduce confusion in the first place!
> 
> Fixing this using the pxx_addr_end() macros is a bit strange, since we
> don't have a physical end variable (nor do we need one), so perhaps
> something like changing the while condition to be:
> 
> 	do {
> 		...
> 	} while (pmd++, phys_addr += (next - addr), addr = next, addr != end);
> 
> would do the trick. What do you reckon?

LGTM.  I like that there isn't a separate calculation for phys_addr's offset.