On 2/16/23 9:41 PM, Peter Xu wrote:
> On Thu, Feb 16, 2023 at 11:25:51AM +0500, Muhammad Usama Anjum wrote:
>> On 2/16/23 2:45 AM, Peter Xu wrote:
>>> On Wed, Feb 15, 2023 at 12:08:11PM +0500, Muhammad Usama Anjum wrote:
>>>> Hi Peter,
>>>>
>>>> Thank you for your review!
>>>>
>>>> On 2/15/23 2:50 AM, Peter Xu wrote:
>>>>> On Tue, Feb 14, 2023 at 01:49:50PM +0500, Muhammad Usama Anjum wrote:
>>>>>> On 2/14/23 2:11 AM, Peter Xu wrote:
>>>>>>> On Mon, Feb 13, 2023 at 10:50:39PM +0500, Muhammad Usama Anjum wrote:
>>>>>>>> On 2/13/23 9:54 PM, Peter Xu wrote:
>>>>>>>>> On Mon, Feb 13, 2023 at 09:31:23PM +0500, Muhammad Usama Anjum wrote:
>>>>>>>>>> mwriteprotect_range() errors out if [start, end) doesn't fall in one
>>>>>>>>>> VMA. We are facing a use case where multiple VMAs are present in one
>>>>>>>>>> range of interest. For example, the following pseudocode reproduces the
>>>>>>>>>> error which we are trying to fix:
>>>>>>>>>>
>>>>>>>>>> - Allocate memory of size 16 pages with PROT_NONE with mmap
>>>>>>>>>> - Register userfaultfd
>>>>>>>>>> - Change protection of the first half (1 to 8 pages) of memory to
>>>>>>>>>> PROT_READ | PROT_WRITE. This breaks the memory area in two VMAs.
>>>>>>>>>> - Now UFFDIO_WRITEPROTECT_MODE_WP on the whole memory of 16 pages errors
>>>>>>>>>> out.
>>>>>>>>>>
>>>>>>>>>> This is a simple use case where user may or may not know if the memory
>>>>>>>>>> area has been divided into multiple VMAs.
>>>>>>>>>>
>>>>>>>>>> Reported-by: Paul Gofman <pgofman@xxxxxxxxxxxxxxx>
>>>>>>>>>> Signed-off-by: Muhammad Usama Anjum <usama.anjum@xxxxxxxxxxxxx>
>>>>>>>>>> ---
>>>>>>>>>> Changes since v1:
>>>>>>>>>> - Correct the start and ending values passed to uffd_wp_range()
>>>>>>>>>> ---
>>>>>>>>>> mm/userfaultfd.c | 38 ++++++++++++++++++++++----------------
>>>>>>>>>> 1 file changed, 22 insertions(+), 16 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
>>>>>>>>>> index 65ad172add27..bccea08005a8 100644
>>>>>>>>>> --- a/mm/userfaultfd.c
>>>>>>>>>> +++ b/mm/userfaultfd.c
>>>>>>>>>> @@ -738,9 +738,12 @@ int mwriteprotect_range(struct mm_struct *dst_mm, unsigned long start,
>>>>>>>>>> unsigned long len, bool enable_wp,
>>>>>>>>>> atomic_t *mmap_changing)
>>>>>>>>>> {
>>>>>>>>>> + unsigned long end = start + len;
>>>>>>>>>> + unsigned long _start, _end;
>>>>>>>>>> struct vm_area_struct *dst_vma;
>>>>>>>>>> unsigned long page_mask;
>>>>>>>>>> int err;
>>>>>>>>>
>>>>>>>>> I think this needs to be initialized or it can return anything when range
>>>>>>>>> not mapped.
>>>>>>>> It is being initialized to -EAGAIN already. It is not visible in this patch.
>>>>>>>
>>>>>>> I see, though -EAGAIN doesn't look suitable at all. The old retcode for
>>>>>>> !vma case is -ENOENT, so I think we'd better keep using it if we want to
>>>>>>> have this patch.
>>>>>> I'll update in next version.
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> + VMA_ITERATOR(vmi, dst_mm, start);
>>>>>>>>>>
>>>>>>>>>> /*
>>>>>>>>>> * Sanitize the command parameters:
>>>>>>>>>> @@ -762,26 +765,29 @@ int mwriteprotect_range(struct mm_struct *dst_mm, unsigned long start,
>>>>>>>>>> if (mmap_changing && atomic_read(mmap_changing))
>>>>>>>>>> goto out_unlock;
>>>>>>>>>>
>>>>>>>>>> - err = -ENOENT;
>>>>>>>>>> - dst_vma = find_dst_vma(dst_mm, start, len);
>>>>>>>>>> + for_each_vma_range(vmi, dst_vma, end) {
>>>>>>>>>> + err = -ENOENT;
>>>>>>>>>>
>>>>>>>>>> - if (!dst_vma)
>>>>>>>>>> - goto out_unlock;
>>>>>>>>>> - if (!userfaultfd_wp(dst_vma))
>>>>>>>>>> - goto out_unlock;
>>>>>>>>>> - if (!vma_can_userfault(dst_vma, dst_vma->vm_flags))
>>>>>>>>>> - goto out_unlock;
>>>>>>>>>> + if (!dst_vma->vm_userfaultfd_ctx.ctx)
>>>>>>>>>> + break;
>>>>>>>>>> + if (!userfaultfd_wp(dst_vma))
>>>>>>>>>> + break;
>>>>>>>>>> + if (!vma_can_userfault(dst_vma, dst_vma->vm_flags))
>>>>>>>>>> + break;
>>>>>>>>>>
>>>>>>>>>> - if (is_vm_hugetlb_page(dst_vma)) {
>>>>>>>>>> - err = -EINVAL;
>>>>>>>>>> - page_mask = vma_kernel_pagesize(dst_vma) - 1;
>>>>>>>>>> - if ((start & page_mask) || (len & page_mask))
>>>>>>>>>> - goto out_unlock;
>>>>>>>>>> - }
>>>>>>>>>> + if (is_vm_hugetlb_page(dst_vma)) {
>>>>>>>>>> + err = -EINVAL;
>>>>>>>>>> + page_mask = vma_kernel_pagesize(dst_vma) - 1;
>>>>>>>>>> + if ((start & page_mask) || (len & page_mask))
>>>>>>>>>> + break;
>>>>>>>>>> + }
>>>>>>>>>>
>>>>>>>>>> - uffd_wp_range(dst_mm, dst_vma, start, len, enable_wp);
>>>>>>>>>> + _start = (dst_vma->vm_start > start) ? dst_vma->vm_start : start;
>>>>>>>>>> + _end = (dst_vma->vm_end < end) ? dst_vma->vm_end : end;
>>>>>>>>>>
>>>>>>>>>> - err = 0;
>>>>>>>>>> + uffd_wp_range(dst_mm, dst_vma, _start, _end - _start, enable_wp);
>>>>>>>>>> + err = 0;
>>>>>>>>>> + }
>>>>>>>>>> out_unlock:
>>>>>>>>>> mmap_read_unlock(dst_mm);
>>>>>>>>>> return err;
>>>>>>>>>
>>>>>>>>> This whole patch also changes the abi, so I'm worried whether there can be
>>>>>>>>> app that relies on the existing behavior.
>>>>>>>> Even if a app is dependent on it, this change would just don't return error
>>>>>>>> if there are multiple VMAs under the hood and handle them correctly. Most
>>>>>>>> apps wouldn't care about VMAs anyways. I don't know if there would be any
>>>>>>>> drastic behavior change, other than the behavior becoming nicer.
>>>>>>>
>>>>>>> So this logic existed since the initial version of uffd-wp. It has a good
>>>>>>> thing that it strictly checks everything and it makes sense since uffd-wp
>>>>>>> is per-vma attribute. In short, the old code fails clearly.
>>>>>>>
>>>>>>> While the new proposal is not: if -ENOENT we really have no idea what
>>>>>>> happened at all; some ranges can be wr-protected but we don't know where
>>>>>>> starts to go wrong.
>>>>>> The return error codes can be made to return in better way somewhat. The
>>>>>> return error codes shouldn't block a correct functionality enhancement patch.
>>>>>>
>>>>>>>
>>>>>>> Now I'm looking at the original problem..
>>>>>>>
>>>>>>> - Allocate memory of size 16 pages with PROT_NONE with mmap
>>>>>>> - Register userfaultfd
>>>>>>> - Change protection of the first half (1 to 8 pages) of memory to
>>>>>>> PROT_READ | PROT_WRITE. This breaks the memory area in two VMAs.
>>>>>>> - Now UFFDIO_WRITEPROTECT_MODE_WP on the whole memory of 16 pages errors
>>>>>>> out.
>>>>>>>
>>>>>>> Why the user app should wr-protect 16 pages at all?
>>>>>> Taking arguments from Paul here.
>>>>>>
>>>>>> The app is free to insert guard pages inside the range (with PROT_NONE) and
>>>>>> change the protection of memory freely. Not sure why it is needed to
>>>>>> complicate things by denying any flexibility. We should never restrict what
>>>>>> is possible and what not. All of these different access attributes and
>>>>>> their any combination of interaction _must_ work without question. The
>>>>>> application should be free to change protection on any sub-range and it
>>>>>> shouldn't break the PAGE_IS_WRITTEN + UFFD_WRITE_PROTECT promise which
>>>>>> PAGEMAP_IOCTL (patches are in progress) and UFFD makes.
>>>>>
>>>>> Because uffd-wp has a limitation on e.g. it cannot nest so far. I'm fine
>>>>> with allowing mprotect() happening, but just to mention so far it cannot do
>>>>> "any combinations" yet.
>>>>>
>>>>>>
>>>>>>>
>>>>>>> If so, uffd_wp_range() will be ran upon a PROT_NONE range which doesn't
>>>>>>> make sense at all, no matter whether the user is aware of vma concept or
>>>>>>> not... because it's destined that it's a vain effort.
>>>>>> It is not a vain effort. The user want to watch/find the dirty pages of a
>>>>>> range while working with it: reserve and watch at once while Write
>>>>>> protecting or un-protecting as needed. There may be several different use
>>>>>> cases. Inserting guard pages to catch out of range access, map something
>>>>>> only when it is needed; unmap or PROT_NONE pages when they are set free in
>>>>>> the app etc.
>>>>>
>>>>> Fair enough.
>>>>>
>>>>>>
>>>>>>>
>>>>>>> So IMHO it's the user app needs fixing here, not the interface? I think
>>>>>>> it's the matter of whether the monitor is aware of mprotect() being
>>>>>>> invoked.
>>>>>> No. The common practice is to allocate a big memory chunk at once and have
>>>>>> own allocator over it (whether it is some specific allocator in a game or a
>>>>>> .net allocator with garbage collector). From the usage point of view it is
>>>>>> very limiting to demand constant memory attributes for the whole range.
>>>>>>
>>>>>> That said, if we do have the way to do exactly what we want with reset
>>>>>> through pagemap fd and it is just UFFD ioctl will be working differently,
>>>>>> it is not a blocker of course, just weird api design.
>>>>>
>>>>> Do you mean you'll disable ENGAGE_WP && !GET in your other series? Yes, if
>>>>> this will service your goal, it'll be perfect to remove that interface.
>>>> No, we cannot remove it.
>>>
>>> If this patch can land, I assume ioctl(UFFDIO_WP) can start to service the
>>> dirty tracking purpose, then why do you still need "ENGAGE_WP && !GET"?
>> We don't need it. We need the following operations only:
>> 1 GET
>> 2 ENGAGE_WP + GET
>> When we have these two operations, we had added the following as well:
>> 3 ENGAGE_WP + !GET or only ENGAGE_WP
>> This (3) can be removed from ioctl(PAGEMAP_IOCTL) if reviewers ask. I can
>> remove it in favour of this ioctl(UFFDIO_WP) patch for sure.
>
> Yes I prefer not having it if this works, because then they'll be
> completely duplicated.
I'll remove it from next version.
>
>>
>>>
>>> Note, I'm not asking to drop ENGAGE_WP entirely, only when !GET.
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> In short, I hope we're working on things that helps at least someone, and
>>>>>>> we should avoid working on things that does not have clear benefit yet.
>>>>>>> With the WP_ENGAGE new interface being proposed, I just didn't see any
>>>>>>> benefit of changing the current interface, especially if the change can
>>>>>>> bring uncertainties itself (e.g., should we fail upon !uffd-wp vmas, or
>>>>>>> should we skip?).
>>>>>> We can work on solving uncertainties in case of error conditions. Fail if
>>>>>> !uffd-wp vma comes.
>>>>>
>>>>> Let me try to double check with you here:
>>>>>
>>>>> I assume you want to skip any vma that is not mapped at all, as the loop
>>>>> already does so. So it'll succeed if there're memory holes.
>>>>>
>>>>> You also want to explicitly fail if some vma is not registered with uffd-wp
>>>>> when walking the vma list, am I right? IOW, the tracee _won't_ ever have a
>>>>> chance to unregister uffd-wp itself, right?
>>>> Yes, fail if any VMA doesn't have uffd-wp. This fail means the
>>>> write-protection or un-protection failed on a region of memory with error
>>>> -ENOENT. This is already happening in this current patch. The unregister
>>>> code would remain same. The register and unregister ioctls are already
>>>> going over all the VMAs in a range. I'm not rigid on anything. Let me
>>>> define the interface below.
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Is this for the new pagemap effort? Can this just be done in the new
>>>>>>>>> interface rather than changing the old?
>>>>>>>> We found this bug while working on pagemap patches. It is already being
>>>>>>>> handled in the new interface. We just thought that this use case can happen
>>>>>>>> pretty easily and unknowingly. So the support should be added.
>>>>>>>
>>>>>>> Thanks. My understanding is that it would have been reported if it
>>>>>>> affected any existing uffd-wp user.
>>>>>> I would consider the UFFD WP a recent functionality and it may not being
>>>>>> used in wide range of app scenarios.
>>>>>
>>>>> Yes I think so.
>>>>>
>>>>> Existing users should in most cases be applying the ioctl upon valid vmas
>>>>> somehow. I think the chance is low that someone relies on the errcode to
>>>>> make other decisions, but I just cannot really tell because the user app
>>>>> can do many weird things.
>>>> Correct. The user can use any combination of operation
>>>> (mmap/mprotect/uffd). They must work in harmony.
>>>
>>> No uffd - that's exactly what I'm saying: mprotect is fine here, but uffd
>>> is probably not, not in a nested way. When you try to UFFDIO_REGISTER upon
>>> some range that already been registered (by the tracee), it'll fail for you
>>> immediately:
>>>
>>> /*
>>> * Check that this vma isn't already owned by a
>>> * different userfaultfd. We can't allow more than one
>>> * userfaultfd to own a single vma simultaneously or we
>>> * wouldn't know which one to deliver the userfaults to.
>>> */
>>> ret = -EBUSY;
>>> if (cur->vm_userfaultfd_ctx.ctx &&
>>> cur->vm_userfaultfd_ctx.ctx != ctx)
>>> goto out_unlock;
>>>
>>> So if this won't work for you, then AFAICT uffd-wp won't work for you (just
>>> like soft-dirty but in another way, sorry), at least not until someone
>>> starts to work on the nested.
>> I was referring to a case where user registers the WP on multiple VMAs and
>> all the VMAs haven't been registered before. It would work. Right?
>
> But what if the user wants to do that during when you're tracing it using
> userfaultfd? Can it happen?
Sorry, I've not tested tracing.
>
> Thanks,
>
--
BR,
Muhammad Usama Anjum
