[PATCH] Missing critical phys_to_virt in lib/swiotlb.c

[PATCH] Missing critical phys_to_virt in lib/swiotlb.c

[David Moore]

From: David Moore <dcm@acm.org>

Adds missing call to phys_to_virt() in the
lib/swiotlb.c:swiotlb_sync_sg() function.  Without this change, a kernel
panic will always occur whenever a SWIOTLB bounce buffer from a
scatter-gather list gets synced.

Signed-off-by: David Moore <dcm@acm.org>
---

This change was originally part of a larger patch by Jan Beulich, which
was more extensive and doesn't look destined to make it into 2.6.20:
http://kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.20-rc6/2.6.20-rc6-mm3/broken-out/ia64-swiotlb-bug-fixes.patch

However, considering the severity of this one-liner bug, I would like to
request that this simplified patch make it into 2.6.20, despite how
close we are to the final cut.  It fixes real crashes:
http://lists.opensuse.org/opensuse-bugs/2006-12/msg02943.html
http://qa.mandriva.com/show_bug.cgi?id=28224
http://www.pchdtv.com/forum/viewtopic.php?t=2063&sid=a959a14a4c2db0eebaab7b0df56103ce

--- linux-2.6.19.x86_64/lib/swiotlb.c.orig	2007-02-04 13:18:41.000000000 -0500
+++ linux-2.6.19.x86_64/lib/swiotlb.c	2007-02-04 13:19:43.000000000 -0500
@@ -750,7 +750,7 @@ swiotlb_sync_sg(struct device *hwdev, st
 
 	for (i = 0; i < nelems; i++, sg++)
 		if (sg->dma_address != SG_ENT_PHYS_ADDRESS(sg))
-			sync_single(hwdev, (void *) sg->dma_address,
+			sync_single(hwdev, phys_to_virt(sg->dma_address),
 				    sg->dma_length, dir, target);
 }
 

Re: [PATCH] Missing critical phys_to_virt in lib/swiotlb.c

[Stefan Richter]

David Moore wrote:
[...]
> considering the severity of this one-liner bug, I would like to
> request that this simplified patch make it into 2.6.20, despite how
> close we are to the final cut.

So we were too close. Maybe the -stable team likes to have it in 2.6.20.1.

> It fixes real crashes:
> http://lists.opensuse.org/opensuse-bugs/2006-12/msg02943.html
> http://qa.mandriva.com/show_bug.cgi?id=28224
> http://www.pchdtv.com/forum/viewtopic.php?t=2063&sid=a959a14a4c2db0eebaab7b0df56103ce

and FireWire crashes too.

> --- linux-2.6.19.x86_64/lib/swiotlb.c.orig	2007-02-04 13:18:41.000000000 -0500
> +++ linux-2.6.19.x86_64/lib/swiotlb.c	2007-02-04 13:19:43.000000000 -0500
> @@ -750,7 +750,7 @@ swiotlb_sync_sg(struct device *hwdev, st
>  
>  	for (i = 0; i < nelems; i++, sg++)
>  		if (sg->dma_address != SG_ENT_PHYS_ADDRESS(sg))
> -			sync_single(hwdev, (void *) sg->dma_address,
> +			sync_single(hwdev, phys_to_virt(sg->dma_address),
>  				    sg->dma_length, dir, target);
>  }
>  
-- 
Stefan Richter
-=====-=-=== --=- --=--
http://arcgraph.de/sr/

Re: [PATCH] Missing critical phys_to_virt in lib/swiotlb.c

[Andrew Morton]

On Sun, 04 Feb 2007 13:39:40 -0500
David Moore <dcm@MIT.EDU> wrote:

> From: David Moore <dcm@acm.org>
> 
> Adds missing call to phys_to_virt() in the
> lib/swiotlb.c:swiotlb_sync_sg() function.  Without this change, a kernel
> panic will always occur whenever a SWIOTLB bounce buffer from a
> scatter-gather list gets synced.
> 
> Signed-off-by: David Moore <dcm@acm.org>
> ---
> 
> This change was originally part of a larger patch by Jan Beulich, which
> was more extensive and doesn't look destined to make it into 2.6.20:
> http://kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.20-rc6/2.6.20-rc6-mm3/broken-out/ia64-swiotlb-bug-fixes.patch
> 
> However, considering the severity of this one-liner bug, I would like to
> request that this simplified patch make it into 2.6.20, despite how
> close we are to the final cut.  It fixes real crashes:
> http://lists.opensuse.org/opensuse-bugs/2006-12/msg02943.html
> http://qa.mandriva.com/show_bug.cgi?id=28224
> http://www.pchdtv.com/forum/viewtopic.php?t=2063&sid=a959a14a4c2db0eebaab7b0df56103ce
> 
> --- linux-2.6.19.x86_64/lib/swiotlb.c.orig	2007-02-04 13:18:41.000000000 -0500
> +++ linux-2.6.19.x86_64/lib/swiotlb.c	2007-02-04 13:19:43.000000000 -0500
> @@ -750,7 +750,7 @@ swiotlb_sync_sg(struct device *hwdev, st
>  
>  	for (i = 0; i < nelems; i++, sg++)
>  		if (sg->dma_address != SG_ENT_PHYS_ADDRESS(sg))
> -			sync_single(hwdev, (void *) sg->dma_address,
> +			sync_single(hwdev, phys_to_virt(sg->dma_address),
>  				    sg->dma_length, dir, target);
>  }
>  

argh.  I didn't know that Jan's patches fixed crashes.  I thought they were
ia64-only things.

Who maintains the swiotlb code?

I shall queue this up, tag it for 2.6.20.1, then I'll fix up Jan's alleged
ia64 patches to account for that.  I'll still push it through Tony, but the
x86_64/ia64 linkage here seems to be a source of problems.

Re: [PATCH] Missing critical phys_to_virt in lib/swiotlb.c

[Andi Kleen]

On Monday 05 February 2007 22:35, Andrew Morton wrote:
> On Sun, 04 Feb 2007 13:39:40 -0500
> David Moore <dcm@MIT.EDU> wrote:
> 
> > From: David Moore <dcm@acm.org>
> > 
> > Adds missing call to phys_to_virt() in the
> > lib/swiotlb.c:swiotlb_sync_sg() function.  Without this change, a kernel
> > panic will always occur whenever a SWIOTLB bounce buffer from a
> > scatter-gather list gets synced.
> > 
> > Signed-off-by: David Moore <dcm@acm.org>
> > ---
> > 
> > This change was originally part of a larger patch by Jan Beulich, which
> > was more extensive and doesn't look destined to make it into 2.6.20:
> > http://kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.20-rc6/2.6.20-rc6-mm3/broken-out/ia64-swiotlb-bug-fixes.patch
> > 
> > However, considering the severity of this one-liner bug, I would like to
> > request that this simplified patch make it into 2.6.20, despite how
> > close we are to the final cut.  It fixes real crashes:
> > http://lists.opensuse.org/opensuse-bugs/2006-12/msg02943.html
> > http://qa.mandriva.com/show_bug.cgi?id=28224
> > http://www.pchdtv.com/forum/viewtopic.php?t=2063&sid=a959a14a4c2db0eebaab7b0df56103ce
> > 
> > --- linux-2.6.19.x86_64/lib/swiotlb.c.orig	2007-02-04 13:18:41.000000000 -0500
> > +++ linux-2.6.19.x86_64/lib/swiotlb.c	2007-02-04 13:19:43.000000000 -0500
> > @@ -750,7 +750,7 @@ swiotlb_sync_sg(struct device *hwdev, st
> >  
> >  	for (i = 0; i < nelems; i++, sg++)
> >  		if (sg->dma_address != SG_ENT_PHYS_ADDRESS(sg))
> > -			sync_single(hwdev, (void *) sg->dma_address,
> > +			sync_single(hwdev, phys_to_virt(sg->dma_address),
> >  				    sg->dma_length, dir, target);
> >  }
> >  
> 
> argh.  I didn't know that Jan's patches fixed crashes.  I thought they were
> ia64-only things.

Sounds weird. If this really didn't work much more should be broken
(e.g. no cdroms/sound on Intel x86-64 boxes with >4GB) 
I'm a little sceptical. Perhaps the TV driver is doing something bogus
here? 

Also I haven't heard of this problem before at all and I'm sure I would
have if sounds/cdroms were broken.

Shouldn't be applied without further analysis.

> Who maintains the swiotlb code?

Nobody. But I hacked last on it.

-Andi

Re: [PATCH] Missing critical phys_to_virt in lib/swiotlb.c

[Stefan Richter]

On 2/6/2007 8:56 AM, Andi Kleen wrote:
>> > From: David Moore <dcm@acm.org>
...
>> > It fixes real crashes:
>> > http://lists.opensuse.org/opensuse-bugs/2006-12/msg02943.html
>> > http://qa.mandriva.com/show_bug.cgi?id=28224
>> > http://www.pchdtv.com/forum/viewtopic.php?t=2063&sid=a959a14a4c2db0eebaab7b0df56103ce
>> > 
>> > --- linux-2.6.19.x86_64/lib/swiotlb.c.orig	2007-02-04 13:18:41.000000000 -0500
>> > +++ linux-2.6.19.x86_64/lib/swiotlb.c	2007-02-04 13:19:43.000000000 -0500
>> > @@ -750,7 +750,7 @@ swiotlb_sync_sg(struct device *hwdev, st
>> >  
>> >  	for (i = 0; i < nelems; i++, sg++)
>> >  		if (sg->dma_address != SG_ENT_PHYS_ADDRESS(sg))
>> > -			sync_single(hwdev, (void *) sg->dma_address,
>> > +			sync_single(hwdev, phys_to_virt(sg->dma_address),
>> >  				    sg->dma_length, dir, target);
>> >  }
...
> Sounds weird. If this really didn't work much more should be broken
> (e.g. no cdroms/sound on Intel x86-64 boxes with >4GB)

Yes, it's weird; e.g. it seems that reports of that on linux1394-user
and -devel came in only recently. But I may have missed something.
(OTOH, this bug was partially hidden for FireWire users because one of
the relevant FireWire drivers was lacking respective _sync_sg calls.)

> I'm a little sceptical. Perhaps the TV driver is doing something bogus
> here? 

sync_single() definitely wants a virtual address there. sg->dma_address
is a physical address. The bug and the fix are obvious.

Unfortunately an author of lib/swiotlb.c chose to call many variables
holding *virtual* addresses "dma_addr". Note how that file at the same
time contains variables like "dma_addr_t dma_handle". A recipe for disaster.

> Also I haven't heard of this problem before at all and I'm sure I would
> have if sounds/cdroms were broken.
> 
> Shouldn't be applied without further analysis.

Maybe some recent changes elsewhere cause more frequent use of
swiotlb-driven bounce buffers? That should be verified.

Or maybe people are deploying affected hardware more often now?
-- 
Stefan Richter
-=====-=-=== --=- --==-
http://arcgraph.de/sr/

Re: [PATCH] Missing critical phys_to_virt in lib/swiotlb.c

[Stefan Richter]

I wrote:
> Unfortunately an author of lib/swiotlb.c chose to call many variables
> holding *virtual* addresses "dma_addr". Note how that file at the same
> time contains variables like "dma_addr_t dma_handle".

And there is even one occurrence of a "dma_addr" holding a physical
address/ bus address, unlike the other dma_addr's:
int
swiotlb_dma_mapping_error(dma_addr_t dma_addr)
{
	return (dma_addr == virt_to_phys(io_tlb_overflow_buffer));
}
-- 
Stefan Richter
-=====-=-=== --=- --==-
http://arcgraph.de/sr/

Re: [PATCH] Missing critical phys_to_virt in lib/swiotlb.c

[Jan Beulich]

>Shouldn't be applied without further analysis.

I don't think further analysis is required before this change be done, the missing
conversion is rather obvious when comparing to all other functions in that file.

Whether the observed crashes really origin from the missing bits here is a
different question.

Jan