Hello,
Changes since v1:
- in "io: Fixup defines and move include to the end" added a linux/types.h
include as there were compiler failurs pointed out by kbuild.
This a series of fixes for OpenRISC sparse warnings. The kbuild robots report
many issues related to issues with OpenRISC headers having missing or incorrect
sparse annotations.
Example kdbuild-all report:
net/ipv4/ip_sockglue.c:1489:13: sparse: sparse: incorrect type in initializer (different address spaces)
https://lists.01.org/hyperkitty/list/[email protected]/thread/MB6SE7BX425ENFTSIL6KAOB3CVS4WJLH/
Also this includes a few cleanups which I noticed while working on the warning
fixups.
-Stafford
Stafford Horne (6):
openrisc: io: Fixup defines and move include to the end
openrisc: uaccess: Fix sparse address space warnings
openrisc: uaccess: Use static inline function in access_ok
openrisc: uaccess: Remove unused macro __addr_ok
openrisc: signal: Fix sparse address space warnings
openrisc: uaccess: Add user address space check to access_ok
arch/openrisc/include/asm/io.h | 9 +++++++--
arch/openrisc/include/asm/uaccess.h | 21 +++++++++++----------
arch/openrisc/kernel/signal.c | 14 +++++++-------
3 files changed, 25 insertions(+), 19 deletions(-)
--
2.26.2
This didn't seem to cause any issues, but while working on fixing up
sparse annotations for OpenRISC I noticed this. This patch moves the
include of asm-generic/io.h to the end of the file. Also, we add
defines of ioremap and iounmap, that way we don't get duplicate
definitions from asm-generic/io.h.
Signed-off-by: Stafford Horne <[email protected]>
---
Changes since v1:
- Add linux/types.h include following report from kbuild
arch/openrisc/include/asm/io.h | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/openrisc/include/asm/io.h b/arch/openrisc/include/asm/io.h
index db02fb2077d9..7d6b4a77b379 100644
--- a/arch/openrisc/include/asm/io.h
+++ b/arch/openrisc/include/asm/io.h
@@ -14,6 +14,8 @@
#ifndef __ASM_OPENRISC_IO_H
#define __ASM_OPENRISC_IO_H
+#include <linux/types.h>
+
/*
* PCI: can we really do 0 here if we have no port IO?
*/
@@ -25,9 +27,12 @@
#define PIO_OFFSET 0
#define PIO_MASK 0
-#include <asm-generic/io.h>
-
+#define ioremap ioremap
void __iomem *ioremap(phys_addr_t offset, unsigned long size);
+
+#define iounmap iounmap
extern void iounmap(void *addr);
+#include <asm-generic/io.h>
+
#endif
--
2.26.2
The OpenRISC user access functions put_user(), get_user() and
clear_user() were missing proper sparse annotations. This generated
warnings like the below.
This patch adds the annotations to fix the warnings.
Example warnings:
net/ipv4/ip_sockglue.c:759:29: warning: incorrect type in argument 1 (different address spaces)
net/ipv4/ip_sockglue.c:759:29: expected void const volatile [noderef] __user *
net/ipv4/ip_sockglue.c:759:29: got int const *__gu_addr
net/ipv4/ip_sockglue.c:764:29: warning: incorrect type in initializer (different address spaces)
net/ipv4/ip_sockglue.c:764:29: expected unsigned char const *__gu_addr
net/ipv4/ip_sockglue.c:764:29: got unsigned char [noderef] __user *
Signed-off-by: Stafford Horne <[email protected]>
---
arch/openrisc/include/asm/uaccess.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/openrisc/include/asm/uaccess.h b/arch/openrisc/include/asm/uaccess.h
index 46e31bb4a9ad..f2fc5c4b88c3 100644
--- a/arch/openrisc/include/asm/uaccess.h
+++ b/arch/openrisc/include/asm/uaccess.h
@@ -100,7 +100,7 @@ extern long __put_user_bad(void);
#define __put_user_check(x, ptr, size) \
({ \
long __pu_err = -EFAULT; \
- __typeof__(*(ptr)) *__pu_addr = (ptr); \
+ __typeof__(*(ptr)) __user *__pu_addr = (ptr); \
if (access_ok(__pu_addr, size)) \
__put_user_size((x), __pu_addr, (size), __pu_err); \
__pu_err; \
@@ -173,7 +173,7 @@ struct __large_struct {
#define __get_user_check(x, ptr, size) \
({ \
long __gu_err = -EFAULT, __gu_val = 0; \
- const __typeof__(*(ptr)) * __gu_addr = (ptr); \
+ const __typeof__(*(ptr)) __user *__gu_addr = (ptr); \
if (access_ok(__gu_addr, size)) \
__get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
(x) = (__force __typeof__(*(ptr)))__gu_val; \
@@ -248,10 +248,10 @@ raw_copy_to_user(void __user *to, const void *from, unsigned long size)
#define INLINE_COPY_FROM_USER
#define INLINE_COPY_TO_USER
-extern unsigned long __clear_user(void *addr, unsigned long size);
+extern unsigned long __clear_user(void __user *addr, unsigned long size);
static inline __must_check unsigned long
-clear_user(void *addr, unsigned long size)
+clear_user(void __user *addr, unsigned long size)
{
if (likely(access_ok(addr, size)))
size = __clear_user(addr, size);
--
2.26.2
As suggested by Linus when reviewing commit 9cb2feb4d21d
("arch/openrisc: Fix issues with access_ok()") last year; making
__range_ok an inline function also fixes the used twice issue that the
commit was fixing. I agree it's a good cleanup. This patch addresses
that as I am currently working on the access_ok macro to fixup sparse
annotations in OpenRISC.
Suggested-by: Linus Torvalds <[email protected]>
Signed-off-by: Stafford Horne <[email protected]>
---
arch/openrisc/include/asm/uaccess.h | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/arch/openrisc/include/asm/uaccess.h b/arch/openrisc/include/asm/uaccess.h
index f2fc5c4b88c3..4b59dc9ad300 100644
--- a/arch/openrisc/include/asm/uaccess.h
+++ b/arch/openrisc/include/asm/uaccess.h
@@ -48,16 +48,19 @@
/* Ensure that the range from addr to addr+size is all within the process'
* address space
*/
-#define __range_ok(addr, size) (size <= get_fs() && addr <= (get_fs()-size))
+static inline int __range_ok(unsigned long addr, unsigned long size)
+{
+ const mm_segment_t fs = get_fs();
+
+ return size <= fs && addr <= (fs - size);
+}
/* Ensure that addr is below task's addr_limit */
#define __addr_ok(addr) ((unsigned long) addr < get_fs())
#define access_ok(addr, size) \
({ \
- unsigned long __ao_addr = (unsigned long)(addr); \
- unsigned long __ao_size = (unsigned long)(size); \
- __range_ok(__ao_addr, __ao_size); \
+ __range_ok((unsigned long)(addr), (size)); \
})
/*
--
2.26.2
Since commit b48b2c3e5043 ("openrisc: use generic strnlen_user()
function") the macro __addr_ok is no longer used. It is safe to remove
so this patch removes it.
Signed-off-by: Stafford Horne <[email protected]>
---
arch/openrisc/include/asm/uaccess.h | 3 ---
1 file changed, 3 deletions(-)
diff --git a/arch/openrisc/include/asm/uaccess.h b/arch/openrisc/include/asm/uaccess.h
index 4b59dc9ad300..85a55359b244 100644
--- a/arch/openrisc/include/asm/uaccess.h
+++ b/arch/openrisc/include/asm/uaccess.h
@@ -55,9 +55,6 @@ static inline int __range_ok(unsigned long addr, unsigned long size)
return size <= fs && addr <= (fs - size);
}
-/* Ensure that addr is below task's addr_limit */
-#define __addr_ok(addr) ((unsigned long) addr < get_fs())
-
#define access_ok(addr, size) \
({ \
__range_ok((unsigned long)(addr), (size)); \
--
2.26.2
The __user annotations in signal.c were mostly missing. The missing
annotations caused the warnings listed below. This patch fixes them up
by adding the __user annotations.
arch/openrisc/kernel/signal.c:71:38: warning: incorrect type in initializer (different address spaces)
arch/openrisc/kernel/signal.c:71:38: expected struct rt_sigframe *frame
arch/openrisc/kernel/signal.c:71:38: got struct rt_sigframe [noderef] __user *
arch/openrisc/kernel/signal.c:82:14: warning: incorrect type in argument 1 (different address spaces)
arch/openrisc/kernel/signal.c:82:14: expected void const volatile [noderef] __user *
arch/openrisc/kernel/signal.c:82:14: got struct rt_sigframe *frame
arch/openrisc/kernel/signal.c:84:37: warning: incorrect type in argument 2 (different address spaces)
arch/openrisc/kernel/signal.c:84:37: expected void const [noderef] __user *from
arch/openrisc/kernel/signal.c:84:37: got struct sigset_t *
arch/openrisc/kernel/signal.c:89:39: warning: incorrect type in argument 2 (different address spaces)
arch/openrisc/kernel/signal.c:89:39: expected struct sigcontext [noderef] __user *sc
arch/openrisc/kernel/signal.c:89:39: got struct sigcontext *
arch/openrisc/kernel/signal.c:92:31: warning: incorrect type in argument 1 (different address spaces)
arch/openrisc/kernel/signal.c:92:31: expected struct sigaltstack const [noderef] [usertype] __user *
arch/openrisc/kernel/signal.c:92:31: got struct sigaltstack *
arch/openrisc/kernel/signal.c:158:15: warning: incorrect type in assignment (different address spaces)
arch/openrisc/kernel/signal.c:158:15: expected struct rt_sigframe *frame
arch/openrisc/kernel/signal.c:158:15: got void [noderef] __user *
arch/openrisc/kernel/signal.c:160:14: warning: incorrect type in argument 1 (different address spaces)
arch/openrisc/kernel/signal.c:160:14: expected void const volatile [noderef] __user *
arch/openrisc/kernel/signal.c:160:14: got struct rt_sigframe *frame
arch/openrisc/kernel/signal.c:165:46: warning: incorrect type in argument 1 (different address spaces)
arch/openrisc/kernel/signal.c:165:46: expected struct siginfo [noderef] [usertype] __user *to
arch/openrisc/kernel/signal.c:165:46: got struct siginfo *
arch/openrisc/kernel/signal.c:170:33: warning: incorrect type in argument 1 (different address spaces)
arch/openrisc/kernel/signal.c:170:33: expected struct sigaltstack [noderef] [usertype] __user *
arch/openrisc/kernel/signal.c:170:33: got struct sigaltstack *
arch/openrisc/kernel/signal.c:171:40: warning: incorrect type in argument 2 (different address spaces)
arch/openrisc/kernel/signal.c:171:40: expected struct sigcontext [noderef] __user *sc
arch/openrisc/kernel/signal.c:171:40: got struct sigcontext *
arch/openrisc/kernel/signal.c:173:32: warning: incorrect type in argument 1 (different address spaces)
arch/openrisc/kernel/signal.c:173:32: expected void [noderef] __user *to
arch/openrisc/kernel/signal.c:173:32: got struct sigset_t *
Signed-off-by: Stafford Horne <[email protected]>
---
arch/openrisc/kernel/signal.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/arch/openrisc/kernel/signal.c b/arch/openrisc/kernel/signal.c
index 4f0754874d78..7ce0728412f6 100644
--- a/arch/openrisc/kernel/signal.c
+++ b/arch/openrisc/kernel/signal.c
@@ -68,7 +68,7 @@ static int restore_sigcontext(struct pt_regs *regs,
asmlinkage long _sys_rt_sigreturn(struct pt_regs *regs)
{
- struct rt_sigframe *frame = (struct rt_sigframe __user *)regs->sp;
+ struct rt_sigframe __user *frame = (struct rt_sigframe __user *)regs->sp;
sigset_t set;
/*
@@ -76,7 +76,7 @@ asmlinkage long _sys_rt_sigreturn(struct pt_regs *regs)
* then frame should be dword aligned here. If it's
* not, then the user is trying to mess with us.
*/
- if (((long)frame) & 3)
+ if (((__force unsigned long)frame) & 3)
goto badframe;
if (!access_ok(frame, sizeof(*frame)))
@@ -151,7 +151,7 @@ static inline void __user *get_sigframe(struct ksignal *ksig,
static int setup_rt_frame(struct ksignal *ksig, sigset_t *set,
struct pt_regs *regs)
{
- struct rt_sigframe *frame;
+ struct rt_sigframe __user *frame;
unsigned long return_ip;
int err = 0;
@@ -181,10 +181,10 @@ static int setup_rt_frame(struct ksignal *ksig, sigset_t *set,
l.ori r11,r0,__NR_sigreturn
l.sys 1
*/
- err |= __put_user(0xa960, (short *)(frame->retcode + 0));
- err |= __put_user(__NR_rt_sigreturn, (short *)(frame->retcode + 2));
- err |= __put_user(0x20000001, (unsigned long *)(frame->retcode + 4));
- err |= __put_user(0x15000000, (unsigned long *)(frame->retcode + 8));
+ err |= __put_user(0xa960, (short __user *)(frame->retcode + 0));
+ err |= __put_user(__NR_rt_sigreturn, (short __user *)(frame->retcode + 2));
+ err |= __put_user(0x20000001, (unsigned long __user *)(frame->retcode + 4));
+ err |= __put_user(0x15000000, (unsigned long __user *)(frame->retcode + 8));
if (err)
return -EFAULT;
--
2.26.2
Now that __user annotations are fixed for openrisc uaccess api's we can
add checking to the access_ok macro. This patch adds the __chk_user_ptr
check, on normal builds the added check is a nop.
Signed-off-by: Stafford Horne <[email protected]>
---
arch/openrisc/include/asm/uaccess.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/openrisc/include/asm/uaccess.h b/arch/openrisc/include/asm/uaccess.h
index 85a55359b244..53ddc66abb3f 100644
--- a/arch/openrisc/include/asm/uaccess.h
+++ b/arch/openrisc/include/asm/uaccess.h
@@ -57,7 +57,8 @@ static inline int __range_ok(unsigned long addr, unsigned long size)
#define access_ok(addr, size) \
({ \
- __range_ok((unsigned long)(addr), (size)); \
+ __chk_user_ptr(addr); \
+ __range_ok((__force unsigned long)(addr), (size)); \
})
/*
--
2.26.2
On Thu, Aug 06, 2020 at 06:07:24AM +0900, Stafford Horne wrote:
> ---
> arch/openrisc/kernel/signal.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/arch/openrisc/kernel/signal.c b/arch/openrisc/kernel/signal.c
> index 4f0754874d78..7ce0728412f6 100644
> --- a/arch/openrisc/kernel/signal.c
> +++ b/arch/openrisc/kernel/signal.c
> @@ -76,7 +76,7 @@ asmlinkage long _sys_rt_sigreturn(struct pt_regs *regs)
> * then frame should be dword aligned here. If it's
> * not, then the user is trying to mess with us.
> */
> - if (((long)frame) & 3)
> + if (((__force unsigned long)frame) & 3)
> goto badframe;
Same as patch 6, the __force is not needed.
-- Luc
On Thu, Aug 06, 2020 at 06:07:25AM +0900, Stafford Horne wrote:
> Now that __user annotations are fixed for openrisc uaccess api's we can
> add checking to the access_ok macro. This patch adds the __chk_user_ptr
> check, on normal builds the added check is a nop.
>
> Signed-off-by: Stafford Horne <[email protected]>
> ---
> arch/openrisc/include/asm/uaccess.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/openrisc/include/asm/uaccess.h b/arch/openrisc/include/asm/uaccess.h
> index 85a55359b244..53ddc66abb3f 100644
> --- a/arch/openrisc/include/asm/uaccess.h
> +++ b/arch/openrisc/include/asm/uaccess.h
> @@ -57,7 +57,8 @@ static inline int __range_ok(unsigned long addr, unsigned long size)
>
> #define access_ok(addr, size) \
> ({ \
> - __range_ok((unsigned long)(addr), (size)); \
> + __chk_user_ptr(addr); \
> + __range_ok((__force unsigned long)(addr), (size)); \
> })
Just for info, __force is not needed when casting a pointer to
unsigned long (or uintptr_t). It's not incorrect to use one
but I think it's to avoid __force as much as possible.
-- Luc
On Thu, Aug 06, 2020 at 06:07:19AM +0900, Stafford Horne wrote:
> Hello,
>
> Changes since v1:
> - in "io: Fixup defines and move include to the end" added a linux/types.h
> include as there were compiler failurs pointed out by kbuild.
>
> This a series of fixes for OpenRISC sparse warnings. The kbuild robots report
> many issues related to issues with OpenRISC headers having missing or incorrect
> sparse annotations.
The changes look quite good to me (I just add 2 nits for patches 5 & 6).
Fell free to add my
Reviewed-by: Luc Van Oostenryck <[email protected]>
On Thu, Aug 06, 2020 at 09:02:29PM +0200, Luc Van Oostenryck wrote:
> On Thu, Aug 06, 2020 at 06:07:25AM +0900, Stafford Horne wrote:
> > Now that __user annotations are fixed for openrisc uaccess api's we can
> > add checking to the access_ok macro. This patch adds the __chk_user_ptr
> > check, on normal builds the added check is a nop.
> >
> > Signed-off-by: Stafford Horne <[email protected]>
> > ---
> > arch/openrisc/include/asm/uaccess.h | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/openrisc/include/asm/uaccess.h b/arch/openrisc/include/asm/uaccess.h
> > index 85a55359b244..53ddc66abb3f 100644
> > --- a/arch/openrisc/include/asm/uaccess.h
> > +++ b/arch/openrisc/include/asm/uaccess.h
> > @@ -57,7 +57,8 @@ static inline int __range_ok(unsigned long addr, unsigned long size)
> >
> > #define access_ok(addr, size) \
> > ({ \
> > - __range_ok((unsigned long)(addr), (size)); \
> > + __chk_user_ptr(addr); \
> > + __range_ok((__force unsigned long)(addr), (size)); \
> > })
>
> Just for info, __force is not needed when casting a pointer to
> unsigned long (or uintptr_t). It's not incorrect to use one
> but I think it's to avoid __force as much as possible.
Thanks, I didn't realize that. I will fix.
-Stafford
On Thu, Aug 06, 2020 at 09:04:49PM +0200, Luc Van Oostenryck wrote:
> On Thu, Aug 06, 2020 at 06:07:24AM +0900, Stafford Horne wrote:
> > ---
> > arch/openrisc/kernel/signal.c | 14 +++++++-------
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/openrisc/kernel/signal.c b/arch/openrisc/kernel/signal.c
> > index 4f0754874d78..7ce0728412f6 100644
> > --- a/arch/openrisc/kernel/signal.c
> > +++ b/arch/openrisc/kernel/signal.c
> > @@ -76,7 +76,7 @@ asmlinkage long _sys_rt_sigreturn(struct pt_regs *regs)
> > * then frame should be dword aligned here. If it's
> > * not, then the user is trying to mess with us.
> > */
> > - if (((long)frame) & 3)
> > + if (((__force unsigned long)frame) & 3)
> > goto badframe;
>
> Same as patch 6, the __force is not needed.
Thanks, I thought this was complaining before, I tested now and there is no
problem so I must have been mixed up with something else.
-Stafford
On Thu, Aug 06, 2020 at 09:11:46PM +0200, Luc Van Oostenryck wrote:
> On Thu, Aug 06, 2020 at 06:07:19AM +0900, Stafford Horne wrote:
> > Hello,
> >
> > Changes since v1:
> > - in "io: Fixup defines and move include to the end" added a linux/types.h
> > include as there were compiler failurs pointed out by kbuild.
> >
> > This a series of fixes for OpenRISC sparse warnings. The kbuild robots report
> > many issues related to issues with OpenRISC headers having missing or incorrect
> > sparse annotations.
>
> The changes look quite good to me (I just add 2 nits for patches 5 & 6).
>
> Fell free to add my
> Reviewed-by: Luc Van Oostenryck <[email protected]>
Thank you.
On Sun, Aug 09, 2020 at 07:48:22AM +0900, Stafford Horne wrote:
> On Thu, Aug 06, 2020 at 09:04:49PM +0200, Luc Van Oostenryck wrote:
> > On Thu, Aug 06, 2020 at 06:07:24AM +0900, Stafford Horne wrote:
> > > ---
> > > arch/openrisc/kernel/signal.c | 14 +++++++-------
> > > 1 file changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/arch/openrisc/kernel/signal.c b/arch/openrisc/kernel/signal.c
> > > index 4f0754874d78..7ce0728412f6 100644
> > > --- a/arch/openrisc/kernel/signal.c
> > > +++ b/arch/openrisc/kernel/signal.c
> > > @@ -76,7 +76,7 @@ asmlinkage long _sys_rt_sigreturn(struct pt_regs *regs)
> > > * then frame should be dword aligned here. If it's
> > > * not, then the user is trying to mess with us.
> > > */
> > > - if (((long)frame) & 3)
> > > + if (((__force unsigned long)frame) & 3)
> > > goto badframe;
> >
> > Same as patch 6, the __force is not needed.
>
> Thanks, I thought this was complaining before, I tested now and there is no
> problem so I must have been mixed up with something else.
Sparse should have complained with with the cast to long but
purposely doesn't with unsigned long (or uintptr_t).
So, no mix up, I think.
Cheers,
-- Luc
On Sun, Aug 09, 2020 at 01:08:42AM +0200, Luc Van Oostenryck wrote:
> On Sun, Aug 09, 2020 at 07:48:22AM +0900, Stafford Horne wrote:
> > On Thu, Aug 06, 2020 at 09:04:49PM +0200, Luc Van Oostenryck wrote:
> > > On Thu, Aug 06, 2020 at 06:07:24AM +0900, Stafford Horne wrote:
> > > > ---
> > > > arch/openrisc/kernel/signal.c | 14 +++++++-------
> > > > 1 file changed, 7 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/arch/openrisc/kernel/signal.c b/arch/openrisc/kernel/signal.c
> > > > index 4f0754874d78..7ce0728412f6 100644
> > > > --- a/arch/openrisc/kernel/signal.c
> > > > +++ b/arch/openrisc/kernel/signal.c
> > > > @@ -76,7 +76,7 @@ asmlinkage long _sys_rt_sigreturn(struct pt_regs *regs)
> > > > * then frame should be dword aligned here. If it's
> > > > * not, then the user is trying to mess with us.
> > > > */
> > > > - if (((long)frame) & 3)
> > > > + if (((__force unsigned long)frame) & 3)
> > > > goto badframe;
> > >
> > > Same as patch 6, the __force is not needed.
> >
> > Thanks, I thought this was complaining before, I tested now and there is no
> > problem so I must have been mixed up with something else.
>
> Sparse should have complained with with the cast to long but
> purposely doesn't with unsigned long (or uintptr_t).
> So, no mix up, I think.
You are right, I noticed that right after I sent that email. Thanks a lot.
-Stafford