NetBSD Problem Report #55737

From rmtodd@servalan.servalan.com  Tue Oct 20 03:30:07 2020
Return-Path: <rmtodd@servalan.servalan.com>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id DB7E21A9217
	for <gnats-bugs@gnats.NetBSD.org>; Tue, 20 Oct 2020 03:30:07 +0000 (UTC)
Message-Id: <20201020031740.GA27771@ichotolot.servalan.com>
Date: Mon, 19 Oct 2020 22:17:40 -0500
From: Richard Todd <rmtodd@servalan.servalan.com>
Reply-To: rmtodd@servalan.servalan.com
To: gnats-bugs@netbsd.org
Subject: Apparent bug in evbarm64 DMA code causes filesystem corruption
X-Send-Pr-Version: 3.95

>Number:         55737
>Category:       port-arm
>Synopsis:       Apparent bug in evbarm64 DMA code causes filesystem corruption
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    port-arm-maintainer
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Oct 20 03:35:00 +0000 2020
>Last-Modified:  Sat Oct 24 07:25:01 +0000 2020
>Originator:     Richard Todd
>Release:        NetBSD 9.99.72
>Organization:

>Environment:
System: NetBSD netbsdarm64test.servalan.com 9.99.72 NetBSD 9.99.72 (RMTGENERIC_VM_a15d1b7af98f) #3: Thu Oct 8 00:00:34 CDT 2020 sysbuild@materia.servalan.com:/amd/ichotolot/u2/netbsd-vmexport/sysbuild/Build/evbarm64/obj/amd/ichotolot/u2/netbsd-vmexport/sysbuild/src/sys/arch/evbarm/compile/RMTGENERIC_VM evbarm
Architecture: aarch64
Machine: evbarm
>Description:
  As per recent discussion on port-arm
http://mail-index.netbsd.org/port-arm/2020/10/13/msg006997.html
various people, including me, have been seeing filesystem corruption with
recent builds of NetBSD-current on evbarm64.  With a good deal of work, I
managed to get a reproducible test case and narrow the apparent culprit down
to this recent commit:

changeset:   936632:a15d1b7af98f
branch:      trunk
user:        skrll <skrll@NetBSD.org>
date:        Tue Sep 08 10:30:17 2020 +0000
files:       sys/arch/arm/arm32/bus_dma.c
extra:       branch=trunk
description:
A few bus_dmatag_subregion fixes

- return EOPNOTSUPP if min_addr isn't less than max_addr
- fix the subset check to ensure that all the ranges in the parent tag are
  within the {min,max}_addr range.  If so we can just continue to use the
  parent tag.
- when building the new ranges read the parent tag range rather than un-
  initialised memory.
- remove the max_addr != 0xffffffff check - the overflow should be handled
  by the unsigned arithmetic for arm32.
- add a KASSERT
- add comments

I've got a console log here ( https://pastebin.com/eHRG8jGy ) exhibiting an
example reproduction of the problem in my usual qemu testing VM.  The first
part has qemu booting with two virtual disks attached, one the "usual"
NetBSD-current disk image I've been using with that VM, and the second a
copy of said image which was going to get its / newfsed and recreated by
copying from the master.  (This was done to mitigate the possiblity that
these crashes were caused by preexisting corruption on the filesystem,
which was the previous theory on port-arm mailing list regarding the source
of these crashes).  The second part is qemu running off of the "copy" disk
image, and going thru a typical attempt to upgrade to an already-built
NetBSD-current release (built elsewhere, stashed on NFS on another machine)
-- first sysupgrade fetches everything, installs the new kernel, then we
boot the new kernel.  Next we try to use sysupgrade to install world, but
we hit a

 panic: ffs_blkfree: bad size: dev = 0x5c20, bno = 2432893633 bsize = 16384, size = 16384, fs = /

panic in the middle of extracting the tests tar file, and then fail
automatic fsck on reboot.


>How-To-Repeat:
      Install a kernel built from -current after the a15d1b7af98f commit,
      reboot, and attempt to install world with sysupgrade.  
>Fix:
      Revert to a kernel built from -current prior to that commit.
      Alternatively, I just tested a build of -current sources from
      Saturday (rev 1b3515410312, date: Sat Oct 17 23:23:06 2020 +0000) with
      reversing the suspect bus_dma.c patch and the later uvm_bio.c patch of
      October 5 mentioned recently on netbsd-current list (and which has
      since been reverted in -current source).  With those sources, we can do
      the install of kernel and world just fine. 


>Audit-Trail:
From: matthew green <mrg@eterna.com.au>
To: gnats-bugs@netbsd.org, skrll@netbsd.org,
    rmtodd@servalan.servalan.com
Cc: port-arm-maintainer@netbsd.org, gnats-admin@netbsd.org,
    netbsd-bugs@netbsd.org
Subject: re: port-arm/55737: Apparent bug in evbarm64 DMA code causes filesystem corruption
Date: Tue, 20 Oct 2020 16:34:12 +1100

 > - fix the subset check to ensure that all the ranges in the parent tag are
 >   within the {min,max}_addr range.  If so we can just continue to use the
 >   parent tag.

 this part of the patch i think is problematic.

 previously, subset started as false, and was transitioned
 to true, and never back.

 now, it's as false, but it transitions back and forth to
 have the last entry's value.

 i think both are wrong.  it should start true, and if any
 entry is outside the parent range, set to false.


 .mrg.

From: matthew green <mrg@eterna.com.au>
To: port-arm-maintainer@netbsd.org, gnats-admin@netbsd.org,
    netbsd-bugs@netbsd.org, gnats-bugs@netbsd.org, skrll@netbsd.org,
    rmtodd@servalan.servalan.com
Cc: 
Subject: re: port-arm/55737: Apparent bug in evbarm64 DMA code causes filesystem corruption
Date: Tue, 20 Oct 2020 17:31:48 +1100

 matthew green writes:
 > i think both are wrong.  it should start true, and if any
 > entry is outside the parent range, set to false.

 i meant to include this untested patch.  not meant to be
 commited as-is, but should confirm or deny my guess with
 a repro attempt.


 .mrg.


 Index: bus_dma.c
 ===================================================================
 RCS file: /cvsroot/src/sys/arch/arm/arm32/bus_dma.c,v
 retrieving revision 1.123
 diff -p -u -r1.123 bus_dma.c
 --- bus_dma.c	8 Sep 2020 10:30:17 -0000	1.123
 +++ bus_dma.c	20 Oct 2020 06:29:52 -0000
 @@ -1816,7 +1816,7 @@ _bus_dmatag_subregion(bus_dma_tag_t tag,

  #ifdef _ARM32_NEED_BUS_DMA_BOUNCE
  	struct arm32_dma_range *dr;
 -	bool subset = false;
 +	bool subset = true;
  	size_t nranges = 0;
  	size_t i;
  	for (i = 0, dr = tag->_ranges; i < tag->_nranges; i++, dr++) {
 @@ -1826,7 +1826,7 @@ _bus_dmatag_subregion(bus_dma_tag_t tag,
  		 */
  		if (dr->dr_sysbase >= min_addr
  		    && dr->dr_sysbase + dr->dr_len - 1 <= max_addr) {
 -			subset = true;
 +			//subset = true;
  		} else {
  			subset = false;
  		}

From: Nick Hudson <nick.hudson@gmx.co.uk>
To: matthew green <mrg@eterna.com.au>, gnats-bugs@netbsd.org,
 rmtodd@servalan.servalan.com
Cc: port-arm-maintainer@netbsd.org, gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org
Subject: Re: port-arm/55737: Apparent bug in evbarm64 DMA code causes
 filesystem corruption
Date: Tue, 20 Oct 2020 09:03:57 +0100

 This is a multi-part message in MIME format.
 --------------64740B43E995EA09E8BB3D94
 Content-Type: text/plain; charset=utf-8; format=flowed
 Content-Transfer-Encoding: quoted-printable


 On 20/10/2020 06:34, matthew green wrote:
 >> - fix the subset check to ensure that all the ranges in the parent tag =
 are
 >>    within the {min,max}_addr range.  If so we can just continue to use =
 the
 >>    parent tag.
 > this part of the patch i think is problematic.
 >
 > previously, subset started as false, and was transitioned
 > to true, and never back.
 >
 > now, it's as false, but it transitions back and forth to
 > have the last entry's value.
 >
 > i think both are wrong.  it should start true, and if any
 > entry is outside the parent range, set to false.

 You're right the code is flawed.

 I think this patch is correct and covers the case that your patch
 doesn't where there are no ranges in the parent tag.

 I think the virtio patch is still valid, however.

 Thanks,

 Nick


 --------------64740B43E995EA09E8BB3D94
 Content-Type: text/x-patch; charset=UTF-8;
  name="bus_dma.c.diff"
 Content-Transfer-Encoding: quoted-printable
 Content-Disposition: attachment;
  filename="bus_dma.c.diff"

 Index: sys/arch/arm/arm32/bus_dma.c
 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
 RCS file: /cvsroot/src/sys/arch/arm/arm32/bus_dma.c,v
 retrieving revision 1.123
 diff -u -p -r1.123 bus_dma.c
 =2D-- sys/arch/arm/arm32/bus_dma.c	8 Sep 2020 10:30:17 -0000	1.123
 +++ sys/arch/arm/arm32/bus_dma.c	20 Oct 2020 08:00:34 -0000
 @@ -1816,18 +1816,18 @@ _bus_dmatag_subregion(bus_dma_tag_t tag,

  #ifdef _ARM32_NEED_BUS_DMA_BOUNCE
  	struct arm32_dma_range *dr;
 -	bool subset =3D false;
 +	bool subset =3D true;
  	size_t nranges =3D 0;
  	size_t i;
  	for (i =3D 0, dr =3D tag->_ranges; i < tag->_nranges; i++, dr++) {
  		/*
 -		 * Are all the ranges in the parent tag a subset of the new
 -		 * range? If yes, we can continue to use the parent tag.
 +		 * If the new {min,max}_addr are narrower than any of the
 +		 * ranges in the parent tag then we need a new tag;
 +		 * otherwise the parent tag is a subset of the new
 +		 * range and can continue to be used.
  		 */
 -		if (dr->dr_sysbase >=3D min_addr
 -		    && dr->dr_sysbase + dr->dr_len - 1 <=3D max_addr) {
 -			subset =3D true;
 -		} else {
 +		if (min_addr > dr->dr_sysbase
 +		    || max_addr < dr->dr_sysbase + dr->dr_len - 1) {
  			subset =3D false;
  		}
  		if (min_addr <=3D dr->dr_sysbase + dr->dr_len
 @@ -1835,6 +1835,10 @@ _bus_dmatag_subregion(bus_dma_tag_t tag,
  			nranges++;
  		}
  	}
 +	if (nranges =3D=3D 0) {
 +		nranges =3D 1;
 +		subset =3D false;
 +	}
  	if (subset) {
  		*newtag =3D tag;
  		/* if the tag must be freed, add a reference */
 @@ -1842,9 +1846,6 @@ _bus_dmatag_subregion(bus_dma_tag_t tag,
  			(tag->_tag_needs_free)++;
  		return 0;
  	}
 -	if (nranges =3D=3D 0) {
 -		nranges =3D 1;
 -	}

  	const size_t tagsize =3D sizeof(*tag) + nranges * sizeof(*dr);
  	if ((*newtag =3D kmem_intr_zalloc(tagsize,

 --------------64740B43E995EA09E8BB3D94--

From: Richard Todd <rmtodd@servalan.servalan.com>
To: Nick Hudson <nick.hudson@gmx.co.uk>
Cc: matthew green <mrg@eterna.com.au>,  gnats-bugs@netbsd.org,
  port-arm-maintainer@netbsd.org,  gnats-admin@netbsd.org,
  netbsd-bugs@netbsd.org
Subject: Re: port-arm/55737: Apparent bug in evbarm64 DMA code causes
 filesystem corruption
Date: Fri, 23 Oct 2020 02:04:29 -0500

 Nick Hudson <nick.hudson@gmx.co.uk> writes:
 >
 > I think this patch is correct and covers the case that your patch
 > doesn't where there are no ranges in the parent tag.
 >
 > I think the virtio patch is still valid, however.

 Okay, here's the (somewhat delayed) results from my testing that I
 promised earlier. 

 1) The virtio patch you provided earlier (here 
 http://mail-index.netbsd.org/port-arm/2020/10/19/msg007017.html)
 seems to do the trick -- applying that to a recentish -current tree
 (21a63fef3aab date:        Mon Oct 19 09:07:29 2020 +0000, if it matters)
 resulted in a kernel that could withstand three rounds of
 sysupgrade installing kernel and world without a single panic.  So that
 patch looks good.

 2) The patch you just gave for bus_dma.c, applied by itself on top of
 the same -current, does *not* seem to fix the problem; we get what looks
 like the same panic at about the same place.


From: "Nick Hudson" <skrll@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/55737 CVS commit: src/sys/dev/acpi
Date: Sat, 24 Oct 2020 07:21:01 +0000

 Module Name:	src
 Committed By:	skrll
 Date:		Sat Oct 24 07:21:01 UTC 2020

 Modified Files:
 	src/sys/dev/acpi: virtio_acpi.c

 Log Message:
 Use the 64bit DMA tag if its valid.

 There appears to be a bug in virtio / ld_virtio and bounce buffers that
 was triggered when the bus_dmatag_subregion code on arm64 was fixed to
 correctly create a new tag for the 32bit tag vs the system (64bit) tag.
 This change avoids the bug.

 PR/55737: Apparent bug in evbarm64 DMA code causes filesystem corruption


 To generate a diff of this commit:
 cvs rdiff -u -r1.2 -r1.3 src/sys/dev/acpi/virtio_acpi.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

>Unformatted:

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.46 2020/01/03 16:35:01 leot Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2020 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.