diff mbox series

userfaultfd: wake on unregister for minor faults as well as missing

Message ID 20221025182149.3076870-1-axelrasmussen@google.com
State New
Headers
Series userfaultfd: wake on unregister for minor faults as well as missing |

Commit Message

Axel Rasmussen Oct. 25, 2022, 6:21 p.m. UTC 
  This was an overlooked edge case when minor faults were added. In
general, minor faults have the same rough edge here as missing faults:
if we unregister while there are waiting threads, they will just remain
waiting forever, as there is no way for userspace to wake them after
unregistration. To work around this, userspace needs to carefully wake
everything before unregistering.

So, wake for minor faults just like we already do for missing faults as
part of the unregistration process.

Cc: stable@vger.kernel.org
Fixes: 7677f7fd8be7 ("userfaultfd: add minor fault registration mode")
Reported-by: Lokesh Gidra <lokeshgidra@google.com>
Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>
---
 fs/userfaultfd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Peter Xu Oct. 25, 2022, 6:34 p.m. UTC | #1 
On Tue, Oct 25, 2022 at 11:21:49AM -0700, Axel Rasmussen wrote:
> This was an overlooked edge case when minor faults were added. In
> general, minor faults have the same rough edge here as missing faults:
> if we unregister while there are waiting threads, they will just remain
> waiting forever, as there is no way for userspace to wake them after
> unregistration. To work around this, userspace needs to carefully wake
> everything before unregistering.
> 
> So, wake for minor faults just like we already do for missing faults as
> part of the unregistration process.
> 
> Cc: stable@vger.kernel.org
> Fixes: 7677f7fd8be7 ("userfaultfd: add minor fault registration mode")
> Reported-by: Lokesh Gidra <lokeshgidra@google.com>
> Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>
> ---
>  fs/userfaultfd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index 07c81ab3fd4d..7daee4b9481c 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -1606,7 +1606,7 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
>  			start = vma->vm_start;
>  		vma_end = min(end, vma->vm_end);
>  
> -		if (userfaultfd_missing(vma)) {
> +		if (userfaultfd_missing(vma) || userfaultfd_minor(vma)) {
>  			/*
>  			 * Wake any concurrent pending userfault while
>  			 * we unregister, so they will not hang
> -- 
> 2.38.0.135.g90850a2211-goog

Thanks, Axel.  Is wr-protect mode also prone to this?  Would a test case
help too?
Axel Rasmussen Oct. 25, 2022, 6:40 p.m. UTC | #2 
On Tue, Oct 25, 2022 at 11:34 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Tue, Oct 25, 2022 at 11:21:49AM -0700, Axel Rasmussen wrote:
> > This was an overlooked edge case when minor faults were added. In
> > general, minor faults have the same rough edge here as missing faults:
> > if we unregister while there are waiting threads, they will just remain
> > waiting forever, as there is no way for userspace to wake them after
> > unregistration. To work around this, userspace needs to carefully wake
> > everything before unregistering.
> >
> > So, wake for minor faults just like we already do for missing faults as
> > part of the unregistration process.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 7677f7fd8be7 ("userfaultfd: add minor fault registration mode")
> > Reported-by: Lokesh Gidra <lokeshgidra@google.com>
> > Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>
> > ---
> >  fs/userfaultfd.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> > index 07c81ab3fd4d..7daee4b9481c 100644
> > --- a/fs/userfaultfd.c
> > +++ b/fs/userfaultfd.c
> > @@ -1606,7 +1606,7 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
> >                       start = vma->vm_start;
> >               vma_end = min(end, vma->vm_end);
> >
> > -             if (userfaultfd_missing(vma)) {
> > +             if (userfaultfd_missing(vma) || userfaultfd_minor(vma)) {
> >                       /*
> >                        * Wake any concurrent pending userfault while
> >                        * we unregister, so they will not hang
> > --
> > 2.38.0.135.g90850a2211-goog
>
> Thanks, Axel.  Is wr-protect mode also prone to this?  Would a test case
> help too?

I'm not quite as familiar with uffd-wp, but I think so? At minimum, it
seems like waking can't *hurt*, and it would simplify the check
slightly -- if (userfaultfd_armed(vma)) {}

It would also mean if we add yet another registration mode in the
future, we wouldn't forget to update this.

I'll send a v2 to address both points.

>
> --
> Peter Xu
>
Lokesh Gidra Oct. 25, 2022, 6:44 p.m. UTC | #3 
Thanks for fixing this issue.

On Tue, Oct 25, 2022 at 11:21 AM Axel Rasmussen
<axelrasmussen@google.com> wrote:
>
> This was an overlooked edge case when minor faults were added. In
> general, minor faults have the same rough edge here as missing faults:
> if we unregister while there are waiting threads, they will just remain
> waiting forever, as there is no way for userspace to wake them after
> unregistration. To work around this, userspace needs to carefully wake
> everything before unregistering.
Actually, WAKE ioctl doesn't check if the provided range is registered
with userfaultfd or not. So, it's possible to wake the waiting threads
after unregisteration.

But, in this context, missing faults are the same as minor faults (and
wp too?). Therefore, waking the waiting threads as part of
unregistration is expected.
>
> So, wake for minor faults just like we already do for missing faults as
> part of the unregistration process.
>
> Cc: stable@vger.kernel.org
> Fixes: 7677f7fd8be7 ("userfaultfd: add minor fault registration mode")
> Reported-by: Lokesh Gidra <lokeshgidra@google.com>
> Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>
> ---
>  fs/userfaultfd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index 07c81ab3fd4d..7daee4b9481c 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -1606,7 +1606,7 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
>                         start = vma->vm_start;
>                 vma_end = min(end, vma->vm_end);
>
> -               if (userfaultfd_missing(vma)) {
> +               if (userfaultfd_missing(vma) || userfaultfd_minor(vma)) {
>                         /*
>                          * Wake any concurrent pending userfault while
>                          * we unregister, so they will not hang
> --
> 2.38.0.135.g90850a2211-goog
>
diff mbox series

Patch

diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 07c81ab3fd4d..7daee4b9481c 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -1606,7 +1606,7 @@  static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
 			start = vma->vm_start;
 		vma_end = min(end, vma->vm_end);
 
-		if (userfaultfd_missing(vma)) {
+		if (userfaultfd_missing(vma) || userfaultfd_minor(vma)) {
 			/*
 			 * Wake any concurrent pending userfault while
 			 * we unregister, so they will not hang