Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user()

[Jon Kohler <jon@nutanix.com>]

> On Nov 26, 2025, at 8:08 PM, Jason Wang <jasowang@redhat.com> wrote:
> 
> On Thu, Nov 27, 2025 at 3:48 AM Jon Kohler <jon@nutanix.com> wrote:
>> 
>> 
>>> On Nov 26, 2025, at 5:25 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> 
>>> On Wed, Nov 26, 2025, at 07:04, Jason Wang wrote:
>>>> On Wed, Nov 26, 2025 at 3:45 AM Jon Kohler <jon@nutanix.com> wrote:
>>>>>> On Nov 19, 2025, at 8:57 PM, Jason Wang <jasowang@redhat.com> wrote:
>>>>>> On Tue, Nov 18, 2025 at 1:35 AM Jon Kohler <jon@nutanix.com> wrote:
>>>>> Same deal goes for __put_user() vs put_user by way of commit
>>>>> e3aa6243434f ("ARM: 8795/1: spectre-v1.1: use put_user() for __put_user()”)
>>>>> 
>>>>> Looking at arch/arm/mm/Kconfig, there are a variety of scenarios
>>>>> where CONFIG_CPU_SPECTRE will be enabled automagically. Looking at
>>>>> commit 252309adc81f ("ARM: Make CONFIG_CPU_V7 valid for 32bit ARMv8 implementations")
>>>>> it says that "ARMv8 is a superset of ARMv7", so I’d guess that just
>>>>> about everything ARM would include this by default?
>>> 
>>> I think the more relevant commit is for 64-bit Arm here, but this does
>>> the same thing, see 84624087dd7e ("arm64: uaccess: Don't bother
>>> eliding access_ok checks in __{get, put}_user").
>> 
>> Ah! Right, this is definitely the important bit, as it makes it
>> crystal clear that these are exactly the same thing. The current
>> code is:
>> #define get_user        __get_user
>> #define put_user        __put_user
>> 
>> So, this patch changing from __* to regular versions is a no-op
>> on arm side of the house, yea?
>> 
>>> I would think that if we change the __get_user() to get_user()
>>> in this driver, the same should be done for the
>>> __copy_{from,to}_user(), which similarly skips the access_ok()
>>> check but not the PAN/SMAP handling.
>> 
>> Perhaps, thats a good call out. I’d file that under one battle
>> at a time. Let’s get get/put user dusted first, then go down
>> that road?
>> 
>>> In general, the access_ok()/__get_user()/__copy_from_user()
>>> pattern isn't really helpful any more, as Linus already
>>> explained. I can't tell from the vhost driver code whether
>>> we can just drop the access_ok() here and use the plain
>>> get_user()/copy_from_user(), or if it makes sense to move
>>> to the newer user_access_begin()/unsafe_get_user()/
>>> unsafe_copy_from_user()/user_access_end() and try optimize
>>> out a few PAN/SMAP flips in the process.
> 
> Right, according to my testing in the past, PAN/SMAP is a killer for
> small packet performance (PPS).

For sure, every little bit helps along that path

> 
>> 
>> In general, I think there are a few spots where we might be
>> able to optimize (vhost_get_vq_desc perhaps?) as that gets
>> called quite a bit and IIRC there are at least two flips
>> in there that perhaps we could elide to one? An investigation
>> for another day I think.
> 
> Did you mean trying to read descriptors in a batch, that would be
> better and with IN_ORDER it would be even faster as a single (at most
> two) copy_from_user() might work (without the need to use
> user_access_begin()/user_access_end().

Yep. I haven’t fully thought through it, just a drive-by idea
from looking at code for the recent work I’ve been doing, just
scratching my head thinking there *must* be something we can do
better there.

Basically on the get rx/tx bufs path as well as the
vhost_add_used_and_signal_n path, I think we could cluster together
some of the get/put users and copy to/from’s. Would take some
massaging, but I think there is something there.

>> 
>> Anyhow, with this info - Jason - is there anything else you
>> can think of that we want to double click on?
> 
> Nope.
> 
> Thanks

Ok thanks. Perhaps we can land this in next and let it soak in,
though, knock on wood, I don’t think there will be fallout
(famous last words!) ?