* [PATCH] fs/userfaultfd.c: simplify the calculation of new_flags @ 2019-08-06 5:38 Wei Yang 2019-09-12 2:54 ` Wei Yang 2019-10-03 0:45 ` Andrea Arcangeli 0 siblings, 2 replies; 7+ messages in thread From: Wei Yang @ 2019-08-06 5:38 UTC (permalink / raw) To: linux-kernel; +Cc: viro, linux-fsdevel, Wei Yang Finally new_flags equals old vm_flags *OR* vm_flags. It is not necessary to mask them first. Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> --- fs/userfaultfd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c index ccbdbd62f0d8..653d8f7c453c 100644 --- a/fs/userfaultfd.c +++ b/fs/userfaultfd.c @@ -1457,7 +1457,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx, start = vma->vm_start; vma_end = min(end, vma->vm_end); - new_flags = (vma->vm_flags & ~vm_flags) | vm_flags; + new_flags = vma->vm_flags | vm_flags; prev = vma_merge(mm, prev, start, vma_end, new_flags, vma->anon_vma, vma->vm_file, vma->vm_pgoff, vma_policy(vma), -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] fs/userfaultfd.c: simplify the calculation of new_flags 2019-08-06 5:38 [PATCH] fs/userfaultfd.c: simplify the calculation of new_flags Wei Yang @ 2019-09-12 2:54 ` Wei Yang 2019-10-03 0:45 ` Andrea Arcangeli 1 sibling, 0 replies; 7+ messages in thread From: Wei Yang @ 2019-09-12 2:54 UTC (permalink / raw) To: Wei Yang; +Cc: linux-kernel, viro, linux-fsdevel Ping~ On Tue, Aug 06, 2019 at 01:38:59PM +0800, Wei Yang wrote: >Finally new_flags equals old vm_flags *OR* vm_flags. > >It is not necessary to mask them first. > >Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> >--- > fs/userfaultfd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c >index ccbdbd62f0d8..653d8f7c453c 100644 >--- a/fs/userfaultfd.c >+++ b/fs/userfaultfd.c >@@ -1457,7 +1457,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx, > start = vma->vm_start; > vma_end = min(end, vma->vm_end); > >- new_flags = (vma->vm_flags & ~vm_flags) | vm_flags; >+ new_flags = vma->vm_flags | vm_flags; > prev = vma_merge(mm, prev, start, vma_end, new_flags, > vma->anon_vma, vma->vm_file, vma->vm_pgoff, > vma_policy(vma), >-- >2.17.1 -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fs/userfaultfd.c: simplify the calculation of new_flags 2019-08-06 5:38 [PATCH] fs/userfaultfd.c: simplify the calculation of new_flags Wei Yang 2019-09-12 2:54 ` Wei Yang @ 2019-10-03 0:45 ` Andrea Arcangeli 2019-10-04 22:46 ` Wei Yang 1 sibling, 1 reply; 7+ messages in thread From: Andrea Arcangeli @ 2019-10-03 0:45 UTC (permalink / raw) To: Wei Yang; +Cc: linux-kernel, viro, linux-fsdevel, Peter Xu Hello, On Tue, Aug 06, 2019 at 01:38:59PM +0800, Wei Yang wrote: > Finally new_flags equals old vm_flags *OR* vm_flags. > > It is not necessary to mask them first. > > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> > --- > fs/userfaultfd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c > index ccbdbd62f0d8..653d8f7c453c 100644 > --- a/fs/userfaultfd.c > +++ b/fs/userfaultfd.c > @@ -1457,7 +1457,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx, > start = vma->vm_start; > vma_end = min(end, vma->vm_end); > > - new_flags = (vma->vm_flags & ~vm_flags) | vm_flags; > + new_flags = vma->vm_flags | vm_flags; > prev = vma_merge(mm, prev, start, vma_end, new_flags, > vma->anon_vma, vma->vm_file, vma->vm_pgoff, > vma_policy(vma), And then how do you clear the flags after the above? It must be possible to clear the flags (from UFFDIO_REGISTER_MODE_MISSING|UFFDIO_REGISTER_MODE_WP to only one set or invert). We have no WP support upstream yet, so maybe that's why it looks superfluous in practice, but in theory it isn't because it would then need to be reversed by Peter's (CC'ed) -wp patchset. The register code has already the right placeholder to support -wp and so it's better not to break them. I would recommend reviewing the uffd-wp support and working on testing the uffd-wp code instead of changing the above. Thanks, Andrea ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fs/userfaultfd.c: simplify the calculation of new_flags 2019-10-03 0:45 ` Andrea Arcangeli @ 2019-10-04 22:46 ` Wei Yang 2019-10-04 23:28 ` Andrea Arcangeli 0 siblings, 1 reply; 7+ messages in thread From: Wei Yang @ 2019-10-04 22:46 UTC (permalink / raw) To: Andrea Arcangeli; +Cc: Wei Yang, linux-kernel, viro, linux-fsdevel, Peter Xu On Wed, Oct 02, 2019 at 08:45:05PM -0400, Andrea Arcangeli wrote: >Hello, > >On Tue, Aug 06, 2019 at 01:38:59PM +0800, Wei Yang wrote: >> Finally new_flags equals old vm_flags *OR* vm_flags. >> >> It is not necessary to mask them first. >> >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> >> --- >> fs/userfaultfd.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c >> index ccbdbd62f0d8..653d8f7c453c 100644 >> --- a/fs/userfaultfd.c >> +++ b/fs/userfaultfd.c >> @@ -1457,7 +1457,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx, >> start = vma->vm_start; >> vma_end = min(end, vma->vm_end); >> >> - new_flags = (vma->vm_flags & ~vm_flags) | vm_flags; >> + new_flags = vma->vm_flags | vm_flags; >> prev = vma_merge(mm, prev, start, vma_end, new_flags, >> vma->anon_vma, vma->vm_file, vma->vm_pgoff, >> vma_policy(vma), > >And then how do you clear the flags after the above? > >It must be possible to clear the flags (from >UFFDIO_REGISTER_MODE_MISSING|UFFDIO_REGISTER_MODE_WP to only one set >or invert). > >We have no WP support upstream yet, so maybe that's why it looks >superfluous in practice, but in theory it isn't because it would then >need to be reversed by Peter's (CC'ed) -wp patchset. > >The register code has already the right placeholder to support -wp and >so it's better not to break them. > >I would recommend reviewing the uffd-wp support and working on testing >the uffd-wp code instead of changing the above. > Sorry, I don't get your point. This change is valid to me even from arithmetic point of view. vm_flags == VM_UFFD_MISSING | VM_UFFD_WP The effect of current code is clear these two bits then add them. This equals to just add these two bits. I am not sure which part I lost. >Thanks, >Andrea -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fs/userfaultfd.c: simplify the calculation of new_flags 2019-10-04 22:46 ` Wei Yang @ 2019-10-04 23:28 ` Andrea Arcangeli 2019-10-04 23:38 ` Wei Yang 2019-10-09 3:51 ` Peter Xu 0 siblings, 2 replies; 7+ messages in thread From: Andrea Arcangeli @ 2019-10-04 23:28 UTC (permalink / raw) To: Wei Yang; +Cc: linux-kernel, viro, linux-fsdevel, Peter Xu On Sat, Oct 05, 2019 at 06:46:40AM +0800, Wei Yang wrote: > On Wed, Oct 02, 2019 at 08:45:05PM -0400, Andrea Arcangeli wrote: > >Hello, > > > >On Tue, Aug 06, 2019 at 01:38:59PM +0800, Wei Yang wrote: > >> Finally new_flags equals old vm_flags *OR* vm_flags. > >> > >> It is not necessary to mask them first. > >> > >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> > >> --- > >> fs/userfaultfd.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c > >> index ccbdbd62f0d8..653d8f7c453c 100644 > >> --- a/fs/userfaultfd.c > >> +++ b/fs/userfaultfd.c > >> @@ -1457,7 +1457,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx, > >> start = vma->vm_start; > >> vma_end = min(end, vma->vm_end); > >> > >> - new_flags = (vma->vm_flags & ~vm_flags) | vm_flags; > >> + new_flags = vma->vm_flags | vm_flags; > >> prev = vma_merge(mm, prev, start, vma_end, new_flags, > >> vma->anon_vma, vma->vm_file, vma->vm_pgoff, > >> vma_policy(vma), > > > >And then how do you clear the flags after the above? > > > >It must be possible to clear the flags (from > >UFFDIO_REGISTER_MODE_MISSING|UFFDIO_REGISTER_MODE_WP to only one set > >or invert). > > > >We have no WP support upstream yet, so maybe that's why it looks > >superfluous in practice, but in theory it isn't because it would then > >need to be reversed by Peter's (CC'ed) -wp patchset. > > > >The register code has already the right placeholder to support -wp and > >so it's better not to break them. > > > >I would recommend reviewing the uffd-wp support and working on testing > >the uffd-wp code instead of changing the above. > > > > Sorry, I don't get your point. This change is valid to me even from arithmetic > point of view. > > vm_flags == VM_UFFD_MISSING | VM_UFFD_WP > > The effect of current code is clear these two bits then add them. This equals > to just add these two bits. > > I am not sure which part I lost. The cleaned removed the "& ~" and that was enough to quickly tell the cleaned up version was wrong. What I should have noticed right away as well is that the code was already wrong, sorry. That code doesn't require a noop code cleanup, it requires a fix and the "& ~" needs to stay. This isn't going to make any difference upstream until the uffd-wp support is merged so it is enough to queue it in Peter's queue, or you can merge it independently. Thanks, Andrea From a0f17bef184c6bb9b99294f202eefb50b6eb43cd Mon Sep 17 00:00:00 2001 From: Andrea Arcangeli <aarcange@redhat.com> Date: Fri, 4 Oct 2019 19:09:59 -0400 Subject: [PATCH 1/1] uffd: wp: clear VM_UFFD_MISSING or VM_UFFD_WP during userfaultfd_register() If the registration is repeated without VM_UFFD_MISSING or VM_UFFD_WP they need to be cleared. Currently setting UFFDIO_REGISTER_MODE_WP returns -EINVAL, so this patch is a noop until the UFFDIO_REGISTER_MODE_WP support is applied. Reported-by: Wei Yang <richardw.yang@linux.intel.com> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com> --- fs/userfaultfd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c index fe6d804a38dc..97596bb65dd5 100644 --- a/fs/userfaultfd.c +++ b/fs/userfaultfd.c @@ -1458,7 +1458,8 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx, start = vma->vm_start; vma_end = min(end, vma->vm_end); - new_flags = (vma->vm_flags & ~vm_flags) | vm_flags; + new_flags = (vma->vm_flags & + ~(VM_UFFD_MISSING|VM_UFFD_WP)) | vm_flags; prev = vma_merge(mm, prev, start, vma_end, new_flags, vma->anon_vma, vma->vm_file, vma->vm_pgoff, vma_policy(vma), ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] fs/userfaultfd.c: simplify the calculation of new_flags 2019-10-04 23:28 ` Andrea Arcangeli @ 2019-10-04 23:38 ` Wei Yang 2019-10-09 3:51 ` Peter Xu 1 sibling, 0 replies; 7+ messages in thread From: Wei Yang @ 2019-10-04 23:38 UTC (permalink / raw) To: Andrea Arcangeli; +Cc: Wei Yang, linux-kernel, viro, linux-fsdevel, Peter Xu On Fri, Oct 04, 2019 at 07:28:34PM -0400, Andrea Arcangeli wrote: >On Sat, Oct 05, 2019 at 06:46:40AM +0800, Wei Yang wrote: >> On Wed, Oct 02, 2019 at 08:45:05PM -0400, Andrea Arcangeli wrote: >> >Hello, >> > >> >On Tue, Aug 06, 2019 at 01:38:59PM +0800, Wei Yang wrote: >> >> Finally new_flags equals old vm_flags *OR* vm_flags. >> >> >> >> It is not necessary to mask them first. >> >> >> >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> >> >> --- >> >> fs/userfaultfd.c | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c >> >> index ccbdbd62f0d8..653d8f7c453c 100644 >> >> --- a/fs/userfaultfd.c >> >> +++ b/fs/userfaultfd.c >> >> @@ -1457,7 +1457,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx, >> >> start = vma->vm_start; >> >> vma_end = min(end, vma->vm_end); >> >> >> >> - new_flags = (vma->vm_flags & ~vm_flags) | vm_flags; >> >> + new_flags = vma->vm_flags | vm_flags; >> >> prev = vma_merge(mm, prev, start, vma_end, new_flags, >> >> vma->anon_vma, vma->vm_file, vma->vm_pgoff, >> >> vma_policy(vma), >> > >> >And then how do you clear the flags after the above? >> > >> >It must be possible to clear the flags (from >> >UFFDIO_REGISTER_MODE_MISSING|UFFDIO_REGISTER_MODE_WP to only one set >> >or invert). >> > >> >We have no WP support upstream yet, so maybe that's why it looks >> >superfluous in practice, but in theory it isn't because it would then >> >need to be reversed by Peter's (CC'ed) -wp patchset. >> > >> >The register code has already the right placeholder to support -wp and >> >so it's better not to break them. >> > >> >I would recommend reviewing the uffd-wp support and working on testing >> >the uffd-wp code instead of changing the above. >> > >> >> Sorry, I don't get your point. This change is valid to me even from arithmetic >> point of view. >> >> vm_flags == VM_UFFD_MISSING | VM_UFFD_WP >> >> The effect of current code is clear these two bits then add them. This equals >> to just add these two bits. >> >> I am not sure which part I lost. > >The cleaned removed the "& ~" and that was enough to quickly tell the >cleaned up version was wrong. > >What I should have noticed right away as well is that the code was >already wrong, sorry. That code doesn't require a noop code cleanup, >it requires a fix and the "& ~" needs to stay. > >This isn't going to make any difference upstream until the uffd-wp >support is merged so it is enough to queue it in Peter's queue, or you >can merge it independently. > ok, I get your point. >Thanks, >Andrea > >>From a0f17bef184c6bb9b99294f202eefb50b6eb43cd Mon Sep 17 00:00:00 2001 >From: Andrea Arcangeli <aarcange@redhat.com> >Date: Fri, 4 Oct 2019 19:09:59 -0400 >Subject: [PATCH 1/1] uffd: wp: clear VM_UFFD_MISSING or VM_UFFD_WP during > userfaultfd_register() > >If the registration is repeated without VM_UFFD_MISSING or VM_UFFD_WP >they need to be cleared. Currently setting UFFDIO_REGISTER_MODE_WP >returns -EINVAL, so this patch is a noop until the >UFFDIO_REGISTER_MODE_WP support is applied. > >Reported-by: Wei Yang <richardw.yang@linux.intel.com> >Signed-off-by: Andrea Arcangeli <aarcange@redhat.com> Reviewed-by: Wei Yang <richardw.yang@linux.intel.com> >--- > fs/userfaultfd.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > >diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c >index fe6d804a38dc..97596bb65dd5 100644 >--- a/fs/userfaultfd.c >+++ b/fs/userfaultfd.c >@@ -1458,7 +1458,8 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx, > start = vma->vm_start; > vma_end = min(end, vma->vm_end); > >- new_flags = (vma->vm_flags & ~vm_flags) | vm_flags; >+ new_flags = (vma->vm_flags & >+ ~(VM_UFFD_MISSING|VM_UFFD_WP)) | vm_flags; > prev = vma_merge(mm, prev, start, vma_end, new_flags, > vma->anon_vma, vma->vm_file, vma->vm_pgoff, > vma_policy(vma), -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fs/userfaultfd.c: simplify the calculation of new_flags 2019-10-04 23:28 ` Andrea Arcangeli 2019-10-04 23:38 ` Wei Yang @ 2019-10-09 3:51 ` Peter Xu 1 sibling, 0 replies; 7+ messages in thread From: Peter Xu @ 2019-10-09 3:51 UTC (permalink / raw) To: Andrea Arcangeli; +Cc: Wei Yang, linux-kernel, viro, linux-fsdevel On Fri, Oct 04, 2019 at 07:28:34PM -0400, Andrea Arcangeli wrote: > On Sat, Oct 05, 2019 at 06:46:40AM +0800, Wei Yang wrote: > > On Wed, Oct 02, 2019 at 08:45:05PM -0400, Andrea Arcangeli wrote: > > >Hello, > > > > > >On Tue, Aug 06, 2019 at 01:38:59PM +0800, Wei Yang wrote: > > >> Finally new_flags equals old vm_flags *OR* vm_flags. > > >> > > >> It is not necessary to mask them first. > > >> > > >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> > > >> --- > > >> fs/userfaultfd.c | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c > > >> index ccbdbd62f0d8..653d8f7c453c 100644 > > >> --- a/fs/userfaultfd.c > > >> +++ b/fs/userfaultfd.c > > >> @@ -1457,7 +1457,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx, > > >> start = vma->vm_start; > > >> vma_end = min(end, vma->vm_end); > > >> > > >> - new_flags = (vma->vm_flags & ~vm_flags) | vm_flags; > > >> + new_flags = vma->vm_flags | vm_flags; > > >> prev = vma_merge(mm, prev, start, vma_end, new_flags, > > >> vma->anon_vma, vma->vm_file, vma->vm_pgoff, > > >> vma_policy(vma), > > > > > >And then how do you clear the flags after the above? > > > > > >It must be possible to clear the flags (from > > >UFFDIO_REGISTER_MODE_MISSING|UFFDIO_REGISTER_MODE_WP to only one set > > >or invert). > > > > > >We have no WP support upstream yet, so maybe that's why it looks > > >superfluous in practice, but in theory it isn't because it would then > > >need to be reversed by Peter's (CC'ed) -wp patchset. > > > > > >The register code has already the right placeholder to support -wp and > > >so it's better not to break them. > > > > > >I would recommend reviewing the uffd-wp support and working on testing > > >the uffd-wp code instead of changing the above. > > > > > > > Sorry, I don't get your point. This change is valid to me even from arithmetic > > point of view. > > > > vm_flags == VM_UFFD_MISSING | VM_UFFD_WP > > > > The effect of current code is clear these two bits then add them. This equals > > to just add these two bits. > > > > I am not sure which part I lost. > > The cleaned removed the "& ~" and that was enough to quickly tell the > cleaned up version was wrong. > > What I should have noticed right away as well is that the code was > already wrong, sorry. That code doesn't require a noop code cleanup, > it requires a fix and the "& ~" needs to stay. > > This isn't going to make any difference upstream until the uffd-wp > support is merged so it is enough to queue it in Peter's queue, or you > can merge it independently. IMHO it's good to have it as independent patch so at least it won't confuse another reader of the master branch. But just in case, I've also queued it in my local tree of uffd-wp. Thanks, -- Peter Xu ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-10-09 3:51 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-06 5:38 [PATCH] fs/userfaultfd.c: simplify the calculation of new_flags Wei Yang 2019-09-12 2:54 ` Wei Yang 2019-10-03 0:45 ` Andrea Arcangeli 2019-10-04 22:46 ` Wei Yang 2019-10-04 23:28 ` Andrea Arcangeli 2019-10-04 23:38 ` Wei Yang 2019-10-09 3:51 ` Peter Xu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).