Re: [PATCH] lib/dma-debug: fix bucket_find_contain

[Sebastian Ott]

On Thu, 19 Mar 2015, Andrew Morton wrote:
> On Mon, 16 Mar 2015 19:24:29 +0100 (CET) Sebastian Ott <sebott@xxxxxxxxxxxxxxxxxx> wrote:
> 
> > bucket_find_contain will search the bucket list for a dma_debug_entry. When
> > the entry isn't found it needs to search other buckets too, since only the
> > start address of a dma range is hashed (which might be in a different bucket).
> > 
> > A copy of the dma_debug_entry is used to get the previous hash bucket but when
> > its list is searched the original dma_debug_entry is to be used not its
> > modified copy.
> > 
> > This fixes false "device driver tries to sync DMA memory it has not allocated"
> > warnings.
> 
> The changelog doesn't help me understand how serious this is.  A 4.0
> thing?  -stable?

Hm, this is broken since quite some time and nobody noticed or cared. I
think it doesn't need to be in stable. Also without the patch you'll just
get some false positive debug_dma warnings.

> 
> > --- a/lib/dma-debug.c
> > +++ b/lib/dma-debug.c
> > @@ -361,7 +361,7 @@ static struct dma_debug_entry *bucket_fi
> >  	unsigned int range = 0;
> >  
> >  	while (range <= max_range) {
> > -		entry = __hash_bucket_find(*bucket, &index, containing_match);
> > +		entry = __hash_bucket_find(*bucket, ref, containing_match);
> >  
> >  		if (entry)
> >  			return entry;
> 
> This code just got more confusing :(  Are you sure we aren't supposed to
> use `ref' in bucket_find_contain()'s call to get_hash_bucket()?

The only caller of bucket_find_contain uses get_hash_bucket(ref).

I suspect that in most cases the dma_handle used for dma_sync will lead to
the same bucket as the handle used for dma_map (and we get a hit in the
first __hash_bucket_find).

But if a large dma mapping is created it could span more than one
bucket. So when dma_sync(X) is called it's possible we didn't find the
entry in the bucket related to X. We use index to get the previous bucket
but we still need to search this bucket for X.

When we would use get_hash_bucket(ref) again we would search the bucket
which we've already searched in the first run.

Regards,
Sebastian

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/