linux-kernel - Re: [PATCH] percpu_counter: `percpu_counter_read_positive' should not return negative number

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 6 Jun 2018 12:56:55 -0700
From:   Tejun Heo <tj@...nel.org>
To:     Mathieu Malaterre <malat@...ian.org>
Cc:     Christoph Lameter <cl@...ux.com>,
        Dennis Zhou <dennisszhou@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] percpu_counter: `percpu_counter_read_positive' should
 not return negative number

On Wed, Jun 06, 2018 at 09:39:40PM +0200, Mathieu Malaterre wrote:
> Since function `percpu_counter_add' may result in a signed integer overflow
> the result stored in `fbc->count' could be negative. Make sure that
> function `percpu_counter_read_positive' does not return a negative number
> in this case. This will match behavior when CONFIG_SMP=y.
> 
> Detected wth CONFIG_UBSAN=y
> 
> [76404.888450] ================================================================================
> [76404.888477] UBSAN: Undefined behaviour in ../include/linux/percpu_counter.h:136:13
> [76404.888485] signed integer overflow:
> [76404.888490] 9223308017647617321 + 76624449492175 cannot be represented in type 'long long int'
> [76404.888501] CPU: 0 PID: 0 Comm: swapper Not tainted 4.17.0+ #50
> [76404.888506] Call Trace:
> [76404.888523] [dffedd30] [c0478b90] ubsan_epilogue+0x18/0x4c (unreliable)
> [76404.888533] [dffedd40] [c0479530] handle_overflow+0xbc/0xdc
> [76404.888548] [dffeddc0] [c0439044] cfq_completed_request+0x560/0x1234
> [76404.888557] [dffede40] [c03f3fc4] __blk_put_request+0xb0/0x2dc
> [76404.888570] [dffede80] [c05a81d0] scsi_end_request+0x19c/0x344
> [76404.888579] [dffedeb0] [c05a9954] scsi_io_completion+0x4b4/0x854
> [76404.888592] [dffedf10] [c04046b4] blk_done_softirq+0xe4/0x1e0
> [76404.888605] [dffedf60] [c07ec1d4] __do_softirq+0x16c/0x5f0
> [76404.888617] [dffedfd0] [c0065160] irq_exit+0x110/0x1a8
> [76404.888629] [dffedff0] [c001646c] call_do_irq+0x24/0x3c
> [76404.888641] [c0cdbe80] [c0009a2c] do_IRQ+0x98/0x1a0
> [76404.888649] [c0cdbeb0] [c001b93c] ret_from_except+0x0/0x14
> [76404.888659] --- interrupt: 501 at arch_cpu_idle+0x30/0x78
>                    LR = arch_cpu_idle+0x30/0x78
> [76404.888667] [c0cdbf70] [c0cda000] 0xc0cda000 (unreliable)
> [76404.888679] [c0cdbf80] [c00a3844] do_idle+0xc4/0x158
> [76404.888687] [c0cdbfb0] [c00a3a90] cpu_startup_entry+0x24/0x28
> [76404.888696] [c0cdbfc0] [c097f820] start_kernel+0x47c/0x490
> [76404.888703] [c0cdbff0] [00003444] 0x3444

So, the _positive versions are there to deal with small negative reads
coming from percpu propagation delays.  It's not there to protect
against actual counter overflow although it can't detect that on SMP.
It's just outright wrong to report 0 when the counter actually
overflowed and applying the suggested patch masks a real problem
undetectable.

I think the right thing to do is actually undersatnding what's going
on (why is a 64bit counter overflowing?) and fix the underlying issue.

Thanks.

-- 
tejun