* [PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash issue @ 2011-03-24 11:16 Aisheng.Dong 2011-03-24 13:56 ` Mikael Pettersson 2011-03-25 8:36 ` Russell King - ARM Linux 0 siblings, 2 replies; 8+ messages in thread From: Aisheng.Dong @ 2011-03-24 11:16 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash issue 2011-03-24 11:16 [PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash issue Aisheng.Dong @ 2011-03-24 13:56 ` Mikael Pettersson 2011-03-28 4:48 ` Dong Aisheng-B29396 2011-03-25 8:36 ` Russell King - ARM Linux 1 sibling, 1 reply; 8+ messages in thread From: Mikael Pettersson @ 2011-03-24 13:56 UTC (permalink / raw) To: linux-arm-kernel 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))) { ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash issue 2011-03-24 13:56 ` Mikael Pettersson @ 2011-03-28 4:48 ` Dong Aisheng-B29396 0 siblings, 0 replies; 8+ messages in thread From: Dong Aisheng-B29396 @ 2011-03-28 4:48 UTC (permalink / raw) To: linux-arm-kernel > -----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))) { ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash issue 2011-03-24 11:16 [PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash issue Aisheng.Dong 2011-03-24 13:56 ` Mikael Pettersson @ 2011-03-25 8:36 ` Russell King - ARM Linux 2011-03-28 5:01 ` Dong Aisheng-B29396 1 sibling, 1 reply; 8+ messages in thread From: Russell King - ARM Linux @ 2011-03-25 8:36 UTC (permalink / raw) To: linux-arm-kernel 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? ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash issue 2011-03-25 8:36 ` Russell King - ARM Linux @ 2011-03-28 5:01 ` Dong Aisheng-B29396 2011-03-28 17:58 ` Russell King - ARM Linux 0 siblings, 1 reply; 8+ messages in thread From: Dong Aisheng-B29396 @ 2011-03-28 5:01 UTC (permalink / raw) To: linux-arm-kernel > -----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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash issue 2011-03-28 5:01 ` Dong Aisheng-B29396 @ 2011-03-28 17:58 ` Russell King - ARM Linux 2011-03-29 3:04 ` Dong Aisheng-B29396 2011-03-30 10:53 ` Dong Aisheng-B29396 0 siblings, 2 replies; 8+ messages in thread From: Russell King - ARM Linux @ 2011-03-28 17:58 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash issue 2011-03-28 17:58 ` Russell King - ARM Linux @ 2011-03-29 3:04 ` Dong Aisheng-B29396 2011-03-30 10:53 ` Dong Aisheng-B29396 1 sibling, 0 replies; 8+ messages in thread From: Dong Aisheng-B29396 @ 2011-03-29 3:04 UTC (permalink / raw) To: linux-arm-kernel > -----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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash issue 2011-03-28 17:58 ` Russell King - ARM Linux 2011-03-29 3:04 ` Dong Aisheng-B29396 @ 2011-03-30 10:53 ` Dong Aisheng-B29396 1 sibling, 0 replies; 8+ messages in thread From: Dong Aisheng-B29396 @ 2011-03-30 10:53 UTC (permalink / raw) To: linux-arm-kernel > -----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? ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-03-30 10:53 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-03-24 11:16 [PATCH 1/1] ARM: dmabounce: fix dmabounce may cause crash issue Aisheng.Dong 2011-03-24 13:56 ` Mikael Pettersson 2011-03-28 4:48 ` Dong Aisheng-B29396 2011-03-25 8:36 ` Russell King - ARM Linux 2011-03-28 5:01 ` Dong Aisheng-B29396 2011-03-28 17:58 ` Russell King - ARM Linux 2011-03-29 3:04 ` Dong Aisheng-B29396 2011-03-30 10:53 ` Dong Aisheng-B29396
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).