Re: [PATCH 3/3] util/userfaultfd: Support /dev/userfaultfd

[Peter Xu <peterx@redhat.com>]

On Thu, Jan 26, 2023 at 09:05:01AM +0000, Daniel P. Berrangé wrote:
> On Thu, Jan 26, 2023 at 09:02:09AM +0000, Daniel P. Berrangé wrote:
> > On Wed, Jan 25, 2023 at 05:40:16PM -0500, Peter Xu wrote:
> > > Teach QEMU to use /dev/userfaultfd when it existed and fallback to the
> > > system call if either it's not there or doesn't have enough permission.
> > > 
> > > Firstly, as long as the app has permission to access /dev/userfaultfd, it
> > > always have the ability to trap kernel faults which QEMU mostly wants.
> > > Meanwhile, in some context (e.g. containers) the userfaultfd syscall can be
> > > forbidden, so it can be the major way to use postcopy in a restricted
> > > environment with strict seccomp setup.
> > > 
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  util/trace-events  |  1 +
> > >  util/userfaultfd.c | 36 ++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 37 insertions(+)
> > > 
> > > diff --git a/util/trace-events b/util/trace-events
> > > index c8f53d7d9f..16f78d8fe5 100644
> > > --- a/util/trace-events
> > > +++ b/util/trace-events
> > > @@ -93,6 +93,7 @@ qemu_vfio_region_info(const char *desc, uint64_t region_ofs, uint64_t region_siz
> > >  qemu_vfio_pci_map_bar(int index, uint64_t region_ofs, uint64_t region_size, int ofs, void *host) "map region bar#%d addr 0x%"PRIx64" size 0x%"PRIx64" ofs 0x%x host %p"
> > >  
> > >  #userfaultfd.c
> > > +uffd_detect_open_mode(int mode) "%d"
> > >  uffd_query_features_nosys(int err) "errno: %i"
> > >  uffd_query_features_api_failed(int err) "errno: %i"
> > >  uffd_create_fd_nosys(int err) "errno: %i"
> > > diff --git a/util/userfaultfd.c b/util/userfaultfd.c
> > > index 9845a2ec81..360ecf8084 100644
> > > --- a/util/userfaultfd.c
> > > +++ b/util/userfaultfd.c
> > > @@ -18,10 +18,46 @@
> > >  #include <poll.h>
> > >  #include <sys/syscall.h>
> > >  #include <sys/ioctl.h>
> > > +#include <fcntl.h>
> > > +
> > > +typedef enum {
> > > +    UFFD_UNINITIALIZED = 0,
> > > +    UFFD_USE_DEV_PATH,
> > > +    UFFD_USE_SYSCALL,
> > > +} uffd_open_mode;
> > > +
> > > +static uffd_open_mode open_mode;
> > > +static int uffd_dev;
> > > +
> > > +static uffd_open_mode uffd_detect_open_mode(void)
> > > +{
> > > +    if (open_mode == UFFD_UNINITIALIZED) {
> > > +        /*
> > > +         * Make /dev/userfaultfd the default approach because it has better
> > > +         * permission controls, meanwhile allows kernel faults without any
> > > +         * privilege requirement (e.g. SYS_CAP_PTRACE).
> > > +         */
> > > +        uffd_dev = open("/dev/userfaultfd", O_RDWR | O_CLOEXEC);
> > 
> > qemu_open(), otherwise FD passing from the mgmt app won't work.

[I've followed this up in the other thread on interfacing libvirt, so will
 skip here] 

> > 
> > > +        if (uffd_dev >= 0) {
> > > +            open_mode = UFFD_USE_DEV_PATH;
> > > +        } else {
> > > +            /* Fallback to the system call */
> > > +            open_mode = UFFD_USE_SYSCALL;
> > > +        }
> > > +        trace_uffd_detect_open_mode(open_mode);
> > > +    }
> > > +
> > > +    return open_mode;
> > > +}
> > 
> > This leaves the /dev/userfaultfd FD open forever once it has been used
> > once. Is this really needed ? IIUC, the place where we call this is
> > not going to be impacted if we open + close it every time we need to
> > create a new FD, and it'll simplify this code right down.
> 
> Having said that, if we want to support passing the FD in from the
> mgmt app, we need to keep it open persistently.

Right, since the plan is to further support libvirt, I'll keep it as is.

Meanwhile, right now QEMU detects uffd features by creating an uffd and
quickly close it, it's also efficient to keep it when it's firstly opened.
IIRC for each postcopy procedure we'll open uffd at least three times
during different phases.

Thanks,

-- 
Peter Xu