Re: [mm] f9ce0be71d: BUG:KCSAN:data-race_in_next_uptodate_page/next_uptodate_page
From: Marco Elver
Date: Tue Aug 31 2021 - 11:47:51 EST
On Tue, Aug 31, 2021 at 03:07PM +0100, Will Deacon wrote:
> On Tue, Aug 31, 2021 at 03:38:17PM +0200, Marco Elver wrote:
> > On Tue, 31 Aug 2021 at 15:13, Will Deacon <will@xxxxxxxxxx> wrote:
> > > > > [ 184.717904][ T1873] ==================================================================
> > > > > [ 184.718938][ T1873] BUG: KCSAN: data-race in next_uptodate_page / unlock_page
> > > > > [ 184.719828][ T1873]
> > > > > [ 184.720103][ T1873] write (marked) to 0xffffea00050f37c0 of 8 bytes by task 1872 on cpu 1:
> > > > > [ 184.721024][ T1873] unlock_page+0x102/0x1b0
> > > > > [ 184.721533][ T1873] filemap_map_pages+0x6c6/0x890
> > > > > [ 184.722102][ T1873] handle_mm_fault+0x179c/0x27f0
> > > > > [ 184.722672][ T1873] do_user_addr_fault+0x3fb/0x830
> > > > > [ 184.723263][ T1873] exc_page_fault+0xc3/0x1a0
> > > > > [ 184.723845][ T1873] asm_exc_page_fault+0x1e/0x30
> > > > > [ 184.724427][ T1873]
> > > > > [ 184.724720][ T1873] read to 0xffffea00050f37c0 of 8 bytes by task 1873 on cpu 0:
> > > > > [ 184.725575][ T1873] next_uptodate_page+0x456/0x830
> > > > > [ 184.726161][ T1873] filemap_map_pages+0x728/0x890
> > > > > [ 184.726747][ T1873] handle_mm_fault+0x179c/0x27f0
> > > > > [ 184.727332][ T1873] do_user_addr_fault+0x3fb/0x830
> > > > > [ 184.727905][ T1873] exc_page_fault+0xc3/0x1a0
> > > > > [ 184.728440][ T1873] asm_exc_page_fault+0x1e/0x30
> > > > > [ 184.729027][ T1873]
> > > > > [ 184.729313][ T1873] Reported by Kernel Concurrency Sanitizer on:
> > > > > [ 184.730019][ T1873] CPU: 0 PID: 1873 Comm: systemd-udevd Not tainted 5.11.0-rc4-00001-gf9ce0be71d1f #1
> > > > > [ 184.731103][ T1873] ==================================================================
> > > >
> > > > Line annotation would be helpful.
[...]
> > Thoughts?
>
> I wasn't complaining about the report! It's more that without line numbers
> we're struggling a bit to figure out where the race is. All the page-flag
> tests on the reader side should be using test_bit(), but the report above
> doesn't seem to think that the read is marked. Given your series adding
> CONFIG_KCSAN_PERMISSIVE and the fact that you try to triage these things, I
> thought maybe you've seen this before and might be able to point at the race
> (which is hopefully benign, but it's annoying when you can't spot it!).
Hmm, I was sure I could find it in either closed or still open syzbot
reports, but sadly I couldn't find anything. :-/
The next best thing was to take the same kernel version, same Clang
version, and same config in the hopes the generated functions of
interest are identical. If I take the report and symbolize it with what
I've built, I get:
| ==================================================================
| BUG: KCSAN: data-race in next_uptodate_page / unlock_page
|
| write (marked) to 0xffffea00050f37c0 of 8 bytes by task 1872 on cpu 1:
| instrument_atomic_write include/linux/instrumented.h:87 [inline]
| clear_bit_unlock_is_negative_byte include/asm-generic/bitops/instrumented-lock.h:74 [inline]
| unlock_page+0x102/0x1b0 mm/filemap.c:1465
| filemap_map_pages+0x6c6/0x890 mm/filemap.c:3057
| do_fault_around mm/memory.c:3904 [inline]
| do_read_fault mm/memory.c:3918 [inline]
| do_fault mm/memory.c:4051 [inline]
| handle_pte_fault mm/memory.c:4302 [inline]
| __handle_mm_fault mm/memory.c:4437 [inline]
| handle_mm_fault+0x179c/0x27f0 mm/memory.c:4535
| do_user_addr_fault+0x3fb/0x830 arch/x86/mm/fault.c:1393
| handle_page_fault arch/x86/mm/fault.c:1450 [inline]
| exc_page_fault+0xc3/0x1a0 arch/x86/mm/fault.c:1506
| asm_exc_page_fault+0x1e/0x30 arch/x86/include/asm/idtentry.h:580
|
| read to 0xffffea00050f37c0 of 8 bytes by task 1873 on cpu 0:
| PagePoisoned include/linux/page-flags.h:204 [inline]
| PageReadahead include/linux/page-flags.h:382 [inline]
| next_uptodate_page+0x456/0x830 mm/filemap.c:2975
| next_map_page mm/filemap.c:3010 [inline]
| filemap_map_pages+0x728/0x890 mm/filemap.c:3066
| do_fault_around mm/memory.c:3904 [inline]
| do_read_fault mm/memory.c:3918 [inline]
| do_fault mm/memory.c:4051 [inline]
| handle_pte_fault mm/memory.c:4302 [inline]
| __handle_mm_fault mm/memory.c:4437 [inline]
| handle_mm_fault+0x179c/0x27f0 mm/memory.c:4535
| do_user_addr_fault+0x3fb/0x830 arch/x86/mm/fault.c:1393
| handle_page_fault arch/x86/mm/fault.c:1450 [inline]
| exc_page_fault+0xc3/0x1a0 arch/x86/mm/fault.c:1506
| asm_exc_page_fault+0x1e/0x30 arch/x86/include/asm/idtentry.h:580
|
| Reported by Kernel Concurrency Sanitizer on:
| CPU: 0 PID: 1873 Comm: systemd-udevd Not tainted 5.11.0-rc4-00001-gf9ce0be71d1f #1
| ==================================================================
And looking at the preprocessed source, this makes sense, because
PageReadahead clearly includes a call to PagePoisoned with the given
config.
My guess is that the below patch would be appropriate here?
(LKP folks: I'd still like bots to enable CONFIG_KCSAN_PERMISSIVE=y,
just to keep the volume of reports down for now.)
Thanks,
-- Marco
------ >8 ------
From: Marco Elver <elver@xxxxxxxxxx>
Date: Tue, 31 Aug 2021 17:27:27 +0200
Subject: [PATCH] mm: fix data race in PagePoisoned()
PagePoisoned() accesses page->flags which can be updated concurrently:
| BUG: KCSAN: data-race in next_uptodate_page / unlock_page
|
| write (marked) to 0xffffea00050f37c0 of 8 bytes by task 1872 on cpu 1:
| instrument_atomic_write include/linux/instrumented.h:87 [inline]
| clear_bit_unlock_is_negative_byte include/asm-generic/bitops/instrumented-lock.h:74 [inline]
| unlock_page+0x102/0x1b0 mm/filemap.c:1465
| filemap_map_pages+0x6c6/0x890 mm/filemap.c:3057
| ...
| read to 0xffffea00050f37c0 of 8 bytes by task 1873 on cpu 0:
| PagePoisoned include/linux/page-flags.h:204 [inline]
| PageReadahead include/linux/page-flags.h:382 [inline]
| next_uptodate_page+0x456/0x830 mm/filemap.c:2975
| ...
| CPU: 0 PID: 1873 Comm: systemd-udevd Not tainted 5.11.0-rc4-00001-gf9ce0be71d1f #1
To avoid the compiler tearing or otherwise optimizing the access, use
READ_ONCE() to access flags.
Reported-by: kernel test robot <oliver.sang@xxxxxxxxx>
Signed-off-by: Marco Elver <elver@xxxxxxxxxx>
---
include/linux/page-flags.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index a68af80649a4..70bf0ec29ee3 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -236,7 +236,7 @@ static __always_inline int PageCompound(struct page *page)
#define PAGE_POISON_PATTERN -1l
static inline int PagePoisoned(const struct page *page)
{
- return page->flags == PAGE_POISON_PATTERN;
+ return READ_ONCE(page->flags) == PAGE_POISON_PATTERN;
}
#ifdef CONFIG_DEBUG_VM
--
2.33.0.259.gc128427fd7-goog