[syzbot] WARNING in c_start

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 9c9155a3509a Merge tag 'drm-next-2022-10-14' of git://anon..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=1030f562880000
kernel config: https://syzkaller.appspot.com/x/.config?x=557e715ffce7a200
dashboard link: https://syzkaller.appspot.com/bug?extid=d0fd2bf0dd6da72496dd
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2

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

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

------------[ cut here ]------------
WARNING: CPU: 1 PID: 3671 at include/linux/cpumask.h:110 cpu_max_bits_warn include/linux/cpumask.h:110 [inline]
WARNING: CPU: 1 PID: 3671 at include/linux/cpumask.h:110 cpumask_check include/linux/cpumask.h:117 [inline]
WARNING: CPU: 1 PID: 3671 at include/linux/cpumask.h:110 cpumask_next include/linux/cpumask.h:178 [inline]
WARNING: CPU: 1 PID: 3671 at include/linux/cpumask.h:110 c_start+0x152/0x1b0 arch/x86/kernel/cpu/proc.c:156
Modules linked in:
CPU: 1 PID: 3671 Comm: syz-fuzzer Not tainted 6.0.0-syzkaller-11990-g9c9155a3509a #0
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.14.0-2 04/01/2014
RIP: 0010:cpu_max_bits_warn include/linux/cpumask.h:110 [inline]
RIP: 0010:cpumask_check include/linux/cpumask.h:117 [inline]
RIP: 0010:cpumask_next include/linux/cpumask.h:178 [inline]
RIP: 0010:c_start+0x152/0x1b0 arch/x86/kernel/cpu/proc.c:156
Code: a0 68 99 8b e8 3f 97 4b 00 5b 5d 4c 89 e0 41 5c 41 5d c3 e8 30 97 4b 00 45 31 e4 5b 4c 89 e0 5d 41 5c 41 5d c3 e8 1e 97 4b 00 <0f> 0b e9 1e ff ff ff 48 c7 c7 40 54 e1 8d e8 fb 19 97 00 e9 6b ff
RSP: 0018:ffffc90002f27c70 EFLAGS: 00010293
RAX: 0000000000000000 RBX: ffffffff8de15440 RCX: 0000000000000000
RDX: ffff88801b3b6100 RSI: ffffffff812ffc02 RDI: 0000000000000004
RBP: 0000000000000008 R08: 0000000000000004 R09: 0000000000000008
R10: 0000000000000008 R11: 0000000000000001 R12: ffff88801c29c278
R13: 0000000000000008 R14: ffff888023b17b3c R15: 0000000000000000
FS: 000000c00004e890(0000) GS:ffff88802c800000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fe53b98da00 CR3: 000000001b4cb000 CR4: 0000000000150ef0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
seq_read_iter+0x2c6/0x1280 fs/seq_file.c:225
proc_reg_read_iter+0x111/0x2d0 fs/proc/inode.c:301
call_read_iter include/linux/fs.h:2185 [inline]
new_sync_read fs/read_write.c:389 [inline]
vfs_read+0x67d/0x930 fs/read_write.c:470
ksys_read+0x127/0x250 fs/read_write.c:613
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x4ae09b
Code: e8 ea 57 fb ff eb 88 cc cc cc cc cc cc cc cc e8 fb 9b fb ff 48 8b 7c 24 10 48 8b 74 24 18 48 8b 54 24 20 48 8b 44 24 08 0f 05 <48> 3d 01 f0 ff ff 76 20 48 c7 44 24 28 ff ff ff ff 48 c7 44 24 30
RSP: 002b:000000c0007233b8 EFLAGS: 00000212 ORIG_RAX: 0000000000000000
RAX: ffffffffffffffda RBX: 000000c00003e800 RCX: 00000000004ae09b
RDX: 0000000000000b49 RSI: 000000c0004fe4b7 RDI: 0000000000000003
RBP: 000000c000723408 R08: 0000000000000001 R09: 000000c000074480
R10: 0000000000000b49 R11: 0000000000000212 R12: 000000c000723570
R13: 0000000000000000 R14: 000000c0000001a0 R15: 00007fe53942c2f3
</TASK>


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

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

[Borislav Petkov]

Let's see if the bot understands fetching patches from mailing lists:

# syz test: https://lore.kernel.org/r/20221014155845....@ventanamicro.com

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[syzbot]

> On 2022/10/15 6:24, Borislav Petkov wrote:
>> Let's see if the bot understands fetching patches from mailing lists:
>>
>> # syz test: https://lore.kernel.org/r/20221014155845....@ventanamicro.com
>>
>

> That's an invalid command line. The correct syntax is:
>
> #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.

>
> From 5aaf6a3df1f83331c5ca2f6a653a2e21e3ae4f40 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin...@I-love.SAKURA.ne.jp>
> Date: Sat, 15 Oct 2022 20:22:18 +0900
> Subject: [PATCH] cpumask: partially relax checking valid cpu range
>
> syzbot is hitting WARN_ON_ONCE(cpu >= nr_cpumask_bits) warning at
> cpu_max_bits_warn() [1], for commit 78e5a3399421 ("cpumask: fix checking
> valid cpu range") incremented "cpu" side but did not increment
> "nr_cpumask_bits" side.
>
> But incrementing "nr_cpumask_bits" side will be worse than reverting.
>
> Andrew Jones proposed a fix for x86 and riscv architectures [2]. But other
> architectures have the same problem. And fixing all callers will not be
> in time for this merge window.
>
> Regarding this merge window, let's use WARN_ON_ONCE(cpu > nr_cpumask_bits)
> for 4 callers which commit 78e5a3399421ad79 ("cpumask: fix checking valid
> cpu range") shifted. After we fixed all affected callers, we can again use
> WARN_ON_ONCE(cpu >= nr_cpumask_bits).
>
> Link: https://syzkaller.appspot.com/bug?extid=d0fd2bf0dd6da72496dd [1]
> Link: https://lkml.kernel.org/r/20221014155845....@ventanamicro.com [2]
> Reported-by: syzbot <syzbot+d0fd2b...@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin...@I-love.SAKURA.ne.jp>
> Fixes: 78e5a3399421 ("cpumask: fix checking valid cpu range")
> ---
> This is urgent for syzbot and should be sent to linux.git before the merge window closes.
> I think that reverting 78e5a3399421ad79 or applying this patch is the safer choice.
>
> $ git grep -nF cpumask_next | grep -F -- '- 1'
> arch/openrisc/kernel/setup.c:371: *pos = cpumask_next(*pos - 1, cpu_online_mask);
> arch/powerpc/kernel/setup-common.c:346: *pos = cpumask_next(*pos - 1, cpu_online_mask);
> arch/riscv/kernel/cpu.c:216: *pos = cpumask_next(*pos - 1, cpu_online_mask);
> arch/s390/kernel/processor.c:338: *pos = cpumask_next(*pos - 1, cpu_online_mask);
> arch/x86/kernel/cpu/proc.c:156: *pos = cpumask_next(*pos - 1, cpu_online_mask);
> drivers/nvme/host/tcp.c:1475: queue->io_cpu = cpumask_next_wrap(n - 1, cpu_online_mask, -1, false);
> kernel/rcu/rcu.h:363: (cpu) = cpumask_next((rnp)->grplo - 1, cpu_possible_mask); \
> kernel/rcu/tasks.h:297: chosen_cpu = cpumask_next(ideal_cpu - 1, cpu_possible_mask);
> kernel/sched/debug.c:882: n = cpumask_next(n - 1, cpu_online_mask);
> kernel/sched/stats.c:196: n = cpumask_next(n - 1, cpu_online_mask);
> kernel/time/clocksource.c:314: cpu = cpumask_next(cpu - 1, cpu_online_mask);
> $ git grep -nF cpumask_next | grep -F -- '-1'
> arch/loongarch/kernel/acpi.c:92: cpu = cpumask_next_zero(-1, cpu_present_mask);
> drivers/nvme/host/tcp.c:1475: queue->io_cpu = cpumask_next_wrap(n - 1, cpu_online_mask, -1, false);
> kernel/padata.c:267: pd->cpu = cpumask_next_wrap(cpu, pd->cpumask.pcpu, -1, false);
> lib/cpumask_kunit.c:103: KUNIT_EXPECT_EQ_MSG(test, 0, cpumask_next_zero(-1, &mask_empty), MASK_MSG(&mask_empty));
> lib/cpumask_kunit.c:104: KUNIT_EXPECT_LE_MSG(test, nr_cpu_ids, cpumask_next_zero(-1, cpu_possible_mask),
> lib/cpumask_kunit.c:107: KUNIT_EXPECT_LE_MSG(test, nr_cpu_ids, cpumask_next(-1, &mask_empty),
> lib/cpumask_kunit.c:109: KUNIT_EXPECT_EQ_MSG(test, 0, cpumask_next(-1, cpu_possible_mask),
>
> include/linux/cpumask.h | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index 2f065ad97541..858741450567 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -118,6 +118,14 @@ static __always_inline unsigned int cpumask_check(unsigned int cpu)
> return cpu;
> }
>
> +static __always_inline unsigned int cpumask_check2(unsigned int cpu)
> +{
> +#ifdef CONFIG_DEBUG_PER_CPU_MAPS
> + WARN_ON_ONCE(cpu > nr_cpumask_bits);
> +#endif /* CONFIG_DEBUG_PER_CPU_MAPS */
> + return cpu;
> +}
> +
> /**
> * cpumask_first - get the first cpu in a cpumask
> * @srcp: the cpumask pointer
> @@ -175,7 +183,7 @@ static inline
> unsigned int cpumask_next(int n, const struct cpumask *srcp)
> {
> /* n is a prior cpu */
> - cpumask_check(n + 1);
> + cpumask_check2(n + 1);
> return find_next_bit(cpumask_bits(srcp), nr_cpumask_bits, n + 1);
> }
>
> @@ -189,7 +197,7 @@ unsigned int cpumask_next(int n, const struct cpumask *srcp)
> static inline unsigned int cpumask_next_zero(int n, const struct cpumask *srcp)
> {
> /* n is a prior cpu */
> - cpumask_check(n + 1);
> + cpumask_check2(n + 1);
> return find_next_zero_bit(cpumask_bits(srcp), nr_cpumask_bits, n+1);
> }
>
> @@ -230,7 +238,7 @@ unsigned int cpumask_next_and(int n, const struct cpumask *src1p,
> const struct cpumask *src2p)
> {
> /* n is a prior cpu */
> - cpumask_check(n + 1);
> + cpumask_check2(n + 1);
> return find_next_and_bit(cpumask_bits(src1p), cpumask_bits(src2p),
> nr_cpumask_bits, n + 1);
> }
> @@ -261,7 +269,7 @@ unsigned int cpumask_next_wrap(int n, const struct cpumask *mask, int start, boo
> {
> cpumask_check(start);
> /* n is a prior cpu */
> - cpumask_check(n + 1);
> + cpumask_check2(n + 1);
>
> /*
> * Return the first available CPU when wrapping, or when starting before cpu0,
> --
> 2.34.1
>
>

[Tetsuo Handa]

On 2022/10/15 6:24, Borislav Petkov wrote:

> Let's see if the bot understands fetching patches from mailing lists:
>
> # syz test: https://lore.kernel.org/r/20221014155845....@ventanamicro.com
>

That's an invalid command line. The correct syntax is:

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

[Borislav Petkov]

On Sat, Oct 15, 2022 at 08:39:19PM +0900, Tetsuo Handa wrote:
> That's an invalid command line. The correct syntax is:
>
> #syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

The fix is not in Linus' tree yet.

> Andrew Jones proposed a fix for x86 and riscv architectures [2]. But
> other architectures have the same problem. And fixing all callers will
> not be in time for this merge window.

Why won't there be time? That's why the -rcs are for.

Also, that thing fires only when CONFIG_DEBUG_PER_CPU_MAPS is enabled.

So no, we will take Andrew's fixes for all arches in time for 6.1.

[Tetsuo Handa]

On 2022/10/15 20:53, Borislav Petkov wrote:
> Why won't there be time? That's why the -rcs are for.

No. Please apply fixes as soon as possible.

Being able to test kernels in merge window is _critical_ for automated bisection.

More delay, less tested kernel (and more difficult to bisect).

Due to this regression 80% of syzbot resource is wasted.
https://syzkaller.appspot.com/upstream/graph/crashes

> Also, that thing fires only when CONFIG_DEBUG_PER_CPU_MAPS is enabled.

syzbot enables CONFIG_DEBUG_PER_CPU_MAPS.

Due to this regression syzbot is failing to test changes in linux.git since 2022/10/10.
https://syzkaller.appspot.com/bug?id=953424a31925ba8796439aabbb384872c6e9797d

> So no, we will take Andrew's fixes for all arches in time for 6.1.

You need to also check non-arch users.

[Tetsuo Handa]

On 2022/10/15 20:53, Borislav Petkov wrote:

> On Sat, Oct 15, 2022 at 08:39:19PM +0900, Tetsuo Handa wrote:
>> That's an invalid command line. The correct syntax is:
>>
>> #syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
>
> The fix is not in Linus' tree yet.
>

This syntax optionally accepts a patch (either inline or attachment).
That is, I requested syzbot to test my patch on top of linux.git master.

But since syzbot has not found a reproducer (the kernel crashes by just
reading /proc/cpuinfo, which I think happens before starting syzkaller
programs), my request just failed.

Anyway, I manually tested my patch on top of linux.git master, and
confirmed that my patch fixes the warning.

[Tetsuo Handa]

On 2022/10/15 21:08, Tetsuo Handa wrote:
>> Also, that thing fires only when CONFIG_DEBUG_PER_CPU_MAPS is enabled.
>
> syzbot enables CONFIG_DEBUG_PER_CPU_MAPS.
>
> Due to this regression syzbot is failing to test changes in linux.git since 2022/10/10.
> https://syzkaller.appspot.com/bug?id=953424a31925ba8796439aabbb384872c6e9797d

Note that checking for

$ git grep -nF cpumask_next | grep -F -- '- 1'

$ git grep -nF cpumask_next | grep -F -- '-1'

cannot catch all users.
For example, https://syzkaller.appspot.com/bug?id=953424a31925ba8796439aabbb384872c6e9797d
is hitting this problem at

cpu = cpumask_next_wrap(cpu, cpu_online_mask, nr_cpu_ids, false);

. In other words, you need to inspect and fix basically all cpumask users
while making sure that bugs do not disturb syzbot's testing.

If fixes cannot be sent to linux.git immediately, syzbot should consider disabling
CONFIG_DEBUG_PER_CPU_MAPS...

[Tetsuo Handa]

This reverts commit 78e5a3399421 ("cpumask: fix checking valid cpu range").

syzbot is hitting WARN_ON_ONCE(cpu >= nr_cpumask_bits) warning at
cpu_max_bits_warn() [1], for commit 78e5a3399421 ("cpumask: fix checking

valid cpu range") is broken. Obviously that patch hits WARN_ON_ONCE() when
e.g. reading /proc/cpuinfo because passing "cpu + 1" instead of "cpu" will
trivially hit cpu == nr_cpumask_bits condition.

Although syzbot found this problem in linux-next.git on 2022/09/27 [2],
this problem was not fixed immediately. As a result, that patch was sent
to linux.git before the patch author recognizes this problem, and syzbot
started failing to test changes in linux.git since 2022/10/10 [3].

Andrew Jones proposed a fix for x86 and riscv architectures [4]. But [2]
and [5] indicate that affected locations are not limited to arch code.
More delay before we find and fix affected locations, less tested kernel
(and more difficult to bisect and fix) before release.

We should have inspected and fixed basically all cpumask users before
applying that patch. We should not crash kernels in order to ask existing
cpumask users to update their code, even if limited to
CONFIG_DEBUG_PER_CPU_MAPS=y case.

Link: https://syzkaller.appspot.com/bug?extid=d0fd2bf0dd6da72496dd [1]
Link: https://syzkaller.appspot.com/bug?extid=21da700f3c9f0bc40150 [2]
Link: https://syzkaller.appspot.com/bug?extid=51a652e2d24d53e75734 [3]
Link: https://lkml.kernel.org/r/20221014155845....@ventanamicro.com [4]
Link: https://syzkaller.appspot.com/bug?extid=4d46c43d81c3bd155060 [5]
Reported-by: Andrew Jones <ajo...@ventanamicro.com>
Reported-by: syzbot+d0fd2b...@syzkaller.appspotmail.com
Signed-off-by: Tetsuo Handa <penguin...@I-love.SAKURA.ne.jp>
Cc: Yury Norov <yury....@gmail.com>
Cc: Borislav Petkov <b...@alien8.de>
---
include/linux/cpumask.h | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index 2f065ad97541..c2aa0aa26b45 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -174,8 +174,9 @@ static inline unsigned int cpumask_last(const struct cpumask *srcp)
static inline

unsigned int cpumask_next(int n, const struct cpumask *srcp)
{

- /* n is a prior cpu */
- cpumask_check(n + 1);
+ /* -1 is a legal arg here. */
+ if (n != -1)
+ cpumask_check(n);

return find_next_bit(cpumask_bits(srcp), nr_cpumask_bits, n + 1);
}

@@ -188,8 +189,9 @@ unsigned int cpumask_next(int n, const struct cpumask *srcp)
*/

static inline unsigned int cpumask_next_zero(int n, const struct cpumask *srcp)
{

- /* n is a prior cpu */
- cpumask_check(n + 1);
+ /* -1 is a legal arg here. */
+ if (n != -1)
+ cpumask_check(n);
return find_next_zero_bit(cpumask_bits(srcp), nr_cpumask_bits, n+1);
}

@@ -229,8 +231,9 @@ static inline

unsigned int cpumask_next_and(int n, const struct cpumask *src1p,
const struct cpumask *src2p)
{

- /* n is a prior cpu */
- cpumask_check(n + 1);
+ /* -1 is a legal arg here. */
+ if (n != -1)
+ cpumask_check(n);

return find_next_and_bit(cpumask_bits(src1p), cpumask_bits(src2p),
nr_cpumask_bits, n + 1);
}

@@ -260,8 +263,8 @@ static inline
unsigned int cpumask_next_wrap(int n, const struct cpumask *mask, int start, bool wrap)
{
cpumask_check(start);
- /* n is a prior cpu */
- cpumask_check(n + 1);
+ if (n != -1)
+ cpumask_check(n);

/*
* Return the first available CPU when wrapping, or when starting before cpu0,
--

2.18.4

[Yury Norov]

Add people from other threads discussing this.

On Sat, Oct 15, 2022 at 01:53:19PM +0200, Borislav Petkov wrote:
> On Sat, Oct 15, 2022 at 08:39:19PM +0900, Tetsuo Handa wrote:
> > That's an invalid command line. The correct syntax is:
> >
> > #syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
>
> The fix is not in Linus' tree yet.
>
> > Andrew Jones proposed a fix for x86 and riscv architectures [2]. But
> > other architectures have the same problem. And fixing all callers will
> > not be in time for this merge window.
>
> Why won't there be time? That's why the -rcs are for.
>
> Also, that thing fires only when CONFIG_DEBUG_PER_CPU_MAPS is enabled.
>
> So no, we will take Andrew's fixes for all arches in time for 6.1.

Summarizing things:

1. cpumask_check() was introduced to make sure that the cpu number
passed into cpumask API belongs to a valid range. But the check is
broken for a very long time. And because of that there are a lot of
places where cpumask API is used wrongly.

2. Underlying bitmap functions handle that correctly - when user
passes out-of-range CPU index, the nr_cpu_ids is returned, and this is
what expected by client code. So if DEBUG_PER_CPU_MAPS config is off,
everything is working smoothly.

3. I fixed all warnings that I was aware at the time of submitting the
patch. 2 follow-up series are on review: "[PATCH v2 0/4] net: drop
netif_attrmask_next*()" and "[PATCH 0/9] lib/cpumask: simplify
cpumask_next_wrap()". Also, Andrew Jones, Alexander Gordeev and Guo Ren
proposed fixes for c_start() in arch code.

4. The code paths mentioned above are all known to me that violate
cpumask_check() rules. (Did I miss something?)

With all that, I agree with Borislav. Unfortunately, syzcall didn't CC
me about this problem with c_start(). But I don't like the idea to revert
cpumask_check() fix. This way we'll never clean that mess.

If for some reason those warnings are unacceptable for -rcs (and like
Boris, I don't understand why), than instead of reverting commits, I'd
suggest moving cpumask sanity check from DEBUG_PER_CPU_MAPS under a new
config, say CONFIG_CPUMASK_DEBUG, which will be inactive until people will
fix their code. I can send a patch shortly, if we'll decide going this way.

How people would even realize that they're doing something wrong if
they will not get warned about it?

Thanks,
Yury

[Tetsuo Handa]

I'm asking you not to use BUG_ON()/WARN_ON() etc. which breaks syzkaller.
Just printing messages (without "BUG:"/"WARNING:" string which also breaks
syzkaller) like below diff is sufficient for people to realize that they're
doing something wrong.

Again, please do revert "cpumask: fix checking valid cpu range" immediately.

include/linux/cpumask.h | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index c2aa0aa26b45..31af2cc5f0c2 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -118,6 +118,18 @@ static __always_inline unsigned int cpumask_check(unsigned int cpu)
return cpu;
}

+/*
+ * We want to avoid passing -1 as a valid cpu argument.
+ * But we should not crash the kernel until all in-tree callers are fixed.
+ */
+static __always_inline void report_negative_cpuid(void)
+{
+#ifdef CONFIG_DEBUG_PER_CPU_MAPS
+ pr_warn_once("FIXME: Passing -1 as CPU argument needs to be avoided.\n");
+ DO_ONCE_LITE(dump_stack);

+#endif /* CONFIG_DEBUG_PER_CPU_MAPS */
+}

+
/**
* cpumask_first - get the first cpu in a cpumask
* @srcp: the cpumask pointer

@@ -177,6 +189,8 @@ unsigned int cpumask_next(int n, const struct cpumask *srcp)

/* -1 is a legal arg here. */

if (n != -1)
cpumask_check(n);
+ else
+ report_negative_cpuid();

return find_next_bit(cpumask_bits(srcp), nr_cpumask_bits, n + 1);
}

@@ -192,6 +206,8 @@ static inline unsigned int cpumask_next_zero(int n, const struct cpumask *srcp)

/* -1 is a legal arg here. */

if (n != -1)
cpumask_check(n);
+ else
+ report_negative_cpuid();
return find_next_zero_bit(cpumask_bits(srcp), nr_cpumask_bits, n+1);
}

@@ -234,6 +250,8 @@ unsigned int cpumask_next_and(int n, const struct cpumask *src1p,

/* -1 is a legal arg here. */

if (n != -1)
cpumask_check(n);
+ else
+ report_negative_cpuid();

return find_next_and_bit(cpumask_bits(src1p), cpumask_bits(src2p),
nr_cpumask_bits, n + 1);
}

@@ -265,6 +283,8 @@ unsigned int cpumask_next_wrap(int n, const struct cpumask *mask, int start, boo
cpumask_check(start);
if (n != -1)
cpumask_check(n);
+ else
+ report_negative_cpuid();

/*
* Return the first available CPU when wrapping, or when starting before cpu0,
--

2.18.4

>
> Thanks,
> Yury

[Randy Dunlap]

Hi--

Why not say that any negative cpu argument is invalid?
Or is it OK to pass -2 as the cpu arg?

~Randy

[Tetsuo Handa]

On 2022/10/16 9:28, Randy Dunlap wrote:
>> +/*
>> + * We want to avoid passing -1 as a valid cpu argument.
>> + * But we should not crash the kernel until all in-tree callers are fixed.
>> + */
>
> Why not say that any negative cpu argument is invalid?

Currently only -1 is accepted as exception.

>> if (n != -1)
>> cpumask_check(n);
>> + else
>> + report_negative_cpuid();

> Or is it OK to pass -2 as the cpu arg?

Passing -2 will hit WARN_ON_ONCE(cpu >= nr_cpumask_bits) path.

[Yury Norov]

It's not me who added WARN_ON() in the cpumask_check(). You're asking
a wrong person.

What for do we have WARN_ON(), if we can't use it?

> Just printing messages (without "BUG:"/"WARNING:" string which also breaks
> syzkaller) like below diff is sufficient for people to realize that they're
> doing something wrong.
>
> Again, please do revert "cpumask: fix checking valid cpu range" immediately.

The revert is already in Jakub's batch for -rc2, AFAIK.

Thanks,
Yury

[Tetsuo Handa]

On 2022/10/16 10:12, Yury Norov wrote:
> On Sun, Oct 16, 2022 at 09:24:57AM +0900, Tetsuo Handa wrote:
>> I'm asking you not to use BUG_ON()/WARN_ON() etc. which breaks syzkaller.
>
> It's not me who added WARN_ON() in the cpumask_check(). You're asking
> a wrong person.

Because you broke the kernel testing. It was obvious that passing "cpu + 1"

instead of "cpu" will trivially hit cpu == nr_cpumask_bits condition.

If your patch were reviewed and tested, we would not have done this discussion.

>
> What for do we have WARN_ON(), if we can't use it?

WARN_ON() could be used which should not happen.
But cpu == nr_cpumask_bits condition shall happen in your patch.

RCs are not for fixing bugs that causes boot failures. Such bugs
should have been tested and fixed before patches are sent to linux.git .

>
>> Just printing messages (without "BUG:"/"WARNING:" string which also breaks
>> syzkaller) like below diff is sufficient for people to realize that they're
>> doing something wrong.
>>
>> Again, please do revert "cpumask: fix checking valid cpu range" immediately.
>
> The revert is already in Jakub's batch for -rc2, AFAIK.

OK.

[Linus Torvalds]

Hmm.

I've looked at this, and at the discussion, and the various reports,
and my gut feel is that the problem is that the whole
"cpumask_check()" is completely bogus for all the "starting at bit X"
cases.

And I think it was wrong even before, and yes, I think the "+1"
simplification just made things worse.

I think that where it makes sense is in contexts where we actually
*use* the bit value as a bit, so cpumask_clear_cpu() doing

clear_bit(cpumask_check(cpu), cpumask_bits(dstp));

makes 100% sense and is unequivocally something that merits a warning.
An out-of-range cpu number is a serious bug in that context.

But all the cases where the function fundamentally already limits
things to the number of CPU's (with comments like "Returns >=
nr_cpu_ids if no further cpus unset.") should simply not have the
cpumask_check() at all.

All it results in just moving the onus of testing things into the
callers, or just makes for odd complications ("-1 is valid, because it
acts as the previous cpu for the beginning because we add one to get
the next CPU").

Anyway, since rc1 is fairly imminent, I will just revert it for now -
I don't want to have a pending revert wait until -rc2.

But I actually suspect that the thing we should really do is to just
remove the check entirely for these functions that are already defined
in terms of "if no more bits, return nr_cpu_ids". They already
basically return an error case, having them *warn* about the error
they are going to return is just obnoxious.

Linus

[Tetsuo Handa]

On 2022/10/17 2:52, Linus Torvalds wrote:
> Anyway, since rc1 is fairly imminent, I will just revert it for now -
> I don't want to have a pending revert wait until -rc2.

Thank you, Linus.

Yury or Jakub, please send a revert request on commit 854701ba4c39 ("net: fix cpu_max_bits_warn()
usage in netif_attrmask_next{,_and}"), for https://syzkaller.appspot.com/bug?extid=9abe5ecc348676215427
says that boot is still failing.

> But I actually suspect that the thing we should really do is to just
> remove the check entirely for these functions that are already defined
> in terms of "if no more bits, return nr_cpu_ids". They already
> basically return an error case, having them *warn* about the error
> they are going to return is just obnoxious.

I agree.

[Jakub Kicinski]

On Mon, 17 Oct 2022 11:54:01 +0900 Tetsuo Handa wrote:
> On 2022/10/17 2:52, Linus Torvalds wrote:
> > Anyway, since rc1 is fairly imminent, I will just revert it for now -
> > I don't want to have a pending revert wait until -rc2.
>
> Thank you, Linus.
>
> Yury or Jakub, please send a revert request on commit 854701ba4c39 ("net: fix cpu_max_bits_warn()
> usage in netif_attrmask_next{,_and}"), for https://syzkaller.appspot.com/bug?extid=9abe5ecc348676215427
> says that boot is still failing.

Yup, we got that revert queued up. LMK if it's a show stopper
otherwise it'll get to Linus on Thu.