Re: [PATCH RFC] uaccess: user_access_begin_after_access_ok()

From: Jason Wang
Date: Tue Jun 02 2020 - 23:57:25 EST



On 2020/6/3 äå9:48, Al Viro wrote:
On Tue, Jun 02, 2020 at 04:45:05AM -0400, Michael S. Tsirkin wrote:
So vhost needs to poke at userspace *a lot* in a quick succession. It
is thus benefitial to enable userspace access, do our thing, then
disable. Except access_ok has already been pre-validated with all the
relevant nospec checks, so we don't need that. Add an API to allow
userspace access after access_ok and barrier_nospec are done.
BTW, what are you going to do about vq->iotlb != NULL case? Because
you sure as hell do *NOT* want e.g. translate_desc() under STAC.
Disable it around the calls of translate_desc()?


translate_desc() itself doesn't do userspace access, so the idea is probably disabling STAC around a batch of vhost_get_uesr()/vhost_put_user().



How widely do you hope to stretch the user_access areas, anyway?


To have best performance for small packets like 64B, if possible, we want to disable STAC not only for the metadata access done by vhost accessors but also the data access via iov iterator.



BTW, speaking of possible annotations: looks like there's a large
subset of call graph that can be reached only from vhost_worker()
or from several ioctls, with all uaccess limited to that subgraph
(thankfully). Having that explicitly marked might be a good idea...

Unrelated question, while we are at it: is there any point having
vhost_get_user() a polymorphic macro? In all callers the third
argument is __virtio16 __user * and the second one is an explicit
*<something> where <something> is __virtio16 *. Similar for
vhost_put_user(): in all callers the third arugment is
__virtio16 __user * and the second - cpu_to_vhost16(vq, something).


This is because all virtqueue metadata that needs to be accessed atomically is __virtio16 now, but this may not be true for future virtio extension.



Incidentally, who had come up with the name __vhost_get_user?
Makes for lovey WTF moment for readers - esp. in vhost_put_user()...


I think the confusion comes since it does not accept userspace pointer (when IOTLB is enabled).

How about renaming it as vhost_read()/vhost_write() ?

Thanks