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)
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)
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
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.
[...]
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
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.
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.
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.
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.
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-stable-mirror@lists.linaro.org
-
Catalin Marinas -
Mark Rutland -
Vincenzo Frascino -
Will Deacon