Re: userfaultfd: usability issue due to lack of UFFD events ordering

[Nadav Amit <nadav.amit@gmail.com>]

> On Jan 31, 2022, at 10:47 AM, Mike Rapoport <rppt@kernel.org> wrote:
> 
> On Mon, Jan 31, 2022 at 03:41:05PM +0100, David Hildenbrand wrote:
>> On 31.01.22 15:28, Mike Rapoport wrote:
>>> On Mon, Jan 31, 2022 at 03:12:36PM +0100, David Hildenbrand wrote:
>>>> On 31.01.22 15:05, Mike Rapoport wrote:
>>>>> On Mon, Jan 31, 2022 at 11:48:27AM +0100, David Hildenbrand wrote:
>>>>>> On 31.01.22 11:42, Mike Rapoport wrote:
>>>>>>> Hi Nadav,
>>>>>>> 
>>>>>>> On Sat, Jan 29, 2022 at 10:23:55PM -0800, Nadav Amit wrote:
>>>>>>>> Using userfautlfd and looking at the kernel code, I encountered a usability
>>>>>>>> issue that complicates userspace UFFD-monitor implementation. I obviosuly
>>>>>>>> might be wrong, so I would appreciate a (polite?) feedback. I do have a
>>>>>>>> userspace workaround, but I thought it is worthy to share and to hear your
>>>>>>>> opinion, as well as feedback from other UFFD users.
>>>>>>>> 
>>>>>>>> The issue I encountered regards the ordering of UFFD events tbat might not
>>>>>>>> reflect the actual order in which events took place.
>>>>>>>> 
>>>>>>>> In more detail, UFFD events (e.g., unmap, fork) are not ordered against
>>>>>>>> themselves [*]. The mm-lock is dropped before notifying the userspace
>>>>>>>> UFFD-monitor, and therefore there is no guarantee as to whether the order of
>>>>>>>> the events actually reflects the order in which the events took place.
>>>>>>>> This can prevent a UFFD-monitor from using the events to track which
>>>>>>>> ranges are mapped. Specifically, UFFD_EVENT_FORK message and a
>>>>>>>> UFFD_EVENT_UNMAP message (which reflects unmap in the parent process) can
>>>>>>>> be reordered, if the events are triggered by two different threads. In
>>>>>>>> this case the UFFD-monitor cannot figure from the events whether the
>>>>>>>> child process has the unmapped memory range still mapped (because fork
>>>>>>>> happened first) or not.
>>>>>>> 
>>>>>>> Yeah, it seems that something like this is possible:
>>>>>>> 
>>>>>>> 
>>>>>>> fork()					munmap()
>>>>>>> 	mmap_write_unlock();
>>>>>>> 						mmap_write_lock_killable();
>>>>>>> 						do_things();
>>>>>>> 						mmap_{read,write}_unlock();
>>>>>>> 						userfaultfd_unmap_complete();
>>>>>>> 	dup_userfaultfd_complete();
>>>>>>> 
>>>>>> 
>>>>>> I was thinking about other possible races, e.g., MADV_DONTNEED/MADV_FREE
>>>>>> racing with UFFD_EVENT_PAGEFAULT -- where we only hold the mmap_lock in
>>>>>> read mode. But not sure if they apply.
>>>>> 
>>>>> The userspace can live with these, at least for uffd missing page faults.
>>>>> If the monitor will try to resolve a page fault for a removed area, the
>>>>> errno from UFFDIO_COPY/ZERO can be used to detect such races.
>>>> 
>>>> I was wondering if the monitor could get confused if he just resolved a
>>>> page fault via UFFDIO_COPY/ZERO and then receives a REMOVE event.
>>> 
>>> And why would it be confused?
>> 
>> My thinking was that the monitor might use REMOVE events to track which
>> pages are actually populated. If you receive REMOVE after
>> UFFDIO_COPY/ZERO the monitor would conclude that the page is not
>> populated, just like if we'd get the MADV_DONTNEED/MADV_REMOVE
>> immediately after placing a page.
> 
> I still don't follow your usecase.
> 
> In CRIU we simply discard whatever content we had to fill when there is
> REMOVE event. If a page fault occurs in that region we use UFFDIO_ZEROPAGE,
> just as it would happen in "normal" page fault processing 
> (note, CRIU does not support uffd with hugetlb or shmem)

I think that the point that David makes is valid.

There are use-cases in which you do need to know the order between
user-initiated MADV_DONTNEED and page-faults. For instance, if you
build a userspace paging mechanism, you need to know whether the
page content is zero or whatever is held in the disk.

I presume mmap_changing was designed for a similar purpose, assuming
that if you had a page-fault that started before MADV_DONTNEED, and
you try to serve it using copy-ioctl, the copy would fail. I think
that this works only if you assume that there is a single UFFD
monitor thread (that reads the uffd and issues appropriate ioctl’s),
and that all operations are performed synchronously (which I am
trying to avoid using io-uring).

Otherwise, a copy ioctl that is initiated before MADV_DONTNEED
(to resolve page-fault) can take place after the userspace was 
already notified of UFFD_EVENT_REMOVE (i.e., mmap_changing==0),
and there is no way to cancel the copy that was initiated. As
a result, following MADV_DONTNEED, the memory would not be zeroed.

As for me, I decided that due to the lack of ordering, I just
cannot use the UFFD events, and I have to rely on ptrace to obtain
order of these events. I might be wrong, but any solution is not
trivial and is likely to require API changes.