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 <[email protected]>
Cc: Andy Walls <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: Ian Armstrong <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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);
--
1.7.1
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 <[email protected]>
> Cc: Andy Walls <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Tejun Heo <[email protected]>
> Cc: Ian Armstrong <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> 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 <[email protected]>
Signed-off-by: Andy Walls <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: Ian Armstrong <[email protected]>
Cc: [email protected]
Cc: [email protected]
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);
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
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
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
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
Stanse found that in ivtvfb_callback_cleanup and ivtvfb_callback_init
there are unneeded tests for itv being NULL. But itv is initialized
as container_of with non-zero offset in those functions, 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 <[email protected]>
Cc: Andy Walls <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: Ian Armstrong <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/media/video/ivtv/ivtvfb.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/video/ivtv/ivtvfb.c b/drivers/media/video/ivtv/ivtvfb.c
index 9ff3425..9c77bfa 100644
--- a/drivers/media/video/ivtv/ivtvfb.c
+++ b/drivers/media/video/ivtv/ivtvfb.c
@@ -1203,7 +1203,7 @@ 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);
- if (itv && (itv->v4l2_cap & V4L2_CAP_VIDEO_OUTPUT)) {
+ 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);
@@ -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);
--
1.7.1
On Mon, 2010-07-19 at 19:39 +0200, Jiri Slaby wrote:
> Stanse found that in ivtvfb_callback_cleanup and ivtvfb_callback_init
> there are unneeded tests for itv being NULL. But itv is initialized
> as container_of with non-zero offset in those functions, 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 <[email protected]>
> Cc: Andy Walls <[email protected]>
Looks fine to me.
Reviewed-by: Andy Walls <[email protected]>
Regards,
Andy
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Tejun Heo <[email protected]>
> Cc: Ian Armstrong <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/media/video/ivtv/ivtvfb.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/video/ivtv/ivtvfb.c b/drivers/media/video/ivtv/ivtvfb.c
> index 9ff3425..9c77bfa 100644
> --- a/drivers/media/video/ivtv/ivtvfb.c
> +++ b/drivers/media/video/ivtv/ivtvfb.c
> @@ -1203,7 +1203,7 @@ 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);
>
> - if (itv && (itv->v4l2_cap & V4L2_CAP_VIDEO_OUTPUT)) {
> + 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);
> @@ -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);