Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 24 Jan 2022 18:41:12 +0100
From:   Heiko Carstens <hca@linux.ibm.com>
To:     Janis Schoetterl-Glausch <scgl@linux.ibm.com>
Cc:     Christian Borntraeger <borntraeger@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Sven Schnelle <svens@linux.ibm.com>,
        Nico Boehr <nrb@linux.ibm.com>,
        Alexander Gordeev <agordeev@linux.ibm.com>,
        linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH] uaccess: Add mechanism for key checked access to
 user memory
Message-ID: <Ye7kuJ51QWFBGoJ4@osiris>
References: <YerCfDceDszqbdHU@osiris>
 <20220124103812.2340666-1-scgl@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220124103812.2340666-1-scgl@linux.ibm.com>
Precedence: bulk

On Mon, Jan 24, 2022 at 11:38:12AM +0100, Janis Schoetterl-Glausch wrote:
> KVM on s390 needs a mechanism to do accesses to guest memory
> that honors storage key protection.
> __copy_from/to_user_with_key is implemented by introducing
> raw_copy_from/to_user_with_key.
> Since the existing uaccess implementation on s390 makes use of move
> instructions that support having an additional access key supplied,
> we can implement raw_copy_from/to_user_with_key by enhancing the
> existing implementation.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
> 
> This works for us and compiles on other architectures (tested x86).
> The patch only implements __copy_from/to_user_with_key, since those
> are the ones we actually need. On other architectures those functions
> don't exists, but they aren't used either, so it's not a problem.

Adding an API where only underscored function names are to be used can be
considered suboptimal.

> Should we also implement single and no underscore variants? Why?
> Completeness?

Please make this _fully_ symmetrical to the existing copy_to/from_user()
implementations, like I tried to say several times. Maybe I wasn't clear
enough about this. Also the default implementation - that is if an
architecture makes use of copy_to_user_key() without providing a
raw_copy_from_user_key() implementation - should fallback to regular
copy_to_user() semantics, like I tried to outline with the ifndef example
of raw_copy_from_user_key() previously.

Furthermore this should be splitted into two patches: one which adds the
common code infrastructure, like described above; and a second patch which
adds the actual s390 architecture backend/override.

The patches should contain a _detailed_ description why the first patch,
aka API, should probably be in common code (staying in sync with code
instrumentation, etc.); and of course it should contain enough information
for people not familiar with s390's storage keys so they can figure out
what this is about.

Hopefully we get some feedback and either this is acceptable for common
code one way or the other, or we have to maintain this on our own, and get
the additional maintenance cost for free.

Please make sure to add Al Viro, Kees Cook, Arnd Bergmann, and Andrew
Morton to cc on your next version, so we hopefully come to a conclusion and
can move on.