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

[Peter Xu <peterx@redhat.com>]

On Tue, Mar 23, 2021 at 07:11:04PM +0100, Alejandro Colomar (man-pages) wrote:
> Hi Peter,

Hi, Alex,

[...]

> > +.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.

[...]

> > +.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.

> 
> > +.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.

> 
> > +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?

Thanks,

-- 
Peter Xu