| Message ID | 20231123141617.259591-1-ben.dooks@codethink.co.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ca0e433b41a6aa5115f752644eb39470f9a12a99 |
| Headers | show |
| Series | riscv: fix __user annotation in traps_misaligned.c | expand |
On Thu, Nov 23, 2023 at 02:16:17PM +0000, Ben Dooks wrote: > @@ -319,7 +319,7 @@ static inline int get_insn(struct pt_regs *regs, ulong mepc, ulong *r_insn) > static inline int load_u8(struct pt_regs *regs, const u8 *addr, u8 *r_val) > { > if (user_mode(regs)) { > - return __get_user(*r_val, addr); > + return __get_user(*r_val, (u8 __user *)addr); > } else { > *r_val = *addr; > return 0; This is the wrong way to approach it. Pass the untype unsigned long from the caller instead and do a single round of casts from that depending on the address_space. And please also remove this horrible else after return entipattern while you're at it.
Hi Ben, I sent a similar patch two days ago (https://lore.kernel.org/linux-riscv/20231122135141.2936663-1-cleger@rivosinc.com/). On 23/11/2023 15:16, Ben Dooks wrote: > The instruction reading code can read from either user or kernel addresses > and thus the use of __user on pointers to instructions depends on which > context. Fix a few sparse warnings by using __user for user-accesses and > remove it when not. > > Fixes: > > arch/riscv/kernel/traps_misaligned.c:361:21: warning: dereference of noderef expression > arch/riscv/kernel/traps_misaligned.c:373:21: warning: dereference of noderef expression > arch/riscv/kernel/traps_misaligned.c:381:21: warning: dereference of noderef expression > arch/riscv/kernel/traps_misaligned.c:322:24: warning: incorrect type in initializer (different address spaces) > arch/riscv/kernel/traps_misaligned.c:322:24: expected unsigned char const [noderef] __user *__gu_ptr > arch/riscv/kernel/traps_misaligned.c:322:24: got unsigned char const [usertype] *addr > arch/riscv/kernel/traps_misaligned.c:361:21: warning: dereference of noderef expression > arch/riscv/kernel/traps_misaligned.c:373:21: warning: dereference of noderef expression > arch/riscv/kernel/traps_misaligned.c:381:21: warning: dereference of noderef expression > arch/riscv/kernel/traps_misaligned.c:332:24: warning: incorrect type in initializer (different address spaces) > arch/riscv/kernel/traps_misaligned.c:332:24: expected unsigned char [noderef] __user *__gu_ptr > arch/riscv/kernel/traps_misaligned.c:332:24: got unsigned char [usertype] *addr > > Fixes: 7c83232161f60 ("riscv: add support for misaligned trap handling in S-mode") > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > --- > arch/riscv/kernel/traps_misaligned.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c > index 5eba37147caa..446e3d4eeea9 100644 > --- a/arch/riscv/kernel/traps_misaligned.c > +++ b/arch/riscv/kernel/traps_misaligned.c > @@ -319,7 +319,7 @@ static inline int get_insn(struct pt_regs *regs, ulong mepc, ulong *r_insn) > static inline int load_u8(struct pt_regs *regs, const u8 *addr, u8 *r_val) > { > if (user_mode(regs)) { > - return __get_user(*r_val, addr); > + return __get_user(*r_val, (u8 __user *)addr); > } else { > *r_val = *addr; > return 0; > @@ -329,7 +329,7 @@ static inline int load_u8(struct pt_regs *regs, const u8 *addr, u8 *r_val) > static inline int store_u8(struct pt_regs *regs, u8 *addr, u8 val) > { > if (user_mode(regs)) { > - return __put_user(val, addr); > + return __put_user(val, (u8 __user *)addr); > } else { > *addr = val; > return 0; > @@ -343,7 +343,7 @@ static inline int store_u8(struct pt_regs *regs, u8 *addr, u8 val) > if (user_mode(regs)) { \ > __ret = __get_user(insn, insn_addr); \ > } else { \ > - insn = *insn_addr; \ > + insn = *(__force u16 *)insn_addr; \ __read_insn() is called with either a u32 or a u16 pointer which is why this macros did not used a specific type. Doing so would result in loading half of what is needed. My patch addresses that. Thanks > __ret = 0; \ > } \ > \
On 24/11/2023 07:05, Christoph Hellwig wrote: > On Thu, Nov 23, 2023 at 02:16:17PM +0000, Ben Dooks wrote: >> @@ -319,7 +319,7 @@ static inline int get_insn(struct pt_regs *regs, ulong mepc, ulong *r_insn) >> static inline int load_u8(struct pt_regs *regs, const u8 *addr, u8 *r_val) >> { >> if (user_mode(regs)) { >> - return __get_user(*r_val, addr); >> + return __get_user(*r_val, (u8 __user *)addr); >> } else { >> *r_val = *addr; >> return 0; > > This is the wrong way to approach it. Pass the untype unsigned long > from the caller instead and do a single round of casts from that > depending on the address_space. I sent a similar patch two days ago with the same modification. I'm not sure to get it. Why is it better to pass the "unsigned long" type from the caller ? I mean, the resulting code would look like this right ? static inline int store_u8(struct pt_regs *regs, unsigned long addr, u8 val) { if (user_mode(regs)) { return __put_user(val, (u8 __user *)addr); } else { *addr = (u8 *)val; return 0; } } Is this better from a "semantic" point of view and be sure the casts are done in a single place ? > > And please also remove this horrible else after return entipattern > while you're at it. Acked, Thanks, > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Fri, Nov 24, 2023 at 11:28:08AM +0100, Clément Léger wrote: > I sent a similar patch two days ago with the same modification. I'm not > sure to get it. Why is it better to pass the "unsigned long" type from > the caller ? I mean, the resulting code would look like this right ? Because you're legimitizing casting between address_space, which is a horrible idea. By casting either from the unsigned long you make it very clear that deep magic is coming in and you make an informed decisions based on the user_mode() predicate. Witht a blind cast to add/remove a __user you don't. I'm actually surprised sparse even allows __user casts without __force.
On 24/11/2023 11:45, Christoph Hellwig wrote: > On Fri, Nov 24, 2023 at 11:28:08AM +0100, Clément Léger wrote: >> I sent a similar patch two days ago with the same modification. I'm not >> sure to get it. Why is it better to pass the "unsigned long" type from >> the caller ? I mean, the resulting code would look like this right ? > > Because you're legimitizing casting between address_space, which is a > horrible idea. By casting either from the unsigned long you make it > very clear that deep magic is coming in and you make an informed > decisions based on the user_mode() predicate. Witht a blind cast > to add/remove a __user you don't. Makes sense indeed, thanks ! Clément > > I'm actually surprised sparse even allows __user casts without __force.
Hello: This patch was applied to riscv/linux.git (for-next) by Palmer Dabbelt <palmer@rivosinc.com>: On Thu, 23 Nov 2023 14:16:17 +0000 you wrote: > The instruction reading code can read from either user or kernel addresses > and thus the use of __user on pointers to instructions depends on which > context. Fix a few sparse warnings by using __user for user-accesses and > remove it when not. > > Fixes: > > [...] Here is the summary with links: - riscv: fix __user annotation in traps_misaligned.c https://git.kernel.org/riscv/c/ca0e433b41a6 You are awesome, thank you!
diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c index 5eba37147caa..446e3d4eeea9 100644 --- a/arch/riscv/kernel/traps_misaligned.c +++ b/arch/riscv/kernel/traps_misaligned.c @@ -319,7 +319,7 @@ static inline int get_insn(struct pt_regs *regs, ulong mepc, ulong *r_insn) static inline int load_u8(struct pt_regs *regs, const u8 *addr, u8 *r_val) { if (user_mode(regs)) { - return __get_user(*r_val, addr); + return __get_user(*r_val, (u8 __user *)addr); } else { *r_val = *addr; return 0; @@ -329,7 +329,7 @@ static inline int load_u8(struct pt_regs *regs, const u8 *addr, u8 *r_val) static inline int store_u8(struct pt_regs *regs, u8 *addr, u8 val) { if (user_mode(regs)) { - return __put_user(val, addr); + return __put_user(val, (u8 __user *)addr); } else { *addr = val; return 0; @@ -343,7 +343,7 @@ static inline int store_u8(struct pt_regs *regs, u8 *addr, u8 val) if (user_mode(regs)) { \ __ret = __get_user(insn, insn_addr); \ } else { \ - insn = *insn_addr; \ + insn = *(__force u16 *)insn_addr; \ __ret = 0; \ } \ \
The instruction reading code can read from either user or kernel addresses and thus the use of __user on pointers to instructions depends on which context. Fix a few sparse warnings by using __user for user-accesses and remove it when not. Fixes: arch/riscv/kernel/traps_misaligned.c:361:21: warning: dereference of noderef expression arch/riscv/kernel/traps_misaligned.c:373:21: warning: dereference of noderef expression arch/riscv/kernel/traps_misaligned.c:381:21: warning: dereference of noderef expression arch/riscv/kernel/traps_misaligned.c:322:24: warning: incorrect type in initializer (different address spaces) arch/riscv/kernel/traps_misaligned.c:322:24: expected unsigned char const [noderef] __user *__gu_ptr arch/riscv/kernel/traps_misaligned.c:322:24: got unsigned char const [usertype] *addr arch/riscv/kernel/traps_misaligned.c:361:21: warning: dereference of noderef expression arch/riscv/kernel/traps_misaligned.c:373:21: warning: dereference of noderef expression arch/riscv/kernel/traps_misaligned.c:381:21: warning: dereference of noderef expression arch/riscv/kernel/traps_misaligned.c:332:24: warning: incorrect type in initializer (different address spaces) arch/riscv/kernel/traps_misaligned.c:332:24: expected unsigned char [noderef] __user *__gu_ptr arch/riscv/kernel/traps_misaligned.c:332:24: got unsigned char [usertype] *addr Fixes: 7c83232161f60 ("riscv: add support for misaligned trap handling in S-mode") Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> --- arch/riscv/kernel/traps_misaligned.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)