dvb shared datastructure bug?

dvb shared datastructure bug?

[Arjan van de Ven]

Hi,

while working on the last pieces of the file_ops constantification, DVB
is the small village in France that is holding the Romans at bay... but
I think I found the final flaw in it now: 

       *pdvbdev = dvbdev = kmalloc(sizeof(struct dvb_device), GFP_KERNEL);

        if (!dvbdev) {
                mutex_unlock(&dvbdev_register_lock);
                return -ENOMEM;
        }

        memcpy(dvbdev, template, sizeof(struct dvb_device));
        dvbdev->type = type;
        dvbdev->id = id;
        dvbdev->adapter = adap;
        dvbdev->priv = priv;

        dvbdev->fops->owner = adap->module;


this is the place in DVB that is writing to a struct file_operations.
But as with almost all such cases in the kernel, this one is buggy:
While the code nicely copies a template dvbdev, that template only has a
pointer to a *shared* fops struct, the copy doesn't help that. So this
code is overwriting the fops owner field for ALL active devices, not
just the ones the copy of the template is for....

I'm lost in the maze of this part of DVB (it seems to have some magic
potion to resist me) but I was hoping some of the local citizens could
take a look at this buglet...

Greetings,
    Arjan van de Ven
-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org

Re: [v4l-dvb-maintainer] dvb shared datastructure bug?

[Manu Abraham]

On 2/13/07, Arjan van de Ven <arjan@infradead.org> wrote:
> Hi,
>
> while working on the last pieces of the file_ops constantification, DVB
> is the small village in France that is holding the Romans at bay... but
> I think I found the final flaw in it now:
>
>        *pdvbdev = dvbdev = kmalloc(sizeof(struct dvb_device), GFP_KERNEL);
>
>         if (!dvbdev) {
>                 mutex_unlock(&dvbdev_register_lock);
>                 return -ENOMEM;
>         }
>
>         memcpy(dvbdev, template, sizeof(struct dvb_device));
>         dvbdev->type = type;
>         dvbdev->id = id;
>         dvbdev->adapter = adap;
>         dvbdev->priv = priv;
>
>         dvbdev->fops->owner = adap->module;
>
>
> this is the place in DVB that is writing to a struct file_operations.
> But as with almost all such cases in the kernel, this one is buggy:
> While the code nicely copies a template dvbdev, that template only has a
> pointer to a *shared* fops struct, the copy doesn't help that. So this
> code is overwriting the fops owner field for ALL active devices, not
> just the ones the copy of the template is for....

While working on a new device node, i stumbled across a similar issue
where attaching the frontend failed due to some sort of memory
corruption. This was seen as all the callbacks suddenly just vanished,
eventhough it existed in the driver

At that point, i figured it could be something due to an error at my
side , since the device that i was working was a bit complex and had
API changes as well.

Thanks for pointing it out. It looks like the issue sounds similar.

regards,
Manu

Re: dvb shared datastructure bug?

[Marcel Siegert]

[-- Attachment #1.1: Type: text/plain, Size: 1696 bytes --]

On Tuesday 13 February 2007, Arjan van de Ven wrote:
> Hi,
> 
> while working on the last pieces of the file_ops constantification, DVB
> is the small village in France that is holding the Romans at bay... but
> I think I found the final flaw in it now: 
> 
>        *pdvbdev = dvbdev = kmalloc(sizeof(struct dvb_device), GFP_KERNEL);
> 
>         if (!dvbdev) {
>                 mutex_unlock(&dvbdev_register_lock);
>                 return -ENOMEM;
>         }
> 
>         memcpy(dvbdev, template, sizeof(struct dvb_device));
>         dvbdev->type = type;
>         dvbdev->id = id;
>         dvbdev->adapter = adap;
>         dvbdev->priv = priv;
> 
>         dvbdev->fops->owner = adap->module;
> 
> 
> this is the place in DVB that is writing to a struct file_operations.
> But as with almost all such cases in the kernel, this one is buggy:
> While the code nicely copies a template dvbdev, that template only has a
> pointer to a *shared* fops struct, the copy doesn't help that. So this
> code is overwriting the fops owner field for ALL active devices, not
> just the ones the copy of the template is for....
> 
> I'm lost in the maze of this part of DVB (it seems to have some magic
> potion to resist me) but I was hoping some of the local citizens could
> take a look at this buglet...
> 
> Greetings,
>     Arjan van de Ven

hi arjan,
thanks for pointing out this issue.

attached find a patch that fixes the problem.

@mauro - please pull changeset a7ac92d208fe
   dvbdev: fix illegal re-usage of fileoperations struct

from  http://www.linuxtv.org/hg/~mws/v4l-dvb-fixtree

for upstream to kernel. thanks.

best regards 
marcel

[-- Attachment #1.2: dvbdevfix.patch --]
[-- Type: text/x-diff, Size: 1673 bytes --]

diff -r 667e84e2e762 linux/drivers/media/dvb/dvb-core/dvbdev.c
--- a/linux/drivers/media/dvb/dvb-core/dvbdev.c	Tue Feb 13 07:00:55 2007 -0200
+++ b/linux/drivers/media/dvb/dvb-core/dvbdev.c	Tue Feb 13 12:00:13 2007 +0100
@@ -211,12 +211,14 @@ int dvb_register_device(struct dvb_adapt
 			const struct dvb_device *template, void *priv, int type)
 {
 	struct dvb_device *dvbdev;
+	struct file_operations *dvbdevfops;
+
 	int id;
 
 	if (mutex_lock_interruptible(&dvbdev_register_lock))
 		return -ERESTARTSYS;
 
-	if ((id = dvbdev_get_free_id (adap, type)) < 0) {
+	if ((id = dvbdev_get_free_id (adap, type)) < 0){
 		mutex_unlock(&dvbdev_register_lock);
 		*pdvbdev = NULL;
 		printk ("%s: could get find free device id...\n", __FUNCTION__);
@@ -225,7 +227,15 @@ int dvb_register_device(struct dvb_adapt
 
 	*pdvbdev = dvbdev = kmalloc(sizeof(struct dvb_device), GFP_KERNEL);
 
-	if (!dvbdev) {
+	if (!dvbdev){
+		mutex_unlock(&dvbdev_register_lock);
+		return -ENOMEM;
+	}
+
+	dvbdevfops = kzalloc(sizeof(struct file_operations), GFP_KERNEL);
+
+	if (!dvbdevfops){
+		kfree (dvbdev);
 		mutex_unlock(&dvbdev_register_lock);
 		return -ENOMEM;
 	}
@@ -235,7 +245,7 @@ int dvb_register_device(struct dvb_adapt
 	dvbdev->id = id;
 	dvbdev->adapter = adap;
 	dvbdev->priv = priv;
-
+	dvbdev->fops = dvbdevfops;
 	dvbdev->fops->owner = adap->module;
 
 	list_add_tail (&dvbdev->list_head, &adap->device_list);
@@ -263,6 +273,7 @@ void dvb_unregister_device(struct dvb_de
 					dvbdev->type, dvbdev->id)));
 
 	list_del (&dvbdev->list_head);
+	kfree (dvbdev->fops);
 	kfree (dvbdev);
 }
 EXPORT_SYMBOL(dvb_unregister_device);

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: dvb shared datastructure bug?

[Manu Abraham]

On 2/13/07, Marcel Siegert <mws@linuxtv.org> wrote:
> On Tuesday 13 February 2007, Arjan van de Ven wrote:
> > Hi,
> >
> > while working on the last pieces of the file_ops constantification, DVB
> > is the small village in France that is holding the Romans at bay... but
> > I think I found the final flaw in it now:
> >
> >        *pdvbdev = dvbdev = kmalloc(sizeof(struct dvb_device), GFP_KERNEL);
> >
> >         if (!dvbdev) {
> >                 mutex_unlock(&dvbdev_register_lock);
> >                 return -ENOMEM;
> >         }
> >
> >         memcpy(dvbdev, template, sizeof(struct dvb_device));
> >         dvbdev->type = type;
> >         dvbdev->id = id;
> >         dvbdev->adapter = adap;
> >         dvbdev->priv = priv;
> >
> >         dvbdev->fops->owner = adap->module;
> >
> >
> > this is the place in DVB that is writing to a struct file_operations.
> > But as with almost all such cases in the kernel, this one is buggy:
> > While the code nicely copies a template dvbdev, that template only has a
> > pointer to a *shared* fops struct, the copy doesn't help that. So this
> > code is overwriting the fops owner field for ALL active devices, not
> > just the ones the copy of the template is for....
> >
> > I'm lost in the maze of this part of DVB (it seems to have some magic
> > potion to resist me) but I was hoping some of the local citizens could
> > take a look at this buglet...
> >
> > Greetings,
> >     Arjan van de Ven
>
> hi arjan,
> thanks for pointing out this issue.
>
> attached find a patch that fixes the problem.
>
> @mauro - please pull changeset a7ac92d208fe
>    dvbdev: fix illegal re-usage of fileoperations struct
>
> from  http://www.linuxtv.org/hg/~mws/v4l-dvb-fixtree
>

Ack'd-by: Manu Abraham <manu@linuxtv.org>

Re: dvb shared datastructure bug?

[Arjan van de Ven]

> attached find a patch that fixes the problem.


Hi,

Thank you for the quick response.
I think there is a small bug in this; at least I don't see where you
copy over the content of the fops template to the newly allocated piece
of memory...

Greetings,
   Arjan van de Ven
-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org

Re: dvb shared datastructure bug?

[Jakub Jelinek]

On Tue, Feb 13, 2007 at 03:14:23PM +0400, Manu Abraham wrote:
> >thanks for pointing out this issue.
> >
> >attached find a patch that fixes the problem.
> >
> >@mauro - please pull changeset a7ac92d208fe
> >   dvbdev: fix illegal re-usage of fileoperations struct
> >
> >from  http://www.linuxtv.org/hg/~mws/v4l-dvb-fixtree
> >
> 
> Ack'd-by: Manu Abraham <manu@linuxtv.org>

Wouldn't it be better to kmalloc both struct dvb_device and
struct file_operations together instead of doing 2 separate allocations?
struct dvd_device_plus_fops
{
  struct dvb_device dev;
  struct file_operations fops;
} *dev_fops = kmalloc (sizeof (struct dvd_device_plus_fops), GFP_KERNEL);
*pdvbdev = dvbdev = (struct dvb_device *)dev_fops;
if (dev_fops == NULL)
  error handling;
memset (&dev_fops->fops, 0, sizeof (dev_fops->fops));
...
dvbdev->fops = &dev_fops->fops;

	Jakub

Re: dvb shared datastructure bug?

[Marcel Siegert]

On Tuesday 13 February 2007, Jakub Jelinek wrote:
> On Tue, Feb 13, 2007 at 03:14:23PM +0400, Manu Abraham wrote:
> > >thanks for pointing out this issue.
> > >
> > >attached find a patch that fixes the problem.
> > >
> > >@mauro - please pull changeset a7ac92d208fe
> > >   dvbdev: fix illegal re-usage of fileoperations struct
> > >
> > >from  http://www.linuxtv.org/hg/~mws/v4l-dvb-fixtree
> > >
> > 
> > Ack'd-by: Manu Abraham <manu@linuxtv.org>
> 
> Wouldn't it be better to kmalloc both struct dvb_device and
> struct file_operations together instead of doing 2 separate allocations?
> struct dvd_device_plus_fops
> {
>   struct dvb_device dev;
>   struct file_operations fops;
> } *dev_fops = kmalloc (sizeof (struct dvd_device_plus_fops), GFP_KERNEL);
> *pdvbdev = dvbdev = (struct dvb_device *)dev_fops;
> if (dev_fops == NULL)
>   error handling;
> memset (&dev_fops->fops, 0, sizeof (dev_fops->fops));
> ...
> dvbdev->fops = &dev_fops->fops;
> 
> 	Jakub
> 

hi jakub, 

it may be worth doing that, but, imho that can be done when we are perfoming
some revise of the whole dvb-core subsystem. 

at the moment i would stay at "as-is", the code is more readable and the more "cost"
of the additional alloc is affordable.

thanks for your comments.
marcel

Re: dvb shared datastructure bug?

[Marcel Siegert]

[-- Attachment #1: Type: text/plain, Size: 721 bytes --]

On Tuesday 13 February 2007, you wrote:
> 
> > attached find a patch that fixes the problem.
> 
> 
> Hi,
> 
> Thank you for the quick response.
> I think there is a small bug in this; at least I don't see where you
> copy over the content of the fops template to the newly allocated piece
> of memory...
> 
> Greetings,
>    Arjan van de Ven

hi arjan,

also fixed that issue. my fault, sorry.

i updated my repository @ linutv.org accordingly.

i will ask mauro to pull changeset   b265e2484422
   dvbdev: fix illegal re-usage of fileoperations struct
from  http://www.linuxtv.org/hg/~mws/v4l-dvb-fixtree

for upstream to kernel., if no more issues appear today.

thanks again.

marcel



[-- Attachment #2: dvbdevfix.patch --]
[-- Type: text/x-diff, Size: 1749 bytes --]

diff -r 667e84e2e762 linux/drivers/media/dvb/dvb-core/dvbdev.c
--- a/linux/drivers/media/dvb/dvb-core/dvbdev.c	Tue Feb 13 07:00:55 2007 -0200
+++ b/linux/drivers/media/dvb/dvb-core/dvbdev.c	Tue Feb 13 13:44:47 2007 +0100
@@ -211,12 +211,14 @@ int dvb_register_device(struct dvb_adapt
 			const struct dvb_device *template, void *priv, int type)
 {
 	struct dvb_device *dvbdev;
+	struct file_operations *dvbdevfops;
+
 	int id;
 
 	if (mutex_lock_interruptible(&dvbdev_register_lock))
 		return -ERESTARTSYS;
 
-	if ((id = dvbdev_get_free_id (adap, type)) < 0) {
+	if ((id = dvbdev_get_free_id (adap, type)) < 0){
 		mutex_unlock(&dvbdev_register_lock);
 		*pdvbdev = NULL;
 		printk ("%s: could get find free device id...\n", __FUNCTION__);
@@ -225,7 +227,15 @@ int dvb_register_device(struct dvb_adapt
 
 	*pdvbdev = dvbdev = kmalloc(sizeof(struct dvb_device), GFP_KERNEL);
 
-	if (!dvbdev) {
+	if (!dvbdev){
+		mutex_unlock(&dvbdev_register_lock);
+		return -ENOMEM;
+	}
+
+	dvbdevfops = kzalloc(sizeof(struct file_operations), GFP_KERNEL);
+
+	if (!dvbdevfops){
+		kfree (dvbdev);
 		mutex_unlock(&dvbdev_register_lock);
 		return -ENOMEM;
 	}
@@ -235,7 +245,9 @@ int dvb_register_device(struct dvb_adapt
 	dvbdev->id = id;
 	dvbdev->adapter = adap;
 	dvbdev->priv = priv;
-
+	dvbdev->fops = dvbdevfops;
+
+	memcpy(dvbdev->fops, template->fops, sizeof(struct file_operations));
 	dvbdev->fops->owner = adap->module;
 
 	list_add_tail (&dvbdev->list_head, &adap->device_list);
@@ -263,6 +275,7 @@ void dvb_unregister_device(struct dvb_de
 					dvbdev->type, dvbdev->id)));
 
 	list_del (&dvbdev->list_head);
+	kfree (dvbdev->fops);
 	kfree (dvbdev);
 }
 EXPORT_SYMBOL(dvb_unregister_device);

Re: [v4l-dvb-maintainer] Re: dvb shared datastructure bug?

[Trent Piepho]

On Tue, 13 Feb 2007, Jakub Jelinek wrote:
> Wouldn't it be better to kmalloc both struct dvb_device and
> struct file_operations together instead of doing 2 separate allocations?
> struct dvd_device_plus_fops
> {
>   struct dvb_device dev;
>   struct file_operations fops;
> } *dev_fops = kmalloc (sizeof (struct dvd_device_plus_fops), GFP_KERNEL);
> *pdvbdev = dvbdev = (struct dvb_device *)dev_fops;
> if (dev_fops == NULL)
>   error handling;
> memset (&dev_fops->fops, 0, sizeof (dev_fops->fops));
> ...
> dvbdev->fops = &dev_fops->fops;

Maybe change struct dvb_device:

 struct dvb_device {
         struct list_head list_head;
-        struct file_operations *fops;
+        struct file_operations fops;
         struct dvb_adapter *adapter;

Re: [v4l-dvb-maintainer] Re: dvb shared datastructure bug?

[Marcel Siegert]

On Tuesday 13 February 2007, Trent Piepho wrote:
> On Tue, 13 Feb 2007, Jakub Jelinek wrote:
> > Wouldn't it be better to kmalloc both struct dvb_device and
> > struct file_operations together instead of doing 2 separate allocations?
> > struct dvd_device_plus_fops
> > {
> >   struct dvb_device dev;
> >   struct file_operations fops;
> > } *dev_fops = kmalloc (sizeof (struct dvd_device_plus_fops), GFP_KERNEL);
> > *pdvbdev = dvbdev = (struct dvb_device *)dev_fops;
> > if (dev_fops == NULL)
> >   error handling;
> > memset (&dev_fops->fops, 0, sizeof (dev_fops->fops));
> > ...
> > dvbdev->fops = &dev_fops->fops;
> 
> Maybe change struct dvb_device:
> 
>  struct dvb_device {
>          struct list_head list_head;
> -        struct file_operations *fops;
> +        struct file_operations fops;
>          struct dvb_adapter *adapter;
> 
> 
hi trent, 

your suggestion is correct and useful, 
but this would mean a lot of more work to do, 
and maybe creates new issues.

we should get a working - non issued - v4l-dvb tree and
afterwards start to review, optimize things.

can we put that on wait for the future?

regards 
marcel

Re: [v4l-dvb-maintainer] Re: dvb shared datastructure bug?

[Manu Abraham]

On 2/13/07, Trent Piepho <xyzzy@speakeasy.org> wrote:
> On Tue, 13 Feb 2007, Jakub Jelinek wrote:
> > Wouldn't it be better to kmalloc both struct dvb_device and
> > struct file_operations together instead of doing 2 separate allocations?
> > struct dvd_device_plus_fops
> > {
> >   struct dvb_device dev;
> >   struct file_operations fops;
> > } *dev_fops = kmalloc (sizeof (struct dvd_device_plus_fops), GFP_KERNEL);
> > *pdvbdev = dvbdev = (struct dvb_device *)dev_fops;
> > if (dev_fops == NULL)
> >   error handling;
> > memset (&dev_fops->fops, 0, sizeof (dev_fops->fops));
> > ...
> > dvbdev->fops = &dev_fops->fops;
>
> Maybe change struct dvb_device:
>
>  struct dvb_device {
>          struct list_head list_head;
> -        struct file_operations *fops;
> +        struct file_operations fops;
>          struct dvb_adapter *adapter;
>

We can of course do that, but if we do that now, i will have to rework
on the changes that which i have, but considering the changes that i
have i wouldn't want to do such a change right now as marcel
explained. But we can surely go in for this, as soon as the rest of
the API changes goes in, ie, multiproto + adaptor changes

manu