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

[Peter Xu <peterx@redhat.com>]

On Fri, Feb 03, 2023 at 10:01:04PM +0100, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > On Thu, Feb 02, 2023 at 11:52:21AM +0100, Juan Quintela wrote:
> >> Peter Xu <peterx@redhat.com> 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.
> >> >
> >> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >> > Signed-off-by: Peter Xu <peterx@redhat.com>
> >> 
> >> 
> >> Hi
> >
> > Hi, Juan,
> 
> 
> >> static int open_userfaultd(void)
> >> {
> >>     /*
> >>      * 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).
> >>      */
> >>      int uffd = open("/dev/userfaultfd", O_RDWR | O_CLOEXEC);
> >>      if (uffd >= 0) {
> >>             return uffd;
> >>      }
> >>      return -1;
> >> }
> >> 
> >> int uffd_open(int flags)
> >> {
> >> #if defined(__linux__) && defined(__NR_userfaultfd)
> 
> Just an incise, checkpatch don't liue that you use __linux__
> 
> This file is compiled under CONFIG_LINUX, so you can drop it.

Yes indeed.  I'll drop it.

> 
> >>     static int uffd = -2;
> >>     if (uffd == -2) {
> >>         uffd = open_userfaultd();
> >>     }
> >>     if (uffd >= 0) {
> >>         return ioctl(uffd, USERFAULTFD_IOC_NEW, flags);
> >>     }
> >>     return syscall(__NR_userfaultfd, flags);
> >> #else
> >>      return -EINVAL;
> >> 
> >> 27 lines vs 42
> >> 
> >> No need for enum type
> >> No need for global variable
> >> 
> >> What do you think?
> >
> > Yes, as I used to reply to Phil I think it can be simplified.  I did this
> > major for (1) better readability, and (2) being crystal clear on which way
> > we used to open /dev/userfaultfd, then guarantee we're keeping using it. so
> > at least I prefer keeping things like trace_uffd_detect_open_mode().
> 
> The trace is ok for me.  I just forgot to copy it on the rework, sorry.
> 
> > I also plan to add another mode when fd-mode is there even if it'll reuse
> > the same USERFAULTFD_IOC_NEW; they can be useful information when a failure
> > happens.
> 
> The other fd mode will change the uffd.
> 
> What I *kind* of object is:
> - Using a global variable when it is not needed
>   i.e. for me using a global variable means that anything else is worse.
>   Not the case IMHO.

IMHO globals are evil when they're used in multiple places; that's bad to
readability.  Here it's not the case because it's set once and for all.  I
wanted to have an easy and clear way to peek what's the mode chosen even
without tracing enabled (e.g. from a dump or a live process).

> - Call uffd_open_mode() for every call, when we know that it can change,
>   it is going to return always the same value, so cache it.

uffd_detect_open_mode() caches the result already?  Or maybe you meant
something else?

> 
> > Though if you insist, I can switch to the simple version too.
> 
> I always told that the person who did the patch has the last word on
> style.  I preffer my version, but it is up to you to take it or not.

Thanks,

-- 
Peter Xu