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.
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
> [email protected]
> 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.