linux-next test error: BUG: using smp_processor_id() in preemptible [ADDR] code: syz-fuzzer/6792

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit: 0e21d462 Add linux-next specific files for 20200602
git tree: linux-next
console output: https://syzkaller.appspot.com/x/log.txt?x=127233ee100000
kernel config: https://syzkaller.appspot.com/x/.config?x=ecc1aef35f550ee3
dashboard link: https://syzkaller.appspot.com/bug?extid=82f324bb69744c5f6969
compiler: gcc (GCC) 9.0.0 20181231 (experimental)

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+82f324...@syzkaller.appspotmail.com

BUG: using smp_processor_id() in preemptible [00000000] code: syz-fuzzer/6792
caller is ext4_mb_new_blocks+0xa4d/0x3b70 fs/ext4/mballoc.c:4711
CPU: 1 PID: 6792 Comm: syz-fuzzer Not tainted 5.7.0-next-20200602-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x18f/0x20d lib/dump_stack.c:118
check_preemption_disabled+0x20d/0x220 lib/smp_processor_id.c:48
ext4_mb_new_blocks+0xa4d/0x3b70 fs/ext4/mballoc.c:4711
ext4_ext_map_blocks+0x201b/0x33e0 fs/ext4/extents.c:4244
ext4_map_blocks+0x4cb/0x1640 fs/ext4/inode.c:626
ext4_getblk+0xad/0x520 fs/ext4/inode.c:833
ext4_bread+0x7c/0x380 fs/ext4/inode.c:883
ext4_append+0x153/0x360 fs/ext4/namei.c:67
ext4_init_new_dir fs/ext4/namei.c:2757 [inline]
ext4_mkdir+0x5e0/0xdf0 fs/ext4/namei.c:2802
vfs_mkdir+0x419/0x690 fs/namei.c:3632
do_mkdirat+0x21e/0x280 fs/namei.c:3655
do_syscall_64+0x60/0xe0 arch/x86/entry/common.c:359
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x4b02a0
Code: Bad RIP value.
RSP: 002b:000000c00010d4b8 EFLAGS: 00000212 ORIG_RAX: 0000000000000102
RAX: ffffffffffffffda RBX: 000000c00002c000 RCX: 00000000004b02a0
RDX: 00000000000001c0 RSI: 000000c000026b40 RDI: ffffffffffffff9c
RBP: 000000c00010d510 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000212 R12: ffffffffffffffff
R13: 000000000000005b R14: 000000000000005a R15: 0000000000000100


---
This bug 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 syzk...@googlegroups.com.

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

[Ritesh Harjani]

#syz test:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
0e21d4620dd047da7952f44a2e1ac777ded2d57e

[syzbot]

> #syz test:

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

> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> 0e21d4620dd047da7952f44a2e1ac777ded2d57e

[Hillf Danton]

Tue, 02 Jun 2020 04:20:16 -0700

> syzbot found the following crash on:
>
> HEAD commit: 0e21d462 Add linux-next specific files for 20200602
> git tree: linux-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=127233ee100000
> kernel config: https://syzkaller.appspot.com/x/.config?x=ecc1aef35f550ee3
> dashboard link: https://syzkaller.appspot.com/bug?extid=82f324bb69744c5f6969
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+82f324...@syzkaller.appspotmail.com
>
> BUG: using smp_processor_id() in preemptible [00000000] code: syz-fuzzer/6792
> caller is ext4_mb_new_blocks+0xa4d/0x3b70 fs/ext4/mballoc.c:4711

Fix 42f56b7a4a7d ("ext4: mballoc: introduce pcpu seqcnt for freeing PA
to improve ENOSPC handling") by redefining discard_pa_seq to be a simple
regular sequence counter to axe the need of percpu operation.

Memory barrier OTOH is added around the counter to mimic the seqcount
in linux/seqlock.h

--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -352,32 +352,35 @@ static void ext4_mb_generate_from_freeli
static void ext4_mb_new_preallocation(struct ext4_allocation_context *ac);

/*
- * The algorithm using this percpu seq counter goes below:
- * 1. We sample the percpu discard_pa_seq counter before trying for block
- * allocation in ext4_mb_new_blocks().
- * 2. We increment this percpu discard_pa_seq counter when we either allocate
- * or free these blocks i.e. while marking those blocks as used/free in
+ * Here a simple sequence counter is used
+ * 1. We sample the discard_pa_seq counter before trying for block allocation
+ * in ext4_mb_new_blocks().
+ * 2. We increment the counter when we either allocate or free these blocks
+ * i.e. while marking those blocks as used/free in
* mb_mark_used()/mb_free_blocks().
- * 3. We also increment this percpu seq counter when we successfully identify
- * that the bb_prealloc_list is not empty and hence proceed for discarding
- * of those PAs inside ext4_mb_discard_group_preallocations().
+ * 3. We also increment it when we successfully identify that the
+ * bb_prealloc_list is not empty and hence proceed for discarding of those
+ * PAs inside ext4_mb_discard_group_preallocations().
*
* Now to make sure that the regular fast path of block allocation is not
- * affected, as a small optimization we only sample the percpu seq counter
- * on that cpu. Only when the block allocation fails and when freed blocks
- * found were 0, that is when we sample percpu seq counter for all cpus using
- * below function ext4_get_discard_pa_seq_sum(). This happens after making
- * sure that all the PAs on grp->bb_prealloc_list got freed or if it's empty.
+ * affected, as a small optimization we only sample the seq counter on that
+ * cpu. Only when the block allocation fails and when freed blocks found were
+ * 0, that is when we sample percpu seq counter for all cpus using below
+ * function ext4_sample_discard_pa_seq(). This happens after making sure that
+ * all the PAs on grp->bb_prealloc_list got freed or if it's empty.
*/
-static DEFINE_PER_CPU(u64, discard_pa_seq);
-static inline u64 ext4_get_discard_pa_seq_sum(void)
+static unsigned int discard_pa_seq;
+
+static inline unsigned int ext4_sample_discard_pa_seq(void)
{
- int __cpu;
- u64 __seq = 0;
+ smp_mb();
+ return discard_pa_seq;
+}

- for_each_possible_cpu(__cpu)
- __seq += per_cpu(discard_pa_seq, __cpu);
- return __seq;
+static inline void ext4_inc_discard_pa_seq(void)
+{
+ discard_pa_seq++;
+ smp_mb();
}

static inline void *mb_correct_addr_and_bit(int *bit, void *addr)
@@ -1491,7 +1494,7 @@ static void mb_free_blocks(struct inode
mb_check_buddy(e4b);
mb_free_blocks_double(inode, e4b, first, count);

- this_cpu_inc(discard_pa_seq);
+ ext4_inc_discard_pa_seq();
e4b->bd_info->bb_free += count;
if (first < e4b->bd_info->bb_first_free)
e4b->bd_info->bb_first_free = first;
@@ -1633,7 +1636,7 @@ static int mb_mark_used(struct ext4_budd
mb_check_buddy(e4b);
mb_mark_used_double(e4b, start, len);

- this_cpu_inc(discard_pa_seq);
+ ext4_inc_discard_pa_seq();
e4b->bd_info->bb_free -= len;
if (e4b->bd_info->bb_first_free == start)
e4b->bd_info->bb_first_free += len;
@@ -4025,7 +4028,7 @@ ext4_mb_discard_group_preallocations(str
INIT_LIST_HEAD(&list);
repeat:
ext4_lock_group(sb, group);
- this_cpu_inc(discard_pa_seq);
+ ext4_inc_discard_pa_seq();
list_for_each_entry_safe(pa, tmp,
&grp->bb_prealloc_list, pa_group_list) {
spin_lock(&pa->pa_lock);
@@ -4608,21 +4611,21 @@ static int ext4_mb_discard_preallocation
}

static bool ext4_mb_discard_preallocations_should_retry(struct super_block *sb,
- struct ext4_allocation_context *ac, u64 *seq)
+ struct ext4_allocation_context *ac, unsigned int *seq)
{
int freed;
- u64 seq_retry = 0;
bool ret = false;
+ unsigned int seq_sample;

freed = ext4_mb_discard_preallocations(sb, ac->ac_o_ex.fe_len);
if (freed) {
ret = true;
goto out_dbg;
}
- seq_retry = ext4_get_discard_pa_seq_sum();
- if (!(ac->ac_flags & EXT4_MB_STRICT_CHECK) || seq_retry != *seq) {
+ seq_sample = ext4_sample_discard_pa_seq();
+ if (!(ac->ac_flags & EXT4_MB_STRICT_CHECK) || seq_sample != *seq) {
ac->ac_flags |= EXT4_MB_STRICT_CHECK;
- *seq = seq_retry;
+ *seq = seq_sample;
ret = true;
}

@@ -4645,7 +4648,7 @@ ext4_fsblk_t ext4_mb_new_blocks(handle_t
ext4_fsblk_t block = 0;
unsigned int inquota = 0;
unsigned int reserv_clstrs = 0;
- u64 seq;
+ unsigned int seq;

might_sleep();
sb = ar->inode->i_sb;
@@ -4708,7 +4711,7 @@ ext4_fsblk_t ext4_mb_new_blocks(handle_t
}

ac->ac_op = EXT4_MB_HISTORY_PREALLOC;
- seq = *this_cpu_ptr(&discard_pa_seq);
+ seq = ext4_sample_discard_pa_seq();
if (!ext4_mb_use_preallocated(ac)) {
ac->ac_op = EXT4_MB_HISTORY_ALLOC;
ext4_mb_normalize_request(ac, ar);
--

[Ritesh Harjani]

On 6/2/20 8:22 PM, Hillf Danton wrote:
>
> Tue, 02 Jun 2020 04:20:16 -0700
>> syzbot found the following crash on:
>>
>> HEAD commit: 0e21d462 Add linux-next specific files for 20200602
>> git tree: linux-next
>> console output: https://syzkaller.appspot.com/x/log.txt?x=127233ee100000
>> kernel config: https://syzkaller.appspot.com/x/.config?x=ecc1aef35f550ee3
>> dashboard link: https://syzkaller.appspot.com/bug?extid=82f324bb69744c5f6969
>> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
>>
>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
>> Reported-by: syzbot+82f324...@syzkaller.appspotmail.com
>>
>> BUG: using smp_processor_id() in preemptible [00000000] code: syz-fuzzer/6792
>> caller is ext4_mb_new_blocks+0xa4d/0x3b70 fs/ext4/mballoc.c:4711
>
> Fix 42f56b7a4a7d ("ext4: mballoc: introduce pcpu seqcnt for freeing PA
> to improve ENOSPC handling") by redefining discard_pa_seq to be a simple
> regular sequence counter to axe the need of percpu operation.

Why remove percpu seqcnt? IIUC, percpu are much better in case of a
multi-threaded use case which could run and allocate blocks in parallel.
Whereas a updating a simple variable across different cpus may lead to
cacheline bouncing problem.
Since in this case we can very well have a use case of multiple threads
trying to allocate blocks at the same time, so why change this to a
simple seqcnt from percpu seqcnt?

-ritesh

[Hillf Danton]

Wed, 3 Jun 2020 15:36:47 +0530 Ritesh Harjani wrote:
> On 6/2/20 8:22 PM, Hillf Danton wrote:
> >
> > Tue, 02 Jun 2020 04:20:16 -0700
> >> syzbot found the following crash on:
> >>
> >> HEAD commit: 0e21d462 Add linux-next specific files for 20200602
> >> git tree: linux-next
> >> console output: https://syzkaller.appspot.com/x/log.txt?x=127233ee100000
> >> kernel config: https://syzkaller.appspot.com/x/.config?x=ecc1aef35f550ee3
> >> dashboard link: https://syzkaller.appspot.com/bug?extid=82f324bb69744c5f6969
> >> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> >>
> >> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> >> Reported-by: syzbot+82f324...@syzkaller.appspotmail.com
> >>
> >> BUG: using smp_processor_id() in preemptible [00000000] code: syz-fuzzer/6792
> >> caller is ext4_mb_new_blocks+0xa4d/0x3b70 fs/ext4/mballoc.c:4711
> >
> > Fix 42f56b7a4a7d ("ext4: mballoc: introduce pcpu seqcnt for freeing PA
> > to improve ENOSPC handling") by redefining discard_pa_seq to be a simple
> > regular sequence counter to axe the need of percpu operation.
>
> Why remove percpu seqcnt? IIUC, percpu are much better in case of a
> multi-threaded use case which could run and allocate blocks in parallel.

Given <linux/seqlock.h> I suspect the need to create another percpu
seqcount from the beginning.

> Whereas a updating a simple variable across different cpus may lead to
> cacheline bouncing problem.

Of course seqcount is not a case of all cure.

> Since in this case we can very well have a use case of multiple threads
> trying to allocate blocks at the same time, so why change this to a
> simple seqcnt from percpu seqcnt?

Syzbot is becoming grumpy.

https://lore.kernel.org/lkml/00000000000087...@google.com/T/#u
https://lore.kernel.org/lkml/0000000000007f...@google.com/T/#u
https://lore.kernel.org/lkml/00000000000082...@google.com/T/#u
https://lore.kernel.org/lkml/000000000000a1...@google.com/T/#u

[syzbot]

> #syz dup: linux-next test error: BUG: using smp_processor_id() in preemptible [ADDR] code: syz-fuzzer/6792

Can't dup bug to itself.

>

[Tetsuo Handa]

#syz dup: linux-next test error: BUG: using smp_processor_id() in preemptible [ADDR] code: syz-fuzzer/6792

[syzbot]

> #syz dup: linux-next test error: BUG: using smp_processor_id() in preemptible [ADDR] code: syz-fuzzer/6792

Can't dup bug to itself.

>

> --
> 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-bug...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/04ce39c8-53b2-a527-2dfa-26ca8766b5d1%40I-love.SAKURA.ne.jp.

[Ido Schimmel]

> >From cc1cf67d99d5fa61db0651c89c288df31bad6b8e Mon Sep 17 00:00:00 2001
> From: Ritesh Harjani <rit...@linux.ibm.com>
> Date: Tue, 2 Jun 2020 17:54:12 +0530
> Subject: [PATCH 1/1] ext4: mballoc: Use raw_cpu_ptr in case if preemption is enabled
>
> It doesn't matter really in ext4_mb_new_blocks() about whether the code
> is rescheduled on any other cpu due to preemption. Because we care
> about discard_pa_seq only when the block allocation fails and then too
> we add the seq counter of all the cpus against the initial sampled one
> to check if anyone has freed any blocks while we were doing allocation.
>
> So just use raw_cpu_ptr to not trigger this BUG.
>
> BUG: using smp_processor_id() in preemptible [00000000] code: syz-fuzzer/6927

> caller is ext4_mb_new_blocks+0xa4d/0x3b70 fs/ext4/mballoc.c:4711

> CPU: 1 PID: 6927 Comm: syz-fuzzer Not tainted 5.7.0-next-20200602-syzkaller #0

> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x18f/0x20d lib/dump_stack.c:118
> check_preemption_disabled+0x20d/0x220 lib/smp_processor_id.c:48
> ext4_mb_new_blocks+0xa4d/0x3b70 fs/ext4/mballoc.c:4711
> ext4_ext_map_blocks+0x201b/0x33e0 fs/ext4/extents.c:4244
> ext4_map_blocks+0x4cb/0x1640 fs/ext4/inode.c:626
> ext4_getblk+0xad/0x520 fs/ext4/inode.c:833
> ext4_bread+0x7c/0x380 fs/ext4/inode.c:883
> ext4_append+0x153/0x360 fs/ext4/namei.c:67
> ext4_init_new_dir fs/ext4/namei.c:2757 [inline]
> ext4_mkdir+0x5e0/0xdf0 fs/ext4/namei.c:2802
> vfs_mkdir+0x419/0x690 fs/namei.c:3632
> do_mkdirat+0x21e/0x280 fs/namei.c:3655
> do_syscall_64+0x60/0xe0 arch/x86/entry/common.c:359
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>

> Signed-off-by: Ritesh Harjani <rit...@linux.ibm.com>
> Reported-by: syzbot+82f324...@syzkaller.appspotmail.com

Hi,

Are you going to submit this patch formally? Without it I'm constantly
seeing the above splat.

Thanks

> ---
> fs/ext4/mballoc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index a9083113a8c0..b79b32dbe3ea 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -4708,7 +4708,7 @@ ext4_fsblk_t ext4_mb_new_blocks(handle_t *handle,

> }
>
> ac->ac_op = EXT4_MB_HISTORY_PREALLOC;
> - seq = *this_cpu_ptr(&discard_pa_seq);

> + seq = *raw_cpu_ptr(&discard_pa_seq);

> if (!ext4_mb_use_preallocated(ac)) {
> ac->ac_op = EXT4_MB_HISTORY_ALLOC;
> ext4_mb_normalize_request(ac, ar);
> --

> 2.21.3
>

[Ritesh Harjani]

I see Ted has already taken v2 of this patch in his dev repo.
Should be able to see in linux tree soon.

https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/commit/?h=dev&id=811985365378df01386c3cfb7ff716e74ca376d5


-ritesh

[Ido Schimmel]

On Fri, Jun 12, 2020 at 07:09:04PM +0530, Ritesh Harjani wrote:
> I see Ted has already taken v2 of this patch in his dev repo.
> Should be able to see in linux tree soon.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/commit/?h=dev&id=811985365378df01386c3cfb7ff716e74ca376d5

Great, thanks a lot. I've replaced previous patch with this one in my
testing tree.