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
On 2/13/07, Arjan van de Ven <[email protected]> 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
On 2/13/07, Marcel Siegert <[email protected]> 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 <[email protected]>
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
> 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
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 <[email protected]>
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
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 <[email protected]>
>
> 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
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
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;
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
On 2/13/07, Trent Piepho <[email protected]> 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