Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 2 Nov 2022 15:12:36 +0100
From:   Heiko Carstens <hca@linux.ibm.com>
To:     Janis Schoetterl-Glausch <scgl@linux.ibm.com>
Cc:     Christian Borntraeger <borntraeger@linux.ibm.com>,
        Janosch Frank <frankja@linux.ibm.com>,
        Claudio Imbrenda <imbrenda@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Alexander Gordeev <agordeev@linux.ibm.com>,
        David Hildenbrand <david@redhat.com>,
        Jonathan Corbet <corbet@lwn.net>, kvm@vger.kernel.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-kselftest@vger.kernel.org, linux-s390@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>,
        Shuah Khan <shuah@kernel.org>,
        Sven Schnelle <svens@linux.ibm.com>
Subject: Re: [PATCH v2 1/9] s390/uaccess: Add storage key checked cmpxchg
 access to user space
Message-ID: <Y2J61LWSV+HolIeT@osiris>
References: <20221012205609.2811294-1-scgl@linux.ibm.com>
 <20221012205609.2811294-2-scgl@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221012205609.2811294-2-scgl@linux.ibm.com>
Precedence: bulk

Hi Janis,

On Wed, Oct 12, 2022 at 10:56:01PM +0200, Janis Schoetterl-Glausch wrote:
> Add cmpxchg functionality similar to that in cmpxchg.h except that the
> target is a user space address and that the address' storage key is
> matched with the access_key argument in order to honor key-controlled
> protection.
> The access is performed by changing to the secondary-spaces mode and
> setting the PSW key for the duration of the compare and swap.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
> 
> 
> Possible variations:
>   * check the assumptions made in cmpxchg_user_key_size and error out
>   * call functions called by copy_to_user
>      * access_ok? is a nop
>      * should_fail_usercopy?
>      * instrument_copy_to_user? doesn't make sense IMO
>   * don't be overly strict in cmpxchg_user_key
> 
> 
>  arch/s390/include/asm/uaccess.h | 189 ++++++++++++++++++++++++++++++++
>  1 file changed, 189 insertions(+)

So finally I send the uaccess/cmpxchg patches in reply to this mail.
Sorry for the long delay!

The first three patches are not required for the functionality you need,
but given that I always stress that the code should be consistent I include
them anyway.

The changes are probably quite obvious:

- Keep uaccess cmpxchg code more or less identical to regular cmpxchg
  code. I wasn't able to come up with a readable code base which could be
  used for both variants.

- Users may only use the cmpxchg_user_key() macro - _not_ the inline
  function, which is an internal API. This will require that you need to
  add a switch statement and couple of casts within the KVM code, but
  shouldn't have much of an impact on the generated code.

- Cause link error for non-integral sizes, similar to other uaccess
  functions.

- cmpxchg_user_key() has now a simple return value: 0 or -EFAULT, and
  writes the old value to a location provided by a pointer. This is quite
  similar to the futex code. Users must compare the old and expected value
  to figure out if something was exchanged. Note that this is in most cases
  more efficient than extracting the condition code from the PSW with ipm,
  since nowadays we have instructions like compare and branch relative on
  condition, etc.

- Couple of other minor changes which I forgot.

Code is untested (of course :) ). Please give it a try and let me know if
this is good enough for your purposes.

I also did not limit the number of retries for the one and two byte
scenarion. Before doing that we need to have proof that there really is a
problem. Maybe Nico or you will give this a try.

Thanks,
Heiko