Re: [PATCH v2] ARC: Improve cmpxchg syscall implementation

[Alexey Brodkin]

Hi Vineet,

On Mon, 2018-06-25 at 13:03 -0700, Vineet Gupta wrote:
> On 06/19/2018 07:22 AM, Alexey Brodkin wrote:
> > From: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > 
> > arc_usr_cmpxchg syscall is supposed to be used on platforms
> > that lack support of Load-Locked/Store-Conditional instructions
> > in hardware. And in that case we mimic missing hardware features
> > with help of kernel's sycall that "atomically" checks current
> > value in memory and then if it matches caller expectation new
> > value is written to that same location.
> > 
> > What's important in the description above:
> >  - Check-and-exchange must be "atomical" which means
> >    preemption must be disabled during entire "transaction"
> >  - Data accessed is from user-space, i.e. we're dealing
> >    with virtual addresses
> > 
> > And in current implementation we have a couple of problems:
> > 
> > 1. We do disable preemprion around __get_user() & __put_user()
> >    but that in its turn disables page fault handler.
> >    That means if a pointer to user's data has no mapping in
> >    the TLB we won't be able to access required data.
> >    Instead software "exception handling" code from __get_user_fn()
> >    will return -EFAULT.
> > 
> > 2. What's worse if we're dealing with data from not yet allocated
> >    page (think of pre-copy-on-write state) we'll successfully
> >    read data but on write we'll silently return to user-space
> >    with correct result (which we really read just before). That leads
> >    to very strange problems in user-space app further down the line
> >    because new value was never written to the destination.
> > 
> > 3. Regardless of what went wrong we'll return from syscall
> >    and user-space application will continue to execute.
> >    Even if user's pointer was completely bogus.
> >    In case of hardware LL/SC that app would have been killed
> >    by the kernel.
> > 
> > With that change we attempt to imrove on all 3 items above:
> > 
> > 1. We still disable preemption around write of user's data but
> >    if we happen to fail with write we're enabling preemption
> >    and try to fix-up page fault so that we have a correct permission
> >    for writing user's data. Then re-try again in "atomic" context.
> > 
> > 2. If real page fault fails or even access_ok() returns false
> >    we send SIGSEGV to the user-space process so if something goes
> >    seriously wrong we'll know about it much earlier.
> > 
> > Signed-off-by: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > Signed-off-by: Alexey Brodkin <abrodkin@xxxxxxxxxxxx>
> > Cc: Vineet Gupta <vgupta@xxxxxxxxxxxx>
> > Cc: Max Filippov <jcmvbkbc@xxxxxxxxx>
> > Cc: linux-arch@xxxxxxxxxxxxxxx
> > ---
> > 
> > Changes v1 -> v2:
> > 
> >  * Peter's almost clean-room reimplmentation with less paranoid checks
> >    and direct invocation of fixup_user_fault() for in-place update of
> >    write permissions.
> > 
> 
> I don't like the changelog - it is way too verbose and doesn't say the exact
> problem we are trying to solve. How about something like below ?
> 
> ----->
> 
>     ARC: Improve cmpxchg syscall implementation
>    
>     This is used in configs lacking hardware atomics to emulate atomic r-m-w
>     for user space, implemented by disabling preemption in kernel.
>    
>     However there are issues in current implementation:
>    
>     1. Process not terminated if invalid user pointer passed:
>        i.e. __get_user() failed.
>    
>     2. The reason for this patch was __put_user() failure not being handled,
>        for COW break scenario. The zero page is initially wired up and
>        read by __get_user() succeeds. However a write by __put_user()
>        doesn't complete the page fault handling due to the page fault
>        disabling from preempt disable. And what's worse is we silently return
>        the stale zero value from __get_user() to user space. So the fix
>        handles the specific case by re-enabling preemption and explicitly
>        fixing up the fault and retrying the whole sequence over.
> 
> OK ?

Sure, care to update the commit log or want me to resend?

-Alexey