| Message ID | 20240626001639.1350646-1-longman@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: Prevent derefencing NULL ptr in pfn_section_valid() | expand |
Hi Waiman, On 6/26/2024 5:46 AM, Waiman Long wrote: > Commit 5ec8e8ea8b77 ("mm/sparsemem: fix race in accessing > memory_section->usage") changed pfn_section_valid() to add a READ_ONCE() > call around "ms->usage" to fix a race with section_deactivate() where > ms->usage can be cleared. The READ_ONCE() call, by itself, is not enough > to prevent NULL pointer dereference. We need to check its value before > dereferencing it. I am unable to see a scenario where ms->usage will be NULL when pfn_section_valid() is called: 1) In pfn_valid, valid_section() check ensures that pfn_section_valid() is not called as the section is marked as invalid. 2) In pfn_to_online_page, online_section() check ensures that pfn_section_valid() is not called. and in the update path, we do: kfree_rcu(ms->usage, rcu); WRITE_ONCE(ms->usage, NULL); Could you help me in understanding about what I am missing here, please? > > Fixes: 5ec8e8ea8b77 ("mm/sparsemem: fix race in accessing memory_section->usage") > Signed-off-by: Waiman Long <longman@redhat.com> > --- > include/linux/mmzone.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h > index 8f9c9590a42c..b1dcf6ddb406 100644 > --- a/include/linux/mmzone.h > +++ b/include/linux/mmzone.h > @@ -1980,8 +1980,9 @@ static inline int subsection_map_index(unsigned long pfn) > static inline int pfn_section_valid(struct mem_section *ms, unsigned long pfn) > { > int idx = subsection_map_index(pfn); > + struct mem_section_usage *usage = READ_ONCE(ms->usage); > > - return test_bit(idx, READ_ONCE(ms->usage)->subsection_map); > + return usage ? test_bit(idx, usage->subsection_map) : 0; > } > #else > static inline int pfn_section_valid(struct mem_section *ms, unsigned long pfn)
On 7/1/24 09:50, Charan Teja Kalla wrote: > Hi Waiman, > > On 6/26/2024 5:46 AM, Waiman Long wrote: >> Commit 5ec8e8ea8b77 ("mm/sparsemem: fix race in accessing >> memory_section->usage") changed pfn_section_valid() to add a READ_ONCE() >> call around "ms->usage" to fix a race with section_deactivate() where >> ms->usage can be cleared. The READ_ONCE() call, by itself, is not enough >> to prevent NULL pointer dereference. We need to check its value before >> dereferencing it. > I am unable to see a scenario where ms->usage will be NULL when > pfn_section_valid() is called: > > 1) In pfn_valid, valid_section() check ensures that pfn_section_valid() > is not called as the section is marked as invalid. > > 2) In pfn_to_online_page, online_section() check ensures that > pfn_section_valid() is not called. > > and in the update path, we do: > kfree_rcu(ms->usage, rcu); > WRITE_ONCE(ms->usage, NULL); > > Could you help me in understanding about what I am missing here, please? > With the below timing sequence: CPU 0 CPU 1 ----- ----- if (!valid_section(ms)) return 0; ms->section_mem_map &= ~SECTION_HAS_MEM_MAP <interrupt> WRITE_ONCE(ms->usage, NULL); READ_ONCE(ms->usage)->subsection_map In the time gap between valid_section() check and accessing ms->usage, it may have been cleared leading to dereferencing a NULL pointer. That is why it will be prudent to do a NULL check first. Regards, Longman
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h index 8f9c9590a42c..b1dcf6ddb406 100644 --- a/include/linux/mmzone.h +++ b/include/linux/mmzone.h @@ -1980,8 +1980,9 @@ static inline int subsection_map_index(unsigned long pfn) static inline int pfn_section_valid(struct mem_section *ms, unsigned long pfn) { int idx = subsection_map_index(pfn); + struct mem_section_usage *usage = READ_ONCE(ms->usage); - return test_bit(idx, READ_ONCE(ms->usage)->subsection_map); + return usage ? test_bit(idx, usage->subsection_map) : 0; } #else static inline int pfn_section_valid(struct mem_section *ms, unsigned long pfn)
Commit 5ec8e8ea8b77 ("mm/sparsemem: fix race in accessing memory_section->usage") changed pfn_section_valid() to add a READ_ONCE() call around "ms->usage" to fix a race with section_deactivate() where ms->usage can be cleared. The READ_ONCE() call, by itself, is not enough to prevent NULL pointer dereference. We need to check its value before dereferencing it. Fixes: 5ec8e8ea8b77 ("mm/sparsemem: fix race in accessing memory_section->usage") Signed-off-by: Waiman Long <longman@redhat.com> --- include/linux/mmzone.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)