[PATCH 1/5] arm64: vdso: don't free unallocated pages

[PATCH 1/5] arm64: vdso: don't free unallocated pages

[Mark Rutland]

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.

This patch removes the erroneous page freeing.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: stable@vger.kernel.org
---
 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)
-- 
2.11.0

Re: [PATCH 1/5] arm64: vdso: don't free unallocated pages

[Vincenzo Frascino]

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"? And
if this is causing a bug (according to the cover-letter), could you please
provide a stack-trace?

> This patch removes the erroneous page freeing.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: stable@vger.kernel.org
> ---
>  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

Re: [PATCH 1/5] arm64: vdso: don't free unallocated pages

[Mark Rutland]

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@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: stable@vger.kernel.org
> > ---
> >  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

Re: [PATCH 1/5] arm64: vdso: don't free unallocated pages

[Vincenzo Frascino]

On 4/14/20 2:27 PM, Mark Rutland wrote:
> 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.

free(NULL) is a no-operation ("no action occurs") according to the C standard
(ISO-IEC 9899 paragraph 7.20.3.2). Hence this should not cause any bug if the
allocator is correctly implemented. From what I can see the implementation of
the page allocator honors this assumption.

Since you say it is a bug (providing evidence), we might have to investigate
because probably there is an issue somewhere else.

> 
> * 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.
> 

Do you know why this is the case? To be compliant with what the page allocator
expects page_to_virt(NULL) should be equal to NULL.

> * 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()
> 

free_pages(x, 0) checks virt_addr_valid(x) only if "addr != 0" (as per C
standard) which makes me infer what I stated above. But maybe I am missing
something.

[...]
-- 
Regards,
Vincenzo

Re: [PATCH 1/5] arm64: vdso: don't free unallocated pages

[Will Deacon]

On Tue, Apr 14, 2020 at 03:53:45PM +0100, Vincenzo Frascino wrote:
> On 4/14/20 2:27 PM, Mark Rutland wrote:
> > On Tue, Apr 14, 2020 at 01:50:38PM +0100, Vincenzo Frascino wrote:
> >> 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.
> 
> free(NULL) is a no-operation ("no action occurs") according to the C standard
> (ISO-IEC 9899 paragraph 7.20.3.2). Hence this should not cause any bug if the
> allocator is correctly implemented. From what I can see the implementation of
> the page allocator honors this assumption.
> 
> Since you say it is a bug (providing evidence), we might have to investigate
> because probably there is an issue somewhere else.

Not sure why you feel the need to throw the C standard around -- the patch
from Mark looks obviously like the right thing to do to me, so:

Acked-by: Will Deacon <will@kernel.org>

Catalin -- please take this one as a fix so that I can queue the rest of
the patches for 5.8 once it's hit mainline.

Will

Re: [PATCH 1/5] arm64: vdso: don't free unallocated pages

[Catalin Marinas]

On Tue, Apr 14, 2020 at 04:10:35PM +0100, Will Deacon wrote:
> On Tue, Apr 14, 2020 at 03:53:45PM +0100, Vincenzo Frascino wrote:
> > On 4/14/20 2:27 PM, Mark Rutland wrote:
> > > On Tue, Apr 14, 2020 at 01:50:38PM +0100, Vincenzo Frascino wrote:
> > >> 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.
> > 
> > free(NULL) is a no-operation ("no action occurs") according to the C standard
> > (ISO-IEC 9899 paragraph 7.20.3.2). Hence this should not cause any bug if the
> > allocator is correctly implemented. From what I can see the implementation of
> > the page allocator honors this assumption.
> > 
> > Since you say it is a bug (providing evidence), we might have to investigate
> > because probably there is an issue somewhere else.
> 
> Not sure why you feel the need to throw the C standard around -- the patch
> from Mark looks obviously like the right thing to do to me, so:
> 
> Acked-by: Will Deacon <will@kernel.org>
> 
> Catalin -- please take this one as a fix so that I can queue the rest of
> the patches for 5.8 once it's hit mainline.

I queued this patch for -rc2. Thanks.

-- 
Catalin

Re: [PATCH 1/5] arm64: vdso: don't free unallocated pages

[Mark Rutland]

On Tue, Apr 14, 2020 at 03:53:45PM +0100, Vincenzo Frascino wrote:
> 
> On 4/14/20 2:27 PM, Mark Rutland wrote:
> > 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.
> 
> free(NULL) is a no-operation ("no action occurs") according to the C standard
> (ISO-IEC 9899 paragraph 7.20.3.2). Hence this should not cause any bug if the
> allocator is correctly implemented.

This is true, but irrelevant. The C standard does not define
free_page(), which is a Linnux kernel internal function, and does not
have the same semantics as free().

> > * 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.

> > * 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()
> > 
> 
> free_pages(x, 0) checks virt_addr_valid(x) only if "addr != 0" (as per C
> standard) which makes me infer what I stated above. But maybe I am missing
> something.

Regardless, this is all academic unless you disagree with the first two
bullets above.

You don't randomly sprinkle a program with free(NULL) for the fun of it.
Similarly, and regardless of how obfuscated, one should not do the same
here.

Thanks,
Mark.

Re: [PATCH 1/5] arm64: vdso: don't free unallocated pages

[Vincenzo Frascino]

Hi Mark,

On 4/14/20 4:12 PM, Mark Rutland wrote:
> Regardless, this is all academic unless you disagree with the first two
> bullets above.

Nothing to object on those.

-- 
Regards,
Vincenzo

Re: [PATCH 1/5] arm64: vdso: don't free unallocated pages

[Catalin Marinas]

On Tue, Apr 14, 2020 at 03:53:45PM +0100, Vincenzo Frascino wrote:
> On 4/14/20 2:27 PM, Mark Rutland wrote:
> > On Tue, Apr 14, 2020 at 01:50:38PM +0100, Vincenzo Frascino wrote:
> >> 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.
> 
> free(NULL) is a no-operation ("no action occurs") according to the C standard
> (ISO-IEC 9899 paragraph 7.20.3.2). Hence this should not cause any bug if the
> allocator is correctly implemented. From what I can see the implementation of
> the page allocator honors this assumption.
[...]
> > * 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.
> 
> Do you know why this is the case? To be compliant with what the page allocator
> expects page_to_virt(NULL) should be equal to NULL.

Since __free_page(page) (note the two underscores and pointer type) does
not accept a NULL argument, I don't see any reason for page_to_virt() to
accept NULL as a valid argument.

-- 
Catalin

Re: [PATCH 1/5] arm64: vdso: don't free unallocated pages

[Catalin Marinas]

On Tue, Apr 14, 2020 at 11:42:48AM +0100, 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.
> 
> This patch removes the erroneous page freeing.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: stable@vger.kernel.org

I presume the cc stable should be limited to:

Fixes: 7c1deeeb0130 ("arm64: compat: VDSO setup for compat layer")
Cc: <stable@vger.kernel.org> # 5.3.x-

I'll fix it up locally.

-- 
Catalin

Re: [PATCH 1/5] arm64: vdso: don't free unallocated pages

[Mark Rutland]

On Wed, Apr 15, 2020 at 11:13:11AM +0100, Catalin Marinas wrote:
> On Tue, Apr 14, 2020 at 11:42:48AM +0100, 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.
> > 
> > This patch removes the erroneous page freeing.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: stable@vger.kernel.org
> 
> I presume the cc stable should be limited to:
> 
> Fixes: 7c1deeeb0130 ("arm64: compat: VDSO setup for compat layer")
> Cc: <stable@vger.kernel.org> # 5.3.x-
> 
> I'll fix it up locally.

Yes, and thanks!

Mark.