Re: [PATCH] arm64: cache: Lower ARCH_DMA_MINALIGN to 64 (L1_CACHE_BYTES)

From: Arnd Bergmann <arnd@arndb.de>
To: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>,
	emma@anholt.net, mripard@kernel.org,
	 Marek Szyprowski <m.szyprowski@samsung.com>,
	linux-arm-kernel@lists.infradead.org,  kernel-team@android.com,
	Catalin Marinas <catalin.marinas@arm.com>,
	 Ard Biesheuvel <ardb@kernel.org>,
	Vincent Whitchurch <vincent.whitchurch@axis.com>,
	 Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH] arm64: cache: Lower ARCH_DMA_MINALIGN to 64 (L1_CACHE_BYTES)
Date: Mon, 7 Jun 2021 15:57:41 +0200	[thread overview]
Message-ID: <CAK8P3a0sj0qtC0VpQv4+Ah-C8jyZaGgfqsx326mChuW+e5mvrQ@mail.gmail.com> (raw)
In-Reply-To: <20210607133953.GB7330@willie-the-truck>

On Mon, Jun 7, 2021 at 3:39 PM Will Deacon <will@kernel.org> wrote:
>
> [Adding VC4 folks -- please see the KASAN splat below!]
>
> Background here is that reducing ARCH_DMA_MINALIGN to 64 on arm64 (queued in
> -next) is causing vc4 to hang on Rpi3b due to a probable driver bug.

The great news for the patch that caused it is that this has nothing to
do with DMA alignment.

> On Mon, Jun 07, 2021 at 02:08:59PM +0100, Mark Rutland wrote:
> > On Mon, Jun 07, 2021 at 01:01:18PM +0100, Mark Rutland wrote:
> > > On Mon, Jun 07, 2021 at 11:58:32AM +0200, Marek Szyprowski wrote:

> > [    3.728042] BUG: KASAN: slab-out-of-bounds in vc4_atomic_commit_tail+0x1cc/0x910
> > [    3.728123] Read of size 8 at addr ffff000007360440 by task kworker/u8:0/7

This is offset 0x40 into struct vc4_hvs_state, which is the
'pending_commit' pointer
for the array index 4, i.e. one after the end of the structure.

> > [    3.728495]  kasan_report+0x1dc/0x240
> > [    3.728529]  __asan_load8+0x98/0xd4
> > [    3.728565]  vc4_atomic_commit_tail+0x1cc/0x910

It seems to be this loop:

        for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
                struct vc4_crtc_state *vc4_crtc_state =
                        to_vc4_crtc_state(old_crtc_state);
                unsigned int channel = vc4_crtc_state->assigned_channel;
                int ret;

                if (channel == VC4_HVS_CHANNEL_DISABLED)
                        continue;

                if (!old_hvs_state->fifo_state[channel].in_use)
                        continue;

                ret =
drm_crtc_commit_wait(old_hvs_state->fifo_state[i].pending_commit);
                if (ret)
                        drm_err(dev, "Timed out waiting for commit\n");
        }

I notice that it checks index 'fifos_state[channel].in_use', but then
uses a different index 'i' for looking at the 'pending_commit' field
beyond the end of the array.

This code was introduced by Maxime Ripard in commit 9ec03d7f1ed3
 ("drm/vc4: kms: Wait on previous FIFO users before a commit").

    Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel