All of lore.kernel.org
 help / color / mirror / Atom feed
From: Catalin Marinas <catalin.marinas@arm.com>
To: Jianyong Wu <Jianyong.Wu@arm.com>
Cc: Anshuman Khandual <Anshuman.Khandual@arm.com>,
	"will@kernel.org" <will@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	 "maz@kernel.org" <maz@kernel.org>,
	"ardb@kernel.org" <ardb@kernel.org>,
	"david@redhat.com" <david@redhat.com>,
	"gshan@redhat.com" <gshan@redhat.com>,
	Justin He <Justin.He@arm.com>, nd <nd@arm.com>
Subject: Re: [PATCH v1] arm64/mm: avoid race condition of update page table when kernel init
Date: Fri, 3 Dec 2021 17:44:11 +0000	[thread overview]
Message-ID: <YapXa8JWPNhkePwO@arm.com> (raw)
In-Reply-To: <AM9PR08MB72767A6DFA5A7ED8117E7C44F4869@AM9PR08MB7276.eurprd08.prod.outlook.com>

On Thu, Oct 28, 2021 at 08:36:07AM +0100, Jianyong Wu wrote:
> From Anshuman Khandual <anshuman.khandual@arm.com>:
> > On 10/27/21 3:18 PM, Jianyong Wu wrote:
> > > Race condition of page table update can happen in kernel boot period
> > > as both of memory hotplug action when kernel init and the
> > > mark_rodata_ro can update page table. For virtio-mem, the function excute flow chart is:
> > >
> > > -------------------------
> > > kernel_init
> > >   kernel_init_freeable
> > >     ...
> > >       do_initcall
> > >         ...
> > >           module_init [A]
> > >
> > >   ...
> > >   mark_readonly
> > >     mark_rodata_ro [B]
> > > -------------------------
[...]
> > > We can see that the error derived from the l3 translation as the pte
> > > value is *0*. That is because the fixmap has been clear when access.
> > >
> > > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> > > ---
> > >  arch/arm64/mm/mmu.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c index
> > > cfd9deb347c3..567dfba8f08a 100644
> > > --- a/arch/arm64/mm/mmu.c
> > > +++ b/arch/arm64/mm/mmu.c
> > > @@ -564,8 +564,10 @@ void mark_rodata_ro(void)
> > >      * to cover NOTES and EXCEPTION_TABLE.
> > >      */
> > >     section_size = (unsigned long)__init_begin - (unsigned long)__start_rodata;
> > > +   get_online_mems();
> > >     update_mapping_prot(__pa_symbol(__start_rodata), (unsigned long)__start_rodata,
> > >                         section_size, PAGE_KERNEL_RO);
> > > +   put_online_mems();
> > >
> > >     debug_checkwx();
> > >  }
> >
> > While this should solve the current problem i.e race between concurrent
> > memory hotplug operation and mark_rodata_ro(), but I am still wondering
> > whether this is the fix at the right place and granularity. Basically a hotplug
> > operation queued in an work queue at [A] can execute during [B] is the root
> > cause of this problem.
> 
> Not exactly, this issue doesn't only happen at the the *pure* kernel
> boot. For example, hotplug memory through VM monitor when VM boot. We
> can't foresee when that happen. Thus, this issue can affect all kinds
> of memory hotplug mechanism, including ACPI based memory hotplug and
> virtio-mem. I'm not sure that fix it here is the best way. If the race
> only happens between kernel init and memory hotplug, I think it's fine
> to fix it here. IMO, this issue results from the race for "fixmap"
> resource. I wonder why this global resource is not protected by a
> lock. Maybe we can add one and fix it there.

IIUC the race is caused by multiple attempts to use the fixmap at the
same time. We can add a fixmap_lock and hold it during
__create_pgd_mapping().

-- 
Catalin

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

WARNING: multiple messages have this Message-ID (diff)
From: Catalin Marinas <catalin.marinas@arm.com>
To: Jianyong Wu <Jianyong.Wu@arm.com>
Cc: Anshuman Khandual <Anshuman.Khandual@arm.com>,
	"will@kernel.org" <will@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"maz@kernel.org" <maz@kernel.org>,
	"ardb@kernel.org" <ardb@kernel.org>,
	"david@redhat.com" <david@redhat.com>,
	"gshan@redhat.com" <gshan@redhat.com>,
	Justin He <Justin.He@arm.com>, nd <nd@arm.com>
Subject: Re: [PATCH v1] arm64/mm: avoid race condition of update page table when kernel init
Date: Fri, 3 Dec 2021 17:44:11 +0000	[thread overview]
Message-ID: <YapXa8JWPNhkePwO@arm.com> (raw)
In-Reply-To: <AM9PR08MB72767A6DFA5A7ED8117E7C44F4869@AM9PR08MB7276.eurprd08.prod.outlook.com>

On Thu, Oct 28, 2021 at 08:36:07AM +0100, Jianyong Wu wrote:
> From Anshuman Khandual <anshuman.khandual@arm.com>:
> > On 10/27/21 3:18 PM, Jianyong Wu wrote:
> > > Race condition of page table update can happen in kernel boot period
> > > as both of memory hotplug action when kernel init and the
> > > mark_rodata_ro can update page table. For virtio-mem, the function excute flow chart is:
> > >
> > > -------------------------
> > > kernel_init
> > >   kernel_init_freeable
> > >     ...
> > >       do_initcall
> > >         ...
> > >           module_init [A]
> > >
> > >   ...
> > >   mark_readonly
> > >     mark_rodata_ro [B]
> > > -------------------------
[...]
> > > We can see that the error derived from the l3 translation as the pte
> > > value is *0*. That is because the fixmap has been clear when access.
> > >
> > > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> > > ---
> > >  arch/arm64/mm/mmu.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c index
> > > cfd9deb347c3..567dfba8f08a 100644
> > > --- a/arch/arm64/mm/mmu.c
> > > +++ b/arch/arm64/mm/mmu.c
> > > @@ -564,8 +564,10 @@ void mark_rodata_ro(void)
> > >      * to cover NOTES and EXCEPTION_TABLE.
> > >      */
> > >     section_size = (unsigned long)__init_begin - (unsigned long)__start_rodata;
> > > +   get_online_mems();
> > >     update_mapping_prot(__pa_symbol(__start_rodata), (unsigned long)__start_rodata,
> > >                         section_size, PAGE_KERNEL_RO);
> > > +   put_online_mems();
> > >
> > >     debug_checkwx();
> > >  }
> >
> > While this should solve the current problem i.e race between concurrent
> > memory hotplug operation and mark_rodata_ro(), but I am still wondering
> > whether this is the fix at the right place and granularity. Basically a hotplug
> > operation queued in an work queue at [A] can execute during [B] is the root
> > cause of this problem.
> 
> Not exactly, this issue doesn't only happen at the the *pure* kernel
> boot. For example, hotplug memory through VM monitor when VM boot. We
> can't foresee when that happen. Thus, this issue can affect all kinds
> of memory hotplug mechanism, including ACPI based memory hotplug and
> virtio-mem. I'm not sure that fix it here is the best way. If the race
> only happens between kernel init and memory hotplug, I think it's fine
> to fix it here. IMO, this issue results from the race for "fixmap"
> resource. I wonder why this global resource is not protected by a
> lock. Maybe we can add one and fix it there.

IIUC the race is caused by multiple attempts to use the fixmap at the
same time. We can add a fixmap_lock and hold it during
__create_pgd_mapping().

-- 
Catalin

  parent reply	other threads:[~2021-12-03 17:45 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-27  9:48 [PATCH v1] arm64/mm: avoid race condition of update page table when kernel init Jianyong Wu
2021-10-27  9:48 ` Jianyong Wu
2021-10-28  5:57 ` Anshuman Khandual
2021-10-28  5:57   ` Anshuman Khandual
2021-10-28  7:04   ` David Hildenbrand
2021-10-28  7:04     ` David Hildenbrand
2021-10-28  7:36   ` Jianyong Wu
2021-10-28  7:36     ` Jianyong Wu
2021-11-15  2:07     ` Jianyong Wu
2021-11-15  2:07       ` Jianyong Wu
2021-12-03 17:44     ` Catalin Marinas [this message]
2021-12-03 17:44       ` Catalin Marinas
2021-12-03 18:13       ` David Hildenbrand
2021-12-03 18:13         ` David Hildenbrand
2021-12-06 15:14         ` Catalin Marinas
2021-12-06 15:14           ` Catalin Marinas
2021-12-06  2:11       ` Jianyong Wu
2021-12-06  2:11         ` Jianyong Wu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YapXa8JWPNhkePwO@arm.com \
    --to=catalin.marinas@arm.com \
    --cc=Anshuman.Khandual@arm.com \
    --cc=Jianyong.Wu@arm.com \
    --cc=Justin.He@arm.com \
    --cc=ardb@kernel.org \
    --cc=david@redhat.com \
    --cc=gshan@redhat.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maz@kernel.org \
    --cc=nd@arm.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.