| Message ID | 20200617203732.2076611-1-atish.patra@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RISC-V: Acquire mmap lock before invoking walk_page_range | expand |
On Wed, Jun 17, 2020 at 1:38 PM Atish Patra <atish.patra@wdc.com> wrote: > As per walk_page_range documentation, mmap lock should be acquired by the > caller before invoking walk_page_range. mmap_assert_locked gets triggered > without that. The details can be found here. > > http://lists.infradead.org/pipermail/linux-riscv/2020-June/010335.html > > Fixes: 395a21ff859c(riscv: add ARCH_HAS_SET_DIRECT_MAP support) > Signed-off-by: Atish Patra <atish.patra@wdc.com> Thanks for the fix. Reviewed-by: Michel Lespinasse <walken@google.com>
On Thu, Jun 18, 2020 at 8:01 AM Michel Lespinasse <walken@google.com> wrote: > > On Wed, Jun 17, 2020 at 1:38 PM Atish Patra <atish.patra@wdc.com> wrote: > > As per walk_page_range documentation, mmap lock should be acquired by the > > caller before invoking walk_page_range. mmap_assert_locked gets triggered > > without that. The details can be found here. > > > > http://lists.infradead.org/pipermail/linux-riscv/2020-June/010335.html > > > > Fixes: 395a21ff859c(riscv: add ARCH_HAS_SET_DIRECT_MAP support) > > Signed-off-by: Atish Patra <atish.patra@wdc.com> > > Thanks for the fix. > > Reviewed-by: Michel Lespinasse <walken@google.com> It also looks good to me. Thanks for the fix. Reviewed-by: Zong Li <zong.li@sifive.com>
On Wed, Jun 17, 2020 at 7:29 PM Zong Li <zong.li@sifive.com> wrote: > > On Thu, Jun 18, 2020 at 8:01 AM Michel Lespinasse <walken@google.com> wrote: > > > > On Wed, Jun 17, 2020 at 1:38 PM Atish Patra <atish.patra@wdc.com> wrote: > > > As per walk_page_range documentation, mmap lock should be acquired by the > > > caller before invoking walk_page_range. mmap_assert_locked gets triggered > > > without that. The details can be found here. > > > > > > http://lists.infradead.org/pipermail/linux-riscv/2020-June/010335.html > > > > > > Fixes: 395a21ff859c(riscv: add ARCH_HAS_SET_DIRECT_MAP support) > > > Signed-off-by: Atish Patra <atish.patra@wdc.com> > > > > Thanks for the fix. > > > > Reviewed-by: Michel Lespinasse <walken@google.com> > > It also looks good to me. Thanks for the fix. > > Reviewed-by: Zong Li <zong.li@sifive.com> > Hi Palmer, Can you include this one in rc2 PR as well ? Anybody who gets this issue with their rootfs can't use rc1 without turning off DEBUG_VM.
On Wed, 17 Jun 2020 13:37:32 PDT (-0700), Atish Patra wrote: > As per walk_page_range documentation, mmap lock should be acquired by the > caller before invoking walk_page_range. mmap_assert_locked gets triggered > without that. The details can be found here. > > http://lists.infradead.org/pipermail/linux-riscv/2020-June/010335.html > > Fixes: 395a21ff859c(riscv: add ARCH_HAS_SET_DIRECT_MAP support) > Signed-off-by: Atish Patra <atish.patra@wdc.com> > --- > arch/riscv/mm/pageattr.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/arch/riscv/mm/pageattr.c b/arch/riscv/mm/pageattr.c > index ec2c70f84994..289a9a5ea5b5 100644 > --- a/arch/riscv/mm/pageattr.c > +++ b/arch/riscv/mm/pageattr.c > @@ -151,6 +151,7 @@ int set_memory_nx(unsigned long addr, int numpages) > > int set_direct_map_invalid_noflush(struct page *page) > { > + int ret; > unsigned long start = (unsigned long)page_address(page); > unsigned long end = start + PAGE_SIZE; > struct pageattr_masks masks = { > @@ -158,11 +159,16 @@ int set_direct_map_invalid_noflush(struct page *page) > .clear_mask = __pgprot(_PAGE_PRESENT) > }; > > - return walk_page_range(&init_mm, start, end, &pageattr_ops, &masks); > + mmap_read_lock(&init_mm); > + ret = walk_page_range(&init_mm, start, end, &pageattr_ops, &masks); > + mmap_read_unlock(&init_mm); > + > + return ret; > } > > int set_direct_map_default_noflush(struct page *page) > { > + int ret; > unsigned long start = (unsigned long)page_address(page); > unsigned long end = start + PAGE_SIZE; > struct pageattr_masks masks = { > @@ -170,7 +176,11 @@ int set_direct_map_default_noflush(struct page *page) > .clear_mask = __pgprot(0) > }; > > - return walk_page_range(&init_mm, start, end, &pageattr_ops, &masks); > + mmap_read_lock(&init_mm); > + ret = walk_page_range(&init_mm, start, end, &pageattr_ops, &masks); > + mmap_read_unlock(&init_mm); > + > + return ret; > } > > void __kernel_map_pages(struct page *page, int numpages, int enable) +Will, who pointed out that we could avoid the lock by using apply_page_range. Given that the bug doesn't reproduce for me, we don't otherwise use apply_page_range, and the commit is somewhat suspect (I screwed up that PR, and the original patch mentions avoiding caching invalid states) I'm going to just take this as is and add it to the list of things to look at. I've put this on fixes: walk_page_range() directly says you must take the lock and I don't want to wait for pedantic reasons on a boot issue, even if it's one that doesn't show up for me. Thanks!
diff --git a/arch/riscv/mm/pageattr.c b/arch/riscv/mm/pageattr.c index ec2c70f84994..289a9a5ea5b5 100644 --- a/arch/riscv/mm/pageattr.c +++ b/arch/riscv/mm/pageattr.c @@ -151,6 +151,7 @@ int set_memory_nx(unsigned long addr, int numpages) int set_direct_map_invalid_noflush(struct page *page) { + int ret; unsigned long start = (unsigned long)page_address(page); unsigned long end = start + PAGE_SIZE; struct pageattr_masks masks = { @@ -158,11 +159,16 @@ int set_direct_map_invalid_noflush(struct page *page) .clear_mask = __pgprot(_PAGE_PRESENT) }; - return walk_page_range(&init_mm, start, end, &pageattr_ops, &masks); + mmap_read_lock(&init_mm); + ret = walk_page_range(&init_mm, start, end, &pageattr_ops, &masks); + mmap_read_unlock(&init_mm); + + return ret; } int set_direct_map_default_noflush(struct page *page) { + int ret; unsigned long start = (unsigned long)page_address(page); unsigned long end = start + PAGE_SIZE; struct pageattr_masks masks = { @@ -170,7 +176,11 @@ int set_direct_map_default_noflush(struct page *page) .clear_mask = __pgprot(0) }; - return walk_page_range(&init_mm, start, end, &pageattr_ops, &masks); + mmap_read_lock(&init_mm); + ret = walk_page_range(&init_mm, start, end, &pageattr_ops, &masks); + mmap_read_unlock(&init_mm); + + return ret; } void __kernel_map_pages(struct page *page, int numpages, int enable)
As per walk_page_range documentation, mmap lock should be acquired by the caller before invoking walk_page_range. mmap_assert_locked gets triggered without that. The details can be found here. http://lists.infradead.org/pipermail/linux-riscv/2020-June/010335.html Fixes: 395a21ff859c(riscv: add ARCH_HAS_SET_DIRECT_MAP support) Signed-off-by: Atish Patra <atish.patra@wdc.com> --- arch/riscv/mm/pageattr.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)