[PATCH] IOMMU SG paranoia

[Olof Johansson]

On Mon, Jan 30, 2006 at 09:51:54PM -0600, Jake Moilanen wrote:
> This patch addresses two items, which are unlikely to be hit if we
> trust drivers.
> 
> The first is moving a memory barrier below where the vmerged SG count
> is passed back, but before the list is set to end.  If those
> instructions were reordered, there could be an issue in iommu_unmap_sg().

Hmm. I'm not sure how instruction reordering could cause this, but a
speculative read and execution down the error path of the driver and it
calling the unmap function could read and use the previous value. It's
amazingly unlikely to happen given the number of tests and branches
between the two, but it doesn't hurt to move the barrier down.

> The second is making sure we terminate the list on the failure case of
> iommu_map_sg().  If a driver does not look at the failure return code,
> it could pass a ill-formed SG list to iommu_unmap_sg().

Only the first entry should need to be set to 0, you're doing a bit of
extra work. It's an error path so it shouldn't be critical.


> Signed-off-by: Jake Moilanen <moilanen at austin.ibm.com>

Acked-by: Olof Johansson <olof at lixom.net>


> Index: 2.6.15/arch/powerpc/kernel/iommu.c
> ===================================================================
> --- 2.6.15.orig/arch/powerpc/kernel/iommu.c	2006-01-03 14:51:31.000000000 -0600
> +++ 2.6.15/arch/powerpc/kernel/iommu.c	2006-01-30 21:30:06.000000000 -0600
> @@ -334,9 +334,6 @@
>  
>  	spin_unlock_irqrestore(&(tbl->it_lock), flags);
>  
> -	/* Make sure updates are seen by hardware */
> -	mb();
> -
>  	DBG("mapped %d elements:\n", outcount);
>  
>  	/* For the sake of iommu_unmap_sg, we clear out the length in the
> @@ -347,6 +344,10 @@
>  		outs->dma_address = DMA_ERROR_CODE;
>  		outs->dma_length = 0;
>  	}
> +
> +	/* Make sure updates are seen by hardware */
> +	mb();
> +
>  	return outcount;
>  
>   failure:
> @@ -358,6 +359,8 @@
>  			npages = (PAGE_ALIGN(s->dma_address + s->dma_length) - vaddr)
>  				>> PAGE_SHIFT;
>  			__iommu_free(tbl, vaddr, npages);
> +			s->dma_address = DMA_ERROR_CODE;
> +			s->dma_length = 0;
>  		}
>  	}
>  	spin_unlock_irqrestore(&(tbl->it_lock), flags);