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

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

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

Index: sys/arch/arm/arm32/bus_dma.c
===================================================================
RCS file: /cvsroot/src/sys/arch/arm/arm32/bus_dma.c,v
retrieving revision 1.123
diff -u -p -r1.123 bus_dma.c
--- 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 = false;
+	bool subset = true;
 	size_t nranges = 0;
 	size_t i;
 	for (i = 0, dr = 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 >= min_addr
-		    && dr->dr_sysbase + dr->dr_len - 1 <= max_addr) {
-			subset = true;
-		} else {
+		if (min_addr > dr->dr_sysbase
+		    || max_addr < dr->dr_sysbase + dr->dr_len - 1) {
 			subset = false;
 		}
 		if (min_addr <= dr->dr_sysbase + dr->dr_len
@@ -1835,6 +1835,10 @@ _bus_dmatag_subregion(bus_dma_tag_t tag,
 			nranges++;
 		}
 	}
+	if (nranges == 0) {
+		nranges = 1;
+		subset = false;
+	}
 	if (subset) {
 		*newtag = 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 == 0) {
-		nranges = 1;
-	}

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