[PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash issue

[PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash issue

[Aisheng.Dong]

The map_single does not check if dev is NULL which may cause kernel panic
like follows:
Unable to handle kernel NULL pointer dereference at virtual address 000000bc
pgd = 80004000
[000000bc] *pgd=00000000
Internal error: Oops: 5 [#1] PREEMPT
last sysfs file:
Modules linked in:
CPU: 0    Not tainted  (2.6.35.3-00845-g204c152-dirty #112)
PC is at dma_map_single+0x2c/0x2a0
LR is at fec_enet_interrupt+0x1d4/0x450
pc : [<8003cf04>]    lr : [<802936b8>]    psr: 60000193
sp : 80809ec8  ip : 00000080  fp : 99308040
r10: 99308040  r9 : 00000002  r8 : 99074f00
r7 : fa0a5000  r6 : 00000000  r5 : 00000060  r4 : 992e4000
r3 : 00000002  r2 : 8080cac0  r1 : 99308040  r0 : 00000000

The patch checks the dev to avoid this issue and align with the
original dma_map_single to allow dev is NULL.

Signed-off-by: Aisheng.Dong <b29396@freescale.com>
Signed-off-by: Jason.Liu <r64343@freescale.com>
---
 arch/arm/common/dmabounce.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arm/common/dmabounce.c b/arch/arm/common/dmabounce.c
index e568163..8b575ef 100644
--- a/arch/arm/common/dmabounce.c
+++ b/arch/arm/common/dmabounce.c
@@ -222,16 +222,19 @@ static struct safe_buffer *find_safe_buffer_dev(struct device *dev,
 static inline dma_addr_t map_single(struct device *dev, void *ptr, size_t size,
 		enum dma_data_direction dir)
 {
-	struct dmabounce_device_info *device_info = dev->archdata.dmabounce;
+	struct dmabounce_device_info *device_info;
 	dma_addr_t dma_addr;
 	int needs_bounce = 0;
 
+	if (dev)
+		device_info = dev->archdata.dmabounce;
+
 	if (device_info)
 		DO_STATS ( device_info->map_op_count++ );
 
 	dma_addr = virt_to_dma(dev, ptr);
 
-	if (dev->dma_mask) {
+	if (dev && dev->dma_mask) {
 		unsigned long mask = *dev->dma_mask;
 		unsigned long limit;
 
@@ -248,7 +251,7 @@ static inline dma_addr_t map_single(struct device *dev, void *ptr, size_t size,
 		needs_bounce = (dma_addr | (dma_addr + size - 1)) & ~mask;
 	}
 
-	if (device_info && (needs_bounce || dma_needs_bounce(dev, dma_addr, size))) {
+	if (dev && device_info && (needs_bounce || dma_needs_bounce(dev, dma_addr, size))) {
 		struct safe_buffer *buf;
 
 		buf = alloc_safe_buffer(device_info, ptr, size, dir);
-- 
1.7.0.4

[PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash issue

[Mikael Pettersson]

Aisheng.Dong writes:
 >  static inline dma_addr_t map_single(struct device *dev, void *ptr, size_t size,
 >  		enum dma_data_direction dir)
 >  {
 > -	struct dmabounce_device_info *device_info = dev->archdata.dmabounce;
 > +	struct dmabounce_device_info *device_info;
 >  	dma_addr_t dma_addr;
 >  	int needs_bounce = 0;
 >  
 > +	if (dev)
 > +		device_info = dev->archdata.dmabounce;
 > +
 >  	if (device_info)
 >  		DO_STATS ( device_info->map_op_count++ );

If dev == NULL then device_info is uninitialized in this if-test.
(gcc should have warned about that.  Did it?)

Just pre-init device_info to NULL, then you can also remove the
following change:

 > -	if (device_info && (needs_bounce || dma_needs_bounce(dev, dma_addr, size))) {
 > +	if (dev && device_info && (needs_bounce || dma_needs_bounce(dev, dma_addr, size))) {

[PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash issue

[Russell King - ARM Linux]

On Thu, Mar 24, 2011 at 07:16:40PM +0800, Aisheng.Dong wrote:
> The map_single does not check if dev is NULL which may cause kernel panic
> like follows:
> Unable to handle kernel NULL pointer dereference at virtual address 000000bc
> pgd = 80004000
> [000000bc] *pgd=00000000
> Internal error: Oops: 5 [#1] PREEMPT
> last sysfs file:
> Modules linked in:
> CPU: 0    Not tainted  (2.6.35.3-00845-g204c152-dirty #112)
> PC is at dma_map_single+0x2c/0x2a0
> LR is at fec_enet_interrupt+0x1d4/0x450
> pc : [<8003cf04>]    lr : [<802936b8>]    psr: 60000193
> sp : 80809ec8  ip : 00000080  fp : 99308040
> r10: 99308040  r9 : 00000002  r8 : 99074f00
> r7 : fa0a5000  r6 : 00000000  r5 : 00000060  r4 : 992e4000
> r3 : 00000002  r2 : 8080cac0  r1 : 99308040  r0 : 00000000
> 
> The patch checks the dev to avoid this issue and align with the
> original dma_map_single to allow dev is NULL.

Is there a reason that the FEC code can't pass the appropriate struct
device?

[PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash issue

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Mikael Pettersson [mailto:mikpe at it.uu.se]
> Sent: Thursday, March 24, 2011 9:56 PM
> To: Dong Aisheng-B29396
> Cc: linux-arm-kernel at lists.infradead.org; linux at arm.linux.org.uk
> Subject: Re: [PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash
> issue
> 
> Aisheng.Dong writes:
>  >  static inline dma_addr_t map_single(struct device *dev, void *ptr,
> size_t size,
>  >  		enum dma_data_direction dir)
>  >  {
>  > -	struct dmabounce_device_info *device_info = dev->archdata.dmabounce;
>  > +	struct dmabounce_device_info *device_info;
>  >  	dma_addr_t dma_addr;
>  >  	int needs_bounce = 0;
>  >
>  > +	if (dev)
>  > +		device_info = dev->archdata.dmabounce;
>  > +
>  >  	if (device_info)
>  >  		DO_STATS ( device_info->map_op_count++ );
> 
> If dev == NULL then device_info is uninitialized in this if-test.
> (gcc should have warned about that.  Did it?)
I did not see gcc warning with -Wall, maybe caused by inline and -O2 option.
(Without inclined it, I could see the warning)
Anyway, I think you're right and we should pre-init device_info.
Thanks for the info.

> Just pre-init device_info to NULL, then you can also remove the following
> change:
> 
>  > -	if (device_info && (needs_bounce || dma_needs_bounce(dev, dma_addr,
> size))) {
>  > +	if (dev && device_info && (needs_bounce || dma_needs_bounce(dev,
> dma_addr, size))) {

[PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash issue

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Friday, March 25, 2011 4:37 PM
> To: Dong Aisheng-B29396
> Cc: linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash
> issue
> 
> On Thu, Mar 24, 2011 at 07:16:40PM +0800, Aisheng.Dong wrote:
> > The map_single does not check if dev is NULL which may cause kernel
> > panic like follows:
> > Unable to handle kernel NULL pointer dereference at virtual address
> > 000000bc pgd = 80004000 [000000bc] *pgd=00000000 Internal error: Oops:
> > 5 [#1] PREEMPT last sysfs file:
> > Modules linked in:
> > CPU: 0    Not tainted  (2.6.35.3-00845-g204c152-dirty #112)
> > PC is at dma_map_single+0x2c/0x2a0
> > LR is at fec_enet_interrupt+0x1d4/0x450
> > pc : [<8003cf04>]    lr : [<802936b8>]    psr: 60000193
> > sp : 80809ec8  ip : 00000080  fp : 99308040
> > r10: 99308040  r9 : 00000002  r8 : 99074f00
> > r7 : fa0a5000  r6 : 00000000  r5 : 00000060  r4 : 992e4000
> > r3 : 00000002  r2 : 8080cac0  r1 : 99308040  r0 : 00000000
> >
> > The patch checks the dev to avoid this issue and align with the
> > original dma_map_single to allow dev is NULL.
> 
> Is there a reason that the FEC code can't pass the appropriate struct
> device?

No special reason.
But dma_map_single allows the 'dev' passed in to be NULL, so, may it be better to also check it for dmabounce in case such a using?
I searched the kernel, there are also many other places calling dma_map_single without passing a dev.
Thus, they may also face the same issue if they use dmabounce.

[PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash issue

[Russell King - ARM Linux]

On Mon, Mar 28, 2011 at 05:01:31AM +0000, Dong Aisheng-B29396 wrote:
> But dma_map_single allows the 'dev' passed in to be NULL, so, may it be
> better to also check it for dmabounce in case such a using?

Programmers are lazy.  They often have a struct device laying around
but they think they'll not bother passing it into the DMA API and
end up just passing NULL as that seems to work for them.

That doesn't mean it'll always work.  The best thing is to always pass
in a struct device if there is one available.

I'm not saying don't for the dmabounce code.  I'm saying _also_ fix the
driver.

[PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash issue

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Tuesday, March 29, 2011 1:59 AM
> To: Dong Aisheng-B29396
> Cc: linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash
> issue
> 
> On Mon, Mar 28, 2011 at 05:01:31AM +0000, Dong Aisheng-B29396 wrote:
> > But dma_map_single allows the 'dev' passed in to be NULL, so, may it
> > be better to also check it for dmabounce in case such a using?
> 
> Programmers are lazy.  They often have a struct device laying around but
> they think they'll not bother passing it into the DMA API and end up just
> passing NULL as that seems to work for them.
> 
> That doesn't mean it'll always work.  The best thing is to always pass in
> a struct device if there is one available.
> 
> I'm not saying don't for the dmabounce code.  I'm saying _also_ fix the
> driver.

I totally agree with you.

But due to current kernel already has many using like this (passing NULL
dev to DMA API). (see the following grep result)
b29396 at shlinux2:~/linux-2.6-imx$ grep -rn "dma_map_single(NULL" .
./drivers/net/arm/at91_ether.c:819:             lp->skb_physaddr = dma_map_single(NULL, skb->data, skb->len, DMA_TO_DEVICE);
./drivers/net/arm/ep93xx_eth.c:509:             d = dma_map_single(NULL, page, PAGE_SIZE, DMA_FROM_DEVICE);
./drivers/net/arm/ep93xx_eth.c:532:             d = dma_map_single(NULL, page, PAGE_SIZE, DMA_TO_DEVICE);
./drivers/net/fec.c:555:                bdp->cbd_bufaddr = dma_map_single(NULL, data, bdp->cbd_datlen,
./drivers/net/meth.c:229:                       dma_map_single(NULL, priv->rx_ring[i],
./drivers/net/meth.c:447:                       dma_map_single(NULL, priv->rx_ring[priv->rx_write],
./drivers/net/meth.c:630:       catbuf = dma_map_single(NULL, buffer_data, buffer_len,
./drivers/net/meth.c:656:       catbuf1 = dma_map_single(NULL, buffer1_data, buffer1_len,
./drivers/net/meth.c:661:       catbuf2 = dma_map_single(NULL, buffer2_data, buffer2_len,
./drivers/net/tsi108_eth.c:706:                 data->txring[tx].buf0 = dma_map_single(NULL, skb->data,
./drivers/net/tsi108_eth.c:810:         data->rxring[rx].buf0 = dma_map_single(NULL, skb->data,
./drivers/net/pxa168_eth.c:362:         p_used_rx_desc->buf_ptr = dma_map_single(NULL,
./drivers/net/pxa168_eth.c:1269:        desc->buf_ptr = dma_map_single(NULL, skb->data, length, DMA_TO_DEVICE);
./drivers/media/video/vino.c:742:                       dma_map_single(NULL,
./drivers/media/video/vino.c:829:                       dma_map_single(NULL,
./drivers/media/video/vino.c:4199:      dma_dummy_address = dma_map_single(NULL,
./drivers/parport/parport_ip32.c:604:   parport_ip32_dma.buf = dma_map_single(NULL, addr, count, dir);
./arch/arm/mach-bcmring/dma.c:1882:                         dma_map_single(NULL, mem, numBytes, memMap->dir);
./arch/arm/kernel/dma-isa.c:92:                 dma->buf.dma_address = dma_map_single(NULL,
./arch/arm/mach-rpc/dma.c:171:                  idma->dma.buf.dma_address = dma_map_single(NULL,

If you want me to fix FEC driver, I can fix it in another patch.
But for others I'm not familiar with them and not sure all of them are ok to fix.

And in fact, the dma_map_single allows the @dev to be NULL for special using.
/**
 * dma_map_single - map a single buffer for streaming DMA
 * @dev: valid struct device pointer, or NULL for ISA and EISA-like devices

[PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash issue

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Dong Aisheng-B29396
> Sent: Tuesday, March 29, 2011 11:04 AM
> To: 'Russell King - ARM Linux'
> Cc: linux-arm-kernel at lists.infradead.org
> Subject: RE: [PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash
> issue
> 
> > -----Original Message-----
> > From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> > Sent: Tuesday, March 29, 2011 1:59 AM
> > To: Dong Aisheng-B29396
> > Cc: linux-arm-kernel at lists.infradead.org
> > Subject: Re: [PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash
> > issue
> >
> > On Mon, Mar 28, 2011 at 05:01:31AM +0000, Dong Aisheng-B29396 wrote:
> > > But dma_map_single allows the 'dev' passed in to be NULL, so, may it
> > > be better to also check it for dmabounce in case such a using?
> >
> > Programmers are lazy.  They often have a struct device laying around
> > but they think they'll not bother passing it into the DMA API and end
> > up just passing NULL as that seems to work for them.
> >
> > That doesn't mean it'll always work.  The best thing is to always pass
> > in a struct device if there is one available.
> >
> > I'm not saying don't for the dmabounce code.  I'm saying _also_ fix
> > the driver.
> 
> I totally agree with you.
> 
> But due to current kernel already has many using like this (passing NULL
> dev to DMA API). (see the following grep result) b29396 at shlinux2:~/linux-
> 2.6-imx$ grep -rn "dma_map_single(NULL" .
> ./drivers/net/arm/at91_ether.c:819:             lp->skb_physaddr =
> dma_map_single(NULL, skb->data, skb->len, DMA_TO_DEVICE);
> ./drivers/net/arm/ep93xx_eth.c:509:             d = dma_map_single(NULL,
> page, PAGE_SIZE, DMA_FROM_DEVICE);
> ./drivers/net/arm/ep93xx_eth.c:532:             d = dma_map_single(NULL,
> page, PAGE_SIZE, DMA_TO_DEVICE);
> ./drivers/net/fec.c:555:                bdp->cbd_bufaddr =
> dma_map_single(NULL, data, bdp->cbd_datlen,
> ./drivers/net/meth.c:229:                       dma_map_single(NULL,
> priv->rx_ring[i],
> ./drivers/net/meth.c:447:                       dma_map_single(NULL,
> priv->rx_ring[priv->rx_write],
> ./drivers/net/meth.c:630:       catbuf = dma_map_single(NULL, buffer_data,
> buffer_len,
> ./drivers/net/meth.c:656:       catbuf1 = dma_map_single(NULL,
> buffer1_data, buffer1_len,
> ./drivers/net/meth.c:661:       catbuf2 = dma_map_single(NULL,
> buffer2_data, buffer2_len,
> ./drivers/net/tsi108_eth.c:706:                 data->txring[tx].buf0 =
> dma_map_single(NULL, skb->data,
> ./drivers/net/tsi108_eth.c:810:         data->rxring[rx].buf0 =
> dma_map_single(NULL, skb->data,
> ./drivers/net/pxa168_eth.c:362:         p_used_rx_desc->buf_ptr =
> dma_map_single(NULL,
> ./drivers/net/pxa168_eth.c:1269:        desc->buf_ptr =
> dma_map_single(NULL, skb->data, length, DMA_TO_DEVICE);
> ./drivers/media/video/vino.c:742:
> dma_map_single(NULL,
> ./drivers/media/video/vino.c:829:
> dma_map_single(NULL,
> ./drivers/media/video/vino.c:4199:      dma_dummy_address =
> dma_map_single(NULL,
> ./drivers/parport/parport_ip32.c:604:   parport_ip32_dma.buf =
> dma_map_single(NULL, addr, count, dir);
> ./arch/arm/mach-bcmring/dma.c:1882:
> dma_map_single(NULL, mem, numBytes, memMap->dir);
> ./arch/arm/kernel/dma-isa.c:92:                 dma->buf.dma_address =
> dma_map_single(NULL,
> ./arch/arm/mach-rpc/dma.c:171:                  idma->dma.buf.dma_address
> = dma_map_single(NULL,
> 
> If you want me to fix FEC driver, I can fix it in another patch.
> But for others I'm not familiar with them and not sure all of them are ok
> to fix.
> 
> And in fact, the dma_map_single allows the @dev to be NULL for special
> using.
> /**
>  * dma_map_single - map a single buffer for streaming DMA
>  * @dev: valid struct device pointer, or NULL for ISA and EISA-like
> devices

Hi Russell,
Any more comment about this patch?