[syzbot] [fs?] [mm?] KCSAN: data-race in __filemap_remove_folio / folio_mapping (2)

[syzbot] [fs?] [mm?] KCSAN: data-race in __filemap_remove_folio / folio_mapping (2)

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    622322f53c6d Merge tag 'mips-fixes_6.3_2' of git://git.ker..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=12342880280000
kernel config:  https://syzkaller.appspot.com/x/.config?x=fa4baf7c6b35b5d5
dashboard link: https://syzkaller.appspot.com/bug?extid=606f94dfeaaa45124c90
compiler:       Debian clang version 15.0.7, GNU ld (GNU Binutils for Debian) 2.35.2

Unfortunately, I don't have any reproducer for this issue yet.

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/8b5f31d96315/disk-622322f5.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/adca7dc8daae/vmlinux-622322f5.xz
kernel image: https://storage.googleapis.com/syzbot-assets/ed78ddc31ccb/bzImage-622322f5.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+606f94dfeaaa45124c90@syzkaller.appspotmail.com

==================================================================
BUG: KCSAN: data-race in __filemap_remove_folio / folio_mapping

write to 0xffffea0004958618 of 8 bytes by task 17586 on cpu 1:
 page_cache_delete mm/filemap.c:145 [inline]
 __filemap_remove_folio+0x210/0x330 mm/filemap.c:225
 invalidate_complete_folio2 mm/truncate.c:586 [inline]
 invalidate_inode_pages2_range+0x506/0x790 mm/truncate.c:673
 iomap_dio_complete+0x383/0x470 fs/iomap/direct-io.c:115
 iomap_dio_rw+0x62/0x90 fs/iomap/direct-io.c:687
 ext4_dio_write_iter fs/ext4/file.c:597 [inline]
 ext4_file_write_iter+0x9e6/0x10e0 fs/ext4/file.c:708
 do_iter_write+0x418/0x700 fs/read_write.c:861
 vfs_iter_write+0x50/0x70 fs/read_write.c:902
 iter_file_splice_write+0x456/0x7d0 fs/splice.c:778
 do_splice_from fs/splice.c:856 [inline]
 direct_splice_actor+0x84/0xa0 fs/splice.c:1022
 splice_direct_to_actor+0x2ee/0x5f0 fs/splice.c:977
 do_splice_direct+0x104/0x180 fs/splice.c:1065
 do_sendfile+0x3b8/0x950 fs/read_write.c:1255
 __do_sys_sendfile64 fs/read_write.c:1323 [inline]
 __se_sys_sendfile64 fs/read_write.c:1309 [inline]
 __x64_sys_sendfile64+0x110/0x150 fs/read_write.c:1309
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd

read to 0xffffea0004958618 of 8 bytes by task 17568 on cpu 0:
 folio_mapping+0x92/0x110 mm/util.c:774
 folio_evictable mm/internal.h:156 [inline]
 lru_add_fn+0x92/0x450 mm/swap.c:181
 folio_batch_move_lru+0x21e/0x2f0 mm/swap.c:217
 folio_batch_add_and_move mm/swap.c:234 [inline]
 folio_add_lru+0xc9/0x130 mm/swap.c:517
 filemap_add_folio+0xfc/0x150 mm/filemap.c:954
 page_cache_ra_unbounded+0x15e/0x2e0 mm/readahead.c:251
 do_page_cache_ra mm/readahead.c:300 [inline]
 page_cache_ra_order mm/readahead.c:560 [inline]
 ondemand_readahead+0x550/0x6c0 mm/readahead.c:682
 page_cache_sync_ra+0x284/0x2a0 mm/readahead.c:709
 page_cache_sync_readahead include/linux/pagemap.h:1214 [inline]
 filemap_get_pages+0x257/0xea0 mm/filemap.c:2598
 filemap_read+0x223/0x680 mm/filemap.c:2693
 generic_file_read_iter+0x76/0x320 mm/filemap.c:2840
 ext4_file_read_iter+0x1cc/0x290
 call_read_iter include/linux/fs.h:1845 [inline]
 generic_file_splice_read+0xe3/0x290 fs/splice.c:402
 do_splice_to fs/splice.c:885 [inline]
 splice_direct_to_actor+0x25a/0x5f0 fs/splice.c:956
 do_splice_direct+0x104/0x180 fs/splice.c:1065
 do_sendfile+0x3b8/0x950 fs/read_write.c:1255
 __do_sys_sendfile64 fs/read_write.c:1323 [inline]
 __se_sys_sendfile64 fs/read_write.c:1309 [inline]
 __x64_sys_sendfile64+0x110/0x150 fs/read_write.c:1309
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd

value changed: 0xffff88810a98f7b0 -> 0x0000000000000000

Reported by Kernel Concurrency Sanitizer on:
CPU: 0 PID: 17568 Comm: syz-executor.2 Not tainted 6.3.0-rc7-syzkaller-00191-g622322f53c6d #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/14/2023
==================================================================


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

Re: [syzbot] [fs?] [mm?] KCSAN: data-race in __filemap_remove_folio / folio_mapping (2)

[Dmitry Vyukov]

On Mon, 24 Apr 2023 at 09:19, syzbot
<syzbot+606f94dfeaaa45124c90@syzkaller.appspotmail.com> wrote:
>
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit:    622322f53c6d Merge tag 'mips-fixes_6.3_2' of git://git.ker..
> git tree:       upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=12342880280000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=fa4baf7c6b35b5d5
> dashboard link: https://syzkaller.appspot.com/bug?extid=606f94dfeaaa45124c90
> compiler:       Debian clang version 15.0.7, GNU ld (GNU Binutils for Debian) 2.35.2
>
> Unfortunately, I don't have any reproducer for this issue yet.
>
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/8b5f31d96315/disk-622322f5.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/adca7dc8daae/vmlinux-622322f5.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/ed78ddc31ccb/bzImage-622322f5.xz
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+606f94dfeaaa45124c90@syzkaller.appspotmail.com

If I am reading this correctly, it can lead to NULL derefs in
folio_mapping() if folio->mapping is read twice. I think
folio->mapping reads/writes need to use READ/WRITE_ONCE if racy.


> ==================================================================
> BUG: KCSAN: data-race in __filemap_remove_folio / folio_mapping
>
> write to 0xffffea0004958618 of 8 bytes by task 17586 on cpu 1:
>  page_cache_delete mm/filemap.c:145 [inline]
>  __filemap_remove_folio+0x210/0x330 mm/filemap.c:225
>  invalidate_complete_folio2 mm/truncate.c:586 [inline]
>  invalidate_inode_pages2_range+0x506/0x790 mm/truncate.c:673
>  iomap_dio_complete+0x383/0x470 fs/iomap/direct-io.c:115
>  iomap_dio_rw+0x62/0x90 fs/iomap/direct-io.c:687
>  ext4_dio_write_iter fs/ext4/file.c:597 [inline]
>  ext4_file_write_iter+0x9e6/0x10e0 fs/ext4/file.c:708
>  do_iter_write+0x418/0x700 fs/read_write.c:861
>  vfs_iter_write+0x50/0x70 fs/read_write.c:902
>  iter_file_splice_write+0x456/0x7d0 fs/splice.c:778
>  do_splice_from fs/splice.c:856 [inline]
>  direct_splice_actor+0x84/0xa0 fs/splice.c:1022
>  splice_direct_to_actor+0x2ee/0x5f0 fs/splice.c:977
>  do_splice_direct+0x104/0x180 fs/splice.c:1065
>  do_sendfile+0x3b8/0x950 fs/read_write.c:1255
>  __do_sys_sendfile64 fs/read_write.c:1323 [inline]
>  __se_sys_sendfile64 fs/read_write.c:1309 [inline]
>  __x64_sys_sendfile64+0x110/0x150 fs/read_write.c:1309
>  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>  do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80
>  entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> read to 0xffffea0004958618 of 8 bytes by task 17568 on cpu 0:
>  folio_mapping+0x92/0x110 mm/util.c:774
>  folio_evictable mm/internal.h:156 [inline]
>  lru_add_fn+0x92/0x450 mm/swap.c:181
>  folio_batch_move_lru+0x21e/0x2f0 mm/swap.c:217
>  folio_batch_add_and_move mm/swap.c:234 [inline]
>  folio_add_lru+0xc9/0x130 mm/swap.c:517
>  filemap_add_folio+0xfc/0x150 mm/filemap.c:954
>  page_cache_ra_unbounded+0x15e/0x2e0 mm/readahead.c:251
>  do_page_cache_ra mm/readahead.c:300 [inline]
>  page_cache_ra_order mm/readahead.c:560 [inline]
>  ondemand_readahead+0x550/0x6c0 mm/readahead.c:682
>  page_cache_sync_ra+0x284/0x2a0 mm/readahead.c:709
>  page_cache_sync_readahead include/linux/pagemap.h:1214 [inline]
>  filemap_get_pages+0x257/0xea0 mm/filemap.c:2598
>  filemap_read+0x223/0x680 mm/filemap.c:2693
>  generic_file_read_iter+0x76/0x320 mm/filemap.c:2840
>  ext4_file_read_iter+0x1cc/0x290
>  call_read_iter include/linux/fs.h:1845 [inline]
>  generic_file_splice_read+0xe3/0x290 fs/splice.c:402
>  do_splice_to fs/splice.c:885 [inline]
>  splice_direct_to_actor+0x25a/0x5f0 fs/splice.c:956
>  do_splice_direct+0x104/0x180 fs/splice.c:1065
>  do_sendfile+0x3b8/0x950 fs/read_write.c:1255
>  __do_sys_sendfile64 fs/read_write.c:1323 [inline]
>  __se_sys_sendfile64 fs/read_write.c:1309 [inline]
>  __x64_sys_sendfile64+0x110/0x150 fs/read_write.c:1309
>  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>  do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80
>  entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> value changed: 0xffff88810a98f7b0 -> 0x0000000000000000
>
> Reported by Kernel Concurrency Sanitizer on:
> CPU: 0 PID: 17568 Comm: syz-executor.2 Not tainted 6.3.0-rc7-syzkaller-00191-g622322f53c6d #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/14/2023
> ==================================================================
>
>
> ---
> This report is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzkaller@googlegroups.com.
>
> syzbot will keep track of this issue. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/000000000000d0737c05fa0fd499%40google.com.

Re: [syzbot] [fs?] [mm?] KCSAN: data-race in __filemap_remove_folio / folio_mapping (2)

[Matthew Wilcox]

On Mon, Apr 24, 2023 at 09:38:43AM +0200, Dmitry Vyukov wrote:
> On Mon, 24 Apr 2023 at 09:19, syzbot
> <syzbot+606f94dfeaaa45124c90@syzkaller.appspotmail.com> wrote:
> If I am reading this correctly, it can lead to NULL derefs in
> folio_mapping() if folio->mapping is read twice. I think
> folio->mapping reads/writes need to use READ/WRITE_ONCE if racy.

You aren't reading it correctly.

        mapping = folio->mapping;
        if ((unsigned long)mapping & PAGE_MAPPING_FLAGS)
                return NULL;

        return mapping;

The racing write is storing NULL.  So it might return NULL or it might
return the old mapping, or it might return NULL.  Either way, the caller
has to be prepared for NULL to be returned.

It's a false posiive, but probably worth silencing with a READ_ONCE().

Re: [syzbot] [fs?] [mm?] KCSAN: data-race in __filemap_remove_folio / folio_mapping (2)

[Dmitry Vyukov]

On Mon, 24 Apr 2023 at 15:21, Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, Apr 24, 2023 at 09:38:43AM +0200, Dmitry Vyukov wrote:
> > On Mon, 24 Apr 2023 at 09:19, syzbot
> > <syzbot+606f94dfeaaa45124c90@syzkaller.appspotmail.com> wrote:
> > If I am reading this correctly, it can lead to NULL derefs in
> > folio_mapping() if folio->mapping is read twice. I think
> > folio->mapping reads/writes need to use READ/WRITE_ONCE if racy.
>
> You aren't reading it correctly.
>
>         mapping = folio->mapping;
>         if ((unsigned long)mapping & PAGE_MAPPING_FLAGS)
>                 return NULL;
>
>         return mapping;
>
> The racing write is storing NULL.  So it might return NULL or it might
> return the old mapping, or it might return NULL.  Either way, the caller
> has to be prepared for NULL to be returned.
>
> It's a false posiive, but probably worth silencing with a READ_ONCE().

Yes, but the end of the function does not limit effects of races. I
think this can still crash on NULL deref.

The simplest example would be to compile this:

struct address_space *folio_mapping(struct folio *folio)
{
        ...
        mapping = folio->mapping;
        if ((unsigned long)mapping & PAGE_MAPPING_FLAGS)
                return NULL;

        return mapping;
}

ret = !mapping_unevictable(folio_mapping(folio)) &&
        !folio_test_mlocked(folio);

static inline bool mapping_unevictable(struct address_space *mapping)
{
        return mapping && test_bit(AS_UNEVICTABLE, &mapping->flags);
}

to this:

if (!((unsigned long)folio->mapping & PAGE_MAPPING_FLAGS) && folio->mapping)
   if (test_bit(AS_UNEVICTABLE, &folio->mapping->flags))

which does crash.

Re: [syzbot] [fs?] [mm?] KCSAN: data-race in __filemap_remove_folio / folio_mapping (2)

[Matthew Wilcox]

On Mon, Apr 24, 2023 at 03:49:04PM +0200, Dmitry Vyukov wrote:
> On Mon, 24 Apr 2023 at 15:21, Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Mon, Apr 24, 2023 at 09:38:43AM +0200, Dmitry Vyukov wrote:
> > > On Mon, 24 Apr 2023 at 09:19, syzbot
> > > <syzbot+606f94dfeaaa45124c90@syzkaller.appspotmail.com> wrote:
> > > If I am reading this correctly, it can lead to NULL derefs in
> > > folio_mapping() if folio->mapping is read twice. I think
> > > folio->mapping reads/writes need to use READ/WRITE_ONCE if racy.
> >
> > You aren't reading it correctly.
> >
> >         mapping = folio->mapping;
> >         if ((unsigned long)mapping & PAGE_MAPPING_FLAGS)
> >                 return NULL;
> >
> >         return mapping;
> >
> > The racing write is storing NULL.  So it might return NULL or it might
> > return the old mapping, or it might return NULL.  Either way, the caller
> > has to be prepared for NULL to be returned.
> >
> > It's a false posiive, but probably worth silencing with a READ_ONCE().
> 
> Yes, but the end of the function does not limit effects of races. I

I thought it did.  I was under the impression that the compiler was not
allowed to extract loads from within the function and move them outside.
Maybe that changed since C99.

> to this:
> 
> if (!((unsigned long)folio->mapping & PAGE_MAPPING_FLAGS) && folio->mapping)
>    if (test_bit(AS_UNEVICTABLE, &folio->mapping->flags))
> 
> which does crash.

Yes, if the compiler is allowed to do that, then that's a possibility.

Re: [syzbot] [fs?] [mm?] KCSAN: data-race in __filemap_remove_folio / folio_mapping (2)

[Dmitry Vyukov]

On Mon, 24 Apr 2023 at 16:10, Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, Apr 24, 2023 at 03:49:04PM +0200, Dmitry Vyukov wrote:
> > On Mon, 24 Apr 2023 at 15:21, Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Mon, Apr 24, 2023 at 09:38:43AM +0200, Dmitry Vyukov wrote:
> > > > On Mon, 24 Apr 2023 at 09:19, syzbot
> > > > <syzbot+606f94dfeaaa45124c90@syzkaller.appspotmail.com> wrote:
> > > > If I am reading this correctly, it can lead to NULL derefs in
> > > > folio_mapping() if folio->mapping is read twice. I think
> > > > folio->mapping reads/writes need to use READ/WRITE_ONCE if racy.
> > >
> > > You aren't reading it correctly.
> > >
> > >         mapping = folio->mapping;
> > >         if ((unsigned long)mapping & PAGE_MAPPING_FLAGS)
> > >                 return NULL;
> > >
> > >         return mapping;
> > >
> > > The racing write is storing NULL.  So it might return NULL or it might
> > > return the old mapping, or it might return NULL.  Either way, the caller
> > > has to be prepared for NULL to be returned.
> > >
> > > It's a false posiive, but probably worth silencing with a READ_ONCE().
> >
> > Yes, but the end of the function does not limit effects of races. I
>
> I thought it did.  I was under the impression that the compiler was not
> allowed to extract loads from within the function and move them outside.
> Maybe that changed since C99.
>
> > to this:
> >
> > if (!((unsigned long)folio->mapping & PAGE_MAPPING_FLAGS) && folio->mapping)
> >    if (test_bit(AS_UNEVICTABLE, &folio->mapping->flags))
> >
> > which does crash.
>
> Yes, if the compiler is allowed to do that, then that's a possibility.

C11/C++11 simply say any data race renders behavior of the whole
program undefined. There is no discussion about values, functions,
anything else.

Before that there was no notion of data races, so it wasn't possible
to talk about possible effects and restrict them. But I don't think
there ever was an intention to do any practical restrictions around
function boundaries. That would mean that inlining can only run as the
latest optimization pass, which would inhibit tons of optimizations.
Users would throw such a compiler away.

Re: [syzbot] [fs?] [mm?] KCSAN: data-race in __filemap_remove_folio / folio_mapping (2)

[Jeongjun Park]

please test data-race in __filemap_remove_folio / folio_mapping

#syz test git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

Re: [syzbot] [fs?] [mm?] KCSAN: data-race in __filemap_remove_folio / folio_mapping (2)

[syzbot]

> please test data-race in __filemap_remove_folio / folio_mapping
>
> #syz test git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

This crash does not have a reproducer. I cannot test it.