[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <alpine.DEB.2.20.1706282336270.1890@nanos>
Date: Wed, 28 Jun 2017 23:49:44 +0200 (CEST)
From: Thomas Gleixner <tglx@...utronix.de>
To: Palmer Dabbelt <palmer@...belt.com>
cc: peterz@...radead.org, mingo@...hat.com, mcgrof@...nel.org,
viro@...iv.linux.org.uk, sfr@...b.auug.org.au,
nicolas.dichtel@...nd.com, rmk+kernel@...linux.org.uk,
msalter@...hat.com, tklauser@...tanz.ch, will.deacon@....com,
james.hogan@...tec.com, paul.gortmaker@...driver.com,
linux@...ck-us.net, linux-kernel@...r.kernel.org,
linux-arch@...r.kernel.org, albert@...ive.com
Subject: Re: [PATCH 8/9] RISC-V: User-facing API
On Wed, 28 Jun 2017, Palmer Dabbelt wrote:
> +
> +SYSCALL_DEFINE3(sysriscv_cmpxchg32, unsigned long, arg1, unsigned long, arg2,
> + unsigned long, arg3)
> +{
> + unsigned long flags;
> + unsigned long prev;
> + unsigned int *ptr;
> + unsigned int err;
> +
> + ptr = (unsigned int *)arg1;
Errm. Why isn't arg1 a proper pointer type and the arguments arg2/3 u32?
And please give the arguments a proper name, so it's obvious what is what.
SYSCALL_DEFINE3(sysriscv_cmpxchg32, u32 __user *, ptr, u32 new, u32 old)
Hmm?
> + if (!access_ok(VERIFY_WRITE, ptr, sizeof(unsigned int)))
> + return -EFAULT;
> +
> + preempt_disable();
> + raw_local_irq_save(flags);
Why do you want to disable interrupts here? This is thread context and
accessing user space memory, so the only protection this needs is against
preemption.
> + err = __get_user(prev, ptr);
> + if (likely(!err && prev == arg2))
> + err = __put_user(arg3, ptr);
> + raw_local_irq_restore(flags);
> + preempt_enable();
> +
> + return unlikely(err) ? err : prev;
> +}
> +
> +SYSCALL_DEFINE3(sysriscv_cmpxchg64, unsigned long, arg1, unsigned long, arg2,
> + unsigned long, arg3)
This one is even worse. How does this implement cmpxchg64 on a 32bit machine?
Answer: Not at all, because arg2 and 3 are 32bit ....
> +{
> + unsigned long flags;
> + unsigned long prev;
> + unsigned int *ptr;
> + unsigned int err;
> +
> + ptr = (unsigned int *)arg1;
Type casting to random pointer types makes the code more obvious
and safe, right? What the heck has a int pointer to do with u64?
> + if (!access_ok(VERIFY_WRITE, ptr, sizeof(unsigned long)))
> + return -EFAULT;
> +
> + preempt_disable();
> + raw_local_irq_save(flags);
Same as above.
> + err = __get_user(prev, ptr);
Sigh. Type safety is overrated, right?
Thanks,
tglx
Powered by blists - more mailing lists