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

[Juan Quintela <quintela@redhat.com>]

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.

>>     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.
- 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.

> 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.

Later, Juan.