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 <[email protected]>
---
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)
--
2.26.2
On Wed, Jun 17, 2020 at 1:38 PM Atish Patra <[email protected]> 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 <[email protected]>
Thanks for the fix.
Reviewed-by: Michel Lespinasse <[email protected]>
On Thu, Jun 18, 2020 at 8:01 AM Michel Lespinasse <[email protected]> wrote:
>
> On Wed, Jun 17, 2020 at 1:38 PM Atish Patra <[email protected]> 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 <[email protected]>
>
> Thanks for the fix.
>
> Reviewed-by: Michel Lespinasse <[email protected]>
It also looks good to me. Thanks for the fix.
Reviewed-by: Zong Li <[email protected]>
On Wed, Jun 17, 2020 at 7:29 PM Zong Li <[email protected]> wrote:
>
> On Thu, Jun 18, 2020 at 8:01 AM Michel Lespinasse <[email protected]> wrote:
> >
> > On Wed, Jun 17, 2020 at 1:38 PM Atish Patra <[email protected]> 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 <[email protected]>
> >
> > Thanks for the fix.
> >
> > Reviewed-by: Michel Lespinasse <[email protected]>
>
> It also looks good to me. Thanks for the fix.
>
> Reviewed-by: Zong Li <[email protected]>
>
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.
--
Regards,
Atish
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 <[email protected]>
> ---
> 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!