[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 3/3] util/userfaultfd: Support /dev/userfaultfd
From: |
Juan Quintela |
Subject: |
Re: [PATCH v2 3/3] util/userfaultfd: Support /dev/userfaultfd |
Date: |
Fri, 03 Feb 2023 22:01:04 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) |
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.
[PATCH v2 2/3] util/userfaultfd: Add uffd_open(), Peter Xu, 2023/02/01