On Tue, Jan 31, 2023 at 10:54:22AM -0800, Suren Baghdasaryan wrote:

On Tue, Jan 31, 2023 at 12:32 AM Hyeonggon Yoo [off-list ref] wrote:

quoted

On Thu, Jan 26, 2023 at 11:37:49AM -0800, Suren Baghdasaryan wrote:

quoted

Replace direct modifications to vma->vm_flags with calls to modifier
functions to be able to track flag changes and to keep vma locking
correctness.

Signed-off-by: Suren Baghdasaryan <surenb@google.com>
Acked-by: Michal Hocko <mhocko@suse.com>
Acked-by: Mel Gorman <redacted>
Acked-by: Mike Rapoport (IBM) <rppt@kernel.org>
Acked-by: Sebastian Reichel <redacted>
---
 arch/arm/kernel/process.c                          |  2 +-
 120 files changed, 188 insertions(+), 199 deletions(-)

Hello Suren,

Hi Hyeonggon,

quoted

[...]

Whoa, it's so long.
Mostly looks fine but two things I'm not sure about:

quoted

diff --git a/drivers/misc/open-dice.c b/drivers/misc/open-dice.c
index 9dda47b3fd70..7be4e6c9f120 100644
--- a/drivers/misc/open-dice.c
+++ b/drivers/misc/open-dice.c

@@ -95,12 +95,12 @@ static int open_dice_mmap(struct file *filp, struct vm_area_struct *vma)
              if (vma->vm_flags & VM_WRITE)
                      return -EPERM;
              /* Ensure userspace cannot acquire VM_WRITE later. */
-             vma->vm_flags &= ~VM_MAYWRITE;
+             vm_flags_clear(vma, VM_MAYSHARE);
      }

I think it should be:
        s/VM_MAYSHARE/VM_MAYWRITE/

Good eye! Yes, this is definitely a bug. Will post a next version with this fix.

quoted

quoted

diff --git a/mm/mlock.c b/mm/mlock.c
index 5c4fff93cd6b..ed49459e343e 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c

@@ -380,7 +380,7 @@ static void mlock_vma_pages_range(struct vm_area_struct *vma,
       */
      if (newflags & VM_LOCKED)
              newflags |= VM_IO;
-     WRITE_ONCE(vma->vm_flags, newflags);
+     vm_flags_reset(vma, newflags);

      lru_add_drain();
      walk_page_range(vma->vm_mm, start, end, &mlock_walk_ops, NULL);

@@ -388,7 +388,7 @@ static void mlock_vma_pages_range(struct vm_area_struct *vma,

      if (newflags & VM_IO) {
              newflags &= ~VM_IO;
-             WRITE_ONCE(vma->vm_flags, newflags);
+             vm_flags_reset(vma, newflags);
      }
 }

wondering the if the comment above is still true?

        /*
         * There is a slight chance that concurrent page migration,
         * or page reclaim finding a page of this now-VM_LOCKED vma,
         * will call mlock_vma_folio() and raise page's mlock_count:
         * double counting, leaving the page unevictable indefinitely.
         * Communicate this danger to mlock_vma_folio() with VM_IO,
         * which is a VM_SPECIAL flag not allowed on VM_LOCKED vmas.
         * mmap_lock is held in write mode here, so this weird
         * combination should not be visible to other mmap_lock users;
         * but WRITE_ONCE so rmap walkers must see VM_IO if VM_LOCKED.
         */

does ACCESS_PRIVATE() still guarentee that compiler cannot mysteriously
optimize writes like WRITE_ONCE()?

I don't see ACCESS_PRIVATE() providing the same guarantees as
WRITE_ONCE(), therefore I think this also needs to be changed. I'll
need to introduce something like vm_flags_reset_once() and use it
here. vm_flags_reset_once() would do WRITE_ONCE() and otherwise would
be identical to vm_flags_reset().

I'll post a new version with the fixes later today.

Thanks for the review!
Suren.

Thanks for quick reply!

Andrew's fix and the new patch looks good to me.
with these two things addressed:

Reviewed-by: Hyeonggon Yoo <redacted>


Regards,
Hyeonggon

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel