* [PATCH] media: videobuf2: fix kernel panic due to missing assign NULL to alloc_ctx @ 2012-06-08 11:55 Albert Wang 2012-06-11 7:59 ` Laurent Pinchart 0 siblings, 1 reply; 6+ messages in thread From: Albert Wang @ 2012-06-08 11:55 UTC (permalink / raw) To: pawel, g.liakhovetski; +Cc: linux-media, Albert Wang In function vb2_dma_contig_cleanup_ctx(), we only kfree the alloc_ctx If we didn't assign NULL to this point after kfree it, we may encounter the following kernel panic: kernel BUG at kernel/cred.c:98! Unable to handle kernel NULL pointer dereference at virtual address 00000000 pgd = c0004000 [00000000] *pgd=00000000 Internal error: Oops: 817 [#1] PREEMPT SMP Modules linked in: runcase_sysfs galcore mv_wtm_drv mv_wtm_prim CPU: 0 Not tainted (3.0.8+ #213) PC is at __bug+0x18/0x24 LR is at __bug+0x14/0x24 pc : [<c0054670>] lr : [<c005466c>] psr: 60000113 sp : c0681ec0 ip : f683e000 fp : 00000000 r10: e8ab4b58 r9 : 00000fff r8 : 00000002 r7 : e8665698 r6 : c10079ec r5 : e8b13d80 r4 : e8b13d98 r3 : 00000000 r2 : c0681eb4 r1 : c05c9ccc r0 : 00000035 Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel Control: 10c53c7d Table: 29c3406a DAC: 00000015 the root cause is we may encounter some i2c or HW issue with sensor which result in driver exit with exception during soc_camera_set_fmt() from soc_camera_open(): ret = soc_camera_set_fmt(icd, &f); if (ret < 0) goto esfmt; it will call ici->ops->remove() in following code: esfmt: pm_runtime_disable(&icd->vdev->dev); eresume: ici->ops->remove(icd); ici->ops->remove() will call vb2_dma_contig_cleanup_ctx() for cleanup but we didn't do ici->ops->init_videobuf2() yet at that time it will result in kfree a non-NULL point twice Change-Id: I1c66dd08438ae90abe555c52edcdbca0d39d829d Signed-off-by: Albert Wang <twang13@marvell.com> --- drivers/media/video/videobuf2-dma-contig.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c index 4b71326..9881171 100755 --- a/drivers/media/video/videobuf2-dma-contig.c +++ b/drivers/media/video/videobuf2-dma-contig.c @@ -178,6 +178,7 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_init_ctx); void vb2_dma_contig_cleanup_ctx(void *alloc_ctx) { kfree(alloc_ctx); + alloc_ctx = NULL; } EXPORT_SYMBOL_GPL(vb2_dma_contig_cleanup_ctx); -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] media: videobuf2: fix kernel panic due to missing assign NULL to alloc_ctx 2012-06-08 11:55 [PATCH] media: videobuf2: fix kernel panic due to missing assign NULL to alloc_ctx Albert Wang @ 2012-06-11 7:59 ` Laurent Pinchart 2012-06-11 8:34 ` Albert Wang 0 siblings, 1 reply; 6+ messages in thread From: Laurent Pinchart @ 2012-06-11 7:59 UTC (permalink / raw) To: Albert Wang; +Cc: pawel, g.liakhovetski, linux-media Hi Albert, On Friday 08 June 2012 19:55:11 Albert Wang wrote: > In function vb2_dma_contig_cleanup_ctx(), we only kfree the alloc_ctx > If we didn't assign NULL to this point after kfree it, > we may encounter the following kernel panic: > > kernel BUG at kernel/cred.c:98! > Unable to handle kernel NULL pointer dereference at virtual address > 00000000 pgd = c0004000 > [00000000] *pgd=00000000 > Internal error: Oops: 817 [#1] PREEMPT SMP > Modules linked in: runcase_sysfs galcore mv_wtm_drv mv_wtm_prim > CPU: 0 Not tainted (3.0.8+ #213) > PC is at __bug+0x18/0x24 > LR is at __bug+0x14/0x24 > pc : [<c0054670>] lr : [<c005466c>] psr: 60000113 > sp : c0681ec0 ip : f683e000 fp : 00000000 > r10: e8ab4b58 r9 : 00000fff r8 : 00000002 > r7 : e8665698 r6 : c10079ec r5 : e8b13d80 r4 : e8b13d98 > r3 : 00000000 r2 : c0681eb4 r1 : c05c9ccc r0 : 00000035 > Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel > Control: 10c53c7d Table: 29c3406a DAC: 00000015 > > the root cause is we may encounter some i2c or HW issue with sensor > which result in driver exit with exception during soc_camera_set_fmt() > from soc_camera_open(): > > ret = soc_camera_set_fmt(icd, &f); > if (ret < 0) > goto esfmt; > > it will call ici->ops->remove() in following code: > > esfmt: > pm_runtime_disable(&icd->vdev->dev); > eresume: > ici->ops->remove(icd); > > ici->ops->remove() will call vb2_dma_contig_cleanup_ctx() for cleanup > but we didn't do ici->ops->init_videobuf2() yet at that time > it will result in kfree a non-NULL point twice I'm not sure to follow you. How is init_videobuf2() related ? The context is allocated once only at probe time from what I can see. Your problem is more likely caused by a double call to vb2_dma_contig_cleanup_ctx(), which looks like a driver bug to me, not a videobuf2 bug. > Change-Id: I1c66dd08438ae90abe555c52edcdbca0d39d829d > Signed-off-by: Albert Wang <twang13@marvell.com> > --- > drivers/media/video/videobuf2-dma-contig.c | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/drivers/media/video/videobuf2-dma-contig.c > b/drivers/media/video/videobuf2-dma-contig.c index 4b71326..9881171 100755 > --- a/drivers/media/video/videobuf2-dma-contig.c > +++ b/drivers/media/video/videobuf2-dma-contig.c > @@ -178,6 +178,7 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_init_ctx); > void vb2_dma_contig_cleanup_ctx(void *alloc_ctx) > { > kfree(alloc_ctx); > + alloc_ctx = NULL; > } > EXPORT_SYMBOL_GPL(vb2_dma_contig_cleanup_ctx); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] media: videobuf2: fix kernel panic due to missing assign NULL to alloc_ctx 2012-06-11 7:59 ` Laurent Pinchart @ 2012-06-11 8:34 ` Albert Wang 2012-06-11 9:21 ` Guennadi Liakhovetski 0 siblings, 1 reply; 6+ messages in thread From: Albert Wang @ 2012-06-11 8:34 UTC (permalink / raw) To: Laurent Pinchart Cc: pawel@osciak.com, g.liakhovetski@gmx.de, linux-media@vger.kernel.org Hi, Laurent Thanks for your reply! We allocated the context when init_videobuf2() which will be called in soc_camera_open(), so if we get exit with exception in soc_camera_set_fmt() Actually we will double call vb2_dma_contig_cleanup_ctx(). ret = soc_camera_set_fmt(icd, &f); if (ret < 0) goto esfmt; if (ici->ops->init_videobuf) { ici->ops->init_videobuf(&icd->vb_vidq, icd); } else { ret = ici->ops->init_videobuf2(&icd->vb2_vidq, icd); if (ret < 0) goto einitvb; } Actually, in current code, we can found some drivers allocated the context in probe(), and some drivers also do that in soc_camera_open(). Of course, we can update our driver and move it to probe(), it will stand aside the issue, that's also OK for us. But we still think it's not safe that leave the point be a non-NULL after we have kfree it. Do you think so? Thanks Albert Wang 86-21-61092656 -----Original Message----- From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com] Sent: Monday, 11 June, 2012 16:00 To: Albert Wang Cc: pawel@osciak.com; g.liakhovetski@gmx.de; linux-media@vger.kernel.org Subject: Re: [PATCH] media: videobuf2: fix kernel panic due to missing assign NULL to alloc_ctx Hi Albert, On Friday 08 June 2012 19:55:11 Albert Wang wrote: > In function vb2_dma_contig_cleanup_ctx(), we only kfree the alloc_ctx > If we didn't assign NULL to this point after kfree it, > we may encounter the following kernel panic: > > kernel BUG at kernel/cred.c:98! > Unable to handle kernel NULL pointer dereference at virtual address > 00000000 pgd = c0004000 > [00000000] *pgd=00000000 > Internal error: Oops: 817 [#1] PREEMPT SMP Modules linked in: > runcase_sysfs galcore mv_wtm_drv mv_wtm_prim > CPU: 0 Not tainted (3.0.8+ #213) > PC is at __bug+0x18/0x24 > LR is at __bug+0x14/0x24 > pc : [<c0054670>] lr : [<c005466c>] psr: 60000113 > sp : c0681ec0 ip : f683e000 fp : 00000000 > r10: e8ab4b58 r9 : 00000fff r8 : 00000002 > r7 : e8665698 r6 : c10079ec r5 : e8b13d80 r4 : e8b13d98 > r3 : 00000000 r2 : c0681eb4 r1 : c05c9ccc r0 : 00000035 > Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel > Control: 10c53c7d Table: 29c3406a DAC: 00000015 > > the root cause is we may encounter some i2c or HW issue with sensor > which result in driver exit with exception during soc_camera_set_fmt() > from soc_camera_open(): > > ret = soc_camera_set_fmt(icd, &f); > if (ret < 0) > goto esfmt; > > it will call ici->ops->remove() in following code: > > esfmt: > pm_runtime_disable(&icd->vdev->dev); > eresume: > ici->ops->remove(icd); > > ici->ops->remove() will call vb2_dma_contig_cleanup_ctx() for cleanup > but we didn't do ici->ops->init_videobuf2() yet at that time > it will result in kfree a non-NULL point twice I'm not sure to follow you. How is init_videobuf2() related ? The context is allocated once only at probe time from what I can see. Your problem is more likely caused by a double call to vb2_dma_contig_cleanup_ctx(), which looks like a driver bug to me, not a videobuf2 bug. > Change-Id: I1c66dd08438ae90abe555c52edcdbca0d39d829d > Signed-off-by: Albert Wang <twang13@marvell.com> > --- > drivers/media/video/videobuf2-dma-contig.c | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/drivers/media/video/videobuf2-dma-contig.c > b/drivers/media/video/videobuf2-dma-contig.c index 4b71326..9881171 > 100755 > --- a/drivers/media/video/videobuf2-dma-contig.c > +++ b/drivers/media/video/videobuf2-dma-contig.c > @@ -178,6 +178,7 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_init_ctx); > void vb2_dma_contig_cleanup_ctx(void *alloc_ctx) { > kfree(alloc_ctx); > + alloc_ctx = NULL; > } > EXPORT_SYMBOL_GPL(vb2_dma_contig_cleanup_ctx); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] media: videobuf2: fix kernel panic due to missing assign NULL to alloc_ctx 2012-06-11 8:34 ` Albert Wang @ 2012-06-11 9:21 ` Guennadi Liakhovetski 2012-06-11 10:00 ` Albert Wang 0 siblings, 1 reply; 6+ messages in thread From: Guennadi Liakhovetski @ 2012-06-11 9:21 UTC (permalink / raw) To: Albert Wang Cc: Laurent Pinchart, pawel@osciak.com, linux-media@vger.kernel.org Hi Albert On Mon, 11 Jun 2012, Albert Wang wrote: > Hi, Laurent > > Thanks for your reply! > > We allocated the context when init_videobuf2() which will be called in > soc_camera_open(), so if we get exit with exception in > soc_camera_set_fmt() > Actually we will double call vb2_dma_contig_cleanup_ctx(). > > ret = soc_camera_set_fmt(icd, &f); > if (ret < 0) > goto esfmt; > > if (ici->ops->init_videobuf) { > ici->ops->init_videobuf(&icd->vb_vidq, icd); > } else { > ret = ici->ops->init_videobuf2(&icd->vb2_vidq, icd); > if (ret < 0) > goto einitvb; > } > > Actually, in current code, we can found some drivers allocated the > context in probe(), and some drivers also do that in soc_camera_open(). Sorry, AFAICS all soc-camera host drivers allocate vb2 context in their .probe() methods. Can you point me out to the one(s) you mean, that do that at open() time? > Of course, we can update our driver and move it to probe(), it will > stand aside the issue, that's also OK for us. > > But we still think it's not safe that leave the point be a non-NULL > after we have kfree it. Do you think so? Your patch doesn't fix anything. It only changes the local variable in the vb2_dma_contig_cleanup_ctx() function, which doesn't affect the caller in any way. Thanks Guennadi > Thanks > Albert Wang > 86-21-61092656 > > -----Original Message----- > From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com] > Sent: Monday, 11 June, 2012 16:00 > To: Albert Wang > Cc: pawel@osciak.com; g.liakhovetski@gmx.de; linux-media@vger.kernel.org > Subject: Re: [PATCH] media: videobuf2: fix kernel panic due to missing assign NULL to alloc_ctx > > Hi Albert, > > On Friday 08 June 2012 19:55:11 Albert Wang wrote: > > In function vb2_dma_contig_cleanup_ctx(), we only kfree the alloc_ctx > > If we didn't assign NULL to this point after kfree it, > > we may encounter the following kernel panic: > > > > kernel BUG at kernel/cred.c:98! > > Unable to handle kernel NULL pointer dereference at virtual address > > 00000000 pgd = c0004000 > > [00000000] *pgd=00000000 > > Internal error: Oops: 817 [#1] PREEMPT SMP Modules linked in: > > runcase_sysfs galcore mv_wtm_drv mv_wtm_prim > > CPU: 0 Not tainted (3.0.8+ #213) > > PC is at __bug+0x18/0x24 > > LR is at __bug+0x14/0x24 > > pc : [<c0054670>] lr : [<c005466c>] psr: 60000113 > > sp : c0681ec0 ip : f683e000 fp : 00000000 > > r10: e8ab4b58 r9 : 00000fff r8 : 00000002 > > r7 : e8665698 r6 : c10079ec r5 : e8b13d80 r4 : e8b13d98 > > r3 : 00000000 r2 : c0681eb4 r1 : c05c9ccc r0 : 00000035 > > Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel > > Control: 10c53c7d Table: 29c3406a DAC: 00000015 > > > > the root cause is we may encounter some i2c or HW issue with sensor > > which result in driver exit with exception during soc_camera_set_fmt() > > from soc_camera_open(): > > > > ret = soc_camera_set_fmt(icd, &f); > > if (ret < 0) > > goto esfmt; > > > > it will call ici->ops->remove() in following code: > > > > esfmt: > > pm_runtime_disable(&icd->vdev->dev); > > eresume: > > ici->ops->remove(icd); > > > > ici->ops->remove() will call vb2_dma_contig_cleanup_ctx() for cleanup > > but we didn't do ici->ops->init_videobuf2() yet at that time > > it will result in kfree a non-NULL point twice > > I'm not sure to follow you. How is init_videobuf2() related ? The context is allocated once only at probe time from what I can see. Your problem is more likely caused by a double call to vb2_dma_contig_cleanup_ctx(), which looks like a driver bug to me, not a videobuf2 bug. > > > Change-Id: I1c66dd08438ae90abe555c52edcdbca0d39d829d > > Signed-off-by: Albert Wang <twang13@marvell.com> > > --- > > drivers/media/video/videobuf2-dma-contig.c | 1 + > > 1 files changed, 1 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/media/video/videobuf2-dma-contig.c > > b/drivers/media/video/videobuf2-dma-contig.c index 4b71326..9881171 > > 100755 > > --- a/drivers/media/video/videobuf2-dma-contig.c > > +++ b/drivers/media/video/videobuf2-dma-contig.c > > @@ -178,6 +178,7 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_init_ctx); > > void vb2_dma_contig_cleanup_ctx(void *alloc_ctx) { > > kfree(alloc_ctx); > > + alloc_ctx = NULL; > > } > > EXPORT_SYMBOL_GPL(vb2_dma_contig_cleanup_ctx); > > -- > Regards, > > Laurent Pinchart > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] media: videobuf2: fix kernel panic due to missing assign NULL to alloc_ctx 2012-06-11 9:21 ` Guennadi Liakhovetski @ 2012-06-11 10:00 ` Albert Wang 2012-06-11 10:20 ` Guennadi Liakhovetski 0 siblings, 1 reply; 6+ messages in thread From: Albert Wang @ 2012-06-11 10:00 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Laurent Pinchart, pawel@osciak.com, linux-media@vger.kernel.org Hi, Guennadi Sorry, I made a mistake, and I found marvell-ccic used vb2_dma_contig_init_ctx() when open, but it didn't use the soc_camera. So for the driver which had used soc_camera, you are right. Our driver should update the usage. So the kernel panic due to our driver misunderstand the usage, we can change the patch title. But I think it's not a "local" variable for vb2_dma_contig_cleanup_ctx(), It's the input parameter. Since this function has the name "cleanup", it should do it cleanly. And the rule should be free point and set point to NULL. For users who use this function in there driver, they think things had finished after called this function. It's safer if user called it twice. Anyway, this patch doesn't damage the driver; on the other hand, it can make it more stable if user used it with wrong method. Doesn't it? Of course, now we should change the patch title. Please correct me if I have something wrong. Thanks Albert Wang 86-21-61092656 -----Original Message----- From: Guennadi Liakhovetski [mailto:g.liakhovetski@gmx.de] Sent: Monday, 11 June, 2012 17:21 To: Albert Wang Cc: Laurent Pinchart; pawel@osciak.com; linux-media@vger.kernel.org Subject: RE: [PATCH] media: videobuf2: fix kernel panic due to missing assign NULL to alloc_ctx Hi Albert On Mon, 11 Jun 2012, Albert Wang wrote: > Hi, Laurent > > Thanks for your reply! > > We allocated the context when init_videobuf2() which will be called in > soc_camera_open(), so if we get exit with exception in > soc_camera_set_fmt() > Actually we will double call vb2_dma_contig_cleanup_ctx(). > > ret = soc_camera_set_fmt(icd, &f); > if (ret < 0) > goto esfmt; > > if (ici->ops->init_videobuf) { > ici->ops->init_videobuf(&icd->vb_vidq, icd); > } else { > ret = ici->ops->init_videobuf2(&icd->vb2_vidq, icd); > if (ret < 0) > goto einitvb; > } > > Actually, in current code, we can found some drivers allocated the > context in probe(), and some drivers also do that in soc_camera_open(). Sorry, AFAICS all soc-camera host drivers allocate vb2 context in their .probe() methods. Can you point me out to the one(s) you mean, that do that at open() time? > Of course, we can update our driver and move it to probe(), it will > stand aside the issue, that's also OK for us. > > But we still think it's not safe that leave the point be a non-NULL > after we have kfree it. Do you think so? Your patch doesn't fix anything. It only changes the local variable in the vb2_dma_contig_cleanup_ctx() function, which doesn't affect the caller in any way. Thanks Guennadi > Thanks > Albert Wang > 86-21-61092656 > > -----Original Message----- > From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com] > Sent: Monday, 11 June, 2012 16:00 > To: Albert Wang > Cc: pawel@osciak.com; g.liakhovetski@gmx.de; > linux-media@vger.kernel.org > Subject: Re: [PATCH] media: videobuf2: fix kernel panic due to missing > assign NULL to alloc_ctx > > Hi Albert, > > On Friday 08 June 2012 19:55:11 Albert Wang wrote: > > In function vb2_dma_contig_cleanup_ctx(), we only kfree the alloc_ctx > > If we didn't assign NULL to this point after kfree it, > > we may encounter the following kernel panic: > > > > kernel BUG at kernel/cred.c:98! > > Unable to handle kernel NULL pointer dereference at virtual address > > 00000000 pgd = c0004000 > > [00000000] *pgd=00000000 > > Internal error: Oops: 817 [#1] PREEMPT SMP Modules linked in: > > runcase_sysfs galcore mv_wtm_drv mv_wtm_prim > > CPU: 0 Not tainted (3.0.8+ #213) > > PC is at __bug+0x18/0x24 > > LR is at __bug+0x14/0x24 > > pc : [<c0054670>] lr : [<c005466c>] psr: 60000113 > > sp : c0681ec0 ip : f683e000 fp : 00000000 > > r10: e8ab4b58 r9 : 00000fff r8 : 00000002 > > r7 : e8665698 r6 : c10079ec r5 : e8b13d80 r4 : e8b13d98 > > r3 : 00000000 r2 : c0681eb4 r1 : c05c9ccc r0 : 00000035 > > Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel > > Control: 10c53c7d Table: 29c3406a DAC: 00000015 > > > > the root cause is we may encounter some i2c or HW issue with sensor > > which result in driver exit with exception during soc_camera_set_fmt() > > from soc_camera_open(): > > > > ret = soc_camera_set_fmt(icd, &f); > > if (ret < 0) > > goto esfmt; > > > > it will call ici->ops->remove() in following code: > > > > esfmt: > > pm_runtime_disable(&icd->vdev->dev); > > eresume: > > ici->ops->remove(icd); > > > > ici->ops->remove() will call vb2_dma_contig_cleanup_ctx() for cleanup > > but we didn't do ici->ops->init_videobuf2() yet at that time > > it will result in kfree a non-NULL point twice > > I'm not sure to follow you. How is init_videobuf2() related ? The context is allocated once only at probe time from what I can see. Your problem is more likely caused by a double call to vb2_dma_contig_cleanup_ctx(), which looks like a driver bug to me, not a videobuf2 bug. > > > Change-Id: I1c66dd08438ae90abe555c52edcdbca0d39d829d > > Signed-off-by: Albert Wang <twang13@marvell.com> > > --- > > drivers/media/video/videobuf2-dma-contig.c | 1 + > > 1 files changed, 1 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/media/video/videobuf2-dma-contig.c > > b/drivers/media/video/videobuf2-dma-contig.c index 4b71326..9881171 > > 100755 > > --- a/drivers/media/video/videobuf2-dma-contig.c > > +++ b/drivers/media/video/videobuf2-dma-contig.c > > @@ -178,6 +178,7 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_init_ctx); > > void vb2_dma_contig_cleanup_ctx(void *alloc_ctx) { > > kfree(alloc_ctx); > > + alloc_ctx = NULL; > > } > > EXPORT_SYMBOL_GPL(vb2_dma_contig_cleanup_ctx); > > -- > Regards, > > Laurent Pinchart > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] media: videobuf2: fix kernel panic due to missing assign NULL to alloc_ctx 2012-06-11 10:00 ` Albert Wang @ 2012-06-11 10:20 ` Guennadi Liakhovetski 0 siblings, 0 replies; 6+ messages in thread From: Guennadi Liakhovetski @ 2012-06-11 10:20 UTC (permalink / raw) To: Albert Wang Cc: Laurent Pinchart, pawel@osciak.com, linux-media@vger.kernel.org On Mon, 11 Jun 2012, Albert Wang wrote: > Hi, Guennadi > > Sorry, I made a mistake, and I found marvell-ccic used > vb2_dma_contig_init_ctx() when open, but it didn't use the soc_camera. > So for the driver which had used soc_camera, you are right. Our driver should update the usage. > > So the kernel panic due to our driver misunderstand the usage, we can change the patch title. > But I think it's not a "local" variable for vb2_dma_contig_cleanup_ctx(), It's the input parameter. Sorry, I'm not in a position to clarify this. Please, consult your C textbook. Thanks Guennadi > > Since this function has the name "cleanup", it should do it cleanly. And the rule should be free point and set point to NULL. > For users who use this function in there driver, they think things had finished after called this function. > It's safer if user called it twice. > > Anyway, this patch doesn't damage the driver; on the other hand, it can make it more stable if user used it with wrong method. > Doesn't it? Of course, now we should change the patch title. > > Please correct me if I have something wrong. > > > Thanks > Albert Wang > 86-21-61092656 > -----Original Message----- > From: Guennadi Liakhovetski [mailto:g.liakhovetski@gmx.de] > Sent: Monday, 11 June, 2012 17:21 > To: Albert Wang > Cc: Laurent Pinchart; pawel@osciak.com; linux-media@vger.kernel.org > Subject: RE: [PATCH] media: videobuf2: fix kernel panic due to missing assign NULL to alloc_ctx > > Hi Albert > > On Mon, 11 Jun 2012, Albert Wang wrote: > > > Hi, Laurent > > > > Thanks for your reply! > > > > We allocated the context when init_videobuf2() which will be called in > > soc_camera_open(), so if we get exit with exception in > > soc_camera_set_fmt() > > Actually we will double call vb2_dma_contig_cleanup_ctx(). > > > > ret = soc_camera_set_fmt(icd, &f); > > if (ret < 0) > > goto esfmt; > > > > if (ici->ops->init_videobuf) { > > ici->ops->init_videobuf(&icd->vb_vidq, icd); > > } else { > > ret = ici->ops->init_videobuf2(&icd->vb2_vidq, icd); > > if (ret < 0) > > goto einitvb; > > } > > > > Actually, in current code, we can found some drivers allocated the > > context in probe(), and some drivers also do that in soc_camera_open(). > > Sorry, AFAICS all soc-camera host drivers allocate vb2 context in their > .probe() methods. Can you point me out to the one(s) you mean, that do that at open() time? > > > Of course, we can update our driver and move it to probe(), it will > > stand aside the issue, that's also OK for us. > > > > But we still think it's not safe that leave the point be a non-NULL > > after we have kfree it. Do you think so? > > Your patch doesn't fix anything. It only changes the local variable in the > vb2_dma_contig_cleanup_ctx() function, which doesn't affect the caller in any way. > > Thanks > Guennadi > > > Thanks > > Albert Wang > > 86-21-61092656 > > > > -----Original Message----- > > From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com] > > Sent: Monday, 11 June, 2012 16:00 > > To: Albert Wang > > Cc: pawel@osciak.com; g.liakhovetski@gmx.de; > > linux-media@vger.kernel.org > > Subject: Re: [PATCH] media: videobuf2: fix kernel panic due to missing > > assign NULL to alloc_ctx > > > > Hi Albert, > > > > On Friday 08 June 2012 19:55:11 Albert Wang wrote: > > > In function vb2_dma_contig_cleanup_ctx(), we only kfree the alloc_ctx > > > If we didn't assign NULL to this point after kfree it, > > > we may encounter the following kernel panic: > > > > > > kernel BUG at kernel/cred.c:98! > > > Unable to handle kernel NULL pointer dereference at virtual address > > > 00000000 pgd = c0004000 > > > [00000000] *pgd=00000000 > > > Internal error: Oops: 817 [#1] PREEMPT SMP Modules linked in: > > > runcase_sysfs galcore mv_wtm_drv mv_wtm_prim > > > CPU: 0 Not tainted (3.0.8+ #213) > > > PC is at __bug+0x18/0x24 > > > LR is at __bug+0x14/0x24 > > > pc : [<c0054670>] lr : [<c005466c>] psr: 60000113 > > > sp : c0681ec0 ip : f683e000 fp : 00000000 > > > r10: e8ab4b58 r9 : 00000fff r8 : 00000002 > > > r7 : e8665698 r6 : c10079ec r5 : e8b13d80 r4 : e8b13d98 > > > r3 : 00000000 r2 : c0681eb4 r1 : c05c9ccc r0 : 00000035 > > > Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel > > > Control: 10c53c7d Table: 29c3406a DAC: 00000015 > > > > > > the root cause is we may encounter some i2c or HW issue with sensor > > > which result in driver exit with exception during soc_camera_set_fmt() > > > from soc_camera_open(): > > > > > > ret = soc_camera_set_fmt(icd, &f); > > > if (ret < 0) > > > goto esfmt; > > > > > > it will call ici->ops->remove() in following code: > > > > > > esfmt: > > > pm_runtime_disable(&icd->vdev->dev); > > > eresume: > > > ici->ops->remove(icd); > > > > > > ici->ops->remove() will call vb2_dma_contig_cleanup_ctx() for cleanup > > > but we didn't do ici->ops->init_videobuf2() yet at that time > > > it will result in kfree a non-NULL point twice > > > > I'm not sure to follow you. How is init_videobuf2() related ? The context is allocated once only at probe time from what I can see. Your problem is more likely caused by a double call to vb2_dma_contig_cleanup_ctx(), which looks like a driver bug to me, not a videobuf2 bug. > > > > > Change-Id: I1c66dd08438ae90abe555c52edcdbca0d39d829d > > > Signed-off-by: Albert Wang <twang13@marvell.com> > > > --- > > > drivers/media/video/videobuf2-dma-contig.c | 1 + > > > 1 files changed, 1 insertions(+), 0 deletions(-) > > > > > > diff --git a/drivers/media/video/videobuf2-dma-contig.c > > > b/drivers/media/video/videobuf2-dma-contig.c index 4b71326..9881171 > > > 100755 > > > --- a/drivers/media/video/videobuf2-dma-contig.c > > > +++ b/drivers/media/video/videobuf2-dma-contig.c > > > @@ -178,6 +178,7 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_init_ctx); > > > void vb2_dma_contig_cleanup_ctx(void *alloc_ctx) { > > > kfree(alloc_ctx); > > > + alloc_ctx = NULL; > > > } > > > EXPORT_SYMBOL_GPL(vb2_dma_contig_cleanup_ctx); > > > > -- > > Regards, > > > > Laurent Pinchart > > > > --- > Guennadi Liakhovetski, Ph.D. > Freelance Open-Source Software Developer http://www.open-technology.de/ > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-06-11 10:20 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-06-08 11:55 [PATCH] media: videobuf2: fix kernel panic due to missing assign NULL to alloc_ctx Albert Wang 2012-06-11 7:59 ` Laurent Pinchart 2012-06-11 8:34 ` Albert Wang 2012-06-11 9:21 ` Guennadi Liakhovetski 2012-06-11 10:00 ` Albert Wang 2012-06-11 10:20 ` Guennadi Liakhovetski
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.