From: Petr Tesarik <[email protected]>
If a segmentation fault is caused by accessing an address in the vmalloc
area, check that the target page is present.
Currently, if the kernel hits a guard page in the vmalloc area, UML blindly
assumes that the fault is caused by a stale mapping and will be fixed by
flush_tlb_kernel_vm(). Unsurprisingly, if the fault is caused by accessing
a guard page, no mapping is created, and when the faulting instruction is
restarted, it will cause exactly the same fault again, effectively creating
an infinite loop.
Signed-off-by: Petr Tesarik <[email protected]>
---
arch/um/kernel/trap.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c
index 6d8ae86ae978..d5b85f1bfe33 100644
--- a/arch/um/kernel/trap.c
+++ b/arch/um/kernel/trap.c
@@ -206,11 +206,15 @@ unsigned long segv(struct faultinfo fi, unsigned long ip, int is_user,
int err;
int is_write = FAULT_WRITE(fi);
unsigned long address = FAULT_ADDRESS(fi);
+ pte_t *pte;
if (!is_user && regs)
current->thread.segv_regs = container_of(regs, struct pt_regs, regs);
if (!is_user && (address >= start_vm) && (address < end_vm)) {
+ pte = virt_to_pte(&init_mm, address);
+ if (!pte_present(*pte))
+ page_fault_oops(regs, address, ip);
flush_tlb_kernel_vm();
goto out;
}
--
2.34.1
On 2/23/2024 3:04 PM, Petr Tesarik wrote:
> From: Petr Tesarik <[email protected]>
>
> If a segmentation fault is caused by accessing an address in the vmalloc
> area, check that the target page is present.
>
> Currently, if the kernel hits a guard page in the vmalloc area, UML blindly
> assumes that the fault is caused by a stale mapping and will be fixed by
> flush_tlb_kernel_vm(). Unsurprisingly, if the fault is caused by accessing
> a guard page, no mapping is created, and when the faulting instruction is
> restarted, it will cause exactly the same fault again, effectively creating
> an infinite loop.
Ping. Any comment on this fix?
Petr T
>
> Signed-off-by: Petr Tesarik <[email protected]>
> ---
> arch/um/kernel/trap.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c
> index 6d8ae86ae978..d5b85f1bfe33 100644
> --- a/arch/um/kernel/trap.c
> +++ b/arch/um/kernel/trap.c
> @@ -206,11 +206,15 @@ unsigned long segv(struct faultinfo fi, unsigned long ip, int is_user,
> int err;
> int is_write = FAULT_WRITE(fi);
> unsigned long address = FAULT_ADDRESS(fi);
> + pte_t *pte;
>
> if (!is_user && regs)
> current->thread.segv_regs = container_of(regs, struct pt_regs, regs);
>
> if (!is_user && (address >= start_vm) && (address < end_vm)) {
> + pte = virt_to_pte(&init_mm, address);
> + if (!pte_present(*pte))
> + page_fault_oops(regs, address, ip);
> flush_tlb_kernel_vm();
> goto out;
> }
On 3/12/2024 4:07 PM, Petr Tesarik wrote:
> On 2/23/2024 3:04 PM, Petr Tesarik wrote:
>> From: Petr Tesarik <[email protected]>
>>
>> If a segmentation fault is caused by accessing an address in the vmalloc
>> area, check that the target page is present.
>>
>> Currently, if the kernel hits a guard page in the vmalloc area, UML blindly
>> assumes that the fault is caused by a stale mapping and will be fixed by
>> flush_tlb_kernel_vm(). Unsurprisingly, if the fault is caused by accessing
>> a guard page, no mapping is created, and when the faulting instruction is
>> restarted, it will cause exactly the same fault again, effectively creating
>> an infinite loop.
>
> Ping. Any comment on this fix?
I don't think I have seen a reply from you. If you did comment, then
your email has not reached me.
Please, can you confirm you have seen my patch?
Kind regards
Petr T
> Petr T
>
>>
>> Signed-off-by: Petr Tesarik <[email protected]>
>> ---
>> arch/um/kernel/trap.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c
>> index 6d8ae86ae978..d5b85f1bfe33 100644
>> --- a/arch/um/kernel/trap.c
>> +++ b/arch/um/kernel/trap.c
>> @@ -206,11 +206,15 @@ unsigned long segv(struct faultinfo fi, unsigned long ip, int is_user,
>> int err;
>> int is_write = FAULT_WRITE(fi);
>> unsigned long address = FAULT_ADDRESS(fi);
>> + pte_t *pte;
>>
>> if (!is_user && regs)
>> current->thread.segv_regs = container_of(regs, struct pt_regs, regs);
>>
>> if (!is_user && (address >= start_vm) && (address < end_vm)) {
>> + pte = virt_to_pte(&init_mm, address);
>> + if (!pte_present(*pte))
>> + page_fault_oops(regs, address, ip);
>> flush_tlb_kernel_vm();
>> goto out;
>> }
>
On Fri, 23 Feb 2024 at 22:07, Petr Tesarik <[email protected]> wrote:
>
> From: Petr Tesarik <[email protected]>
>
> If a segmentation fault is caused by accessing an address in the vmalloc
> area, check that the target page is present.
>
> Currently, if the kernel hits a guard page in the vmalloc area, UML blindly
> assumes that the fault is caused by a stale mapping and will be fixed by
> flush_tlb_kernel_vm(). Unsurprisingly, if the fault is caused by accessing
> a guard page, no mapping is created, and when the faulting instruction is
> restarted, it will cause exactly the same fault again, effectively creating
> an infinite loop.
>
> Signed-off-by: Petr Tesarik <[email protected]>
> ---
> arch/um/kernel/trap.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c
> index 6d8ae86ae978..d5b85f1bfe33 100644
> --- a/arch/um/kernel/trap.c
> +++ b/arch/um/kernel/trap.c
> @@ -206,11 +206,15 @@ unsigned long segv(struct faultinfo fi, unsigned long ip, int is_user,
> int err;
> int is_write = FAULT_WRITE(fi);
> unsigned long address = FAULT_ADDRESS(fi);
> + pte_t *pte;
>
> if (!is_user && regs)
> current->thread.segv_regs = container_of(regs, struct pt_regs, regs);
>
> if (!is_user && (address >= start_vm) && (address < end_vm)) {
> + pte = virt_to_pte(&init_mm, address);
> + if (!pte_present(*pte))
> + page_fault_oops(regs, address, ip);
page_fault_oops() appears to be private to arch/x86/mm/fault.c, so
can't be used here?
Also, it accepts struct pt_regs*, not struct uml_pt_regs*, so would
need to at least handle the type difference here.
Could we equally avoid the infinite loop here by putting the
'flush_tlb_kernel_vm();goto out;' behind a if (pte_present(...))
check, and let the rest of the UML checks panic or oops if required.
(Actually OOPSing where we can under UML would be nice to do at some
point anyway, but is a bigger issue than just fixing a bug, IMO.)
Or am I lacking a prerequisite patch or applying this to the wrong
version (or otherwise missing something), as it definitely doesn't
build here.
Cheers,
-- David
On 3/21/2024 5:44 AM, David Gow wrote:
> On Fri, 23 Feb 2024 at 22:07, Petr Tesarik <[email protected]> wrote:
>>
>> From: Petr Tesarik <[email protected]>
>>
>> If a segmentation fault is caused by accessing an address in the vmalloc
>> area, check that the target page is present.
>>
>> Currently, if the kernel hits a guard page in the vmalloc area, UML blindly
>> assumes that the fault is caused by a stale mapping and will be fixed by
>> flush_tlb_kernel_vm(). Unsurprisingly, if the fault is caused by accessing
>> a guard page, no mapping is created, and when the faulting instruction is
>> restarted, it will cause exactly the same fault again, effectively creating
>> an infinite loop.
>>
>> Signed-off-by: Petr Tesarik <[email protected]>
>> ---
>> arch/um/kernel/trap.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c
>> index 6d8ae86ae978..d5b85f1bfe33 100644
>> --- a/arch/um/kernel/trap.c
>> +++ b/arch/um/kernel/trap.c
>> @@ -206,11 +206,15 @@ unsigned long segv(struct faultinfo fi, unsigned long ip, int is_user,
>> int err;
>> int is_write = FAULT_WRITE(fi);
>> unsigned long address = FAULT_ADDRESS(fi);
>> + pte_t *pte;
>>
>> if (!is_user && regs)
>> current->thread.segv_regs = container_of(regs, struct pt_regs, regs);
>>
>> if (!is_user && (address >= start_vm) && (address < end_vm)) {
>> + pte = virt_to_pte(&init_mm, address);
>> + if (!pte_present(*pte))
>> + page_fault_oops(regs, address, ip);
>
> page_fault_oops() appears to be private to arch/x86/mm/fault.c, so
> can't be used here?
> Also, it accepts struct pt_regs*, not struct uml_pt_regs*, so would
> need to at least handle the type difference here.
Argh, you're right. This was originally a two-patch series, but Richard
wanted improvements in the implementation which would require more
effort, see here:
http://lists.infradead.org/pipermail/linux-um/2024-January/006406.html
So I wanted to fix only the infinite loop, but in the mean time I forgot
about the dependency on the first patch:
http://lists.infradead.org/pipermail/linux-um/2023-December/006380.html
That's because a quick git grep page_fault_oops found the function. It
was my mistake that I did not notice the other page_fault_oops() earlier.
OK, please forget about this patch for now; I must rework it.
> Could we equally avoid the infinite loop here by putting the
> 'flush_tlb_kernel_vm();goto out;' behind a if (pte_present(...))
> check, and let the rest of the UML checks panic or oops if required.
> (Actually OOPSing where we can under UML would be nice to do at some
> point anyway, but is a bigger issue than just fixing a bug, IMO.)
Yes, that would be the best quick fix until I get to implementing all
the blows and whistles (oops_* helpers, notification chains, tainting,
etc.).
Petr T
> Or am I lacking a prerequisite patch or applying this to the wrong
> version (or otherwise missing something), as it definitely doesn't
> build here.
>
> Cheers,
> -- David
On 20/03/2024 14:09, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
>> Von: "Petr Tesarik" <[email protected]>
>>> Yes. I'm just way behind my maintainer schedule. :-/
>>
>> Understood. Thank you for your reply.
>>
>> By the way, are you looking for more people to help with the amount of work?
>
> Yes, help is always welcome!
> Johannes and Anton do already a great job but more maintenance power is always good.
> You could help with reviewing patches, testing stuff, etc. :-)
> It's not that UML itself is a lot of work, it's just that $dayjob is not UML related at all...
Same here.
>
> Thanks,
> //richard
>
>
--
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/
On 3/19/2024 11:18 PM, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
>> Von: "Petr Tesarik" <[email protected]>
>> An: "richard" <[email protected]>, "anton ivanov" <[email protected]>, "Johannes Berg"
>> <[email protected]>, "linux-um" <[email protected]>, "linux-kernel" <[email protected]>
>> CC: "Roberto Sassu" <[email protected]>, "petr" <[email protected]>
>> Gesendet: Montag, 18. März 2024 14:09:07
>> Betreff: Re: [PATCH RESEND 1/1] um: oops on accessing a non-present page in the vmalloc area
>
>> On 3/12/2024 4:07 PM, Petr Tesarik wrote:
>>> On 2/23/2024 3:04 PM, Petr Tesarik wrote:
>>>> From: Petr Tesarik <[email protected]>
>>>>
>>>> If a segmentation fault is caused by accessing an address in the vmalloc
>>>> area, check that the target page is present.
>>>>
>>>> Currently, if the kernel hits a guard page in the vmalloc area, UML blindly
>>>> assumes that the fault is caused by a stale mapping and will be fixed by
>>>> flush_tlb_kernel_vm(). Unsurprisingly, if the fault is caused by accessing
>>>> a guard page, no mapping is created, and when the faulting instruction is
>>>> restarted, it will cause exactly the same fault again, effectively creating
>>>> an infinite loop.
>>>
>>> Ping. Any comment on this fix?
>>
>> I don't think I have seen a reply from you. If you did comment, then
>> your email has not reached me.
>>
>> Please, can you confirm you have seen my patch?
>
> Yes. I'm just way behind my maintainer schedule. :-/
Understood. Thank you for your reply.
By the way, are you looking for more people to help with the amount of work?
Petr T
----- Ursprüngliche Mail -----
> Von: "Petr Tesarik" <[email protected]>
>> Yes. I'm just way behind my maintainer schedule. :-/
>
> Understood. Thank you for your reply.
>
> By the way, are you looking for more people to help with the amount of work?
Yes, help is always welcome!
Johannes and Anton do already a great job but more maintenance power is always good.
You could help with reviewing patches, testing stuff, etc. :-)
It's not that UML itself is a lot of work, it's just that $dayjob is not UML related at all...
Thanks,
//richard
----- Ursprüngliche Mail -----
> Von: "Petr Tesarik" <[email protected]>
> An: "richard" <[email protected]>, "anton ivanov" <[email protected]>, "Johannes Berg"
> <[email protected]>, "linux-um" <[email protected]>, "linux-kernel" <[email protected]>
> CC: "Roberto Sassu" <[email protected]>, "petr" <[email protected]>
> Gesendet: Montag, 18. März 2024 14:09:07
> Betreff: Re: [PATCH RESEND 1/1] um: oops on accessing a non-present page in the vmalloc area
> On 3/12/2024 4:07 PM, Petr Tesarik wrote:
>> On 2/23/2024 3:04 PM, Petr Tesarik wrote:
>>> From: Petr Tesarik <[email protected]>
>>>
>>> If a segmentation fault is caused by accessing an address in the vmalloc
>>> area, check that the target page is present.
>>>
>>> Currently, if the kernel hits a guard page in the vmalloc area, UML blindly
>>> assumes that the fault is caused by a stale mapping and will be fixed by
>>> flush_tlb_kernel_vm(). Unsurprisingly, if the fault is caused by accessing
>>> a guard page, no mapping is created, and when the faulting instruction is
>>> restarted, it will cause exactly the same fault again, effectively creating
>>> an infinite loop.
>>
>> Ping. Any comment on this fix?
>
> I don't think I have seen a reply from you. If you did comment, then
> your email has not reached me.
>
> Please, can you confirm you have seen my patch?
Yes. I'm just way behind my maintainer schedule. :-/
Thanks,
//richard