Re: [PATCH v2][next] x86/mm/pgtable: Fix -Wstringop-overflow warnings

[Gustavo A. R. Silva]

On Tue, May 10, 2022 at 09:12:02AM -0500, Gustavo A. R. Silva wrote:
> > > > > --- a/arch/x86/mm/pgtable.c
> > > > > +++ b/arch/x86/mm/pgtable.c
> > > > > @@ -434,14 +434,18 @@ pgd_t *pgd_alloc(struct mm_struct *mm)
> > > > >  
> > > > >  	mm->pgd = pgd;
> > > > >  
> > > > > -	if (preallocate_pmds(mm, pmds, PREALLOCATED_PMDS) != 0)
> > > > > -		goto out_free_pgd;
> > > > > +	if (MAX_PREALLOCATED_PMDS != 0 && MAX_PREALLOCATED_USER_PMDS != 0) {
> > > > > +		if (preallocate_pmds(mm, pmds, PREALLOCATED_PMDS) != 0)
> > > > > +			goto out_free_pgd;
> > > > >  
> > > > > -	if (preallocate_pmds(mm, u_pmds, PREALLOCATED_USER_PMDS) != 0)
> > > > > -		goto out_free_pmds;
> > > > > +		if (preallocate_pmds(mm, u_pmds, PREALLOCATED_USER_PMDS) != 0)
> > > > > +			goto out_free_pmds;
> > > > >  
> > > > > -	if (paravirt_pgd_alloc(mm) != 0)
> > > > > -		goto out_free_user_pmds;
> > > > > +		if (paravirt_pgd_alloc(mm) != 0)
> > > > > +			goto out_free_user_pmds;
> > > > > +	} else {
> > > > > +		goto out_free_pgd;
> > > > 
> > > > The "all 0" case shouldn't be a failure mode; it should just skip the
> > > > preallocate_pmds() calls.
> > > 
> > > Do you mean something like this:
> > > 
> > > diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
> > > index f16059e9a85e..4dae168408f1 100644
> > > --- a/arch/x86/mm/pgtable.c
> > > +++ b/arch/x86/mm/pgtable.c
> > > @@ -434,11 +434,13 @@ pgd_t *pgd_alloc(struct mm_struct *mm)
> > > 
> > >         mm->pgd = pgd;
> > > 
> > > -       if (preallocate_pmds(mm, pmds, PREALLOCATED_PMDS) != 0)
> > > -               goto out_free_pgd;
> > > +       if (MAX_PREALLOCATED_PMDS != 0 && MAX_PREALLOCATED_USER_PMDS != 0) {
> > > +               if (preallocate_pmds(mm, pmds, PREALLOCATED_PMDS) != 0)
> > > +                       goto out_free_pgd;
> > > 
> > > -       if (preallocate_pmds(mm, u_pmds, PREALLOCATED_USER_PMDS) != 0)
> > > -               goto out_free_pmds;
> > > +               if (preallocate_pmds(mm, u_pmds, PREALLOCATED_USER_PMDS) != 0)
> > > +                       goto out_free_pmds;
> > > +       }
> > > 
> > >         if (paravirt_pgd_alloc(mm) != 0)
> > >                 goto out_free_user_pmds;
> > > 
> > > It seems that the above is not enough, because we have the same issue
> > > when calling pgd_prepopulate_pmd(), pgd_prepopulate_user_pmd() and
> > > free_pmds():
> > > 
> > >   CC      arch/x86/mm/pgtable.o
> > > arch/x86/mm/pgtable.c: In function 'pgd_alloc':
> > > arch/x86/mm/pgtable.c:464:9: warning: 'free_pmds' accessing 8 bytes in a region of size 0 [-Wstringop-overflow=]
> > >   464 |         free_pmds(mm, u_pmds, PREALLOCATED_USER_PMDS);
> > >       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > Ugh. Perhaps just marking both preallocate_pmds() and free_pmds() as
> > inline is enough to let the compiler "see" everything correctly?
> 
> It doesn't seem to work... however, the following piece of code implies
> that pmds and u_pmds should be first preallocated through preallocate_pmds(),
> which cannot happen if (MAX_PREALLOCATED_PMDS != 0 && MAX_PREALLOCATED_USER_PMDS != 0)

I wanted to say: which cannot happen if MAX_PREALLOCATED_PMDS == 0 && MAX_PREALLOCATED_USER_PMDS == 0

> 
> 448         /*
> 449          * Make sure that pre-populating the pmds is atomic with
> 450          * respect to anything walking the pgd_list, so that they
> 451          * never see a partially populated pgd.
> 452          */
> 453         spin_lock(&pgd_lock);
> 454
> 455         pgd_ctor(mm, pgd);
> 456         pgd_prepopulate_pmd(mm, pgd, pmds);
> 457         pgd_prepopulate_user_pmd(mm, pgd, u_pmds);
> 458
> 459         spin_unlock(&pgd_lock);
> 460
> 461         return pgd;
> 
> So, my question here is why do you think the "all 0" case should only skip the
> preallocate_pmds() calls and not the pgd_prepopulate_pmd() calls too?
> 
> > 
> > Otherwise, they'll likely each need the same check that was added to
> > pgd_prepopulate_pmd() ages ago for a similar situation...
> 
> uhm... that doesn't seem to have an impact nowadays, or at least now
> Wstringop-overflow sees the problem first, because now the issue is
> detected at the moment of passing the arguments to the the function
> and not when actually executing the function?
> 
> otherwise, I think we wouldn't see this error:
> 
> arch/x86/mm/pgtable.c:454:9: warning: 'pgd_prepopulate_pmd' accessing 8 bytes in a region of size 0 [-Wstringop-overflow=]
>   454 |         pgd_prepopulate_pmd(mm, pgd, pmds);
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> arch/x86/mm/pgtable.c:454:9: note: referencing argument 3 of type 'pmd_t *[0]'
> arch/x86/mm/pgtable.c:296:13: note: in a call to function 'pgd_prepopulate_pmd'
>   296 | static void pgd_prepopulate_pmd(struct mm_struct *mm, pgd_t *pgd, pmd_t *pmds[])
>       |             ^~~~~~~~~~~~~~~~~~~
> 

Thanks
--
Gustavo