[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 3/3] util/userfaultfd: Support /dev/userfaultfd
From: |
Peter Xu |
Subject: |
Re: [PATCH v2 3/3] util/userfaultfd: Support /dev/userfaultfd |
Date: |
Mon, 6 Feb 2023 16:31:00 -0500 |
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
[PATCH v2 2/3] util/userfaultfd: Add uffd_open(), Peter Xu, 2023/02/01