Re: port-arm/55737: Apparent bug in evbarm64 DMA code causes filesystem corruption

[Nick Hudson <nick.hudson%gmx.co.uk@localhost>]

The following reply was made to PR port-arm/55737; it has been noted by GNATS.

From: Nick Hudson <nick.hudson%gmx.co.uk@localhost>
To: matthew green <mrg%eterna.com.au@localhost>, gnats-bugs%netbsd.org@localhost,
 rmtodd%servalan.servalan.com@localhost
Cc: port-arm-maintainer%netbsd.org@localhost, gnats-admin%netbsd.org@localhost,
 netbsd-bugs%netbsd.org@localhost
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--