Re: [PATCH] ARC: Improve cmpxchng syscall implementation

[Alexey Brodkin]

Hi Vineet,

On Wed, 2018-04-18 at 11:16 -0700, Vineet Gupta wrote:
> On 03/21/2018 04:54 AM, Alexey Brodkin wrote:
> > /*
> > > >    	 * This is only for old cores lacking LLOCK/SCOND, which by defintion
> > > > @@ -60,23 +62,48 @@ SYSCALL_DEFINE3(arc_usr_cmpxchg, int *, uaddr, int, expected, int, new)
> > > >    	/* Z indicates to userspace if operation succeded */
> > > >    	regs->status32 &= ~STATUS_Z_MASK;
> > > >    
> > > > -	if (!access_ok(VERIFY_WRITE, uaddr, sizeof(int)))
> > > > -		return -EFAULT;
> > > > +	ret = access_ok(VERIFY_WRITE, uaddr, sizeof(*uaddr));
> > > > +	if (!ret)
> > > > +		goto fail;
> > > >    
> > > > +again:
> > > >    	preempt_disable();
> > > >    
> > > > -	if (__get_user(uval, uaddr))
> > > > -		goto done;
> > > > -
> > > > -	if (uval == expected) {
> > > > -		if (!__put_user(new, uaddr))
> > > > +	ret = __get_user(val, uaddr);
> > > > +	if (ret == -EFAULT) {
> > > 
> > > Lets see if this warrants adding complexity ! This implies that TLB entry with
> > > Read permissions didn't exist for reading the var and page fault handler could not
> > > wire up even a zero page due to preempt_disable, meaning it was something not
> > > touched by userspace already - sort of uninitialized variable in user code.
> > 
> > Ok I completely missed the fact that fast path TLB miss handler is being
> > executed even if we have preemption disabled. So given the mapping exist
> > we do not need to retry with enabled preemption.
> > 
> > Still maybe I'm a bit paranoid here but IMHO it's good to be ready for a corner-case
> > when the pointer is completely bogus and there's no mapping for him.
> > I understand that today we only expect this syscall to be used from libc's
> > internals but as long as syscall exists nobody stops anybody from using it
> > directly without libc. So maybe instead of doing get_user_pages_fast() just
> > send a SIGSEGV to the process? At least user will realize there's some problem
> > at earlier stage.
> 
> if the pointer is bogus, we currently return -EFAULT, is that not enough ! I'm 
> fine if u want to change that to segv.

Ok good.

> > > Otherwise it is extremely unlikely to start with a TLB entry with Read
> > > permissions, followed by syscall Trap only to find the entry missing, unless a
> > > global TLB flush came from other cores, right in the middle. But this syscall is
> > > not guaranteed to work with SMP anyways, so lets ignore any SMP misdoings here.
> > 
> > Well but that's exactly the situation I was debugging: we start from data from read-only
> > page and on attempt to write back modified value COW machinery gets involved.
> 
> No exactly your situation. In your case the TLB entry *did* exist with Read 
> permission. What I was pointing to is that case where it woudl vanish between user 
> reading the backing page and making a syscall !

Probably I'm missing something here. Indeed there's already TLB entry with READ permission
and we need ProtV exception to happen to update this entry such that it becomes READ-WRITE
enabled. And in its turn for ProtV exception to happen we need to enable preemption and
execute __put_user().

> > > Now in case it was *an* uninitialized var, do we have to guarantee any well
> > > defined semantics for the kernel emulation of cmpxchg ? IMO it should be fine to
> > > return 0 or -EFAULT etc. Infact -EFAULT is better as it will force a retry loop on
> > > user side, given the typical cmpxchg usage pattern.
> > 
> > The problem is libc only expects to get a value read from memory.
> > And in theory expected value might be -14 which is basically -EFAULT.
> > I'm not talking about 0 at all because in some cases that's exactly what
> > user-space expects.
> > 
> > So if we read unexpected value then we'll just return it without even attempting
> > to write.
> > 
> > If we read expected data but fail to write then we'll send a SIGSEGV and
> > return whatever... let it be -EFAULT - anyways the app will be killed on exit from
> > this syscall.
> 
> I'm not sure what you mean here. I'm fine with adding segv kill semantics, but 
> don't think complexity for get_user is worth it !

This complexity adds predictability - if we cannot read or write data we kill the app as if
our normal LD/ST fails fro whatever reason.

-Alexey