diff mbox series

[-fixes] riscv: uaccess: Return the number of bytes effectively copied

Message ID 20230811110304.1613032-1-alexghiti@rivosinc.com
State New
Headers
Series [-fixes] riscv: uaccess: Return the number of bytes effectively copied |

Commit Message

Alexandre Ghiti Aug. 11, 2023, 11:03 a.m. UTC 
  It was reported that the riscv kernel hangs while executing the test
in [1].

Indeed, the test hangs when trying to write a buffer to a file. The
problem is that the riscv implementation of raw_copy_from_user() does not
return the number of bytes written when an exception happens and is fixed
up.

generic_perform_write() pre-faults the user pages and bails out if nothing
can be written, otherwise it will access the userspace buffer: here the
riscv implementation keeps returning it was not able to copy any byte
though the pre-faulting indicates otherwise. So generic_perform_write()
keeps retrying to access the user memory and ends up in an infinite
loop.

Note that before the commit mentioned in [1] that introduced this
regression, it worked because generic_perform_write() would bail out if
only one byte could not be written.

So fix this by returning the number of bytes effectively written in
__asm_copy_[to|from]_user() and __clear_user(), as it is expected.

[1] https://lore.kernel.org/linux-riscv/20230309151841.bomov6hq3ybyp42a@debian/

Fixes: ebcbd75e3962 ("riscv: Fix the bug in memory access fixup code")
Reported-by: Bo YU <tsu.yubo@gmail.com>
Closes: https://lore.kernel.org/linux-riscv/20230309151841.bomov6hq3ybyp42a@debian/#t
Reported-by: Aurelien Jarno <aurelien@aurel32.net>
Closes: https://lore.kernel.org/linux-riscv/ZNOnCakhwIeue3yr@aurel32.net/
Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
---
 arch/riscv/lib/uaccess.S | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Björn Töpel Aug. 11, 2023, 12:33 p.m. UTC | #1 
Alexandre Ghiti <alex@ghiti.fr> writes:

> On 11/08/2023 13:03, Alexandre Ghiti wrote:
>> It was reported that the riscv kernel hangs while executing the test
>> in [1].
>>
>> Indeed, the test hangs when trying to write a buffer to a file. The
>> problem is that the riscv implementation of raw_copy_from_user() does not
>> return the number of bytes written when an exception happens and is fixed
>> up.
>
>
> I'll respin another version as the changelog and the title are 
> incorrect: the uaccess routines should not return the number of bytes 
> copied but actually the number of bytes not copied (this is what this 
> patch implements).
>
> I'll wait for feedbacks before doing so!

Yikes! Nice catch.

Functions like fault_in_readable() will fail horribly w/o this. I wonder
why we haven't seen this problem more?

Feel free to add my RB to your next spin!

Reviewed-by: Björn Töpel <bjorn@rivosinc.com>
Aurelien Jarno Aug. 11, 2023, 2:35 p.m. UTC | #2 
Hi Alexandre,

On 2023-08-11 13:03, Alexandre Ghiti wrote:
> It was reported that the riscv kernel hangs while executing the test
> in [1].
> 
> Indeed, the test hangs when trying to write a buffer to a file. The
> problem is that the riscv implementation of raw_copy_from_user() does not
> return the number of bytes written when an exception happens and is fixed
> up.
> 
> generic_perform_write() pre-faults the user pages and bails out if nothing
> can be written, otherwise it will access the userspace buffer: here the
> riscv implementation keeps returning it was not able to copy any byte
> though the pre-faulting indicates otherwise. So generic_perform_write()
> keeps retrying to access the user memory and ends up in an infinite
> loop.
> 
> Note that before the commit mentioned in [1] that introduced this
> regression, it worked because generic_perform_write() would bail out if
> only one byte could not be written.
> 
> So fix this by returning the number of bytes effectively written in
> __asm_copy_[to|from]_user() and __clear_user(), as it is expected.
> 
> [1] https://lore.kernel.org/linux-riscv/20230309151841.bomov6hq3ybyp42a@debian/
> 
> Fixes: ebcbd75e3962 ("riscv: Fix the bug in memory access fixup code")
> Reported-by: Bo YU <tsu.yubo@gmail.com>
> Closes: https://lore.kernel.org/linux-riscv/20230309151841.bomov6hq3ybyp42a@debian/#t
> Reported-by: Aurelien Jarno <aurelien@aurel32.net>
> Closes: https://lore.kernel.org/linux-riscv/ZNOnCakhwIeue3yr@aurel32.net/
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> ---
>  arch/riscv/lib/uaccess.S | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)

Thanks for providing a patch so fast. I confirm that the issue I
reported is fixed, but also that the full strace testsuite now passes
successfully.

The code looks all good to me, so:

Tested-by: Aurelien Jarno <aurelien@aurel32.net>
Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>
diff mbox series

Patch

diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S
index ec486e5369d9..09b47ebacf2e 100644
--- a/arch/riscv/lib/uaccess.S
+++ b/arch/riscv/lib/uaccess.S
@@ -17,8 +17,11 @@  ENTRY(__asm_copy_from_user)
 	li t6, SR_SUM
 	csrs CSR_STATUS, t6
 
-	/* Save for return value */
-	mv	t5, a2
+	/*
+	 * Save the terminal address which will be used to compute the number
+	 * of bytes copied in case of a fixup exception.
+	 */
+	add	t5, a0, a2
 
 	/*
 	 * Register allocation for code below:
@@ -176,7 +179,7 @@  ENTRY(__asm_copy_from_user)
 10:
 	/* Disable access to user memory */
 	csrc CSR_STATUS, t6
-	mv a0, t5
+	sub a0, t5, a0
 	ret
 ENDPROC(__asm_copy_to_user)
 ENDPROC(__asm_copy_from_user)
@@ -228,7 +231,7 @@  ENTRY(__clear_user)
 11:
 	/* Disable access to user memory */
 	csrc CSR_STATUS, t6
-	mv a0, a1
+	sub a0, a3, a0
 	ret
 ENDPROC(__clear_user)
 EXPORT_SYMBOL(__clear_user)