Re: [PATCH v4 4/4] ioctl_userfaultfd.2: Add write-protect mode docs - Alejandro Colomar (man-pages)

From: "Alejandro Colomar (man-pages)" <alx.manpages@gmail.com>
To: Peter Xu <peterx@redhat.com>
Cc: linux-man@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org, Mike Rapoport <rppt@linux.vnet.ibm.com>,
	Andrea Arcangeli <aarcange@redhat.com>,
	Axel Rasmussen <axelrasmussen@google.com>,
	Michael Kerrisk <mtk.manpages@gmail.com>,
	Nadav Amit <nadav.amit@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH v4 4/4] ioctl_userfaultfd.2: Add write-protect mode docs
Date: Thu, 25 Mar 2021 22:32:20 +0100	[thread overview]
Message-ID: <dbc37834-9fb1-ca44-7ed5-9f8f6cc53741@gmail.com> (raw)
In-Reply-To: <20210323191618.GJ6486@xz-x1>

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/
http://www.alejandro-colomar.es/