On Tue, Oct 20, 2026 at 11:22:59AM +0200, Eugenio Perez Martin wrote:
> On Tue, Oct 20, 2026 at 8:31 AM Michael S. Tsirkin <[email protected]> wrote:
> >
> > On Mon, Oct 19, 2026 at 03:14:46PM +0800, Wang Haoran wrote:
> > > Eugenio previously offered some additional suggestions in reply to
> > > my patch email. Should these also be addressed?
> >
> > The following seems to work for me. Testing would be appreciated.
> >
> > From: Michael S. Tsirkin <[email protected]>
> > Subject: [PATCH v2] vringh: use get_user/put_user in the use_va path
> >
> > getu16_iotlb()/putu16_iotlb() pass the address produced by
> > iotlb_translate() straight to __get_user()/__put_user(),
> > without access_ok checked anywhere. Not good.
> >
> > Switch to get_user()/put_user(), which are always
> > correct, and safer.
> >
> > But the fact is that accessing user memory like this
> > will sometimes (rarely) race with page being swapped
> > out, trigger EFAULT, and it should be retried
> > from a worker, and vdpa_sim currently does not do this.
> >
> > Fixes: 42823a871fd4 ("vringh: support VA with iotlb")
> > Cc: Stefano Garzarella <[email protected]>
> > Signed-off-by: Michael S. Tsirkin <[email protected]>
> >
> > ---
> >
> > Notes (changelog):
> > v1->v2:
> > Use get_user()/put_user() wrapped in pagefault_disable() instead of
> > access_ok() followed by __get_user()/__put_user(); the helpers are
> > reached from BH-disabled sections, and __get_user() is not a valid
> > way around that (David Laight, Linus Torvalds)
> >
> > drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 2 ++
> > drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 2 ++
> > drivers/vhost/vringh.c | 4 ++--
> > 3 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> > b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> > index 76dd5b0828d7..325ec8f0ab85 100644
> > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> > @@ -347,8 +347,10 @@ static void vdpasim_blk_work(struct vdpasim *vdpasim)
> > smp_wmb();
> >
> > local_bh_disable();
> > + pagefault_disable();
> > if (vringh_need_notify_iotlb(&vq->vring) > 0)
> > vringh_notify(&vq->vring);
> > + pagefault_enable();
> > local_bh_enable();
> >
> > if (++reqs > 4) {
> > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > index a6514b5ccd86..964cdf422467 100644
> > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > @@ -77,8 +77,10 @@ static void vdpasim_net_complete(struct
> > vdpasim_virtqueue *vq, size_t len)
> > smp_wmb();
> >
> > local_bh_disable();
> > + pagefault_disable();
> > if (vringh_need_notify_iotlb(&vq->vring) > 0)
> > vringh_notify(&vq->vring);
> > + pagefault_enable();
> > local_bh_enable();
> > }
> >
>
> Why not move the pagefault disable_enable to wrap just the accesses?
We want most accessed with PF enabled, so memory can be faulted in.
> Future vdpa drivers could use va and we might miss the call. Also, the
> reverse is also true, vdpa_sim could be configured to not use it, even
> if it is not a big waste.
>
> > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> > index 9066f9f12ba1..74836eb60c7a 100644
> > --- a/drivers/vhost/vringh.c
> > +++ b/drivers/vhost/vringh.c
> > @@ -1213,7 +1213,7 @@ static inline int getu16_iotlb(const struct vringh
> > *vrh,
> > return ret;
> >
> > if (vrh->use_va) {
> > - ret = __get_user(tmp, (__virtio16 __user
> > *)ivec.iov.iovec[0].iov_base);
> > + ret = get_user(tmp, (__virtio16 __user
> > *)ivec.iov.iovec[0].iov_base);
> > if (ret)
> > return ret;
> > } else {
> > @@ -1251,7 +1251,7 @@ static inline int putu16_iotlb(const struct vringh
> > *vrh,
> > tmp = cpu_to_vringh16(vrh, val);
> >
> > if (vrh->use_va) {
> > - ret = __put_user(tmp, (__virtio16 __user
> > *)ivec.iov.iovec[0].iov_base);
> > + ret = put_user(tmp, (__virtio16 __user
> > *)ivec.iov.iovec[0].iov_base);
> > if (ret)
> > return ret;
> > } else {
> >
