Re: [mm] f9ce0be71d: BUG:KCSAN:data-race_in_next_uptodate_page/next_uptodate_page

[Marco Elver <elver@google.com>]

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@kernel.org> 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@google.com>
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@intel.com>
Signed-off-by: Marco Elver <elver@google.com>
---
 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