| Message ID | 20230919190206.388896-9-axelrasmussen@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/10] userfaultfd.2: briefly mention two-step feature handshake process | expand |
Hi Axel, On Tue, Sep 19, 2023 at 12:02:04PM -0700, Axel Rasmussen wrote: > The old FIXME noted that the zeroing was done to differentiate the two > EINVAL cases. It's possible something like this was true historically, > but in current Linux we zero it in *both* EINVAL cases, so this is at > least no longer true. > > After reading the code, I can't determine any clear reason why we zero > it in some cases but not in others. So, some simple advice we can give > userspace is: if an error occurs, treat the contents of the structure as > unspecified. Just re-initialize it before retrying UFFDIO_API again. > > Signed-off-by: Axel Rasmussen <axelrasmussen@google.com> I can't apply this patch due to conflicts (due to not having applied two of the previous ones). Please resend all remaining patches in following revisions of the patch set. The applied ones are here: <https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/log/?h=contrib> It's kind of like Linux's 'next' branch. Cheers, Alex > --- > man2/ioctl_userfaultfd.2 | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/man2/ioctl_userfaultfd.2 b/man2/ioctl_userfaultfd.2 > index 1aa9654be..29dca1f6b 100644 > --- a/man2/ioctl_userfaultfd.2 > +++ b/man2/ioctl_userfaultfd.2 > @@ -272,6 +272,14 @@ operation returns 0 on success. > On error, \-1 is returned and > .I errno > is set to indicate the error. > +If an error occurs, > +the kernel may zero the provided > +.I uffdio_api > +structure. > +The caller should treat its contents as unspecified, > +and reinitialize it before re-attempting another > +.B UFFDIO_API > +call. > Possible errors include: > .TP > .B EFAULT > @@ -305,14 +313,6 @@ twice, > the first time with no features set, > is explicitly allowed > as per the two-step feature detection handshake. > -.\" FIXME In the above error case, the returned 'uffdio_api' structure is > -.\" zeroed out. Why is this done? This should be explained in the manual page. > -.\" > -.\" Mike Rapoport: > -.\" In my understanding the uffdio_api > -.\" structure is zeroed to allow the caller > -.\" to distinguish the reasons for -EINVAL. > -.\" > .SS UFFDIO_REGISTER > (Since Linux 4.3.) > Register a memory address range with the userfaultfd object. > -- > 2.42.0.459.ge4e396fd5e-goog >
On Mon, Sep 25, 2023 at 4:56 PM Alejandro Colomar <alx@kernel.org> wrote: > > Hi Axel, > > On Tue, Sep 19, 2023 at 12:02:04PM -0700, Axel Rasmussen wrote: > > The old FIXME noted that the zeroing was done to differentiate the two > > EINVAL cases. It's possible something like this was true historically, > > but in current Linux we zero it in *both* EINVAL cases, so this is at > > least no longer true. > > > > After reading the code, I can't determine any clear reason why we zero > > it in some cases but not in others. So, some simple advice we can give > > userspace is: if an error occurs, treat the contents of the structure as > > unspecified. Just re-initialize it before retrying UFFDIO_API again. > > > > Signed-off-by: Axel Rasmussen <axelrasmussen@google.com> > > I can't apply this patch due to conflicts (due to not having applied two > of the previous ones). Please resend all remaining patches in following > revisions of the patch set. > > The applied ones are here: > > <https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/log/?h=contrib> > > It's kind of like Linux's 'next' branch. Thanks for the review Alex! I'll fix up the issues noted and send the remaining few patches this week. :) > > Cheers, > Alex > > > --- > > man2/ioctl_userfaultfd.2 | 16 ++++++++-------- > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/man2/ioctl_userfaultfd.2 b/man2/ioctl_userfaultfd.2 > > index 1aa9654be..29dca1f6b 100644 > > --- a/man2/ioctl_userfaultfd.2 > > +++ b/man2/ioctl_userfaultfd.2 > > @@ -272,6 +272,14 @@ operation returns 0 on success. > > On error, \-1 is returned and > > .I errno > > is set to indicate the error. > > +If an error occurs, > > +the kernel may zero the provided > > +.I uffdio_api > > +structure. > > +The caller should treat its contents as unspecified, > > +and reinitialize it before re-attempting another > > +.B UFFDIO_API > > +call. > > Possible errors include: > > .TP > > .B EFAULT > > @@ -305,14 +313,6 @@ twice, > > the first time with no features set, > > is explicitly allowed > > as per the two-step feature detection handshake. > > -.\" FIXME In the above error case, the returned 'uffdio_api' structure is > > -.\" zeroed out. Why is this done? This should be explained in the manual page. > > -.\" > > -.\" Mike Rapoport: > > -.\" In my understanding the uffdio_api > > -.\" structure is zeroed to allow the caller > > -.\" to distinguish the reasons for -EINVAL. > > -.\" > > .SS UFFDIO_REGISTER > > (Since Linux 4.3.) > > Register a memory address range with the userfaultfd object. > > -- > > 2.42.0.459.ge4e396fd5e-goog > >
On Tue, Sep 19, 2023 at 12:02:04PM -0700, Axel Rasmussen wrote: > The old FIXME noted that the zeroing was done to differentiate the two > EINVAL cases. It's possible something like this was true historically, > but in current Linux we zero it in *both* EINVAL cases, so this is at > least no longer true. > > After reading the code, I can't determine any clear reason why we zero > it in some cases but not in others. So, some simple advice we can give > userspace is: if an error occurs, treat the contents of the structure as > unspecified. Just re-initialize it before retrying UFFDIO_API again. In old kernels (e.g. 4.20 and I didn't go to check when this changed) we had two -EINVALS: one when UFFDIO_API was called when state != UFFD_STATE_WAIT_API and another for API version or features mismatch and we zeroed uffd_api struct only in the second case. In the current code the first case does not exits anymore. > Signed-off-by: Axel Rasmussen <axelrasmussen@google.com> Reviewed-by: Mike Rapoport (IBM) <rppt@kernel.org> > --- > man2/ioctl_userfaultfd.2 | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/man2/ioctl_userfaultfd.2 b/man2/ioctl_userfaultfd.2 > index 1aa9654be..29dca1f6b 100644 > --- a/man2/ioctl_userfaultfd.2 > +++ b/man2/ioctl_userfaultfd.2 > @@ -272,6 +272,14 @@ operation returns 0 on success. > On error, \-1 is returned and > .I errno > is set to indicate the error. > +If an error occurs, > +the kernel may zero the provided > +.I uffdio_api > +structure. > +The caller should treat its contents as unspecified, > +and reinitialize it before re-attempting another > +.B UFFDIO_API > +call. > Possible errors include: > .TP > .B EFAULT > @@ -305,14 +313,6 @@ twice, > the first time with no features set, > is explicitly allowed > as per the two-step feature detection handshake. > -.\" FIXME In the above error case, the returned 'uffdio_api' structure is > -.\" zeroed out. Why is this done? This should be explained in the manual page. > -.\" > -.\" Mike Rapoport: > -.\" In my understanding the uffdio_api > -.\" structure is zeroed to allow the caller > -.\" to distinguish the reasons for -EINVAL. > -.\" > .SS UFFDIO_REGISTER > (Since Linux 4.3.) > Register a memory address range with the userfaultfd object. > -- > 2.42.0.459.ge4e396fd5e-goog > >
diff --git a/man2/ioctl_userfaultfd.2 b/man2/ioctl_userfaultfd.2 index 1aa9654be..29dca1f6b 100644 --- a/man2/ioctl_userfaultfd.2 +++ b/man2/ioctl_userfaultfd.2 @@ -272,6 +272,14 @@ operation returns 0 on success. On error, \-1 is returned and .I errno is set to indicate the error. +If an error occurs, +the kernel may zero the provided +.I uffdio_api +structure. +The caller should treat its contents as unspecified, +and reinitialize it before re-attempting another +.B UFFDIO_API +call. Possible errors include: .TP .B EFAULT @@ -305,14 +313,6 @@ twice, the first time with no features set, is explicitly allowed as per the two-step feature detection handshake. -.\" FIXME In the above error case, the returned 'uffdio_api' structure is -.\" zeroed out. Why is this done? This should be explained in the manual page. -.\" -.\" Mike Rapoport: -.\" In my understanding the uffdio_api -.\" structure is zeroed to allow the caller -.\" to distinguish the reasons for -EINVAL. -.\" .SS UFFDIO_REGISTER (Since Linux 4.3.) Register a memory address range with the userfaultfd object.
The old FIXME noted that the zeroing was done to differentiate the two EINVAL cases. It's possible something like this was true historically, but in current Linux we zero it in *both* EINVAL cases, so this is at least no longer true. After reading the code, I can't determine any clear reason why we zero it in some cases but not in others. So, some simple advice we can give userspace is: if an error occurs, treat the contents of the structure as unspecified. Just re-initialize it before retrying UFFDIO_API again. Signed-off-by: Axel Rasmussen <axelrasmussen@google.com> --- man2/ioctl_userfaultfd.2 | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)