* dvb shared datastructure bug? @ 2007-02-13 7:47 Arjan van de Ven 2007-02-13 8:49 ` [v4l-dvb-maintainer] " Manu Abraham 2007-02-13 11:04 ` Marcel Siegert 0 siblings, 2 replies; 11+ messages in thread From: Arjan van de Ven @ 2007-02-13 7:47 UTC (permalink / raw) To: mchehab, v4l-dvb-maintainer; +Cc: linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v4l-dvb-maintainer] dvb shared datastructure bug? 2007-02-13 7:47 dvb shared datastructure bug? Arjan van de Ven @ 2007-02-13 8:49 ` Manu Abraham 2007-02-13 11:04 ` Marcel Siegert 1 sibling, 0 replies; 11+ messages in thread From: Manu Abraham @ 2007-02-13 8:49 UTC (permalink / raw) To: Arjan van de Ven; +Cc: mchehab, v4l-dvb-maintainer, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: dvb shared datastructure bug? 2007-02-13 7:47 dvb shared datastructure bug? Arjan van de Ven 2007-02-13 8:49 ` [v4l-dvb-maintainer] " Manu Abraham @ 2007-02-13 11:04 ` Marcel Siegert 2007-02-13 11:14 ` Manu Abraham 2007-02-13 11:35 ` Arjan van de Ven 1 sibling, 2 replies; 11+ messages in thread From: Marcel Siegert @ 2007-02-13 11:04 UTC (permalink / raw) To: Arjan van de Ven; +Cc: mchehab, v4l-dvb-maintainer, linux-kernel, Manu Abraham [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: dvb shared datastructure bug? 2007-02-13 11:04 ` Marcel Siegert @ 2007-02-13 11:14 ` Manu Abraham 2007-02-13 11:35 ` Jakub Jelinek 2007-02-13 11:35 ` Arjan van de Ven 1 sibling, 1 reply; 11+ messages in thread From: Manu Abraham @ 2007-02-13 11:14 UTC (permalink / raw) To: Marcel Siegert Cc: Arjan van de Ven, mchehab, v4l-dvb-maintainer, linux-kernel 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: dvb shared datastructure bug? 2007-02-13 11:14 ` Manu Abraham @ 2007-02-13 11:35 ` Jakub Jelinek 2007-02-13 12:02 ` Marcel Siegert 2007-02-13 13:09 ` [v4l-dvb-maintainer] " Trent Piepho 0 siblings, 2 replies; 11+ messages in thread From: Jakub Jelinek @ 2007-02-13 11:35 UTC (permalink / raw) To: Manu Abraham Cc: Marcel Siegert, Arjan van de Ven, mchehab, v4l-dvb-maintainer, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: dvb shared datastructure bug? 2007-02-13 11:35 ` Jakub Jelinek @ 2007-02-13 12:02 ` Marcel Siegert 2007-02-13 13:09 ` [v4l-dvb-maintainer] " Trent Piepho 1 sibling, 0 replies; 11+ messages in thread From: Marcel Siegert @ 2007-02-13 12:02 UTC (permalink / raw) To: Jakub Jelinek Cc: Manu Abraham, Arjan van de Ven, mchehab, v4l-dvb-maintainer, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v4l-dvb-maintainer] Re: dvb shared datastructure bug? 2007-02-13 11:35 ` Jakub Jelinek 2007-02-13 12:02 ` Marcel Siegert @ 2007-02-13 13:09 ` Trent Piepho 2007-02-13 13:21 ` Marcel Siegert 2007-02-13 13:21 ` Manu Abraham 1 sibling, 2 replies; 11+ messages in thread From: Trent Piepho @ 2007-02-13 13:09 UTC (permalink / raw) To: Jakub Jelinek Cc: Manu Abraham, v4l-dvb-maintainer, mchehab, linux-kernel, Arjan van de Ven 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; ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v4l-dvb-maintainer] Re: dvb shared datastructure bug? 2007-02-13 13:09 ` [v4l-dvb-maintainer] " Trent Piepho @ 2007-02-13 13:21 ` Marcel Siegert 2007-02-13 13:21 ` Manu Abraham 1 sibling, 0 replies; 11+ messages in thread From: Marcel Siegert @ 2007-02-13 13:21 UTC (permalink / raw) To: Trent Piepho Cc: Jakub Jelinek, Manu Abraham, v4l-dvb-maintainer, mchehab, linux-kernel, Arjan van de Ven 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v4l-dvb-maintainer] Re: dvb shared datastructure bug? 2007-02-13 13:09 ` [v4l-dvb-maintainer] " Trent Piepho 2007-02-13 13:21 ` Marcel Siegert @ 2007-02-13 13:21 ` Manu Abraham 1 sibling, 0 replies; 11+ messages in thread From: Manu Abraham @ 2007-02-13 13:21 UTC (permalink / raw) To: Trent Piepho Cc: Jakub Jelinek, v4l-dvb-maintainer, mchehab, linux-kernel, Arjan van de Ven 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: dvb shared datastructure bug? 2007-02-13 11:04 ` Marcel Siegert 2007-02-13 11:14 ` Manu Abraham @ 2007-02-13 11:35 ` Arjan van de Ven 2007-02-13 13:05 ` Marcel Siegert 1 sibling, 1 reply; 11+ messages in thread From: Arjan van de Ven @ 2007-02-13 11:35 UTC (permalink / raw) To: Marcel Siegert; +Cc: mchehab, v4l-dvb-maintainer, linux-kernel, Manu Abraham > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: dvb shared datastructure bug? 2007-02-13 11:35 ` Arjan van de Ven @ 2007-02-13 13:05 ` Marcel Siegert 0 siblings, 0 replies; 11+ messages in thread From: Marcel Siegert @ 2007-02-13 13:05 UTC (permalink / raw) To: Arjan van de Ven; +Cc: mchehab, v4l-dvb-maintainer, Manu Abraham, linux-kernel [-- 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); ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2007-02-13 13:21 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-02-13 7:47 dvb shared datastructure bug? Arjan van de Ven 2007-02-13 8:49 ` [v4l-dvb-maintainer] " Manu Abraham 2007-02-13 11:04 ` Marcel Siegert 2007-02-13 11:14 ` Manu Abraham 2007-02-13 11:35 ` Jakub Jelinek 2007-02-13 12:02 ` Marcel Siegert 2007-02-13 13:09 ` [v4l-dvb-maintainer] " Trent Piepho 2007-02-13 13:21 ` Marcel Siegert 2007-02-13 13:21 ` Manu Abraham 2007-02-13 11:35 ` Arjan van de Ven 2007-02-13 13:05 ` Marcel Siegert
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.