Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20180606193940.16126-1-malat@debian.org> <20180606195655.GO1351649@devbig577.frc2.facebook.com>
In-Reply-To: <20180606195655.GO1351649@devbig577.frc2.facebook.com>
From:   Mathieu Malaterre <malat@debian.org>
Date:   Thu, 7 Jun 2018 08:23:44 +0200
Message-ID: <CA+7wUsw6DsKWzPKZxi0xA4YRiBASBDFF203RCuHSdWYWLyRPfQ@mail.gmail.com>
Subject: Re: [PATCH] percpu_counter: `percpu_counter_read_positive' should not
 return negative number
To:     Tejun Heo <tj@kernel.org>
Cc:     Christoph Lameter <cl@linux.com>,
        Dennis Zhou <dennisszhou@gmail.com>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Jun 6, 2018 at 9:57 PM Tejun Heo <tj@kernel.org> wrote:
>
> 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 see, thanks for the explanation.

> 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