[PATCH 0/2] riscv: misaligned: fix misaligned accesses handling in put/get_user()

[PATCH 0/2] riscv: misaligned: fix misaligned accesses handling in put/get_user()

[Clément Léger]

While debugging a few problems with the misaligned access kselftest,
Alexandre discovered some crash with the current code. Indeed, some
misaligned access was done by the kernel using put_user(). This
was resulting in trap and a kernel crash since. The path was the
following:
user -> kernel -> access to user memory -> misaligned trap -> trap ->
kernel -> misaligned handling -> memcpy -> crash due to failed page fault
while in interrupt disabled section.

Last discussion about kernel misaligned handling and interrupt reenabling
were actually not to reenable interrupt when handling misaligned access
being done by kernel. The best solution being not to do any misaligned
accesses to userspace memory, we considered a few options:

- Remove any call to put/get_user() potientally doing misaligned
  accesses
- Do not do any misaligned accesses in put/get_user() itself

The second solution was the one chosen as there are too many callsite to
put/get_user() that could potentially do misaligned accesses. We tried
two approaches for that, either split access in two aligned accesses
(and do RMW for put_user()) or call copy_from/to_user() which does not
do any misaligned accesses. The later one was the simpler to implement
(although the performances are probably lower than split aligned
accesses but still way better than doing misaligned access emulation)
and allows to support what we wanted.

These commits are based on top of Alex dev/alex/get_user_misaligned_v1
branch.

Clément Léger (2):
  riscv: process: use unsigned int instead of unsigned long for
    put_user()
  riscv: uaccess: do not do misaligned accesses in get/put_user()

 arch/riscv/include/asm/uaccess.h | 28 ++++++++++++++++++++++------
 arch/riscv/kernel/process.c      |  2 +-
 2 files changed, 23 insertions(+), 7 deletions(-)

-- 
2.49.0

[PATCH 1/2] riscv: process: use unsigned int instead of unsigned long for put_user()

[Clément Léger]

The specification of prctl() for GET_UNALIGN_CTL states that the value is
returned in an unsigned int * address passed as an unsigned long. Change
the type to match that and avoid an unaligned access as well.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 arch/riscv/kernel/process.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
index 15d8f75902f8..9ee6d816b98b 100644
--- a/arch/riscv/kernel/process.c
+++ b/arch/riscv/kernel/process.c
@@ -57,7 +57,7 @@ int get_unalign_ctl(struct task_struct *tsk, unsigned long adr)
 	if (!unaligned_ctl_available())
 		return -EINVAL;
 
-	return put_user(tsk->thread.align_ctl, (unsigned long __user *)adr);
+	return put_user(tsk->thread.align_ctl, (unsigned int __user *)adr);
 }
 
 void __show_regs(struct pt_regs *regs)
-- 
2.49.0

[PATCH 2/2] riscv: uaccess: do not do misaligned accesses in get/put_user()

[Clément Léger]

Doing misaligned access to userspace memory would make a trap on
platform where it is emulated. Latest fixes removed the kernel
capability to do unaligned accesses to userspace memory safely since
interrupts are kept disabled at all time during that. Thus doing so
would crash the kernel.

Such behavior was detected with GET_UNALIGN_CTL() that was doing
a put_user() with an unsigned long* address that should have been an
unsigned int*. Reenabling kernel misaligned access emulation is a bit
risky and it would also degrade performances. Rather than doing that,
we will try to avoid any misaligned accessed by using copy_from/to_user()
which does not do any misaligned accesses. This can be done only for
!CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS and thus allows to only generate
a bit more code for this config.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 arch/riscv/include/asm/uaccess.h | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
index 046de7ced09c..b542c05f394f 100644
--- a/arch/riscv/include/asm/uaccess.h
+++ b/arch/riscv/include/asm/uaccess.h
@@ -169,8 +169,21 @@ do {								\
 
 #endif /* CONFIG_64BIT */
 
+unsigned long __must_check __asm_copy_to_user(void __user *to,
+	const void *from, unsigned long n);
+unsigned long __must_check __asm_copy_from_user(void *to,
+	const void __user *from, unsigned long n);
+
 #define __get_user_nocheck(x, __gu_ptr, label)			\
 do {								\
+	if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)) {			\
+		if (!IS_ALIGNED((uintptr_t)__gu_ptr, sizeof(*__gu_ptr))) {		\
+			if (__asm_copy_from_user(&(x), __gu_ptr, sizeof(*__gu_ptr)))	\
+				goto label;			\
+			else					\
+				break;				\
+		}						\
+	}							\
 	switch (sizeof(*__gu_ptr)) {				\
 	case 1:							\
 		__get_user_asm("lb", (x), __gu_ptr, label);	\
@@ -297,6 +310,15 @@ do {								\
 
 #define __put_user_nocheck(x, __gu_ptr, label)			\
 do {								\
+	if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)) {			\
+		if (!IS_ALIGNED((uintptr_t)__gu_ptr, sizeof(*__gu_ptr))) {		\
+			unsigned long val = (unsigned long)(x);				\
+			if (__asm_copy_to_user(__gu_ptr, &(val), sizeof(*__gu_ptr)))	\
+				goto label;			\
+			else					\
+				break;				\
+		}						\
+	}							\
 	switch (sizeof(*__gu_ptr)) {				\
 	case 1:							\
 		__put_user_asm("sb", (x), __gu_ptr, label);	\
@@ -385,12 +407,6 @@ err_label:							\
 		-EFAULT;					\
 })
 
-
-unsigned long __must_check __asm_copy_to_user(void __user *to,
-	const void *from, unsigned long n);
-unsigned long __must_check __asm_copy_from_user(void *to,
-	const void __user *from, unsigned long n);
-
 static inline unsigned long
 raw_copy_from_user(void *to, const void __user *from, unsigned long n)
 {
-- 
2.49.0

Re: [PATCH 1/2] riscv: process: use unsigned int instead of unsigned long for put_user()

[Alexandre Ghiti]

Hi Clément,

On 5/30/25 22:56, Clément Léger wrote:
> The specification of prctl() for GET_UNALIGN_CTL states that the value is
> returned in an unsigned int * address passed as an unsigned long. Change
> the type to match that and avoid an unaligned access as well.
>
> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> ---
>   arch/riscv/kernel/process.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
> index 15d8f75902f8..9ee6d816b98b 100644
> --- a/arch/riscv/kernel/process.c
> +++ b/arch/riscv/kernel/process.c
> @@ -57,7 +57,7 @@ int get_unalign_ctl(struct task_struct *tsk, unsigned long adr)
>   	if (!unaligned_ctl_available())
>   		return -EINVAL;
>   
> -	return put_user(tsk->thread.align_ctl, (unsigned long __user *)adr);
> +	return put_user(tsk->thread.align_ctl, (unsigned int __user *)adr);
>   }
>   
>   void __show_regs(struct pt_regs *regs)


Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>

Thanks,

Alex

Re: [PATCH 2/2] riscv: uaccess: do not do misaligned accesses in get/put_user()

[Alexandre Ghiti]

On 5/30/25 22:56, Clément Léger wrote:
> Doing misaligned access to userspace memory would make a trap on
> platform where it is emulated. Latest fixes removed the kernel
> capability to do unaligned accesses to userspace memory safely since
> interrupts are kept disabled at all time during that. Thus doing so
> would crash the kernel.
>
> Such behavior was detected with GET_UNALIGN_CTL() that was doing
> a put_user() with an unsigned long* address that should have been an
> unsigned int*. Reenabling kernel misaligned access emulation is a bit
> risky and it would also degrade performances. Rather than doing that,
> we will try to avoid any misaligned accessed by using copy_from/to_user()
> which does not do any misaligned accesses. This can be done only for
> !CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS and thus allows to only generate
> a bit more code for this config.
>
> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> ---
>   arch/riscv/include/asm/uaccess.h | 28 ++++++++++++++++++++++------
>   1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
> index 046de7ced09c..b542c05f394f 100644
> --- a/arch/riscv/include/asm/uaccess.h
> +++ b/arch/riscv/include/asm/uaccess.h
> @@ -169,8 +169,21 @@ do {								\
>   
>   #endif /* CONFIG_64BIT */
>   
> +unsigned long __must_check __asm_copy_to_user(void __user *to,
> +	const void *from, unsigned long n);
> +unsigned long __must_check __asm_copy_from_user(void *to,
> +	const void __user *from, unsigned long n);
> +
>   #define __get_user_nocheck(x, __gu_ptr, label)			\
>   do {								\
> +	if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)) {			\
> +		if (!IS_ALIGNED((uintptr_t)__gu_ptr, sizeof(*__gu_ptr))) {		\


Nit: I would use && instead of 2 ifs.


> +			if (__asm_copy_from_user(&(x), __gu_ptr, sizeof(*__gu_ptr)))	\
> +				goto label;			\
> +			else					\
> +				break;				\


Here I would remove the else


> +		}						\
> +	}							\
>   	switch (sizeof(*__gu_ptr)) {				\
>   	case 1:							\
>   		__get_user_asm("lb", (x), __gu_ptr, label);	\
> @@ -297,6 +310,15 @@ do {								\
>   
>   #define __put_user_nocheck(x, __gu_ptr, label)			\
>   do {								\
> +	if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)) {			\
> +		if (!IS_ALIGNED((uintptr_t)__gu_ptr, sizeof(*__gu_ptr))) {		\
> +			unsigned long val = (unsigned long)(x);				\


Here it sems like __inttype(*(__gu_ptr)) is more accurate than unsigned 
long, even though I think unsigned long works fine too.


> +			if (__asm_copy_to_user(__gu_ptr, &(val), sizeof(*__gu_ptr)))	\
> +				goto label;			\
> +			else					\
> +				break;				\
> +		}						\
> +	}							\
>   	switch (sizeof(*__gu_ptr)) {				\
>   	case 1:							\
>   		__put_user_asm("sb", (x), __gu_ptr, label);	\
> @@ -385,12 +407,6 @@ err_label:							\
>   		-EFAULT;					\
>   })
>   
> -
> -unsigned long __must_check __asm_copy_to_user(void __user *to,
> -	const void *from, unsigned long n);
> -unsigned long __must_check __asm_copy_from_user(void *to,
> -	const void __user *from, unsigned long n);
> -
>   static inline unsigned long
>   raw_copy_from_user(void *to, const void __user *from, unsigned long n)
>   {

Re: [PATCH 0/2] riscv: misaligned: fix misaligned accesses handling in put/get_user()

[Alexandre Ghiti]

On 5/30/25 22:56, Clément Léger wrote:
> While debugging a few problems with the misaligned access kselftest,
> Alexandre discovered some crash with the current code. Indeed, some
> misaligned access was done by the kernel using put_user(). This
> was resulting in trap and a kernel crash since. The path was the
> following:
> user -> kernel -> access to user memory -> misaligned trap -> trap ->
> kernel -> misaligned handling -> memcpy -> crash due to failed page fault
> while in interrupt disabled section.
>
> Last discussion about kernel misaligned handling and interrupt reenabling
> were actually not to reenable interrupt when handling misaligned access
> being done by kernel. The best solution being not to do any misaligned
> accesses to userspace memory, we considered a few options:
>
> - Remove any call to put/get_user() potientally doing misaligned
>    accesses
> - Do not do any misaligned accesses in put/get_user() itself
>
> The second solution was the one chosen as there are too many callsite to
> put/get_user() that could potentially do misaligned accesses. We tried
> two approaches for that, either split access in two aligned accesses
> (and do RMW for put_user()) or call copy_from/to_user() which does not
> do any misaligned accesses. The later one was the simpler to implement
> (although the performances are probably lower than split aligned
> accesses but still way better than doing misaligned access emulation)
> and allows to support what we wanted.
>
> These commits are based on top of Alex dev/alex/get_user_misaligned_v1
> branch.
>
> Clément Léger (2):
>    riscv: process: use unsigned int instead of unsigned long for
>      put_user()
>    riscv: uaccess: do not do misaligned accesses in get/put_user()
>
>   arch/riscv/include/asm/uaccess.h | 28 ++++++++++++++++++++++------
>   arch/riscv/kernel/process.c      |  2 +-
>   2 files changed, 23 insertions(+), 7 deletions(-)


We also need to prevent unsafe routines to trigger misaligned accesses, 
I have a patch for this here 
https://github.com/linux-riscv/linux/commit/7c172121aeb235dedeb6f5e06740527530edd6af

Clément, can you add this one to the series please?

I have just  triggered a CI with those fixes on top of my sbi 3.0 branch.

Thanks,

Alex

Re: [PATCH 2/2] riscv: uaccess: do not do misaligned accesses in get/put_user()

[David Laight]

On Fri, 30 May 2025 22:56:58 +0200
Clément Léger <cleger@rivosinc.com> wrote:

> Doing misaligned access to userspace memory would make a trap on
> platform where it is emulated. Latest fixes removed the kernel
> capability to do unaligned accesses to userspace memory safely since
> interrupts are kept disabled at all time during that. Thus doing so
> would crash the kernel.
> 
> Such behavior was detected with GET_UNALIGN_CTL() that was doing
> a put_user() with an unsigned long* address that should have been an
> unsigned int*. Reenabling kernel misaligned access emulation is a bit
> risky and it would also degrade performances. Rather than doing that,
> we will try to avoid any misaligned accessed by using copy_from/to_user()
> which does not do any misaligned accesses. This can be done only for
> !CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS and thus allows to only generate
> a bit more code for this config.

For get_user() you are much better off reading the two words that contain
the value and then doing 'shift' and 'or' to get the correct value.

Even for put_user() doing the explicit byte accesses will be faster than
going though the generic copy_to/from_user() function.

	David

> 
> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> ---
>  arch/riscv/include/asm/uaccess.h | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
> index 046de7ced09c..b542c05f394f 100644
> --- a/arch/riscv/include/asm/uaccess.h
> +++ b/arch/riscv/include/asm/uaccess.h
> @@ -169,8 +169,21 @@ do {								\
>  
>  #endif /* CONFIG_64BIT */
>  
> +unsigned long __must_check __asm_copy_to_user(void __user *to,
> +	const void *from, unsigned long n);
> +unsigned long __must_check __asm_copy_from_user(void *to,
> +	const void __user *from, unsigned long n);
> +
>  #define __get_user_nocheck(x, __gu_ptr, label)			\
>  do {								\
> +	if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)) {			\
> +		if (!IS_ALIGNED((uintptr_t)__gu_ptr, sizeof(*__gu_ptr))) {		\
> +			if (__asm_copy_from_user(&(x), __gu_ptr, sizeof(*__gu_ptr)))	\
> +				goto label;			\
> +			else					\
> +				break;				\
> +		}						\
> +	}							\
>  	switch (sizeof(*__gu_ptr)) {				\
>  	case 1:							\
>  		__get_user_asm("lb", (x), __gu_ptr, label);	\
> @@ -297,6 +310,15 @@ do {								\
>  
>  #define __put_user_nocheck(x, __gu_ptr, label)			\
>  do {								\
> +	if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)) {			\
> +		if (!IS_ALIGNED((uintptr_t)__gu_ptr, sizeof(*__gu_ptr))) {		\
> +			unsigned long val = (unsigned long)(x);				\
> +			if (__asm_copy_to_user(__gu_ptr, &(val), sizeof(*__gu_ptr)))	\
> +				goto label;			\
> +			else					\
> +				break;				\
> +		}						\
> +	}							\
>  	switch (sizeof(*__gu_ptr)) {				\
>  	case 1:							\
>  		__put_user_asm("sb", (x), __gu_ptr, label);	\
> @@ -385,12 +407,6 @@ err_label:							\
>  		-EFAULT;					\
>  })
>  
> -
> -unsigned long __must_check __asm_copy_to_user(void __user *to,
> -	const void *from, unsigned long n);
> -unsigned long __must_check __asm_copy_from_user(void *to,
> -	const void __user *from, unsigned long n);
> -
>  static inline unsigned long
>  raw_copy_from_user(void *to, const void __user *from, unsigned long n)
>  {

Re: [PATCH 2/2] riscv: uaccess: do not do misaligned accesses in get/put_user()

[Maciej W. Rozycki]

On Sat, 31 May 2025, David Laight wrote:

> > Such behavior was detected with GET_UNALIGN_CTL() that was doing
> > a put_user() with an unsigned long* address that should have been an
> > unsigned int*. Reenabling kernel misaligned access emulation is a bit
> > risky and it would also degrade performances. Rather than doing that,
> > we will try to avoid any misaligned accessed by using copy_from/to_user()
> > which does not do any misaligned accesses. This can be done only for
> > !CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS and thus allows to only generate
> > a bit more code for this config.
> 
> For get_user() you are much better off reading the two words that contain
> the value and then doing 'shift' and 'or' to get the correct value.
> 
> Even for put_user() doing the explicit byte accesses will be faster than
> going though the generic copy_to/from_user() function.

 FWIW I think optimising copy_to/from_user for such cases would be a more 
robust approach moving forward than sprinkling open-coded implementations 
across code.

  Maciej

Re: [PATCH 0/2] riscv: misaligned: fix misaligned accesses handling in put/get_user()

[Clément Léger]

On 31/05/2025 15:32, Alexandre Ghiti wrote:
> On 5/30/25 22:56, Clément Léger wrote:
>> While debugging a few problems with the misaligned access kselftest,
>> Alexandre discovered some crash with the current code. Indeed, some
>> misaligned access was done by the kernel using put_user(). This
>> was resulting in trap and a kernel crash since. The path was the
>> following:
>> user -> kernel -> access to user memory -> misaligned trap -> trap ->
>> kernel -> misaligned handling -> memcpy -> crash due to failed page fault
>> while in interrupt disabled section.
>>
>> Last discussion about kernel misaligned handling and interrupt reenabling
>> were actually not to reenable interrupt when handling misaligned access
>> being done by kernel. The best solution being not to do any misaligned
>> accesses to userspace memory, we considered a few options:
>>
>> - Remove any call to put/get_user() potientally doing misaligned
>>    accesses
>> - Do not do any misaligned accesses in put/get_user() itself
>>
>> The second solution was the one chosen as there are too many callsite to
>> put/get_user() that could potentially do misaligned accesses. We tried
>> two approaches for that, either split access in two aligned accesses
>> (and do RMW for put_user()) or call copy_from/to_user() which does not
>> do any misaligned accesses. The later one was the simpler to implement
>> (although the performances are probably lower than split aligned
>> accesses but still way better than doing misaligned access emulation)
>> and allows to support what we wanted.
>>
>> These commits are based on top of Alex dev/alex/get_user_misaligned_v1
>> branch.
>>
>> Clément Léger (2):
>>    riscv: process: use unsigned int instead of unsigned long for
>>      put_user()
>>    riscv: uaccess: do not do misaligned accesses in get/put_user()
>>
>>   arch/riscv/include/asm/uaccess.h | 28 ++++++++++++++++++++++------
>>   arch/riscv/kernel/process.c      |  2 +-
>>   2 files changed, 23 insertions(+), 7 deletions(-)
> 
> 
> We also need to prevent unsafe routines to trigger misaligned accesses,
> I have a patch for this here https://github.com/linux-riscv/linux/
> commit/7c172121aeb235dedeb6f5e06740527530edd6af
> 
> Clément, can you add this one to the series please?

Yep sure.

> 
> I have just  triggered a CI with those fixes on top of my sbi 3.0 branch.
> 
> Thanks,
> 
> Alex
> 
> 

Re: [PATCH 2/2] riscv: uaccess: do not do misaligned accesses in get/put_user()

[Clément Léger]

On 31/05/2025 20:28, David Laight wrote:
> On Fri, 30 May 2025 22:56:58 +0200
> Clément Léger <cleger@rivosinc.com> wrote:
> 
>> Doing misaligned access to userspace memory would make a trap on
>> platform where it is emulated. Latest fixes removed the kernel
>> capability to do unaligned accesses to userspace memory safely since
>> interrupts are kept disabled at all time during that. Thus doing so
>> would crash the kernel.
>>
>> Such behavior was detected with GET_UNALIGN_CTL() that was doing
>> a put_user() with an unsigned long* address that should have been an
>> unsigned int*. Reenabling kernel misaligned access emulation is a bit
>> risky and it would also degrade performances. Rather than doing that,
>> we will try to avoid any misaligned accessed by using copy_from/to_user()
>> which does not do any misaligned accesses. This can be done only for
>> !CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS and thus allows to only generate
>> a bit more code for this config.
> 
> For get_user() you are much better off reading the two words that contain
> the value and then doing 'shift' and 'or' to get the correct value.
> 
> Even for put_user() doing the explicit byte accesses will be faster than
> going though the generic copy_to/from_user() function.

Hi David,

Alexandre tried that approach as well but that added a bit more code and
it was more complex than just calling copy_from/to_user(). That can
still be done in another commit if we need more performance later. As a
side note, prior to that patch, these misaligned accesses were using
trap-and-emulate so that is still a performance improvement.

Thanks,

Clément

> 
> 	David
> 
>>
>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>> ---
>>  arch/riscv/include/asm/uaccess.h | 28 ++++++++++++++++++++++------
>>  1 file changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
>> index 046de7ced09c..b542c05f394f 100644
>> --- a/arch/riscv/include/asm/uaccess.h
>> +++ b/arch/riscv/include/asm/uaccess.h
>> @@ -169,8 +169,21 @@ do {								\
>>  
>>  #endif /* CONFIG_64BIT */
>>  
>> +unsigned long __must_check __asm_copy_to_user(void __user *to,
>> +	const void *from, unsigned long n);
>> +unsigned long __must_check __asm_copy_from_user(void *to,
>> +	const void __user *from, unsigned long n);
>> +
>>  #define __get_user_nocheck(x, __gu_ptr, label)			\
>>  do {								\
>> +	if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)) {			\
>> +		if (!IS_ALIGNED((uintptr_t)__gu_ptr, sizeof(*__gu_ptr))) {		\
>> +			if (__asm_copy_from_user(&(x), __gu_ptr, sizeof(*__gu_ptr)))	\
>> +				goto label;			\
>> +			else					\
>> +				break;				\
>> +		}						\
>> +	}							\
>>  	switch (sizeof(*__gu_ptr)) {				\
>>  	case 1:							\
>>  		__get_user_asm("lb", (x), __gu_ptr, label);	\
>> @@ -297,6 +310,15 @@ do {								\
>>  
>>  #define __put_user_nocheck(x, __gu_ptr, label)			\
>>  do {								\
>> +	if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)) {			\
>> +		if (!IS_ALIGNED((uintptr_t)__gu_ptr, sizeof(*__gu_ptr))) {		\
>> +			unsigned long val = (unsigned long)(x);				\
>> +			if (__asm_copy_to_user(__gu_ptr, &(val), sizeof(*__gu_ptr)))	\
>> +				goto label;			\
>> +			else					\
>> +				break;				\
>> +		}						\
>> +	}							\
>>  	switch (sizeof(*__gu_ptr)) {				\
>>  	case 1:							\
>>  		__put_user_asm("sb", (x), __gu_ptr, label);	\
>> @@ -385,12 +407,6 @@ err_label:							\
>>  		-EFAULT;					\
>>  })
>>  
>> -
>> -unsigned long __must_check __asm_copy_to_user(void __user *to,
>> -	const void *from, unsigned long n);
>> -unsigned long __must_check __asm_copy_from_user(void *to,
>> -	const void __user *from, unsigned long n);
>> -
>>  static inline unsigned long
>>  raw_copy_from_user(void *to, const void __user *from, unsigned long n)
>>  {
> 

Re: [PATCH 2/2] riscv: uaccess: do not do misaligned accesses in get/put_user()

[Clément Léger]

On 01/06/2025 19:35, Maciej W. Rozycki wrote:
> On Sat, 31 May 2025, David Laight wrote:
> 
>>> Such behavior was detected with GET_UNALIGN_CTL() that was doing
>>> a put_user() with an unsigned long* address that should have been an
>>> unsigned int*. Reenabling kernel misaligned access emulation is a bit
>>> risky and it would also degrade performances. Rather than doing that,
>>> we will try to avoid any misaligned accessed by using copy_from/to_user()
>>> which does not do any misaligned accesses. This can be done only for
>>> !CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS and thus allows to only generate
>>> a bit more code for this config.
>>
>> For get_user() you are much better off reading the two words that contain
>> the value and then doing 'shift' and 'or' to get the correct value.
>>
>> Even for put_user() doing the explicit byte accesses will be faster than
>> going though the generic copy_to/from_user() function.
> 
>  FWIW I think optimising copy_to/from_user for such cases would be a more 
> robust approach moving forward than sprinkling open-coded implementations 
> across code.

Hi Maciej,

Indeed, that's a good idea, we could optimize small copy in
copy_from/to_user so that will benefit all the users as well.

Thanks,

Clément

> 
>   Maciej

Re: [PATCH 2/2] riscv: uaccess: do not do misaligned accesses in get/put_user()

[Clément Léger]

On 31/05/2025 14:35, Alexandre Ghiti wrote:
> On 5/30/25 22:56, Clément Léger wrote:
>> Doing misaligned access to userspace memory would make a trap on
>> platform where it is emulated. Latest fixes removed the kernel
>> capability to do unaligned accesses to userspace memory safely since
>> interrupts are kept disabled at all time during that. Thus doing so
>> would crash the kernel.
>>
>> Such behavior was detected with GET_UNALIGN_CTL() that was doing
>> a put_user() with an unsigned long* address that should have been an
>> unsigned int*. Reenabling kernel misaligned access emulation is a bit
>> risky and it would also degrade performances. Rather than doing that,
>> we will try to avoid any misaligned accessed by using copy_from/to_user()
>> which does not do any misaligned accesses. This can be done only for
>> !CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS and thus allows to only generate
>> a bit more code for this config.
>>
>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>> ---
>>   arch/riscv/include/asm/uaccess.h | 28 ++++++++++++++++++++++------
>>   1 file changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/
>> asm/uaccess.h
>> index 046de7ced09c..b542c05f394f 100644
>> --- a/arch/riscv/include/asm/uaccess.h
>> +++ b/arch/riscv/include/asm/uaccess.h
>> @@ -169,8 +169,21 @@ do {                                \
>>     #endif /* CONFIG_64BIT */
>>   +unsigned long __must_check __asm_copy_to_user(void __user *to,
>> +    const void *from, unsigned long n);
>> +unsigned long __must_check __asm_copy_from_user(void *to,
>> +    const void __user *from, unsigned long n);
>> +
>>   #define __get_user_nocheck(x, __gu_ptr, label)            \
>>   do {                                \
>> +    if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS))
>> {            \
>> +        if (!IS_ALIGNED((uintptr_t)__gu_ptr, sizeof(*__gu_ptr)))
>> {        \
> 
> 
> Nit: I would use && instead of 2 ifs.
> 
> 
>> +            if (__asm_copy_from_user(&(x), __gu_ptr,
>> sizeof(*__gu_ptr)))    \
>> +                goto label;            \
>> +            else                    \
>> +                break;                \
> 
> 
> Here I would remove the else

Hi Alex,

The "else" is needed to break from the outer do/while loop or it will go
though the next switch case (and it will crash due to misaligned accesses).

> 
> 
>> +        }                        \
>> +    }                            \
>>       switch (sizeof(*__gu_ptr)) {                \
>>       case 1:                            \
>>           __get_user_asm("lb", (x), __gu_ptr, label);    \
>> @@ -297,6 +310,15 @@ do {                                \
>>     #define __put_user_nocheck(x, __gu_ptr, label)            \
>>   do {                                \
>> +    if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS))
>> {            \
>> +        if (!IS_ALIGNED((uintptr_t)__gu_ptr, sizeof(*__gu_ptr)))
>> {        \
>> +            unsigned long val = (unsigned long)(x);                \
> 
> 
> Here it sems like __inttype(*(__gu_ptr)) is more accurate than unsigned
> long, even though I think unsigned long works fine too.

Wasn't aware of __inttype, but it sounds good.

Thanks,

Clément

> 
> 
>> +            if (__asm_copy_to_user(__gu_ptr, &(val),
>> sizeof(*__gu_ptr)))    \
>> +                goto label;            \
>> +            else                    \
>> +                break;                \
>> +        }                        \
>> +    }                            \
>>       switch (sizeof(*__gu_ptr)) {                \
>>       case 1:                            \
>>           __put_user_asm("sb", (x), __gu_ptr, label);    \
>> @@ -385,12 +407,6 @@ err_label:                            \
>>           -EFAULT;                    \
>>   })
>>   -
>> -unsigned long __must_check __asm_copy_to_user(void __user *to,
>> -    const void *from, unsigned long n);
>> -unsigned long __must_check __asm_copy_from_user(void *to,
>> -    const void __user *from, unsigned long n);
>> -
>>   static inline unsigned long
>>   raw_copy_from_user(void *to, const void __user *from, unsigned long n)
>>   {

Re: [PATCH 2/2] riscv: uaccess: do not do misaligned accesses in get/put_user()

[Alexandre Ghiti]

Hi Clément,

On 6/2/25 09:37, Clément Léger wrote:
>
> On 31/05/2025 14:35, Alexandre Ghiti wrote:
>> On 5/30/25 22:56, Clément Léger wrote:
>>> Doing misaligned access to userspace memory would make a trap on
>>> platform where it is emulated. Latest fixes removed the kernel
>>> capability to do unaligned accesses to userspace memory safely since
>>> interrupts are kept disabled at all time during that. Thus doing so
>>> would crash the kernel.
>>>
>>> Such behavior was detected with GET_UNALIGN_CTL() that was doing
>>> a put_user() with an unsigned long* address that should have been an
>>> unsigned int*. Reenabling kernel misaligned access emulation is a bit
>>> risky and it would also degrade performances. Rather than doing that,
>>> we will try to avoid any misaligned accessed by using copy_from/to_user()
>>> which does not do any misaligned accesses. This can be done only for
>>> !CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS and thus allows to only generate
>>> a bit more code for this config.
>>>
>>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>>> ---
>>>    arch/riscv/include/asm/uaccess.h | 28 ++++++++++++++++++++++------
>>>    1 file changed, 22 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/
>>> asm/uaccess.h
>>> index 046de7ced09c..b542c05f394f 100644
>>> --- a/arch/riscv/include/asm/uaccess.h
>>> +++ b/arch/riscv/include/asm/uaccess.h
>>> @@ -169,8 +169,21 @@ do {                                \
>>>      #endif /* CONFIG_64BIT */
>>>    +unsigned long __must_check __asm_copy_to_user(void __user *to,
>>> +    const void *from, unsigned long n);
>>> +unsigned long __must_check __asm_copy_from_user(void *to,
>>> +    const void __user *from, unsigned long n);
>>> +
>>>    #define __get_user_nocheck(x, __gu_ptr, label)            \
>>>    do {                                \
>>> +    if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS))
>>> {            \
>>> +        if (!IS_ALIGNED((uintptr_t)__gu_ptr, sizeof(*__gu_ptr)))
>>> {        \
>>
>> Nit: I would use && instead of 2 ifs.
>>
>>
>>> +            if (__asm_copy_from_user(&(x), __gu_ptr,
>>> sizeof(*__gu_ptr)))    \
>>> +                goto label;            \
>>> +            else                    \
>>> +                break;                \
>>
>> Here I would remove the else
> Hi Alex,
>
> The "else" is needed to break from the outer do/while loop or it will go
> though the next switch case (and it will crash due to misaligned accesses).


I meant only the "else", not the "break"!

Thanks,

Alex


>
>>
>>> +        }                        \
>>> +    }                            \
>>>        switch (sizeof(*__gu_ptr)) {                \
>>>        case 1:                            \
>>>            __get_user_asm("lb", (x), __gu_ptr, label);    \
>>> @@ -297,6 +310,15 @@ do {                                \
>>>      #define __put_user_nocheck(x, __gu_ptr, label)            \
>>>    do {                                \
>>> +    if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS))
>>> {            \
>>> +        if (!IS_ALIGNED((uintptr_t)__gu_ptr, sizeof(*__gu_ptr)))
>>> {        \
>>> +            unsigned long val = (unsigned long)(x);                \
>>
>> Here it sems like __inttype(*(__gu_ptr)) is more accurate than unsigned
>> long, even though I think unsigned long works fine too.
> Wasn't aware of __inttype, but it sounds good.
>
> Thanks,
>
> Clément
>
>>
>>> +            if (__asm_copy_to_user(__gu_ptr, &(val),
>>> sizeof(*__gu_ptr)))    \
>>> +                goto label;            \
>>> +            else                    \
>>> +                break;                \
>>> +        }                        \
>>> +    }                            \
>>>        switch (sizeof(*__gu_ptr)) {                \
>>>        case 1:                            \
>>>            __put_user_asm("sb", (x), __gu_ptr, label);    \
>>> @@ -385,12 +407,6 @@ err_label:                            \
>>>            -EFAULT;                    \
>>>    })
>>>    -
>>> -unsigned long __must_check __asm_copy_to_user(void __user *to,
>>> -    const void *from, unsigned long n);
>>> -unsigned long __must_check __asm_copy_from_user(void *to,
>>> -    const void __user *from, unsigned long n);
>>> -
>>>    static inline unsigned long
>>>    raw_copy_from_user(void *to, const void __user *from, unsigned long n)
>>>    {
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv