VIDEO: ivtvfb, remove unneeded NULL test
in [Kernel]
|
Prev: [PATCH] omfs: fix memory leak
Next: xstat: Implement a requestable extra result to procure some inode flags [ver #4]
From: Andy Walls on 4 Jul 2010 00:20 On Tue, 2010-06-22 at 13:41 +0200, Jiri Slaby wrote: > Stanse found that in ivtvfb_callback_cleanup there is an unneeded test > for itv being NULL. But itv is initialized as container_of with > non-zero offset, so it is never NULL (even if v4l2_dev is). This was > found because itv is dereferenced earlier than the test. > > Signed-off-by: Jiri Slaby <jslaby(a)suse.cz> > Cc: Andy Walls <awalls(a)md.metrocast.net> > Cc: Mauro Carvalho Chehab <mchehab(a)infradead.org> > Cc: Tejun Heo <tj(a)kernel.org> > Cc: Ian Armstrong <ian(a)iarmst.demon.co.uk> > Cc: ivtv-devel(a)ivtvdriver.org > Cc: linux-media(a)vger.kernel.org > --- > drivers/media/video/ivtv/ivtvfb.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/media/video/ivtv/ivtvfb.c b/drivers/media/video/ivtv/ivtvfb.c > index 9ff3425..5dc460e 100644 > --- a/drivers/media/video/ivtv/ivtvfb.c > +++ b/drivers/media/video/ivtv/ivtvfb.c > @@ -1219,7 +1219,7 @@ static int ivtvfb_callback_cleanup(struct device *dev, void *p) > struct ivtv *itv = container_of(v4l2_dev, struct ivtv, v4l2_dev); > struct osd_info *oi = itv->osd_info; > > - if (itv && (itv->v4l2_cap & V4L2_CAP_VIDEO_OUTPUT)) { > + if (itv->v4l2_cap & V4L2_CAP_VIDEO_OUTPUT) { > if (unregister_framebuffer(&itv->osd_info->ivtvfb_info)) { > IVTVFB_WARN("Framebuffer %d is in use, cannot unload\n", > itv->instance); Jiri, You missed an identical instance of the useless test 10 lines prior in ivtvfb_callback_init(). :) How about the patch below, instead? Regards, Andy [PATCH] VIDEO: ivtvfb, fix NULL check Jiri Slaby reported that stanse found ivtvfb_callback_cleanup has an unneeded test for itv being NULL. itv was initialized as container_of with non-zero offset, so it was never NULL (even if v4l2_dev was). This fix now checks for v4l2_dev being NULL, and not itv. Thanks to Jiri Slaby for reporting this problem and providing an initial patch. Reported-by: Jiri Slaby <jslaby(a)suse.cz> Signed-off-by: Andy Walls <awalls(a)md.metrocast.net> Cc: Mauro Carvalho Chehab <mchehab(a)infradead.org> Cc: Tejun Heo <tj(a)kernel.org> Cc: Ian Armstrong <ian(a)iarmst.demon.co.uk> Cc: ivtv-devel(a)ivtvdriver.org Cc: linux-media(a)vger.kernel.org drivers/media/video/ivtv/ivtvfb.c | 21 ++++++++++++++++----- 1 files changed, 16 insertions(+), 5 deletions(-) diff --git a/drivers/media/video/ivtv/ivtvfb.c b/drivers/media/video/ivtv/ivtvfb index 9ff3425..2b3259c 100644 --- a/drivers/media/video/ivtv/ivtvfb.c +++ b/drivers/media/video/ivtv/ivtvfb.c @@ -1201,9 +1201,14 @@ static int ivtvfb_init_card(struct ivtv *itv) static int __init ivtvfb_callback_init(struct device *dev, void *p) { struct v4l2_device *v4l2_dev = dev_get_drvdata(dev); - struct ivtv *itv = container_of(v4l2_dev, struct ivtv, v4l2_dev); + struct ivtv *itv; - if (itv && (itv->v4l2_cap & V4L2_CAP_VIDEO_OUTPUT)) { + if (v4l2_dev == NULL) + return 0; + + itv = container_of(v4l2_dev, struct ivtv, v4l2_dev); + + if (itv->v4l2_cap & V4L2_CAP_VIDEO_OUTPUT) { if (ivtvfb_init_card(itv) == 0) { IVTVFB_INFO("Framebuffer registered on %s\n", itv->v4l2_dev.name); @@ -1216,10 +1221,16 @@ static int __init ivtvfb_callback_init(struct device *de static int ivtvfb_callback_cleanup(struct device *dev, void *p) { struct v4l2_device *v4l2_dev = dev_get_drvdata(dev); - struct ivtv *itv = container_of(v4l2_dev, struct ivtv, v4l2_dev); - struct osd_info *oi = itv->osd_info; + struct ivtv *itv; + struct osd_info *oi; + + if (v4l2_dev == NULL) + return 0; + + itv = container_of(v4l2_dev, struct ivtv, v4l2_dev); + oi = itv->osd_info; - if (itv && (itv->v4l2_cap & V4L2_CAP_VIDEO_OUTPUT)) { + if (itv->v4l2_cap & V4L2_CAP_VIDEO_OUTPUT) { if (unregister_framebuffer(&itv->osd_info->ivtvfb_info)) { IVTVFB_WARN("Framebuffer %d is in use, cannot unload\n", itv->instance); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Jiri Slaby on 4 Jul 2010 03:30 On 07/04/2010 06:11 AM, Andy Walls wrote: > You missed an identical instance of the useless test 10 lines prior in > ivtvfb_callback_init(). :) Ah, thank you for pointing out. Find my comment below. > --- a/drivers/media/video/ivtv/ivtvfb.c > +++ b/drivers/media/video/ivtv/ivtvfb.c > @@ -1201,9 +1201,14 @@ static int ivtvfb_init_card(struct ivtv *itv) > static int __init ivtvfb_callback_init(struct device *dev, void *p) > { > struct v4l2_device *v4l2_dev = dev_get_drvdata(dev); > - struct ivtv *itv = container_of(v4l2_dev, struct ivtv, v4l2_dev); > + struct ivtv *itv; > > - if (itv && (itv->v4l2_cap & V4L2_CAP_VIDEO_OUTPUT)) { > + if (v4l2_dev == NULL) > + return 0; > + > + itv = container_of(v4l2_dev, struct ivtv, v4l2_dev); > + > + if (itv->v4l2_cap & V4L2_CAP_VIDEO_OUTPUT) { > if (ivtvfb_init_card(itv) == 0) { > IVTVFB_INFO("Framebuffer registered on %s\n", > itv->v4l2_dev.name); > @@ -1216,10 +1221,16 @@ static int __init ivtvfb_callback_init(struct device *de > static int ivtvfb_callback_cleanup(struct device *dev, void *p) > { > struct v4l2_device *v4l2_dev = dev_get_drvdata(dev); > - struct ivtv *itv = container_of(v4l2_dev, struct ivtv, v4l2_dev); > - struct osd_info *oi = itv->osd_info; > + struct ivtv *itv; > + struct osd_info *oi; > + > + if (v4l2_dev == NULL) > + return 0; From my POV I NACK this. Given that it never triggered and drvdata are set in v4l2_device_register called from ivtv_probe I can't see how v4l2_dev be NULL. Could you elaborate? -- js suse labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Andy Walls on 4 Jul 2010 09:30 On Sun, 2010-07-04 at 09:24 +0200, Jiri Slaby wrote: > On 07/04/2010 06:11 AM, Andy Walls wrote: > > You missed an identical instance of the useless test 10 lines prior in > > ivtvfb_callback_init(). :) > > Ah, thank you for pointing out. Find my comment below. > > > --- a/drivers/media/video/ivtv/ivtvfb.c > > +++ b/drivers/media/video/ivtv/ivtvfb.c > > @@ -1201,9 +1201,14 @@ static int ivtvfb_init_card(struct ivtv *itv) > > static int __init ivtvfb_callback_init(struct device *dev, void *p) > > { > > struct v4l2_device *v4l2_dev = dev_get_drvdata(dev); > > - struct ivtv *itv = container_of(v4l2_dev, struct ivtv, v4l2_dev); > > + struct ivtv *itv; > > > > - if (itv && (itv->v4l2_cap & V4L2_CAP_VIDEO_OUTPUT)) { > > + if (v4l2_dev == NULL) > > + return 0; > > + > > + itv = container_of(v4l2_dev, struct ivtv, v4l2_dev); > > + > > + if (itv->v4l2_cap & V4L2_CAP_VIDEO_OUTPUT) { > > if (ivtvfb_init_card(itv) == 0) { > > IVTVFB_INFO("Framebuffer registered on %s\n", > > itv->v4l2_dev.name); > > @@ -1216,10 +1221,16 @@ static int __init ivtvfb_callback_init(struct device *de > > static int ivtvfb_callback_cleanup(struct device *dev, void *p) > > { > > struct v4l2_device *v4l2_dev = dev_get_drvdata(dev); > > - struct ivtv *itv = container_of(v4l2_dev, struct ivtv, v4l2_dev); > > - struct osd_info *oi = itv->osd_info; > > + struct ivtv *itv; > > + struct osd_info *oi; > > + > > + if (v4l2_dev == NULL) > > + return 0; > > >From my POV I NACK this. Given that it never triggered and drvdata are > set in v4l2_device_register called from ivtv_probe I can't see how > v4l2_dev be NULL. Could you elaborate? I hemmed and hawed over that too. I didn't do a very thorough analysis of the restrictions on unloading modules, but here was my line of reasoning: 1. I assumed the check was there because presumably someone has run into an Ooops there before, prior to the conversion to the v4l2_device infrastrucutre. I have reasearched that with git log this morning, and found that assumption was false. The itv NULL check is a hold-over from when the ivtvfb and ivtv were apparently more tightly coupled. The itv pointer was originally read from an array in ivtv that could have entries set to NULL. That array is long gone. 2. Note that the ivtvfb driver is not automatically loaded nor unloaded by the kernel ivtv driver. Something from userspace will reqeust the load and unload of the module. There are windows of time where a struct device * will exist for a card in the ivtv driver, but a struct v4l2_device * may not: the end of ivtv_remove() and the beginning of ivtv_probe(). I was contemplating a case where user space requested unloading both the ivtvfb and the ivtv driver. Given all the I2C devices these cards can have, I thought v4l2_device_unregister() at the end of ivtv_remove() could present a window of time large enough to worry about a race. v4l2_device_unregister() sets the struct device drvdata pointer to NULL, and then begins unregistering the i2c clients. I haven't profiled the process to know how long it typically takes, though. I also don't know if kernel mechanisms will absolutely prevent initiating the unload of ivtv.ko before the unload of ivtvfb.ko is completely finished. Will they? $ modinfo ivtvfb | grep -E '(depend)|(alias)' depends: ivtv $ modinfo ivtv | grep -E '(depend)|(alias)' alias: pci:v00004444d00000016sv*sd*bc*sc*i* alias: pci:v00004444d00000803sv*sd*bc*sc*i* depends: cx2341x,videodev,tveeprom,v4l2-common,i2c-core,i2c-algo-bit Regards, Andy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Jiri Slaby on 5 Jul 2010 03:20 On 07/04/2010 03:22 PM, Andy Walls wrote: > On Sun, 2010-07-04 at 09:24 +0200, Jiri Slaby wrote: >> On 07/04/2010 06:11 AM, Andy Walls wrote: >>> --- a/drivers/media/video/ivtv/ivtvfb.c >>> +++ b/drivers/media/video/ivtv/ivtvfb.c >>> @@ -1201,9 +1201,14 @@ static int ivtvfb_init_card(struct ivtv *itv) >>> static int __init ivtvfb_callback_init(struct device *dev, void *p) >>> { >>> struct v4l2_device *v4l2_dev = dev_get_drvdata(dev); >>> - struct ivtv *itv = container_of(v4l2_dev, struct ivtv, v4l2_dev); >>> + struct ivtv *itv; >>> >>> - if (itv && (itv->v4l2_cap & V4L2_CAP_VIDEO_OUTPUT)) { >>> + if (v4l2_dev == NULL) >>> + return 0; >>> + >>> + itv = container_of(v4l2_dev, struct ivtv, v4l2_dev); >>> + >>> + if (itv->v4l2_cap & V4L2_CAP_VIDEO_OUTPUT) { >>> if (ivtvfb_init_card(itv) == 0) { >>> IVTVFB_INFO("Framebuffer registered on %s\n", >>> itv->v4l2_dev.name); >>> @@ -1216,10 +1221,16 @@ static int __init ivtvfb_callback_init(struct device *de >>> static int ivtvfb_callback_cleanup(struct device *dev, void *p) >>> { >>> struct v4l2_device *v4l2_dev = dev_get_drvdata(dev); >>> - struct ivtv *itv = container_of(v4l2_dev, struct ivtv, v4l2_dev); >>> - struct osd_info *oi = itv->osd_info; >>> + struct ivtv *itv; >>> + struct osd_info *oi; >>> + >>> + if (v4l2_dev == NULL) >>> + return 0; >> >> >From my POV I NACK this. Given that it never triggered and drvdata are >> set in v4l2_device_register called from ivtv_probe I can't see how >> v4l2_dev be NULL. Could you elaborate? > > I hemmed and hawed over that too. I didn't do a very thorough analysis > of the restrictions on unloading modules, but here was my line of > reasoning: .... > 2. Note that the ivtvfb driver is not automatically loaded nor unloaded > by the kernel ivtv driver. Something from userspace will reqeust the > load and unload of the module. > > There are windows of time where a struct device * will exist for a card > in the ivtv driver, but a struct v4l2_device * may not: the end of > ivtv_remove() and the beginning of ivtv_probe(). If there is no locking or refcounting, this won't change with the added check. The window is still there, but it begins after the check with your patch. Hence will still cause oopses. > I was contemplating a case where user space requested unloading both the > ivtvfb and the ivtv driver. Given all the I2C devices these cards can > have, I thought v4l2_device_unregister() at the end of ivtv_remove() > could present a window of time large enough to worry about a race. > v4l2_device_unregister() sets the struct device drvdata pointer to > NULL, and then begins unregistering the i2c clients. I haven't profiled > the process to know how long it typically takes, though. > > I also don't know if kernel mechanisms will absolutely prevent > initiating the unload of ivtv.ko before the unload of ivtvfb.ko is > completely finished. Will they? Given ivtvfb uses functions from ivtv, no, it can't unload ivtv earlier than ivtvfb. regards, -- js suse labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Andy Walls on 5 Jul 2010 12:30
On Mon, 2010-07-05 at 09:10 +0200, Jiri Slaby wrote: > On 07/04/2010 03:22 PM, Andy Walls wrote: > > On Sun, 2010-07-04 at 09:24 +0200, Jiri Slaby wrote: > >> On 07/04/2010 06:11 AM, Andy Walls wrote: > > There are windows of time where a struct device * will exist for a card > > in the ivtv driver, but a struct v4l2_device * may not: the end of > > ivtv_remove() and the beginning of ivtv_probe(). > > If there is no locking or refcounting, this won't change with the added > check. The window is still there, but it begins after the check with > your patch. Hence will still cause oopses. Jiri, Of course, you're absolutley right. Please resubmit a version of your original patch fixing both instances of the check. I'll add my ack and SOB. If any users ever report an Oops, then I'll bother to add interlocking. Regards, Andy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |