Re: [PATCH v4 4/4] ioctl_userfaultfd.2: Add write-protect mode docs

Alejandro Colomar (man-pages)
2021-03-25T21:32:26Z

Hi Peter,

On 3/23/21 8:16 PM, Peter Xu wrote:
> On Tue, Mar 23, 2021 at 07:11:04PM +0100, Alejandro Colomar (man-pages) wrote:
>>> +.TP
>>> +.B UFFDIO_COPY_MODE_WP
>>> +Copy the page with read-only permission.
>>> +This allows the user to trap the next write to the page,
>>> +which will block and generate another write-protect userfault message.
>>
>> s/write-protect/write-protected/
>> ?
>
> I think here "write-protect" is the wording I wanted to use, it is the name of
> the type of the message in plain text.

Okay.

>
> [...]
>
>>> +.B EAGAIN
>>> +The process was interrupted and need to retry.
>>
>> Maybe: "The process was interrupted; retry this call."?
>> I don't know what other pager say about this kind of error.
>
> Frankly I see no difference between the two.. If you prefer the latter, I can
> switch.

I understand yours, but technically it's a bit incorrect: The subject
of the sentence changes: in "The process was interrupted" it's the
process, and in "need to retry" it's [you]. By separating the sentence
into two, it's more natural. :)

>
>>
>>> +.TP
>>> +.B ENOENT
>>> +The range specified in
>>> +.I range
>>> +is not valid.
>>
>> I'm not sure how this is different from the wording above in EINVAL. An
>> "otherwise invalid range" was already giving EINVAL?
>
> This can be returned when vma is not found (mwriteprotect_range()):
>
> err = -ENOENT;
> dst_vma = find_dst_vma(dst_mm, start, len);
>
> if (!dst_vma)
> goto out_unlock;
>
> I think maybe I could simply remove this entry, because from an user app
> developer pov I'd only be interested in specific error that I'd be able to
> detect and (even better) recover from. For such error I'd say there's not much
> to do besides failing the app.

If there's any possibility that the error can happen, it should be
documented, even if it's to say "Fatal error; abort!". Just try to
explain the causes and how to avoid causing them and/or possibly what to
do when they happen (abort?).

>
>>
>>> +For example, the virtual address does not exist,
>>> +or not registered with userfaultfd write-protect mode.
>>> +.TP
>>> +.B EFAULT
>>> +Encountered a generic fault during processing.
>>
>> What is a "generic fault"?
>
> For example when the user copy failed due to some reason. See
> userfaultfd_writeprotect():
>
> if (copy_from_user(&uffdio_wp, user_uffdio_wp,
> sizeof(struct uffdio_writeprotect)))
> return -EFAULT;
>
> But I didn't check other places, generally I'd return -EFAULT if I can't find a
> proper other replacement which has a clearer meaning.
>
> I don't think this is really helpful to user app too because no user app would
> start to read this -EFAULT to do anything useful.. how about I drop it too if
> you think the description is confusing?

Same as above.

Thanks,

Alex

--
Alejandro Colomar
Linux man-pages comaintainer; https://www.kernel.org/doc/man-pages/