On Tue, Apr 14, 2020 at 01:50:38PM +0100, Vincenzo Frascino wrote:
> Hi Mark,
>
> On 4/14/20 11:42 AM, Mark Rutland wrote:
> > The aarch32_vdso_pages[] array never has entries allocated in the C_VVAR
> > or C_VDSO slots, and as the array is zero initialized these contain
> > NULL.
> >
> > However in __aarch32_alloc_vdso_pages() when
> > aarch32_alloc_kuser_vdso_page() fails we attempt to free the page whose
> > struct page is at NULL, which is obviously nonsensical.
>
> Could you please explain why do you think that free(NULL) is "nonsensical"?
Regardless of the below, can you please explain why it is sensical? I'm
struggling to follow your argument here.
* It serves no legitimate purpose. One cannot free a page without a
corresponding struct page.
* It is redundant. Removing the code does not detract from the utility
of the remainging code, or make that remaing code more complex.
* It hinders comprehension of the code. When a developer sees the
free_page() they will assume that the page was allocated somewhere,
but there is no corresponding allocation as the pointers are never
assigned to. Even if the code in question is not harmful to the
functional correctness of the code, it is an unnecessary burden to
developers.
* page_to_virt(NULL) does not have a well-defined result, and
page_to_virt() should only be called for a valid struct page pointer.
The result of page_to_virt(NULL) may not be a pointer into the linear
map as would be expected.
* free_page(x) calls free_pages(x, 0), which checks virt_addr_valid(x).
As page_to_virt(NULL) is not a valid linear map address, this can
trigger a VM_BUG_ON()
* Even if page_to_virt(virt_to_page(NULL)) is NULL, within
__free_pages(NULL, 0) we'd call put_page_testzero(NULL) which will
fault when dereferencing NULL to get at the fields in struct page.
Likewise for everything under free_the_page(NULL, 0).
> And if this is causing a bug (according to the cover-letter), could
> you please provide a stack-trace?
I haven't triggered it, as I found it by inspection. As above the
behaviour is clearly erroneous and can trigger several failures to
occur.
Note that this only happens if aarch32_alloc_kuser_vdso_page() fails in
the boot path, which is unlikely.
Thanks,
Mark.
> > This patch removes the erroneous page freeing.
> >
> > Signed-off-by: Mark Rutland <mark.rutland@xxxxxxx>
> > Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> > Cc: Vincenzo Frascino <vincenzo.frascino@xxxxxxx>
> > Cc: Will Deacon <will@xxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > ---
> > arch/arm64/kernel/vdso.c | 13 +------------
> > 1 file changed, 1 insertion(+), 12 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
> > index 354b11e27c07..033a48f30dbb 100644
> > --- a/arch/arm64/kernel/vdso.c
> > +++ b/arch/arm64/kernel/vdso.c
> > @@ -260,18 +260,7 @@ static int __aarch32_alloc_vdso_pages(void)
> > if (ret)
> > return ret;
> >
> > - ret = aarch32_alloc_kuser_vdso_page();
> > - if (ret) {
> > - unsigned long c_vvar =
> > - (unsigned long)page_to_virt(aarch32_vdso_pages[C_VVAR]);
> > - unsigned long c_vdso =
> > - (unsigned long)page_to_virt(aarch32_vdso_pages[C_VDSO]);
> > -
> > - free_page(c_vvar);
> > - free_page(c_vdso);
> > - }
> > -
> > - return ret;
> > + return aarch32_alloc_kuser_vdso_page();
> > }
> > #else
> > static int __aarch32_alloc_vdso_pages(void)
> >
>
> --
> Regards,
> Vincenzo
