The RISC-V implementation seems to be quite clean regarding
sparse annotations. There is anyway three small glitches
which are corrected by this series.
Note: the 3rd patch may need another solution.
Luc Van Oostenryck (3):
riscv: use NULL instead of a plain 0
riscv: no __user for probe_kernel_address()
riscv: fix __user annotation for __copy_user()
arch/riscv/include/asm/cacheflush.h | 2 +-
arch/riscv/include/asm/tlbflush.h | 2 +-
arch/riscv/include/asm/uaccess.h | 8 ++++----
arch/riscv/kernel/traps.c | 2 +-
4 files changed, 7 insertions(+), 7 deletions(-)
--
2.17.1
In is_valid_bugaddr(), probe_kernel_address() is called with
the PC casted to (bug_inst_t __user *) but this function
only take a plain void* as argument, not a __user pointer.
Fix this by removing the unnneded __user in the cast.
Signed-off-by: Luc Van Oostenryck <[email protected]>
---
arch/riscv/kernel/traps.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index 93132cb59..4c92e5af8 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -160,7 +160,7 @@ int is_valid_bugaddr(unsigned long pc)
if (pc < PAGE_OFFSET)
return 0;
- if (probe_kernel_address((bug_insn_t __user *)pc, insn))
+ if (probe_kernel_address((bug_insn_t *)pc, insn))
return 0;
return (insn == __BUG_INSN);
}
--
2.17.1
sbi_remote_sfence_vma() & sbi_remote_fence_i() takes
a pointer as first argument but some macros call them with
a plain 0 which, while legal C, is frowned upon in the kernel.
Change this by replacing the 0 by NULL.
Signed-off-by: Luc Van Oostenryck <[email protected]>
---
arch/riscv/include/asm/cacheflush.h | 2 +-
arch/riscv/include/asm/tlbflush.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/riscv/include/asm/cacheflush.h b/arch/riscv/include/asm/cacheflush.h
index efd89a88d..8f1307441 100644
--- a/arch/riscv/include/asm/cacheflush.h
+++ b/arch/riscv/include/asm/cacheflush.h
@@ -47,7 +47,7 @@ static inline void flush_dcache_page(struct page *page)
#else /* CONFIG_SMP */
-#define flush_icache_all() sbi_remote_fence_i(0)
+#define flush_icache_all() sbi_remote_fence_i(NULL)
void flush_icache_mm(struct mm_struct *mm, bool local);
#endif /* CONFIG_SMP */
diff --git a/arch/riscv/include/asm/tlbflush.h b/arch/riscv/include/asm/tlbflush.h
index 7b209aec3..85c2d8bae 100644
--- a/arch/riscv/include/asm/tlbflush.h
+++ b/arch/riscv/include/asm/tlbflush.h
@@ -49,7 +49,7 @@ static inline void flush_tlb_range(struct vm_area_struct *vma,
#include <asm/sbi.h>
-#define flush_tlb_all() sbi_remote_sfence_vma(0, 0, -1)
+#define flush_tlb_all() sbi_remote_sfence_vma(NULL, 0, -1)
#define flush_tlb_page(vma, addr) flush_tlb_range(vma, addr, 0)
#define flush_tlb_range(vma, start, end) \
sbi_remote_sfence_vma(mm_cpumask((vma)->vm_mm)->bits, \
--
2.17.1
__copy_user() is a function, written in assembly, used to copy
memory between kernel & user space. As such its to & from args
may both take a user pointer or a kernel pointer.
However the prototype for this function declare these two args
as 'void __user *', which is no more & no less correct than
declaring them as 'void *'. In fact theer is no possible correct
annotation for such a function.
The problem is worked around here by declaring these args as
unsigned long and casting them to the right type in each of
two callers raw_copy_{to,from}_user() as some kind of cast would
be needed anyway.
Note: another solution, maybe cleaner but slightly more complex,
would be to declare two version of __copy_user,
either in the asm file or via an alias, each having already
the correct typing for raw_copy_{to,from}_user().
Signed-off-by: Luc Van Oostenryck <[email protected]>
---
arch/riscv/include/asm/uaccess.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
index 14b0b22fb..c7a6a4a4a 100644
--- a/arch/riscv/include/asm/uaccess.h
+++ b/arch/riscv/include/asm/uaccess.h
@@ -392,19 +392,19 @@ do { \
})
-extern unsigned long __must_check __copy_user(void __user *to,
- const void __user *from, unsigned long n);
+extern unsigned long __must_check __copy_user(unsigned long to,
+ const unsigned long from, unsigned long n);
static inline unsigned long
raw_copy_from_user(void *to, const void __user *from, unsigned long n)
{
- return __copy_user(to, from, n);
+ return __copy_user((unsigned long)to, (unsigned long)from, n);
}
static inline unsigned long
raw_copy_to_user(void __user *to, const void *from, unsigned long n)
{
- return __copy_user(to, from, n);
+ return __copy_user((unsigned long)to, (unsigned long)from, n);
}
extern long strncpy_from_user(char *dest, const char __user *src, long count);
--
2.17.1
On 6/1/18 8:22 AM, Luc Van Oostenryck wrote:
> __copy_user() is a function, written in assembly, used to copy
> memory between kernel & user space. As such its to & from args
> may both take a user pointer or a kernel pointer.
>
> However the prototype for this function declare these two args
> as 'void __user *', which is no more & no less correct than
> declaring them as 'void *'. In fact theer is no possible correct
/s/theer/there
> annotation for such a function.
>
> The problem is worked around here by declaring these args as
> unsigned long and casting them to the right type in each of
> two callers raw_copy_{to,from}_user() as some kind of cast would
> be needed anyway.
>
> Note: another solution, maybe cleaner but slightly more complex,
> would be to declare two version of __copy_user,
> either in the asm file or via an alias, each having already
> the correct typing for raw_copy_{to,from}_user().
>
I feel that would be a better solution as it is implemented similarly
in ARM as well.
I am unable to understand how "unsigned long" is better than "void*".
x86 implementation has both arguments as void*. Can you please clarify ?
Regards,
Atish
> Signed-off-by: Luc Van Oostenryck <[email protected]>
> ---
> arch/riscv/include/asm/uaccess.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
> index 14b0b22fb..c7a6a4a4a 100644
> --- a/arch/riscv/include/asm/uaccess.h
> +++ b/arch/riscv/include/asm/uaccess.h
> @@ -392,19 +392,19 @@ do { \
> })
>
>
> -extern unsigned long __must_check __copy_user(void __user *to,
> - const void __user *from, unsigned long n);
> +extern unsigned long __must_check __copy_user(unsigned long to,
> + const unsigned long from, unsigned long n);
>
> static inline unsigned long
> raw_copy_from_user(void *to, const void __user *from, unsigned long n)
> {
> - return __copy_user(to, from, n);
> + return __copy_user((unsigned long)to, (unsigned long)from, n);
> }
>
> static inline unsigned long
> raw_copy_to_user(void __user *to, const void *from, unsigned long n)
> {
> - return __copy_user(to, from, n);
> + return __copy_user((unsigned long)to, (unsigned long)from, n);
> }
>
> extern long strncpy_from_user(char *dest, const char __user *src, long count);
>
On Mon, Jun 04, 2018 at 11:46:50AM -0700, Atish Patra wrote:
> On 6/1/18 8:22 AM, Luc Van Oostenryck wrote:
> > __copy_user() is a function, written in assembly, used to copy
> > memory between kernel & user space. As such its to & from args
> > may both take a user pointer or a kernel pointer.
> >
> > However the prototype for this function declare these two args
> > as 'void __user *', which is no more & no less correct than
> > declaring them as 'void *'. In fact theer is no possible correct
>
> /s/theer/there
>
> > annotation for such a function.
> >
> > The problem is worked around here by declaring these args as
> > unsigned long and casting them to the right type in each of
> > two callers raw_copy_{to,from}_user() as some kind of cast would
> > be needed anyway.
> >
> > Note: another solution, maybe cleaner but slightly more complex,
> > would be to declare two version of __copy_user,
> > either in the asm file or via an alias, each having already
> > the correct typing for raw_copy_{to,from}_user().
> >
>
> I feel that would be a better solution as it is implemented similarly
> in ARM as well.
>
> I am unable to understand how "unsigned long" is better than "void*".
> x86 implementation has both arguments as void*. Can you please clarify ?
"better" is quite relative and it must be understood that sparse
allow to cast pointers o fany kinds to and from unsigned long
without any warnings (while doing a cast between different address
space will emit a warning unless you use '__force').
As I tried to explain here above, the fact that this function is
declared as taking 2 __user pointers requires to use of casts
(ugly casts with __force) to get over the __user. By declaring
them as taking unsigned long, you still have to use casts but, IMO,
it's cleaner
Note: they're generic pointers/addresses anyway, they can't be
dereferenced anyway so unsigned is as good as a plain void*
or a void __user*
Note: using unsigned long here, fundamentally to bypass the __user,
is the same as casting a const pointer back to a plain pointer
via an intermediate cast to unsigned long. People can argue
that's kinda cheating, and they would be right of course, but
using __force or declaring twice the function with two different
names and prototype is also a form of cheating.
Note: if this would be my code, I would choose the solution with
two declarations.
Best regards,
-- Luc
On 6/4/18 12:09 PM, Luc Van Oostenryck wrote:
> On Mon, Jun 04, 2018 at 11:46:50AM -0700, Atish Patra wrote:
>> On 6/1/18 8:22 AM, Luc Van Oostenryck wrote:
>>> __copy_user() is a function, written in assembly, used to copy
>>> memory between kernel & user space. As such its to & from args
>>> may both take a user pointer or a kernel pointer.
>>>
>>> However the prototype for this function declare these two args
>>> as 'void __user *', which is no more & no less correct than
>>> declaring them as 'void *'. In fact theer is no possible correct
>>
>> /s/theer/there
>>
>>> annotation for such a function.
>>>
>>> The problem is worked around here by declaring these args as
>>> unsigned long and casting them to the right type in each of
>>> two callers raw_copy_{to,from}_user() as some kind of cast would
>>> be needed anyway.
>>>
>>> Note: another solution, maybe cleaner but slightly more complex,
>>> would be to declare two version of __copy_user,
>>> either in the asm file or via an alias, each having already
>>> the correct typing for raw_copy_{to,from}_user().
>>>
>>
>> I feel that would be a better solution as it is implemented similarly
>> in ARM as well.
>>
>> I am unable to understand how "unsigned long" is better than "void*".
>> x86 implementation has both arguments as void*. Can you please clarify ?
>
> "better" is quite relative and it must be understood that sparse
> allow to cast pointers o fany kinds to and from unsigned long
> without any warnings (while doing a cast between different address
> space will emit a warning unless you use '__force').
>
Got it.
> As I tried to explain here above, the fact that this function is
> declared as taking 2 __user pointers requires to use of casts
> (ugly casts with __force) to get over the __user. By declaring
> them as taking unsigned long, you still have to use casts but, IMO,
> it's cleaner
>
Thanks for the detailed explanation.
> Note: they're generic pointers/addresses anyway, they can't be
> dereferenced anyway so unsigned is as good as a plain void*
> or a void __user*
> Note: using unsigned long here, fundamentally to bypass the __user,
> is the same as casting a const pointer back to a plain pointer
> via an intermediate cast to unsigned long. People can argue
> that's kinda cheating, and they would be right of course, but
> using __force or declaring twice the function with two different
> names and prototype is also a form of cheating.
> Note: if this would be my code, I would choose the solution with
> two declarations.
I prefer that as well.
Regards,
Atish
>
>
> Best regards,
> -- Luc
>
On Mon, 04 Jun 2018 12:28:47 PDT (-0700), [email protected] wrote:
> On 6/4/18 12:09 PM, Luc Van Oostenryck wrote:
>> On Mon, Jun 04, 2018 at 11:46:50AM -0700, Atish Patra wrote:
>>> On 6/1/18 8:22 AM, Luc Van Oostenryck wrote:
>>>> __copy_user() is a function, written in assembly, used to copy
>>>> memory between kernel & user space. As such its to & from args
>>>> may both take a user pointer or a kernel pointer.
>>>>
>>>> However the prototype for this function declare these two args
>>>> as 'void __user *', which is no more & no less correct than
>>>> declaring them as 'void *'. In fact theer is no possible correct
>>>
>>> /s/theer/there
>>>
>>>> annotation for such a function.
>>>>
>>>> The problem is worked around here by declaring these args as
>>>> unsigned long and casting them to the right type in each of
>>>> two callers raw_copy_{to,from}_user() as some kind of cast would
>>>> be needed anyway.
>>>>
>>>> Note: another solution, maybe cleaner but slightly more complex,
>>>> would be to declare two version of __copy_user,
>>>> either in the asm file or via an alias, each having already
>>>> the correct typing for raw_copy_{to,from}_user().
>>>>
>>>
>>> I feel that would be a better solution as it is implemented similarly
>>> in ARM as well.
>>>
>>> I am unable to understand how "unsigned long" is better than "void*".
>>> x86 implementation has both arguments as void*. Can you please clarify ?
>>
>> "better" is quite relative and it must be understood that sparse
>> allow to cast pointers o fany kinds to and from unsigned long
>> without any warnings (while doing a cast between different address
>> space will emit a warning unless you use '__force').
>>
>
> Got it.
>> As I tried to explain here above, the fact that this function is
>> declared as taking 2 __user pointers requires to use of casts
>> (ugly casts with __force) to get over the __user. By declaring
>> them as taking unsigned long, you still have to use casts but, IMO,
>> it's cleaner
>>
>
> Thanks for the detailed explanation.
>
>> Note: they're generic pointers/addresses anyway, they can't be
>> dereferenced anyway so unsigned is as good as a plain void*
>> or a void __user*
>> Note: using unsigned long here, fundamentally to bypass the __user,
>> is the same as casting a const pointer back to a plain pointer
>> via an intermediate cast to unsigned long. People can argue
>> that's kinda cheating, and they would be right of course, but
>> using __force or declaring twice the function with two different
>> names and prototype is also a form of cheating.
>> Note: if this would be my code, I would choose the solution with
>> two declarations.
>
> I prefer that as well.
I haven't compiled this, but I think it should work. It's on the fix-sparse
branch of kernel.org/palmer/linux.git .
commit 88ffc46f92c16db0fa37edb79038d37ced0a8b41
gpg: Signature made Thu 07 Jun 2018 09:29:02 AM PDT
gpg: using RSA key 00CE76D1834960DFCE886DF8EF4CA1502CCBAB41
gpg: issuer "[email protected]"
gpg: Good signature from "Palmer Dabbelt <[email protected]>" [ultimate]
gpg: aka "Palmer Dabbelt <[email protected]>" [ultimate]
Author: Palmer Dabbelt <[email protected]>
Date: Thu Jun 7 09:20:57 2018 -0700
RISC-V: Split the definition of __copy_user
We use a single __copy_user assembly function to copy memory both from
and to userspace. While this works, it triggers sparse errors because
we're implicitly casting between the kernel and user address spaces by
calling __copy_user.
This patch splits the C declaration into a pair of functions,
__asm_copy_{to,from}_user, that have sane semantics WRT __user. This
split tricks sparse into treating these function calls as safe. The
assembly implementation then just aliases these two symbols to
__copy_user, which I renamed to __asm_copy_tofrom_user. The result is
an spare-safe implementation that pays to performance or code size
penalty.
Signed-off-by: Palmer Dabbelt <[email protected]>
diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
index 14b0b22fb578..0dbbbab05dac 100644
--- a/arch/riscv/include/asm/uaccess.h
+++ b/arch/riscv/include/asm/uaccess.h
@@ -392,19 +392,21 @@ do { \
})
-extern unsigned long __must_check __copy_user(void __user *to,
+extern unsigned long __must_check __asm_copy_from_user_user(void *to,
const void __user *from, unsigned long n);
+extern unsigned long __must_check __asm_copy_to_user_user(void __user *to,
+ const void *from, unsigned long n);
static inline unsigned long
raw_copy_from_user(void *to, const void __user *from, unsigned long n)
{
- return __copy_user(to, from, n);
+ return __asm_copy_from_user(to, from, n);
}
static inline unsigned long
raw_copy_to_user(void __user *to, const void *from, unsigned long n)
{
- return __copy_user(to, from, n);
+ return __asm_copy_to_user(to, from, n);
}
extern long strncpy_from_user(char *dest, const char __user *src, long count);
diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S
index 58fb2877c865..bd51e47ebd44 100644
--- a/arch/riscv/lib/uaccess.S
+++ b/arch/riscv/lib/uaccess.S
@@ -13,7 +13,15 @@ _epc:
.previous
.endm
-ENTRY(__copy_user)
+/* __asm_copy_to_user and __asm_copy_from_user are actually the same function,
+ * they're just provided as two different symbols to C code so sparse doesn't
+ * yell about casting between two different address spaces. */
+.global __asm_copy_to_user
+.set __asm_copy_to_user,__asm_copy_tofrom_user
+.global __asm_copy_from_user
+.set __asm_copy_from_user,__asm_copy_tofrom_user
+
+ENTRY(__asm_copy_tofrom_user)
/* Enable access to user memory */
li t6, SR_SUM
On Thu, Jun 07, 2018 at 09:30:19AM -0700, Palmer Dabbelt wrote:
>
> I haven't compiled this, but I think it should work. It's on the
> fix-sparse branch of kernel.org/palmer/linux.git .
It's essentially what I was thinking but I have some small remarks.
> diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
> index 14b0b22fb578..0dbbbab05dac 100644
> --- a/arch/riscv/include/asm/uaccess.h
> +++ b/arch/riscv/include/asm/uaccess.h
> @@ -392,19 +392,21 @@ do { \
> })
>
>
> -extern unsigned long __must_check __copy_user(void __user *to,
> +extern unsigned long __must_check __asm_copy_from_user_user(void *to,
> const void __user *from, unsigned long n);
> +extern unsigned long __must_check __asm_copy_to_user_user(void __user *to,
> + const void *from, unsigned long n);
>
> static inline unsigned long
> raw_copy_from_user(void *to, const void __user *from, unsigned long n)
> {
> - return __copy_user(to, from, n);
> + return __asm_copy_from_user(to, from, n);
> }
>
> static inline unsigned long
> raw_copy_to_user(void __user *to, const void *from, unsigned long n)
> {
> - return __copy_user(to, from, n);
> + return __asm_copy_to_user(to, from, n);
> }
The asm function could have directly be named raw_copy_{to,from}_user()
because the inline functions are just no-ops but it's very clear like so.
> extern long strncpy_from_user(char *dest, const char __user *src, long count);
> diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S
> index 58fb2877c865..bd51e47ebd44 100644
> --- a/arch/riscv/lib/uaccess.S
> +++ b/arch/riscv/lib/uaccess.S
> @@ -13,7 +13,15 @@ _epc:
> .previous
> .endm
>
> -ENTRY(__copy_user)
> +/* __asm_copy_to_user and __asm_copy_from_user are actually the same function,
> + * they're just provided as two different symbols to C code so sparse doesn't
> + * yell about casting between two different address spaces. */
> +.global __asm_copy_to_user
> +.set __asm_copy_to_user,__asm_copy_tofrom_user
> +.global __asm_copy_from_user
> +.set __asm_copy_from_user,__asm_copy_tofrom_user
> +
> +ENTRY(__asm_copy_tofrom_user)
I don't think that the size (as reported by objdump, for example) will
be correct or even present for __asm_copy_to_user & __asm_copy_to_user.
What can be done is:
ENTRY(__asm_copy_to_user)
ENTRY(__asm_copy_from_user)
<function definition>
ENDPROC(__asm_copy_to_user)
ENDPROC(__asm_copy_from_user)
Finally, the symbol table need also something like:
diff --git a/arch/riscv/kernel/riscv_ksyms.c b/arch/riscv/kernel/riscv_ksyms.c
index 551734248..1e8f8fa54 100644
--- a/arch/riscv/kernel/riscv_ksyms.c
+++ b/arch/riscv/kernel/riscv_ksyms.c
@@ -13,6 +13,7 @@
* Assembly functions that may be used (directly or indirectly) by modules
*/
EXPORT_SYMBOL(__clear_user);
-EXPORT_SYMBOL(__copy_user);
+EXPORT_SYMBOL(__asm_copy_to_user);
+EXPORT_SYMBOL(__asm_copy_from_user);
EXPORT_SYMBOL(memset);
EXPORT_SYMBOL(memcpy);
Best regards,
-- Luc
On 6/7/18 9:30 AM, Palmer Dabbelt wrote:
> On Mon, 04 Jun 2018 12:28:47 PDT (-0700), [email protected] wrote:
>> On 6/4/18 12:09 PM, Luc Van Oostenryck wrote:
>>> On Mon, Jun 04, 2018 at 11:46:50AM -0700, Atish Patra wrote:
>>>> On 6/1/18 8:22 AM, Luc Van Oostenryck wrote:
>>>>> __copy_user() is a function, written in assembly, used to copy
>>>>> memory between kernel & user space. As such its to & from args
>>>>> may both take a user pointer or a kernel pointer.
>>>>>
>>>>> However the prototype for this function declare these two args
>>>>> as 'void __user *', which is no more & no less correct than
>>>>> declaring them as 'void *'. In fact theer is no possible correct
>>>>
>>>> /s/theer/there
>>>>
>>>>> annotation for such a function.
>>>>>
>>>>> The problem is worked around here by declaring these args as
>>>>> unsigned long and casting them to the right type in each of
>>>>> two callers raw_copy_{to,from}_user() as some kind of cast would
>>>>> be needed anyway.
>>>>>
>>>>> Note: another solution, maybe cleaner but slightly more complex,
>>>>> would be to declare two version of __copy_user,
>>>>> either in the asm file or via an alias, each having already
>>>>> the correct typing for raw_copy_{to,from}_user().
>>>>>
>>>>
>>>> I feel that would be a better solution as it is implemented similarly
>>>> in ARM as well.
>>>>
>>>> I am unable to understand how "unsigned long" is better than "void*".
>>>> x86 implementation has both arguments as void*. Can you please clarify ?
>>>
>>> "better" is quite relative and it must be understood that sparse
>>> allow to cast pointers o fany kinds to and from unsigned long
>>> without any warnings (while doing a cast between different address
>>> space will emit a warning unless you use '__force').
>>>
>>
>> Got it.
>>> As I tried to explain here above, the fact that this function is
>>> declared as taking 2 __user pointers requires to use of casts
>>> (ugly casts with __force) to get over the __user. By declaring
>>> them as taking unsigned long, you still have to use casts but, IMO,
>>> it's cleaner
>>>
>>
>> Thanks for the detailed explanation.
>>
>>> Note: they're generic pointers/addresses anyway, they can't be
>>> dereferenced anyway so unsigned is as good as a plain void*
>>> or a void __user*
>>> Note: using unsigned long here, fundamentally to bypass the __user,
>>> is the same as casting a const pointer back to a plain pointer
>>> via an intermediate cast to unsigned long. People can argue
>>> that's kinda cheating, and they would be right of course, but
>>> using __force or declaring twice the function with two different
>>> names and prototype is also a form of cheating.
>>> Note: if this would be my code, I would choose the solution with
>>> two declarations.
>>
>> I prefer that as well.
>
> I haven't compiled this, but I think it should work. It's on the fix-sparse
> branch of kernel.org/palmer/linux.git .
>
> commit 88ffc46f92c16db0fa37edb79038d37ced0a8b41
> gpg: Signature made Thu 07 Jun 2018 09:29:02 AM PDT
> gpg: using RSA key 00CE76D1834960DFCE886DF8EF4CA1502CCBAB41
> gpg: issuer "[email protected]"
> gpg: Good signature from "Palmer Dabbelt <[email protected]>" [ultimate]
> gpg: aka "Palmer Dabbelt <[email protected]>" [ultimate]
> Author: Palmer Dabbelt <[email protected]>
> Date: Thu Jun 7 09:20:57 2018 -0700
>
> RISC-V: Split the definition of __copy_user
>
> We use a single __copy_user assembly function to copy memory both from
> and to userspace. While this works, it triggers sparse errors because
> we're implicitly casting between the kernel and user address spaces by
> calling __copy_user.
>
> This patch splits the C declaration into a pair of functions,
> __asm_copy_{to,from}_user, that have sane semantics WRT __user. This
> split tricks sparse into treating these function calls as safe. The
> assembly implementation then just aliases these two symbols to
> __copy_user, which I renamed to __asm_copy_tofrom_user. The result is
> an spare-safe implementation that pays to performance or code size
> penalty.
>
> Signed-off-by: Palmer Dabbelt <[email protected]>
>
> diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
> index 14b0b22fb578..0dbbbab05dac 100644
> --- a/arch/riscv/include/asm/uaccess.h
> +++ b/arch/riscv/include/asm/uaccess.h
> @@ -392,19 +392,21 @@ do { \
> })
>
>
> -extern unsigned long __must_check __copy_user(void __user *to,
> +extern unsigned long __must_check __asm_copy_from_user_user(void *to,
> const void __user *from, unsigned long n);
Minor typos:
/s/__asm_copy_from_user_user/__asm_copy_from_user/
> +extern unsigned long __must_check __asm_copy_to_user_user(void __user *to,
> + const void *from, unsigned long n);
/s/__asm_copy_to_user_user/__asm_copy_to_user/
>
> static inline unsigned long
> raw_copy_from_user(void *to, const void __user *from, unsigned long n)
> {
> - return __copy_user(to, from, n);
> + return __asm_copy_from_user(to, from, n);
> }
>
> static inline unsigned long
> raw_copy_to_user(void __user *to, const void *from, unsigned long n)
> {
> - return __copy_user(to, from, n);
> + return __asm_copy_to_user(to, from, n);
> }
>
> extern long strncpy_from_user(char *dest, const char __user *src, long count);
> diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S
> index 58fb2877c865..bd51e47ebd44 100644
> --- a/arch/riscv/lib/uaccess.S
> +++ b/arch/riscv/lib/uaccess.S
> @@ -13,7 +13,15 @@ _epc:
> .previous
> .endm
>
> -ENTRY(__copy_user)
> +/* __asm_copy_to_user and __asm_copy_from_user are actually the same function,
> + * they're just provided as two different symbols to C code so sparse doesn't
> + * yell about casting between two different address spaces. */
> +.global __asm_copy_to_user
> +.set __asm_copy_to_user,__asm_copy_tofrom_user
> +.global __asm_copy_from_user
> +.set __asm_copy_from_user,__asm_copy_tofrom_user
> +
> +ENTRY(__asm_copy_tofrom_user)
>
> /* Enable access to user memory */
> li t6, SR_SUM
>
-ENDPROC(__copy_user)
+ENDPROC(__asm_copy_tofrom_user)
I have done boot testing with above typo fixes.
Regards,
Atish
On Thu, 07 Jun 2018 09:51:33 PDT (-0700), [email protected] wrote:
> On Thu, Jun 07, 2018 at 09:30:19AM -0700, Palmer Dabbelt wrote:
>>
>> I haven't compiled this, but I think it should work. It's on the
>> fix-sparse branch of kernel.org/palmer/linux.git .
>
> It's essentially what I was thinking but I have some small remarks.
>
>> diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
>> index 14b0b22fb578..0dbbbab05dac 100644
>> --- a/arch/riscv/include/asm/uaccess.h
>> +++ b/arch/riscv/include/asm/uaccess.h
>> @@ -392,19 +392,21 @@ do { \
>> })
>>
>>
>> -extern unsigned long __must_check __copy_user(void __user *to,
>> +extern unsigned long __must_check __asm_copy_from_user_user(void *to,
>> const void __user *from, unsigned long n);
>> +extern unsigned long __must_check __asm_copy_to_user_user(void __user *to,
>> + const void *from, unsigned long n);
>>
>> static inline unsigned long
>> raw_copy_from_user(void *to, const void __user *from, unsigned long n)
>> {
>> - return __copy_user(to, from, n);
>> + return __asm_copy_from_user(to, from, n);
>> }
>>
>> static inline unsigned long
>> raw_copy_to_user(void __user *to, const void *from, unsigned long n)
>> {
>> - return __copy_user(to, from, n);
>> + return __asm_copy_to_user(to, from, n);
>> }
>
> The asm function could have directly be named raw_copy_{to,from}_user()
> because the inline functions are just no-ops but it's very clear like so.
>
>> extern long strncpy_from_user(char *dest, const char __user *src, long count);
>> diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S
>> index 58fb2877c865..bd51e47ebd44 100644
>> --- a/arch/riscv/lib/uaccess.S
>> +++ b/arch/riscv/lib/uaccess.S
>> @@ -13,7 +13,15 @@ _epc:
>> .previous
>> .endm
>>
>> -ENTRY(__copy_user)
>> +/* __asm_copy_to_user and __asm_copy_from_user are actually the same function,
>> + * they're just provided as two different symbols to C code so sparse doesn't
>> + * yell about casting between two different address spaces. */
>> +.global __asm_copy_to_user
>> +.set __asm_copy_to_user,__asm_copy_tofrom_user
>> +.global __asm_copy_from_user
>> +.set __asm_copy_from_user,__asm_copy_tofrom_user
>> +
>> +ENTRY(__asm_copy_tofrom_user)
>
> I don't think that the size (as reported by objdump, for example) will
> be correct or even present for __asm_copy_to_user & __asm_copy_to_user.
>
> What can be done is:
> ENTRY(__asm_copy_to_user)
> ENTRY(__asm_copy_from_user)
>
> <function definition>
>
> ENDPROC(__asm_copy_to_user)
> ENDPROC(__asm_copy_from_user)
>
>
> Finally, the symbol table need also something like:
> diff --git a/arch/riscv/kernel/riscv_ksyms.c b/arch/riscv/kernel/riscv_ksyms.c
> index 551734248..1e8f8fa54 100644
> --- a/arch/riscv/kernel/riscv_ksyms.c
> +++ b/arch/riscv/kernel/riscv_ksyms.c
> @@ -13,6 +13,7 @@
> * Assembly functions that may be used (directly or indirectly) by modules
> */
> EXPORT_SYMBOL(__clear_user);
> -EXPORT_SYMBOL(__copy_user);
> +EXPORT_SYMBOL(__asm_copy_to_user);
> +EXPORT_SYMBOL(__asm_copy_from_user);
> EXPORT_SYMBOL(memset);
> EXPORT_SYMBOL(memcpy);
Thanks. Do you mind checking to make sure this works and submitting a patch?
On Fri, Jun 08, 2018 at 03:33:36PM -0700, Palmer Dabbelt wrote:
> On Thu, 07 Jun 2018 09:51:33 PDT (-0700), [email protected] wrote:
> > On Thu, Jun 07, 2018 at 09:30:19AM -0700, Palmer Dabbelt wrote:
> > > diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S
> > > index 58fb2877c865..bd51e47ebd44 100644
> > > --- a/arch/riscv/lib/uaccess.S
> > > +++ b/arch/riscv/lib/uaccess.S
> > > @@ -13,7 +13,15 @@ _epc:
> > > .previous
> > > .endm
> > >
> > > -ENTRY(__copy_user)
> > > +/* __asm_copy_to_user and __asm_copy_from_user are actually the same function,
> > > + * they're just provided as two different symbols to C code so sparse doesn't
> > > + * yell about casting between two different address spaces. */
> > > +.global __asm_copy_to_user
> > > +.set __asm_copy_to_user,__asm_copy_tofrom_user
> > > +.global __asm_copy_from_user
> > > +.set __asm_copy_from_user,__asm_copy_tofrom_user
> > > +
> > > +ENTRY(__asm_copy_tofrom_user)
> >
> > I don't think that the size (as reported by objdump, for example) will
> > be correct or even present for __asm_copy_to_user & __asm_copy_to_user.
> >
> > What can be done is:
> > ENTRY(__asm_copy_to_user)
> > ENTRY(__asm_copy_from_user)
> >
> > <function definition>
> >
> > ENDPROC(__asm_copy_to_user)
> > ENDPROC(__asm_copy_from_user)
> >
> Thanks. Do you mind checking to make sure this works and submitting a patch?
Not at all.
I should have done it already when I sent the previous email.
I tried it and ... the preprocessed asm is as expected:
.globl __asm_copy_to_user ; .balign 4 ; __asm_copy_to_user:
.globl __asm_copy_from_user ; .balign 4 ; __asm_copy_from_user:
li t6, 0x00040000
csrs sstatus, t6
...
But the nm -S returns different sizes for them:
0000000000000004 000000000000006c T __asm_copy_from_user
0000000000000002 000000000000006e T __asm_copy_to_user
and the object code is:
0000000000000000 <__asm_copy_to_user-0x2>:
0: 0001 nop
0000000000000002 <__asm_copy_to_user>:
2: 0001 nop
0000000000000004 <__asm_copy_from_user>:
4: 00040fb7 lui t6,0x40
8: 100fa073 csrs sstatus,t6
...
Why these unnneded nops?
Is this a known problem of my toolchain (I use a plain gcc 7.3 +
binutils 2.29, both configured as riscv64-none-elf)?
If I remove the two ENTRY() and use instead:
.globl __asm_copy_to_user ; __asm_copy_to_user:
.globl __asm_copy_from_user ; __asm_copy_from_user:
(IOW, I drop the .balign) then I get the expected result.
But well, this seems unrelated to the double ENTRY.
I can't test it more for now because I've some link errors (which,
I understand are probably solved in the riscv tree of binutils).
I'll send you the patch anyway since, as far as I understand the changes
specific to this copy_to/from_user is OK.
Regards,
-- Luc
We use a single __copy_user assembly function to copy memory both from
and to userspace. While this works, it triggers sparse errors because
we're implicitly casting between the kernel and user address spaces by
calling __copy_user.
This patch splits the C declaration into a pair of functions,
__asm_copy_{to,from}_user, that have sane semantics WRT __user. This
split make things fine from sparse's point of view. The assembly
implementation keeps a single definition but add a double ENTRY() for it,
one for __asm_copy_to_user and another one for __asm_copy_from_user.
The result is a spare-safe implementation that pays no performance
or code size penalty.
Signed-off-by: Palmer Dabbelt <[email protected]>
Signed-off-by: Luc Van Oostenryck <[email protected]>
---
arch/riscv/include/asm/uaccess.h | 8 +++++---
arch/riscv/kernel/riscv_ksyms.c | 3 ++-
arch/riscv/lib/uaccess.S | 6 ++++--
3 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
index 14b0b22fb..473cfc84e 100644
--- a/arch/riscv/include/asm/uaccess.h
+++ b/arch/riscv/include/asm/uaccess.h
@@ -392,19 +392,21 @@ do { \
})
-extern unsigned long __must_check __copy_user(void __user *to,
+extern unsigned long __must_check __asm_copy_to_user(void __user *to,
+ const void *from, unsigned long n);
+extern 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)
{
- return __copy_user(to, from, n);
+ return __asm_copy_to_user(to, from, n);
}
static inline unsigned long
raw_copy_to_user(void __user *to, const void *from, unsigned long n)
{
- return __copy_user(to, from, n);
+ return __asm_copy_from_user(to, from, n);
}
extern long strncpy_from_user(char *dest, const char __user *src, long count);
diff --git a/arch/riscv/kernel/riscv_ksyms.c b/arch/riscv/kernel/riscv_ksyms.c
index 551734248..f247d6d21 100644
--- a/arch/riscv/kernel/riscv_ksyms.c
+++ b/arch/riscv/kernel/riscv_ksyms.c
@@ -13,6 +13,7 @@
* Assembly functions that may be used (directly or indirectly) by modules
*/
EXPORT_SYMBOL(__clear_user);
-EXPORT_SYMBOL(__copy_user);
+EXPORT_SYMBOL(__asm_copy_to_user);
+EXPORT_SYMBOL(__asm_copy_from_user);
EXPORT_SYMBOL(memset);
EXPORT_SYMBOL(memcpy);
diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S
index 58fb2877c..f8e6440ca 100644
--- a/arch/riscv/lib/uaccess.S
+++ b/arch/riscv/lib/uaccess.S
@@ -13,7 +13,8 @@ _epc:
.previous
.endm
-ENTRY(__copy_user)
+ENTRY(__asm_copy_to_user)
+ENTRY(__asm_copy_from_user)
/* Enable access to user memory */
li t6, SR_SUM
@@ -63,7 +64,8 @@ ENTRY(__copy_user)
addi a0, a0, 1
bltu a1, a3, 5b
j 3b
-ENDPROC(__copy_user)
+ENDPROC(__asm_copy_to_user)
+ENDPROC(__asm_copy_from_user)
ENTRY(__clear_user)
--
2.17.1
On Fri, 08 Jun 2018 17:13:12 PDT (-0700), [email protected] wrote:
> On Fri, Jun 08, 2018 at 03:33:36PM -0700, Palmer Dabbelt wrote:
>> On Thu, 07 Jun 2018 09:51:33 PDT (-0700), [email protected] wrote:
>> > On Thu, Jun 07, 2018 at 09:30:19AM -0700, Palmer Dabbelt wrote:
>> > > diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S
>> > > index 58fb2877c865..bd51e47ebd44 100644
>> > > --- a/arch/riscv/lib/uaccess.S
>> > > +++ b/arch/riscv/lib/uaccess.S
>> > > @@ -13,7 +13,15 @@ _epc:
>> > > .previous
>> > > .endm
>> > >
>> > > -ENTRY(__copy_user)
>> > > +/* __asm_copy_to_user and __asm_copy_from_user are actually the same function,
>> > > + * they're just provided as two different symbols to C code so sparse doesn't
>> > > + * yell about casting between two different address spaces. */
>> > > +.global __asm_copy_to_user
>> > > +.set __asm_copy_to_user,__asm_copy_tofrom_user
>> > > +.global __asm_copy_from_user
>> > > +.set __asm_copy_from_user,__asm_copy_tofrom_user
>> > > +
>> > > +ENTRY(__asm_copy_tofrom_user)
>> >
>> > I don't think that the size (as reported by objdump, for example) will
>> > be correct or even present for __asm_copy_to_user & __asm_copy_to_user.
>> >
>> > What can be done is:
>> > ENTRY(__asm_copy_to_user)
>> > ENTRY(__asm_copy_from_user)
>> >
>> > <function definition>
>> >
>> > ENDPROC(__asm_copy_to_user)
>> > ENDPROC(__asm_copy_from_user)
>> >
>> Thanks. Do you mind checking to make sure this works and submitting a patch?
>
> Not at all.
> I should have done it already when I sent the previous email.
>
> I tried it and ... the preprocessed asm is as expected:
> .globl __asm_copy_to_user ; .balign 4 ; __asm_copy_to_user:
> .globl __asm_copy_from_user ; .balign 4 ; __asm_copy_from_user:
>
>
> li t6, 0x00040000
> csrs sstatus, t6
> ...
>
> But the nm -S returns different sizes for them:
> 0000000000000004 000000000000006c T __asm_copy_from_user
> 0000000000000002 000000000000006e T __asm_copy_to_user
>
> and the object code is:
> 0000000000000000 <__asm_copy_to_user-0x2>:
> 0: 0001 nop
>
> 0000000000000002 <__asm_copy_to_user>:
> 2: 0001 nop
>
> 0000000000000004 <__asm_copy_from_user>:
> 4: 00040fb7 lui t6,0x40
> 8: 100fa073 csrs sstatus,t6
> ...
>
> Why these unnneded nops?
> Is this a known problem of my toolchain (I use a plain gcc 7.3 +
> binutils 2.29, both configured as riscv64-none-elf)?
>
> If I remove the two ENTRY() and use instead:
> .globl __asm_copy_to_user ; __asm_copy_to_user:
> .globl __asm_copy_from_user ; __asm_copy_from_user:
> (IOW, I drop the .balign) then I get the expected result.
> But well, this seems unrelated to the double ENTRY.
>
> I can't test it more for now because I've some link errors (which,
> I understand are probably solved in the riscv tree of binutils).
>
> I'll send you the patch anyway since, as far as I understand the changes
> specific to this copy_to/from_user is OK.
I think it's probably a bug in binutils-2.29 that should be fixed by 2.30 --
IIRC we had some bugs that looked like this and they got fixed, though it might
be just in master (so 2.31).
Either way it looks innocuous WRT the patch.
Thanks!
On Sat, Jun 09, 2018 at 01:00:08PM -0700, Palmer Dabbelt wrote:
> On Fri, 08 Jun 2018 17:13:12 PDT (-0700), [email protected] wrote:
> > I tried it and ... the preprocessed asm is as expected:
> > .globl __asm_copy_to_user ; .balign 4 ; __asm_copy_to_user:
> > .globl __asm_copy_from_user ; .balign 4 ; __asm_copy_from_user:
> >
> >
> > li t6, 0x00040000
> > csrs sstatus, t6
> > ...
> >
> > But the nm -S returns different sizes for them:
> > 0000000000000004 000000000000006c T __asm_copy_from_user
> > 0000000000000002 000000000000006e T __asm_copy_to_user
> >
> > and the object code is:
> > 0000000000000000 <__asm_copy_to_user-0x2>:
> > 0: 0001 nop
> >
> > 0000000000000002 <__asm_copy_to_user>:
> > 2: 0001 nop
> >
> > 0000000000000004 <__asm_copy_from_user>:
> > 4: 00040fb7 lui t6,0x40
> > 8: 100fa073 csrs sstatus,t6
> > ...
> >
> > Why these unnneded nops?
> > Is this a known problem of my toolchain (I use a plain gcc 7.3 +
> > binutils 2.29, both configured as riscv64-none-elf)?
> >
> > If I remove the two ENTRY() and use instead:
> > .globl __asm_copy_to_user ; __asm_copy_to_user:
> > .globl __asm_copy_from_user ; __asm_copy_from_user:
> > (IOW, I drop the .balign) then I get the expected result.
> > But well, this seems unrelated to the double ENTRY.
> >
> > I can't test it more for now because I've some link errors (which,
> > I understand are probably solved in the riscv tree of binutils).
> >
> > I'll send you the patch anyway since, as far as I understand the changes
> > specific to this copy_to/from_user is OK.
>
> I think it's probably a bug in binutils-2.29 that should be fixed by
> 2.30 -- IIRC we had some bugs that looked like this and they got
> fixed, though it might be just in master (so 2.31).
I've tried binutils-2.30 and riscv-binutils-gdb, both still have
the problem and master binutils-gdb doesn't compile for me.
OTOH, everything is fine if I disabled CONFIG_RISCV_ISA_C.
> Either way it looks innocuous WRT the patch.
Indeed.
With this, the RISC-V arch should be sparse clean.
I'll recheck after -rc1.
Cheers,
-- Luc
On Sat, 09 Jun 2018 14:42:12 PDT (-0700), [email protected] wrote:
> On Sat, Jun 09, 2018 at 01:00:08PM -0700, Palmer Dabbelt wrote:
>> On Fri, 08 Jun 2018 17:13:12 PDT (-0700), [email protected] wrote:
>> > I tried it and ... the preprocessed asm is as expected:
>> > .globl __asm_copy_to_user ; .balign 4 ; __asm_copy_to_user:
>> > .globl __asm_copy_from_user ; .balign 4 ; __asm_copy_from_user:
>> >
>> >
>> > li t6, 0x00040000
>> > csrs sstatus, t6
>> > ...
>> >
>> > But the nm -S returns different sizes for them:
>> > 0000000000000004 000000000000006c T __asm_copy_from_user
>> > 0000000000000002 000000000000006e T __asm_copy_to_user
>> >
>> > and the object code is:
>> > 0000000000000000 <__asm_copy_to_user-0x2>:
>> > 0: 0001 nop
>> >
>> > 0000000000000002 <__asm_copy_to_user>:
>> > 2: 0001 nop
>> >
>> > 0000000000000004 <__asm_copy_from_user>:
>> > 4: 00040fb7 lui t6,0x40
>> > 8: 100fa073 csrs sstatus,t6
>> > ...
>> >
>> > Why these unnneded nops?
>> > Is this a known problem of my toolchain (I use a plain gcc 7.3 +
>> > binutils 2.29, both configured as riscv64-none-elf)?
>> >
>> > If I remove the two ENTRY() and use instead:
>> > .globl __asm_copy_to_user ; __asm_copy_to_user:
>> > .globl __asm_copy_from_user ; __asm_copy_from_user:
>> > (IOW, I drop the .balign) then I get the expected result.
>> > But well, this seems unrelated to the double ENTRY.
>> >
>> > I can't test it more for now because I've some link errors (which,
>> > I understand are probably solved in the riscv tree of binutils).
>> >
>> > I'll send you the patch anyway since, as far as I understand the changes
>> > specific to this copy_to/from_user is OK.
>>
>> I think it's probably a bug in binutils-2.29 that should be fixed by
>> 2.30 -- IIRC we had some bugs that looked like this and they got
>> fixed, though it might be just in master (so 2.31).
>
> I've tried binutils-2.30 and riscv-binutils-gdb, both still have
> the problem and master binutils-gdb doesn't compile for me.
> OTOH, everything is fine if I disabled CONFIG_RISCV_ISA_C.
OK, I'll try and figure out what's going on. We've had a handful of headaches
trying to get things like '.align 2; .align 2' to actually produce no NOPs for
the second alignment directive, which is surprisingly complicated due to the
aggressive linker relaxation we do.
>> Either way it looks innocuous WRT the patch.
>
> Indeed.
> With this, the RISC-V arch should be sparse clean.
> I'll recheck after -rc1.
This will be part of the PR that I tag today, so I anticipate it'll be in.
Thanks!
On Mon, Jun 11, 2018 at 12:01:37PM -0700, Palmer Dabbelt wrote:
> On Sat, 09 Jun 2018 14:42:12 PDT (-0700), [email protected] wrote:
> > On Sat, Jun 09, 2018 at 01:00:08PM -0700, Palmer Dabbelt wrote:
> > > On Fri, 08 Jun 2018 17:13:12 PDT (-0700), [email protected] wrote:
> > > > I tried it and ... the preprocessed asm is as expected:
> > > > .globl __asm_copy_to_user ; .balign 4 ; __asm_copy_to_user:
> > > > .globl __asm_copy_from_user ; .balign 4 ; __asm_copy_from_user:
> > > >
> > > >
> > > > li t6, 0x00040000
> > > > csrs sstatus, t6
> > > > ...
> > > >
> > > > But the nm -S returns different sizes for them:
> > > > 0000000000000004 000000000000006c T __asm_copy_from_user
> > > > 0000000000000002 000000000000006e T __asm_copy_to_user
> > > >
> > > > and the object code is:
> > > > 0000000000000000 <__asm_copy_to_user-0x2>:
> > > > 0: 0001 nop
> > > >
> > > > 0000000000000002 <__asm_copy_to_user>:
> > > > 2: 0001 nop
> > > >
> > > > 0000000000000004 <__asm_copy_from_user>:
> > > > 4: 00040fb7 lui t6,0x40
> > > > 8: 100fa073 csrs sstatus,t6
> > > > ...
> > > >
> > > > Why these unnneded nops?
> > > > Is this a known problem of my toolchain (I use a plain gcc 7.3 +
> > > > binutils 2.29, both configured as riscv64-none-elf)?
> > > >
> > > > If I remove the two ENTRY() and use instead:
> > > > .globl __asm_copy_to_user ; __asm_copy_to_user:
> > > > .globl __asm_copy_from_user ; __asm_copy_from_user:
> > > > (IOW, I drop the .balign) then I get the expected result.
> > > > But well, this seems unrelated to the double ENTRY.
> > > >
> > > > I can't test it more for now because I've some link errors (which,
> > > > I understand are probably solved in the riscv tree of binutils).
> > > >
> > > > I'll send you the patch anyway since, as far as I understand the changes
> > > > specific to this copy_to/from_user is OK.
> > >
> > > I think it's probably a bug in binutils-2.29 that should be fixed by
> > > 2.30 -- IIRC we had some bugs that looked like this and they got
> > > fixed, though it might be just in master (so 2.31).
> >
> > I've tried binutils-2.30 and riscv-binutils-gdb, both still have
> > the problem and master binutils-gdb doesn't compile for me.
> > OTOH, everything is fine if I disabled CONFIG_RISCV_ISA_C.
>
> OK, I'll try and figure out what's going on. We've had a handful of
> headaches trying to get things like '.align 2; .align 2' to actually produce
> no NOPs for the second alignment directive, which is surprisingly
> complicated due to the aggressive linker relaxation we do.
OK. I imagine indeed but note that no linker is involved here so,
if the problem is still present, it must already be in the assembler.
> > With this, the RISC-V arch should be sparse clean.
> > I'll recheck after -rc1.
>
> This will be part of the PR that I tag today, so I anticipate it'll be in.
Cool!
-- Luc
On Mon, 11 Jun 2018 20:00:08 PDT (-0700), [email protected] wrote:
> On Mon, Jun 11, 2018 at 12:01:37PM -0700, Palmer Dabbelt wrote:
>> On Sat, 09 Jun 2018 14:42:12 PDT (-0700), [email protected] wrote:
>> > On Sat, Jun 09, 2018 at 01:00:08PM -0700, Palmer Dabbelt wrote:
>> > > On Fri, 08 Jun 2018 17:13:12 PDT (-0700), [email protected] wrote:
>> > > > I tried it and ... the preprocessed asm is as expected:
>> > > > .globl __asm_copy_to_user ; .balign 4 ; __asm_copy_to_user:
>> > > > .globl __asm_copy_from_user ; .balign 4 ; __asm_copy_from_user:
>> > > >
>> > > >
>> > > > li t6, 0x00040000
>> > > > csrs sstatus, t6
>> > > > ...
>> > > >
>> > > > But the nm -S returns different sizes for them:
>> > > > 0000000000000004 000000000000006c T __asm_copy_from_user
>> > > > 0000000000000002 000000000000006e T __asm_copy_to_user
>> > > >
>> > > > and the object code is:
>> > > > 0000000000000000 <__asm_copy_to_user-0x2>:
>> > > > 0: 0001 nop
>> > > >
>> > > > 0000000000000002 <__asm_copy_to_user>:
>> > > > 2: 0001 nop
>> > > >
>> > > > 0000000000000004 <__asm_copy_from_user>:
>> > > > 4: 00040fb7 lui t6,0x40
>> > > > 8: 100fa073 csrs sstatus,t6
>> > > > ...
>> > > >
>> > > > Why these unnneded nops?
>> > > > Is this a known problem of my toolchain (I use a plain gcc 7.3 +
>> > > > binutils 2.29, both configured as riscv64-none-elf)?
>> > > >
>> > > > If I remove the two ENTRY() and use instead:
>> > > > .globl __asm_copy_to_user ; __asm_copy_to_user:
>> > > > .globl __asm_copy_from_user ; __asm_copy_from_user:
>> > > > (IOW, I drop the .balign) then I get the expected result.
>> > > > But well, this seems unrelated to the double ENTRY.
>> > > >
>> > > > I can't test it more for now because I've some link errors (which,
>> > > > I understand are probably solved in the riscv tree of binutils).
>> > > >
>> > > > I'll send you the patch anyway since, as far as I understand the changes
>> > > > specific to this copy_to/from_user is OK.
>> > >
>> > > I think it's probably a bug in binutils-2.29 that should be fixed by
>> > > 2.30 -- IIRC we had some bugs that looked like this and they got
>> > > fixed, though it might be just in master (so 2.31).
>> >
>> > I've tried binutils-2.30 and riscv-binutils-gdb, both still have
>> > the problem and master binutils-gdb doesn't compile for me.
>> > OTOH, everything is fine if I disabled CONFIG_RISCV_ISA_C.
>>
>> OK, I'll try and figure out what's going on. We've had a handful of
>> headaches trying to get things like '.align 2; .align 2' to actually produce
>> no NOPs for the second alignment directive, which is surprisingly
>> complicated due to the aggressive linker relaxation we do.
>
> OK. I imagine indeed but note that no linker is involved here so,
> if the problem is still present, it must already be in the assembler.
Ah, OK -- in that case then it's just not a bug. In RISC-V land we handle
alignment as part of relaxation in the linker, so if you're looking at the
output of the assembler then you'll always see a bunch of NOPs for every
alignment directive. If you 'objdump -dr' you should be able to see the
relocations that get emitted, there should be a R_RISCV_ALIGN that points to
the run of NOPs.
>> > With this, the RISC-V arch should be sparse clean.
>> > I'll recheck after -rc1.
>>
>> This will be part of the PR that I tag today, so I anticipate it'll be in.
>
> Cool!
>
> -- Luc
On Tue, Jun 12, 2018 at 7:12 PM, Palmer Dabbelt <[email protected]> wrote:
> On Mon, 11 Jun 2018 20:00:08 PDT (-0700), [email protected] wrote:
>> On Mon, Jun 11, 2018 at 12:01:37PM -0700, Palmer Dabbelt wrote:
>>>
>>> OK, I'll try and figure out what's going on. We've had a handful of
>>> headaches trying to get things like '.align 2; .align 2' to actually
>>> produce
>>> no NOPs for the second alignment directive, which is surprisingly
>>> complicated due to the aggressive linker relaxation we do.
>>
>>
>> OK. I imagine indeed but note that no linker is involved here so,
>> if the problem is still present, it must already be in the assembler.
>
>
> Ah, OK -- in that case then it's just not a bug. In RISC-V land we handle
> alignment as part of relaxation in the linker, so if you're looking at the
> output of the assembler then you'll always see a bunch of NOPs for every
> alignment directive. If you 'objdump -dr' you should be able to see the
> relocations that get emitted, there should be a R_RISCV_ALIGN that points to
> the run of NOPs.
Ah OK. Indeed I see the R_RISCV_ALIGN.
Thanks for the explanation.
-- Luc
On Tue, 12 Jun 2018 11:19:57 PDT (-0700), [email protected] wrote:
> On Tue, Jun 12, 2018 at 7:12 PM, Palmer Dabbelt <[email protected]> wrote:
>> On Mon, 11 Jun 2018 20:00:08 PDT (-0700), [email protected] wrote:
>>> On Mon, Jun 11, 2018 at 12:01:37PM -0700, Palmer Dabbelt wrote:
>>>>
>>>> OK, I'll try and figure out what's going on. We've had a handful of
>>>> headaches trying to get things like '.align 2; .align 2' to actually
>>>> produce
>>>> no NOPs for the second alignment directive, which is surprisingly
>>>> complicated due to the aggressive linker relaxation we do.
>>>
>>>
>>> OK. I imagine indeed but note that no linker is involved here so,
>>> if the problem is still present, it must already be in the assembler.
>>
>>
>> Ah, OK -- in that case then it's just not a bug. In RISC-V land we handle
>> alignment as part of relaxation in the linker, so if you're looking at the
>> output of the assembler then you'll always see a bunch of NOPs for every
>> alignment directive. If you 'objdump -dr' you should be able to see the
>> relocations that get emitted, there should be a R_RISCV_ALIGN that points to
>> the run of NOPs.
>
> Ah OK. Indeed I see the R_RISCV_ALIGN.
> Thanks for the explanation.
No problem. There's heaps of info about this in some blog posts I made a while
ago if you're interested.