[PATCH 0/5] arm64: vdso: cleanups

[PATCH 0/5] arm64: vdso: cleanups

[Mark Rutland]

Hi all,

While attempting to review an arm64 vdso patch, I noticed some of the existing
code was somewhat baroque, making it harder than necessary to understand and
extend. These patches attempt to improve this by making the code more
consistent and avoiding unnecessary duplication.

The first patch in the series fixes a bug in a boot time error path. This bug
was made obvious during the refactoring but I've moved it to the start so that
it can be backported more easily.

The series is based on v5.7-rc1 and can be found in my arm64/vdso-cleanup
branch [1].

Thanks,
Mark.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/vdso-cleanup

Mark Rutland (5):
  arm64: vdso: don't free unallocated pages
  arm64: vdso: remove aarch32_vdso_pages[]
  arm64: vdso: simplify arch_vdso_type ifdeffery
  arm64: vdso: use consistent 'abi' nomenclature
  arm64: vdso: use consistent 'map' nomenclature

 arch/arm64/kernel/vdso.c | 162 ++++++++++++++++++++---------------------------
 1 file changed, 69 insertions(+), 93 deletions(-)

-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[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

[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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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.

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/5] arm64: vdso: remove aarch32_vdso_pages[]

[Mark Rutland]

The aarch32_vdso_pages[] array is unnecessarily confusing. We only ever
use the C_VECTORS and C_SIGPAGE slots, and the other slots are unused
despite having corresponding mappings (sharing pages with the AArch64
vDSO).

Let's make this clearer by using separate variables for the vectors page
and the sigreturn page. A subsequent patch will clean up the C_* naming
and conflation of pages with mappings.

Note that since both the vectors page and sig page are single
pages, and the mapping is a single page long, their pages array do not
need to be NULL-terminated (and this was not the case with the existing
code for the sig page as it was the last entry in the aarch32_vdso_pages
array).

There should be no functional change as a result of this patch.

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>
---
 arch/arm64/kernel/vdso.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
index 033a48f30dbb..263bc6084c71 100644
--- a/arch/arm64/kernel/vdso.c
+++ b/arch/arm64/kernel/vdso.c
@@ -206,11 +206,16 @@ static int aarch32_vdso_mremap(const struct vm_special_mapping *sm,
 #define C_SIGPAGE	1
 #define C_PAGES		(C_SIGPAGE + 1)
 #endif /* CONFIG_COMPAT_VDSO */
-static struct page *aarch32_vdso_pages[C_PAGES] __ro_after_init;
+
+static struct page *aarch32_vectors_page __ro_after_init;
+#ifndef CONFIG_COMPAT_VDSO
+static struct page *aarch32_sig_page __ro_after_init;
+#endif
+
 static struct vm_special_mapping aarch32_vdso_spec[C_PAGES] = {
 	{
 		.name	= "[vectors]", /* ABI */
-		.pages	= &aarch32_vdso_pages[C_VECTORS],
+		.pages	= &aarch32_vectors_page,
 	},
 #ifdef CONFIG_COMPAT_VDSO
 	{
@@ -223,7 +228,7 @@ static struct vm_special_mapping aarch32_vdso_spec[C_PAGES] = {
 #else
 	{
 		.name	= "[sigpage]", /* ABI */
-		.pages	= &aarch32_vdso_pages[C_SIGPAGE],
+		.pages	= &aarch32_sig_page,
 	},
 #endif /* CONFIG_COMPAT_VDSO */
 };
@@ -243,8 +248,8 @@ static int aarch32_alloc_kuser_vdso_page(void)
 
 	memcpy((void *)(vdso_page + 0x1000 - kuser_sz), __kuser_helper_start,
 	       kuser_sz);
-	aarch32_vdso_pages[C_VECTORS] = virt_to_page(vdso_page);
-	flush_dcache_page(aarch32_vdso_pages[C_VECTORS]);
+	aarch32_vectors_page = virt_to_page(vdso_page);
+	flush_dcache_page(aarch32_vectors_page);
 	return 0;
 }
 
@@ -275,8 +280,8 @@ static int __aarch32_alloc_vdso_pages(void)
 		return -ENOMEM;
 
 	memcpy((void *)sigpage, __aarch32_sigret_code_start, sigret_sz);
-	aarch32_vdso_pages[C_SIGPAGE] = virt_to_page(sigpage);
-	flush_dcache_page(aarch32_vdso_pages[C_SIGPAGE]);
+	aarch32_sig_page = virt_to_page(sigpage);
+	flush_dcache_page(aarch32_sig_page);
 
 	ret = aarch32_alloc_kuser_vdso_page();
 	if (ret)
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/5] arm64: vdso: simplify arch_vdso_type ifdeffery

[Mark Rutland]

Currently we have some ifdeffery to determine the number of elements in
enum arch_vdso_type as VDSO_TYPES, rather that the usual pattern of
having the enum define this:

| enum foo_type {
|         FOO_TYPE_A,
|         FOO_TYPE_B,
| #ifdef CONFIG_C
|         FOO_TYPE_C,
| #endif
|         NR_FOO_TYPES
| }

... however, given we only use this number to size the vdso_lookup[]
array, this is redundant anyway as the compiler can automatically size
the array to fit all defined elements.

So let's remove the VDSO_TYPES to simplify the code.

At the same time, let's use designated initializers for the array
elements so that these are guarnateed to be at the expected indices,
regardless of how we modify the structure. For clariy the redundant
explicit initialization of the enum elements is dropped.

There should be no functional change as a result of this patch.

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>
---
 arch/arm64/kernel/vdso.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
index 263bc6084c71..b4b01ac30112 100644
--- a/arch/arm64/kernel/vdso.c
+++ b/arch/arm64/kernel/vdso.c
@@ -35,16 +35,11 @@ extern char vdso32_start[], vdso32_end[];
 
 /* vdso_lookup arch_index */
 enum arch_vdso_type {
-	ARM64_VDSO = 0,
+	ARM64_VDSO,
 #ifdef CONFIG_COMPAT_VDSO
-	ARM64_VDSO32 = 1,
+	ARM64_VDSO32,
 #endif /* CONFIG_COMPAT_VDSO */
 };
-#ifdef CONFIG_COMPAT_VDSO
-#define VDSO_TYPES		(ARM64_VDSO32 + 1)
-#else
-#define VDSO_TYPES		(ARM64_VDSO + 1)
-#endif /* CONFIG_COMPAT_VDSO */
 
 struct __vdso_abi {
 	const char *name;
@@ -57,14 +52,14 @@ struct __vdso_abi {
 	struct vm_special_mapping *cm;
 };
 
-static struct __vdso_abi vdso_lookup[VDSO_TYPES] __ro_after_init = {
-	{
+static struct __vdso_abi vdso_lookup[] __ro_after_init = {
+	[ARM64_VDSO] = {
 		.name = "vdso",
 		.vdso_code_start = vdso_start,
 		.vdso_code_end = vdso_end,
 	},
 #ifdef CONFIG_COMPAT_VDSO
-	{
+	[ARM64_VDSO32] = {
 		.name = "vdso32",
 		.vdso_code_start = vdso32_start,
 		.vdso_code_end = vdso32_end,
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/5] arm64: vdso: use consistent 'abi' nomenclature

[Mark Rutland]

The current code doesn't use a consistent naming scheme for structures,
enums, or variables, making it harder than necessary to determine the
relationship between these.

Let's make this easier by consistently using 'vdso_abi' nomenclature.
The 'vdso_lookup' array is renamed to 'vdso_info' to describe what it
contains rather than how it is consumed.

There should be no functional change as a result of this patch.

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>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/kernel/vdso.c | 65 ++++++++++++++++++++++++------------------------
 1 file changed, 32 insertions(+), 33 deletions(-)

diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
index b4b01ac30112..4a0bc8db9b29 100644
--- a/arch/arm64/kernel/vdso.c
+++ b/arch/arm64/kernel/vdso.c
@@ -33,15 +33,14 @@ extern char vdso_start[], vdso_end[];
 extern char vdso32_start[], vdso32_end[];
 #endif /* CONFIG_COMPAT_VDSO */
 
-/* vdso_lookup arch_index */
-enum arch_vdso_type {
-	ARM64_VDSO,
+enum vdso_abi {
+	VDSO_ABI_AA64,
 #ifdef CONFIG_COMPAT_VDSO
-	ARM64_VDSO32,
+	VDSO_ABI_AA32,
 #endif /* CONFIG_COMPAT_VDSO */
 };
 
-struct __vdso_abi {
+struct vdso_abi_info {
 	const char *name;
 	const char *vdso_code_start;
 	const char *vdso_code_end;
@@ -52,14 +51,14 @@ struct __vdso_abi {
 	struct vm_special_mapping *cm;
 };
 
-static struct __vdso_abi vdso_lookup[] __ro_after_init = {
-	[ARM64_VDSO] = {
+static struct vdso_abi_info vdso_info[] __ro_after_init = {
+	[VDSO_ABI_AA64] = {
 		.name = "vdso",
 		.vdso_code_start = vdso_start,
 		.vdso_code_end = vdso_end,
 	},
 #ifdef CONFIG_COMPAT_VDSO
-	[ARM64_VDSO32] = {
+	[VDSO_ABI_AA32] = {
 		.name = "vdso32",
 		.vdso_code_start = vdso32_start,
 		.vdso_code_end = vdso32_end,
@@ -76,13 +75,13 @@ static union {
 } vdso_data_store __page_aligned_data;
 struct vdso_data *vdso_data = vdso_data_store.data;
 
-static int __vdso_remap(enum arch_vdso_type arch_index,
+static int __vdso_remap(enum vdso_abi abi,
 			const struct vm_special_mapping *sm,
 			struct vm_area_struct *new_vma)
 {
 	unsigned long new_size = new_vma->vm_end - new_vma->vm_start;
-	unsigned long vdso_size = vdso_lookup[arch_index].vdso_code_end -
-				  vdso_lookup[arch_index].vdso_code_start;
+	unsigned long vdso_size = vdso_info[abi].vdso_code_end -
+				  vdso_info[abi].vdso_code_start;
 
 	if (vdso_size != new_size)
 		return -EINVAL;
@@ -92,24 +91,24 @@ static int __vdso_remap(enum arch_vdso_type arch_index,
 	return 0;
 }
 
-static int __vdso_init(enum arch_vdso_type arch_index)
+static int __vdso_init(enum vdso_abi abi)
 {
 	int i;
 	struct page **vdso_pagelist;
 	unsigned long pfn;
 
-	if (memcmp(vdso_lookup[arch_index].vdso_code_start, "\177ELF", 4)) {
+	if (memcmp(vdso_info[abi].vdso_code_start, "\177ELF", 4)) {
 		pr_err("vDSO is not a valid ELF object!\n");
 		return -EINVAL;
 	}
 
-	vdso_lookup[arch_index].vdso_pages = (
-			vdso_lookup[arch_index].vdso_code_end -
-			vdso_lookup[arch_index].vdso_code_start) >>
+	vdso_info[abi].vdso_pages = (
+			vdso_info[abi].vdso_code_end -
+			vdso_info[abi].vdso_code_start) >>
 			PAGE_SHIFT;
 
 	/* Allocate the vDSO pagelist, plus a page for the data. */
-	vdso_pagelist = kcalloc(vdso_lookup[arch_index].vdso_pages + 1,
+	vdso_pagelist = kcalloc(vdso_info[abi].vdso_pages + 1,
 				sizeof(struct page *),
 				GFP_KERNEL);
 	if (vdso_pagelist == NULL)
@@ -120,18 +119,18 @@ static int __vdso_init(enum arch_vdso_type arch_index)
 
 
 	/* Grab the vDSO code pages. */
-	pfn = sym_to_pfn(vdso_lookup[arch_index].vdso_code_start);
+	pfn = sym_to_pfn(vdso_info[abi].vdso_code_start);
 
-	for (i = 0; i < vdso_lookup[arch_index].vdso_pages; i++)
+	for (i = 0; i < vdso_info[abi].vdso_pages; i++)
 		vdso_pagelist[i + 1] = pfn_to_page(pfn + i);
 
-	vdso_lookup[arch_index].dm->pages = &vdso_pagelist[0];
-	vdso_lookup[arch_index].cm->pages = &vdso_pagelist[1];
+	vdso_info[abi].dm->pages = &vdso_pagelist[0];
+	vdso_info[abi].cm->pages = &vdso_pagelist[1];
 
 	return 0;
 }
 
-static int __setup_additional_pages(enum arch_vdso_type arch_index,
+static int __setup_additional_pages(enum vdso_abi abi,
 				    struct mm_struct *mm,
 				    struct linux_binprm *bprm,
 				    int uses_interp)
@@ -139,7 +138,7 @@ static int __setup_additional_pages(enum arch_vdso_type arch_index,
 	unsigned long vdso_base, vdso_text_len, vdso_mapping_len;
 	void *ret;
 
-	vdso_text_len = vdso_lookup[arch_index].vdso_pages << PAGE_SHIFT;
+	vdso_text_len = vdso_info[abi].vdso_pages << PAGE_SHIFT;
 	/* Be sure to map the data page */
 	vdso_mapping_len = vdso_text_len + PAGE_SIZE;
 
@@ -151,7 +150,7 @@ static int __setup_additional_pages(enum arch_vdso_type arch_index,
 
 	ret = _install_special_mapping(mm, vdso_base, PAGE_SIZE,
 				       VM_READ|VM_MAYREAD,
-				       vdso_lookup[arch_index].dm);
+				       vdso_info[abi].dm);
 	if (IS_ERR(ret))
 		goto up_fail;
 
@@ -160,7 +159,7 @@ static int __setup_additional_pages(enum arch_vdso_type arch_index,
 	ret = _install_special_mapping(mm, vdso_base, vdso_text_len,
 				       VM_READ|VM_EXEC|
 				       VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC,
-				       vdso_lookup[arch_index].cm);
+				       vdso_info[abi].cm);
 	if (IS_ERR(ret))
 		goto up_fail;
 
@@ -179,7 +178,7 @@ static int __setup_additional_pages(enum arch_vdso_type arch_index,
 static int aarch32_vdso_mremap(const struct vm_special_mapping *sm,
 		struct vm_area_struct *new_vma)
 {
-	return __vdso_remap(ARM64_VDSO32, sm, new_vma);
+	return __vdso_remap(VDSO_ABI_AA32, sm, new_vma);
 }
 #endif /* CONFIG_COMPAT_VDSO */
 
@@ -256,7 +255,7 @@ static int __aarch32_alloc_vdso_pages(void)
 	vdso_lookup[ARM64_VDSO32].dm = &aarch32_vdso_spec[C_VVAR];
 	vdso_lookup[ARM64_VDSO32].cm = &aarch32_vdso_spec[C_VDSO];
 
-	ret = __vdso_init(ARM64_VDSO32);
+	ret = __vdso_init(VDSO_ABI_AA32);
 	if (ret)
 		return ret;
 
@@ -354,7 +353,7 @@ int aarch32_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
 		goto out;
 
 #ifdef CONFIG_COMPAT_VDSO
-	ret = __setup_additional_pages(ARM64_VDSO32,
+	ret = __setup_additional_pages(VDSO_ABI_AA32,
 				       mm,
 				       bprm,
 				       uses_interp);
@@ -371,7 +370,7 @@ int aarch32_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
 static int vdso_mremap(const struct vm_special_mapping *sm,
 		struct vm_area_struct *new_vma)
 {
-	return __vdso_remap(ARM64_VDSO, sm, new_vma);
+	return __vdso_remap(VDSO_ABI_AA64, sm, new_vma);
 }
 
 /*
@@ -394,10 +393,10 @@ static struct vm_special_mapping vdso_spec[A_PAGES] __ro_after_init = {
 
 static int __init vdso_init(void)
 {
-	vdso_lookup[ARM64_VDSO].dm = &vdso_spec[A_VVAR];
-	vdso_lookup[ARM64_VDSO].cm = &vdso_spec[A_VDSO];
+	vdso_info[VDSO_ABI_AA64].dm = &vdso_spec[A_VVAR];
+	vdso_info[VDSO_ABI_AA64].cm = &vdso_spec[A_VDSO];
 
-	return __vdso_init(ARM64_VDSO);
+	return __vdso_init(VDSO_ABI_AA64);
 }
 arch_initcall(vdso_init);
 
@@ -410,7 +409,7 @@ int arch_setup_additional_pages(struct linux_binprm *bprm,
 	if (down_write_killable(&mm->mmap_sem))
 		return -EINTR;
 
-	ret = __setup_additional_pages(ARM64_VDSO,
+	ret = __setup_additional_pages(VDSO_ABI_AA64,
 				       mm,
 				       bprm,
 				       uses_interp);
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/5] arm64: vdso: use consistent 'map' nomenclature

[Mark Rutland]

The current code doesn't use a consistent naming scheme for structures,
enums, or variables, making it harder than necessary to determine the
relationship between these.

Let's make this easier by consistently using 'map' nomenclature for
mappings created in userspace, minimizing redundant comments, and
using designated array initializers to tie indices to their respective
elements.

There should be no functional change as a result of this patch.

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>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/kernel/vdso.c | 64 ++++++++++++++++++++----------------------------
 1 file changed, 26 insertions(+), 38 deletions(-)

diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
index 4a0bc8db9b29..3aee2a3edb5f 100644
--- a/arch/arm64/kernel/vdso.c
+++ b/arch/arm64/kernel/vdso.c
@@ -182,45 +182,36 @@ static int aarch32_vdso_mremap(const struct vm_special_mapping *sm,
 }
 #endif /* CONFIG_COMPAT_VDSO */
 
-/*
- * aarch32_vdso_pages:
- * 0 - kuser helpers
- * 1 - sigreturn code
- * or (CONFIG_COMPAT_VDSO):
- * 0 - kuser helpers
- * 1 - vdso data
- * 2 - vdso code
- */
-#define C_VECTORS	0
+enum aarch32_map {
+	AA32_MAP_VECTORS, /* kuser helpers */
 #ifdef CONFIG_COMPAT_VDSO
-#define C_VVAR		1
-#define C_VDSO		2
-#define C_PAGES		(C_VDSO + 1)
+	AA32_MAP_VVAR,
+	AA32_MAP_VDSO,
 #else
-#define C_SIGPAGE	1
-#define C_PAGES		(C_SIGPAGE + 1)
-#endif /* CONFIG_COMPAT_VDSO */
+	AA32_MAP_SIGPAGE
+#endif
+};
 
 static struct page *aarch32_vectors_page __ro_after_init;
 #ifndef CONFIG_COMPAT_VDSO
 static struct page *aarch32_sig_page __ro_after_init;
 #endif
 
-static struct vm_special_mapping aarch32_vdso_spec[C_PAGES] = {
-	{
+static struct vm_special_mapping aarch32_vdso_maps[] = {
+	[AA32_MAP_VECTORS] = {
 		.name	= "[vectors]", /* ABI */
 		.pages	= &aarch32_vectors_page,
 	},
 #ifdef CONFIG_COMPAT_VDSO
-	{
+	[AA32_MAP_VVAR] = {
 		.name = "[vvar]",
 	},
-	{
+	[AA32_MAP_VDSO] = {
 		.name = "[vdso]",
 		.mremap = aarch32_vdso_mremap,
 	},
 #else
-	{
+	[AA32_MAP_SIGPAGE] = {
 		.name	= "[sigpage]", /* ABI */
 		.pages	= &aarch32_sig_page,
 	},
@@ -252,8 +243,8 @@ static int __aarch32_alloc_vdso_pages(void)
 {
 	int ret;
 
-	vdso_lookup[ARM64_VDSO32].dm = &aarch32_vdso_spec[C_VVAR];
-	vdso_lookup[ARM64_VDSO32].cm = &aarch32_vdso_spec[C_VDSO];
+	vdso_info[VDSO_ABI_AARCH32].dm = &aarch32_vdso_maps[AA32_MAP_VVAR];
+	vdso_info[VDSO_ABI_AARCH32].cm = &aarch32_vdso_maps[AA32_MAP_VDSO];
 
 	ret = __vdso_init(VDSO_ABI_AA32);
 	if (ret)
@@ -305,7 +296,7 @@ static int aarch32_kuser_helpers_setup(struct mm_struct *mm)
 	ret = _install_special_mapping(mm, AARCH32_VECTORS_BASE, PAGE_SIZE,
 				       VM_READ | VM_EXEC |
 				       VM_MAYREAD | VM_MAYEXEC,
-				       &aarch32_vdso_spec[C_VECTORS]);
+				       &aarch32_vdso_maps[AA32_MAP_VECTORS]);
 
 	return PTR_ERR_OR_ZERO(ret);
 }
@@ -329,7 +320,7 @@ static int aarch32_sigreturn_setup(struct mm_struct *mm)
 	ret = _install_special_mapping(mm, addr, PAGE_SIZE,
 				       VM_READ | VM_EXEC | VM_MAYREAD |
 				       VM_MAYWRITE | VM_MAYEXEC,
-				       &aarch32_vdso_spec[C_SIGPAGE]);
+				       &aarch32_vdso_maps[AA32_MAP_SIGPAGE]);
 	if (IS_ERR(ret))
 		goto out;
 
@@ -373,19 +364,16 @@ static int vdso_mremap(const struct vm_special_mapping *sm,
 	return __vdso_remap(VDSO_ABI_AA64, sm, new_vma);
 }
 
-/*
- * aarch64_vdso_pages:
- * 0 - vvar
- * 1 - vdso
- */
-#define A_VVAR		0
-#define A_VDSO		1
-#define A_PAGES		(A_VDSO + 1)
-static struct vm_special_mapping vdso_spec[A_PAGES] __ro_after_init = {
-	{
+enum aarch64_map {
+	AA64_MAP_VVAR,
+	AA64_MAP_VDSO,
+};
+
+static struct vm_special_mapping aarch64_vdso_maps[] __ro_after_init = {
+	[AA64_MAP_VVAR] = {
 		.name	= "[vvar]",
 	},
-	{
+	[AA64_MAP_VDSO] = {
 		.name	= "[vdso]",
 		.mremap = vdso_mremap,
 	},
@@ -393,8 +381,8 @@ static struct vm_special_mapping vdso_spec[A_PAGES] __ro_after_init = {
 
 static int __init vdso_init(void)
 {
-	vdso_info[VDSO_ABI_AA64].dm = &vdso_spec[A_VVAR];
-	vdso_info[VDSO_ABI_AA64].cm = &vdso_spec[A_VDSO];
+	vdso_info[VDSO_ABI_AA64].dm = &aarch64_vdso_maps[AA64_MAP_VVAR];
+	vdso_info[VDSO_ABI_AA64].cm = &aarch64_vdso_maps[AA64_MAP_VDSO];
 
 	return __vdso_init(VDSO_ABI_AA64);
 }
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/5] arm64: vdso: cleanups

[Will Deacon]

On Tue, Apr 14, 2020 at 11:42:47AM +0100, Mark Rutland wrote:
> While attempting to review an arm64 vdso patch, I noticed some of the existing
> code was somewhat baroque, making it harder than necessary to understand and
> extend. These patches attempt to improve this by making the code more
> consistent and avoiding unnecessary duplication.
> 
> The first patch in the series fixes a bug in a boot time error path. This bug
> was made obvious during the refactoring but I've moved it to the start so that
> it can be backported more easily.
> 
> The series is based on v5.7-rc1 and can be found in my arm64/vdso-cleanup
> branch [1].

Cheers, this looks really good to me. The only thing I'm slightly confused
by is that we still have something like this in __vdso_init():

	/* Allocate the vDSO pagelist, plus a page for the data. */
	vdso_pagelist = kcalloc(vdso_lookup[arch_index].vdso_pages + 1,
				sizeof(struct page *),
				GFP_KERNEL);

But I don't see why this needs to be dynamic, and don't we leak the
allocation on failure? Not a big deal, but seems silly if we could just
have a couple of static page * arrays.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/5] arm64: vdso: cleanups

[Mark Rutland]

On Tue, Apr 14, 2020 at 04:20:58PM +0100, Will Deacon wrote:
> On Tue, Apr 14, 2020 at 11:42:47AM +0100, Mark Rutland wrote:
> > While attempting to review an arm64 vdso patch, I noticed some of the existing
> > code was somewhat baroque, making it harder than necessary to understand and
> > extend. These patches attempt to improve this by making the code more
> > consistent and avoiding unnecessary duplication.
> > 
> > The first patch in the series fixes a bug in a boot time error path. This bug
> > was made obvious during the refactoring but I've moved it to the start so that
> > it can be backported more easily.
> > 
> > The series is based on v5.7-rc1 and can be found in my arm64/vdso-cleanup
> > branch [1].
> 
> Cheers, this looks really good to me. The only thing I'm slightly confused
> by is that we still have something like this in __vdso_init():
> 
> 	/* Allocate the vDSO pagelist, plus a page for the data. */
> 	vdso_pagelist = kcalloc(vdso_lookup[arch_index].vdso_pages + 1,
> 				sizeof(struct page *),
> 				GFP_KERNEL);
> 
> But I don't see why this needs to be dynamic, and don't we leak the
> allocation on failure? Not a big deal, but seems silly if we could just
> have a couple of static page * arrays.

Unfortunately it has to be dynamic as the number of vdso code pages
isn't known until the vdso is linked into the kernel proper. The only
way to allocate that at build time would be as part of the linker
script, and I think that'd be far more confusing.

We calculate the number of pages from the bounds of the code:

|         vdso_info[abi].vdso_pages = (
|                         vdso_info[abi].vdso_code_end -
|                         vdso_info[abi].vdso_code_start) >>
|                         PAGE_SHIFT;
| 
|         /* Allocate the vDSO pagelist, plus a page for the data. */
|         vdso_pagelist = kcalloc(vdso_info[abi].vdso_pages + 1,
|                                 sizeof(struct page *),
|                                 GFP_KERNEL);

... that said, conflating the data page list and code page list is
harming legibility, and I reckon that's worth cleaning up. The data page
list can be statically allocate given it's a single page.

There is a leak if we fail to allocate the compat vdso pages, but I
don't see a nice way of cleaning that up. It looks like
do_one_initcall() will WARN() in that case as we'll return a non-zero
error code.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/5] arm64: vdso: cleanups

[Will Deacon]

On Tue, Apr 14, 2020 at 04:43:19PM +0100, Mark Rutland wrote:
> On Tue, Apr 14, 2020 at 04:20:58PM +0100, Will Deacon wrote:
> > On Tue, Apr 14, 2020 at 11:42:47AM +0100, Mark Rutland wrote:
> > > While attempting to review an arm64 vdso patch, I noticed some of the existing
> > > code was somewhat baroque, making it harder than necessary to understand and
> > > extend. These patches attempt to improve this by making the code more
> > > consistent and avoiding unnecessary duplication.
> > > 
> > > The first patch in the series fixes a bug in a boot time error path. This bug
> > > was made obvious during the refactoring but I've moved it to the start so that
> > > it can be backported more easily.
> > > 
> > > The series is based on v5.7-rc1 and can be found in my arm64/vdso-cleanup
> > > branch [1].
> > 
> > Cheers, this looks really good to me. The only thing I'm slightly confused
> > by is that we still have something like this in __vdso_init():
> > 
> > 	/* Allocate the vDSO pagelist, plus a page for the data. */
> > 	vdso_pagelist = kcalloc(vdso_lookup[arch_index].vdso_pages + 1,
> > 				sizeof(struct page *),
> > 				GFP_KERNEL);
> > 
> > But I don't see why this needs to be dynamic, and don't we leak the
> > allocation on failure? Not a big deal, but seems silly if we could just
> > have a couple of static page * arrays.
> 
> Unfortunately it has to be dynamic as the number of vdso code pages
> isn't known until the vdso is linked into the kernel proper. The only
> way to allocate that at build time would be as part of the linker
> script, and I think that'd be far more confusing.

I was wondering whether we could extend gen_vdso_offsets.sh to emit this
information. Why isn't looking at the shared object enough? That said,
it does get grotty so maybe it's not worth doing.

> We calculate the number of pages from the bounds of the code:
> 
> |         vdso_info[abi].vdso_pages = (
> |                         vdso_info[abi].vdso_code_end -
> |                         vdso_info[abi].vdso_code_start) >>
> |                         PAGE_SHIFT;
> | 
> |         /* Allocate the vDSO pagelist, plus a page for the data. */
> |         vdso_pagelist = kcalloc(vdso_info[abi].vdso_pages + 1,
> |                                 sizeof(struct page *),
> |                                 GFP_KERNEL);
> 
> ... that said, conflating the data page list and code page list is
> harming legibility, and I reckon that's worth cleaning up. The data page
> list can be statically allocate given it's a single page.
> 
> There is a leak if we fail to allocate the compat vdso pages, but I
> don't see a nice way of cleaning that up. It looks like
> do_one_initcall() will WARN() in that case as we'll return a non-zero
> error code.

Yeah, I'm not really worried about that since I think we only fail if a
subsequent allocation fails, it's just that the dynamic allocation seemed
a bit OTT on first glance.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/5] arm64: vdso: cleanups

[Mark Rutland]

On Tue, Apr 14, 2020 at 04:52:48PM +0100, Will Deacon wrote:
> On Tue, Apr 14, 2020 at 04:43:19PM +0100, Mark Rutland wrote:
> > On Tue, Apr 14, 2020 at 04:20:58PM +0100, Will Deacon wrote:
> > > On Tue, Apr 14, 2020 at 11:42:47AM +0100, Mark Rutland wrote:
> > > > While attempting to review an arm64 vdso patch, I noticed some of the existing
> > > > code was somewhat baroque, making it harder than necessary to understand and
> > > > extend. These patches attempt to improve this by making the code more
> > > > consistent and avoiding unnecessary duplication.
> > > > 
> > > > The first patch in the series fixes a bug in a boot time error path. This bug
> > > > was made obvious during the refactoring but I've moved it to the start so that
> > > > it can be backported more easily.
> > > > 
> > > > The series is based on v5.7-rc1 and can be found in my arm64/vdso-cleanup
> > > > branch [1].
> > > 
> > > Cheers, this looks really good to me. The only thing I'm slightly confused
> > > by is that we still have something like this in __vdso_init():
> > > 
> > > 	/* Allocate the vDSO pagelist, plus a page for the data. */
> > > 	vdso_pagelist = kcalloc(vdso_lookup[arch_index].vdso_pages + 1,
> > > 				sizeof(struct page *),
> > > 				GFP_KERNEL);
> > > 
> > > But I don't see why this needs to be dynamic, and don't we leak the
> > > allocation on failure? Not a big deal, but seems silly if we could just
> > > have a couple of static page * arrays.
> > 
> > Unfortunately it has to be dynamic as the number of vdso code pages
> > isn't known until the vdso is linked into the kernel proper. The only
> > way to allocate that at build time would be as part of the linker
> > script, and I think that'd be far more confusing.
> 
> I was wondering whether we could extend gen_vdso_offsets.sh to emit this
> information. Why isn't looking at the shared object enough? That said,
> it does get grotty so maybe it's not worth doing.

I hadn't considered that, but having taken a look just now I think that
gets quite grotty and more subtle than what we have today, and I think
that using the vdso_start/vdso_end symbols as we do now leaves the least
scope for issues.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/5] arm64: vdso: cleanups

[Will Deacon]

On Tue, Apr 14, 2020 at 11:42:47AM +0100, Mark Rutland wrote:
> While attempting to review an arm64 vdso patch, I noticed some of the existing
> code was somewhat baroque, making it harder than necessary to understand and
> extend. These patches attempt to improve this by making the code more
> consistent and avoiding unnecessary duplication.
> 
> The first patch in the series fixes a bug in a boot time error path. This bug
> was made obvious during the refactoring but I've moved it to the start so that
> it can be backported more easily.
> 
> The series is based on v5.7-rc1 and can be found in my arm64/vdso-cleanup
> branch [1].
> 
> Thanks,
> Mark.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/vdso-cleanup
> 
> Mark Rutland (5):
>   arm64: vdso: don't free unallocated pages
>   arm64: vdso: remove aarch32_vdso_pages[]
>   arm64: vdso: simplify arch_vdso_type ifdeffery
>   arm64: vdso: use consistent 'abi' nomenclature
>   arm64: vdso: use consistent 'map' nomenclature

I tried to queue 2-5 but the compat vDSO fails to build for me:


arch/arm64/kernel/vdso.c:246:12: error: use of undeclared identifier 'VDSO_ABI_AARCH32'; did you mean 'VDSO_ABI_AA32'?
        vdso_info[VDSO_ABI_AARCH32].dm = &aarch32_vdso_maps[AA32_MAP_VVAR];
                  ^~~~~~~~~~~~~~~~
                  VDSO_ABI_AA32
arch/arm64/kernel/vdso.c:39:2: note: 'VDSO_ABI_AA32' declared here
        VDSO_ABI_AA32,
        ^
arch/arm64/kernel/vdso.c:247:12: error: use of undeclared identifier 'VDSO_ABI_AARCH32'; did you mean 'VDSO_ABI_AA32'?
        vdso_info[VDSO_ABI_AARCH32].cm = &aarch32_vdso_maps[AA32_MAP_VDSO];
                  ^~~~~~~~~~~~~~~~
                  VDSO_ABI_AA32
arch/arm64/kernel/vdso.c:39:2: note: 'VDSO_ABI_AA32' declared here
        VDSO_ABI_AA32,
        ^
2 errors generated.


Cheers,

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/5] arm64: vdso: cleanups

[Mark Rutland]

On Tue, Apr 28, 2020 at 01:49:46PM +0100, Will Deacon wrote:
> On Tue, Apr 14, 2020 at 11:42:47AM +0100, Mark Rutland wrote:
> > While attempting to review an arm64 vdso patch, I noticed some of the existing
> > code was somewhat baroque, making it harder than necessary to understand and
> > extend. These patches attempt to improve this by making the code more
> > consistent and avoiding unnecessary duplication.
> > 
> > The first patch in the series fixes a bug in a boot time error path. This bug
> > was made obvious during the refactoring but I've moved it to the start so that
> > it can be backported more easily.
> > 
> > The series is based on v5.7-rc1 and can be found in my arm64/vdso-cleanup
> > branch [1].
> > 
> > Thanks,
> > Mark.
> > 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/vdso-cleanup
> > 
> > Mark Rutland (5):
> >   arm64: vdso: don't free unallocated pages
> >   arm64: vdso: remove aarch32_vdso_pages[]
> >   arm64: vdso: simplify arch_vdso_type ifdeffery
> >   arm64: vdso: use consistent 'abi' nomenclature
> >   arm64: vdso: use consistent 'map' nomenclature
> 
> I tried to queue 2-5 but the compat vDSO fails to build for me:
> 
> 
> arch/arm64/kernel/vdso.c:246:12: error: use of undeclared identifier 'VDSO_ABI_AARCH32'; did you mean 'VDSO_ABI_AA32'?
>         vdso_info[VDSO_ABI_AARCH32].dm = &aarch32_vdso_maps[AA32_MAP_VVAR];
>                   ^~~~~~~~~~~~~~~~
>                   VDSO_ABI_AA32
> arch/arm64/kernel/vdso.c:39:2: note: 'VDSO_ABI_AA32' declared here
>         VDSO_ABI_AA32,
>         ^
> arch/arm64/kernel/vdso.c:247:12: error: use of undeclared identifier 'VDSO_ABI_AARCH32'; did you mean 'VDSO_ABI_AA32'?
>         vdso_info[VDSO_ABI_AARCH32].cm = &aarch32_vdso_maps[AA32_MAP_VDSO];
>                   ^~~~~~~~~~~~~~~~
>                   VDSO_ABI_AA32
> arch/arm64/kernel/vdso.c:39:2: note: 'VDSO_ABI_AA32' declared here
>         VDSO_ABI_AA32,
>         ^
> 2 errors generated.

Sorry for that; I clearly messed up patch 5 when rebasing to rename
things.

I'll send out a v2 once I've fixed and build-tested.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel