userfaultfd: check VM_MAYWRITE was set after verifying the uffd is registered
Subsystems (2)
cover userfaultfd: check VM_MAYWRITE was set after verifying the uffd is registered
Hello,
The juxtaposition with the other bugchecks didn't make it apparent that the this WARN_ON was one line too soon.
An app trying to unregister a not-yet-registered range will trigger an _harmless_ false positive WARN_ON. No real app would do that so it went unnoticed during testing.
This should be applied on top of 29ec90660d68 ("userfaultfd:
shmem/hugetlbfs: only allow to register VM_MAYWRITE vmas") to shut off the false positive warning.
Thanks,
Andrea
Andrea Arcangeli (1):
userfaultfd: check VM_MAYWRITE was set after verifying the uffd is
registered
fs/userfaultfd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
PATCH 1/1 userfaultfd: check VM_MAYWRITE was set after verifying the uffd is registered
Calling UFFDIO_UNREGISTER on virtual ranges not yet registered in uffd could trigger an harmless false positive WARN_ON. Check the vma is already registered before checking VM_MAYWRITE to shut off the false positive warning.
--- fs/userfaultfd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
fs/userfaultfd.c +2 -1
@@ -1566,7 +1566,6 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,cond_resched ();BUG_ON (!vma_can_userfault (vma)); -WARN_ON (!(vma->vm_flags &VM_MAYWRITE ));/* * Nothing to do: this vma is already registered into this
@@ -1575,6 +1574,8 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,if (!vma->vm_userfaultfd_ctx.ctx) goto skip; +WARN_ON (!(vma->vm_flags &VM_MAYWRITE )); +if (vma->vm_start > start) start = vma->vm_start; vma_end =min (end, vma->vm_end);
Quoted 9 lines · Andrea Arcangeli
Calling
UFFDIO_UNREGISTERon virtual ranges not yet registered in uffd could trigger an harmless false positiveWARN_ON. Check the vma is already registered before checkingVM_MAYWRITEto shut off the false positive warning.Cc <[email protected]>Fixes 29ec90660d68 ("userfaultfd: shmem/hugetlbfs: only allow to register VM_MAYWRITE vmas")Reported-by [email protected]Signed-off-by Andrea Arcangeli <[email protected]>
Quoted 26 lines
--- fs/userfaultfd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)fs/userfaultfd.c +2
-1@@ -1566,7 +1566,6 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,cond_resched ();BUG_ON (!vma_can_userfault (vma)); -WARN_ON (!(vma->vm_flags &VM_MAYWRITE ));/* * Nothing to do: this vma is already registered into this @@ -1575,6 +1574,8 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,if (!vma->vm_userfaultfd_ctx.ctx) goto skip; +WARN_ON (!(vma->vm_flags &VM_MAYWRITE )); +if (vma->vm_start > start) start = vma->vm_start; vma_end =min (end, vma->vm_end);
Signature · 2 lines
Sincerely yours, Mike.
Quoted 9 lines · Andrea Arcangeli
Calling
UFFDIO_UNREGISTERon virtual ranges not yet registered in uffd could trigger an harmless false positiveWARN_ON. Check the vma is already registered before checkingVM_MAYWRITEto shut off the false positive warning.Cc <[email protected]>Fixes 29ec90660d68 ("userfaultfd: shmem/hugetlbfs: only allow to register VM_MAYWRITE vmas")Reported-by [email protected]Signed-off-by Andrea Arcangeli <[email protected]>
Quoted 26 lines
--- fs/userfaultfd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)fs/userfaultfd.c +2
-1@@ -1566,7 +1566,6 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,cond_resched ();BUG_ON (!vma_can_userfault (vma)); -WARN_ON (!(vma->vm_flags &VM_MAYWRITE ));/* * Nothing to do: this vma is already registered into this @@ -1575,6 +1574,8 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,if (!vma->vm_userfaultfd_ctx.ctx) goto skip; +WARN_ON (!(vma->vm_flags &VM_MAYWRITE )); +if (vma->vm_start > start) start = vma->vm_start; vma_end =min (end, vma->vm_end);
Quoted 18 lines · Andrea Arcangeli
Calling
UFFDIO_UNREGISTERon virtual ranges not yet registered in uffd could trigger an harmless false positiveWARN_ON. Check the vma is already registered before checkingVM_MAYWRITEto shut off the false positive warning.Cc <[email protected]>Fixes 29ec90660d68 ("userfaultfd: shmem/hugetlbfs: only allow to register VM_MAYWRITE vmas")Reported-by [email protected]Signed-off-by Andrea Arcangeli <[email protected]>--- fs/userfaultfd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)fs/userfaultfd.c +0
-0
fs/userfaultfd.c static int userfaultfd_unregister(struct userfaultfd_ctx *ctx, · line 1571
Quoted 7 lines
fs/userfaultfd.c +0
-1@@ -1566,7 +1566,6 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,cond_resched ();BUG_ON (!vma_can_userfault (vma)); -WARN_ON (!(vma->vm_flags &VM_MAYWRITE ));/* * Nothing to do: this vma is already registered into this
Maybe we want to fix this comment too some day:
/* * Nothing to do: this vma is already registered into this * userfaultfd and with the right tracking mode too. */
But I don't think it's anything urgent since it's clear it means the other way round and it can potentially be touched up in any further cleanup/fixes of uffd.
fs/userfaultfd.c static int userfaultfd_unregister(struct userfaultfd_ctx *ctx, · line 1581
Quoted 9 lines
fs/userfaultfd.c +2
-0@@ -1575,6 +1574,8 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,if (!vma->vm_userfaultfd_ctx.ctx) goto skip; +WARN_ON (!(vma->vm_flags &VM_MAYWRITE )); +if (vma->vm_start > start) start = vma->vm_start; vma_end =min (end, vma->vm_end);
Thanks,
Signature · 1 line
Peter Xu