* [Xenomai] Analogy: subdev list not intitialized if driver private data is NULL
@ 2014-04-10 8:58 Andreas Glatz
2014-04-11 13:15 ` Philippe Gerum
0 siblings, 1 reply; 3+ messages in thread
From: Andreas Glatz @ 2014-04-10 8:58 UTC (permalink / raw)
To: xenomai
Currently my analogy driver for a new DAQ board doesn't need the
private data struct of the driver, so I set the size to 0:
/* Analogy driver structure */
static a4l_drv_t a4l_drv_pandadaq = {
.owner = THIS_MODULE,
.board_name = "analogy_pandadaq",
.attach = dev_pandadaq_attach,
.detach = dev_pandadaq_detach,
.privdata_size = 0,
};
However, due to a potential bug the kernel crashes with a NULL ptr
exception after linking analogy0 to my driver.
If I apply this patch to the git head of xenomai-2.6 everything works
for me:
diff --git a/ksrc/drivers/analogy/device.c b/ksrc/drivers/analogy/
device.c
index b93ae72..c4095a2 100644
--- a/ksrc/drivers/analogy/device.c
+++ b/ksrc/drivers/analogy/device.c
@@ -278,15 +278,14 @@ int a4l_assign_driver(a4l_cxt_t * cxt,
a4l_dev_t *dev = a4l_get_dev(cxt);
dev->driver = drv;
+
+ INIT_LIST_HEAD(&dev->subdvsq);
if (drv->privdata_size == 0)
__a4l_dbg(1, core_dbg,
"a4l_assign_driver: warning! "
"the field priv will not be usable\n");
else {
-
- INIT_LIST_HEAD(&dev->subdvsq);
-
dev->priv = rtdm_malloc(drv->privdata_size);
if (dev->priv == NULL && drv->privdata_size != 0) {
__a4l_err("a4l_assign_driver: "
A.
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Xenomai] Analogy: subdev list not intitialized if driver private data is NULL
2014-04-10 8:58 [Xenomai] Analogy: subdev list not intitialized if driver private data is NULL Andreas Glatz
@ 2014-04-11 13:15 ` Philippe Gerum
2014-04-12 12:14 ` Andreas Glatz
0 siblings, 1 reply; 3+ messages in thread
From: Philippe Gerum @ 2014-04-11 13:15 UTC (permalink / raw)
To: Andreas Glatz, xenomai
On 04/10/2014 10:58 AM, Andreas Glatz wrote:
> Currently my analogy driver for a new DAQ board doesn't need the private
> data struct of the driver, so I set the size to 0:
>
> /* Analogy driver structure */
> static a4l_drv_t a4l_drv_pandadaq = {
> .owner = THIS_MODULE,
> .board_name = "analogy_pandadaq",
> .attach = dev_pandadaq_attach,
> .detach = dev_pandadaq_detach,
> .privdata_size = 0,
> };
>
> However, due to a potential bug the kernel crashes with a NULL ptr
> exception after linking analogy0 to my driver.
>
> If I apply this patch to the git head of xenomai-2.6 everything works
> for me:
>
> diff --git a/ksrc/drivers/analogy/device.c b/ksrc/drivers/analogy/device.c
> index b93ae72..c4095a2 100644
> --- a/ksrc/drivers/analogy/device.c
> +++ b/ksrc/drivers/analogy/device.c
> @@ -278,15 +278,14 @@ int a4l_assign_driver(a4l_cxt_t * cxt,
> a4l_dev_t *dev = a4l_get_dev(cxt);
>
> dev->driver = drv;
> +
> + INIT_LIST_HEAD(&dev->subdvsq);
>
> if (drv->privdata_size == 0)
> __a4l_dbg(1, core_dbg,
> "a4l_assign_driver: warning! "
> "the field priv will not be usable\n");
> else {
> -
> - INIT_LIST_HEAD(&dev->subdvsq);
> -
> dev->priv = rtdm_malloc(drv->privdata_size);
> if (dev->priv == NULL && drv->privdata_size != 0) {
> __a4l_err("a4l_assign_driver: "
>
>
Makes sense. This release routine may also attempt to free a NULL
pointer, which is fortunately caught by the underlying allocator, but is
definitely invalid. Finally, the open-coded iterator for subdevices
reinvents a wheel. Could you try this patch adding a few fixups to
yours? TIA,
diff --git a/ksrc/drivers/analogy/device.c b/ksrc/drivers/analogy/device.c
index b93ae72..60b2977 100644
--- a/ksrc/drivers/analogy/device.c
+++ b/ksrc/drivers/analogy/device.c
@@ -278,17 +278,15 @@ int a4l_assign_driver(a4l_cxt_t * cxt,
a4l_dev_t *dev = a4l_get_dev(cxt);
dev->driver = drv;
+ INIT_LIST_HEAD(&dev->subdvsq);
if (drv->privdata_size == 0)
__a4l_dbg(1, core_dbg,
"a4l_assign_driver: warning! "
"the field priv will not be usable\n");
else {
-
- INIT_LIST_HEAD(&dev->subdvsq);
-
dev->priv = rtdm_malloc(drv->privdata_size);
- if (dev->priv == NULL && drv->privdata_size != 0) {
+ if (dev->priv == NULL) {
__a4l_err("a4l_assign_driver: "
"call(alloc) failed\n");
ret = -ENOMEM;
@@ -325,27 +323,26 @@ out_assign_driver:
int a4l_release_driver(a4l_cxt_t * cxt)
{
- int ret = 0;
a4l_dev_t *dev = a4l_get_dev(cxt);
+ a4l_subd_t *subd, *tmp;
+ int ret = 0;
if ((ret = dev->driver->detach(dev)) != 0)
goto out_release_driver;
- /* Decrease module's count
- so as to allow module unloading */
module_put(dev->driver->owner);
/* In case, the driver developer did not free the subdevices */
- while (&dev->subdvsq != dev->subdvsq.next) {
- struct list_head *this = dev->subdvsq.next;
- a4l_subd_t *tmp = list_entry(this, a4l_subd_t, list);
-
- list_del(this);
- rtdm_free(tmp);
- }
+ if (!list_empty(&dev->subdvsq))
+ list_for_each_entry_safe(subd, tmp, &dev->subdvsq, list) {
+ list_del(&subd->list);
+ rtdm_free(subd);
+ }
/* Free the private field */
- rtdm_free(dev->priv);
+ if (dev->priv)
+ rtdm_free(dev->priv);
+
dev->driver = NULL;
out_release_driver:
--
Philippe.
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Xenomai] Analogy: subdev list not intitialized if driver private data is NULL
2014-04-11 13:15 ` Philippe Gerum
@ 2014-04-12 12:14 ` Andreas Glatz
0 siblings, 0 replies; 3+ messages in thread
From: Andreas Glatz @ 2014-04-12 12:14 UTC (permalink / raw)
To: Philippe Gerum; +Cc: xenomai
On 11 Apr 2014, at 14:15, Philippe Gerum wrote:
> On 04/10/2014 10:58 AM, Andreas Glatz wrote:
>> Currently my analogy driver for a new DAQ board doesn't need the
>> private
>> data struct of the driver, so I set the size to 0:
>>
>> /* Analogy driver structure */
>> static a4l_drv_t a4l_drv_pandadaq = {
>> .owner = THIS_MODULE,
>> .board_name = "analogy_pandadaq",
>> .attach = dev_pandadaq_attach,
>> .detach = dev_pandadaq_detach,
>> .privdata_size = 0,
>> };
>>
>> However, due to a potential bug the kernel crashes with a NULL ptr
>> exception after linking analogy0 to my driver.
>>
>> If I apply this patch to the git head of xenomai-2.6 everything works
>> for me:
>>
>> diff --git a/ksrc/drivers/analogy/device.c b/ksrc/drivers/analogy/
>> device.c
>> index b93ae72..c4095a2 100644
>> --- a/ksrc/drivers/analogy/device.c
>> +++ b/ksrc/drivers/analogy/device.c
>> @@ -278,15 +278,14 @@ int a4l_assign_driver(a4l_cxt_t * cxt,
>> a4l_dev_t *dev = a4l_get_dev(cxt);
>>
>> dev->driver = drv;
>> +
>> + INIT_LIST_HEAD(&dev->subdvsq);
>>
>> if (drv->privdata_size == 0)
>> __a4l_dbg(1, core_dbg,
>> "a4l_assign_driver: warning! "
>> "the field priv will not be usable\n");
>> else {
>> -
>> - INIT_LIST_HEAD(&dev->subdvsq);
>> -
>> dev->priv = rtdm_malloc(drv->privdata_size);
>> if (dev->priv == NULL && drv->privdata_size != 0) {
>> __a4l_err("a4l_assign_driver: "
>>
>>
>
> Makes sense. This release routine may also attempt to free a NULL
> pointer, which is fortunately caught by the underlying allocator,
> but is definitely invalid. Finally, the open-coded iterator for
> subdevices reinvents a wheel. Could you try this patch adding a few
> fixups to yours? TIA,
>
> diff --git a/ksrc/drivers/analogy/device.c b/ksrc/drivers/analogy/
> device.c
> index b93ae72..60b2977 100644
> --- a/ksrc/drivers/analogy/device.c
> +++ b/ksrc/drivers/analogy/device.c
> @@ -278,17 +278,15 @@ int a4l_assign_driver(a4l_cxt_t * cxt,
> a4l_dev_t *dev = a4l_get_dev(cxt);
>
> dev->driver = drv;
> + INIT_LIST_HEAD(&dev->subdvsq);
>
> if (drv->privdata_size == 0)
> __a4l_dbg(1, core_dbg,
> "a4l_assign_driver: warning! "
> "the field priv will not be usable\n");
> else {
> -
> - INIT_LIST_HEAD(&dev->subdvsq);
> -
> dev->priv = rtdm_malloc(drv->privdata_size);
> - if (dev->priv == NULL && drv->privdata_size != 0) {
> + if (dev->priv == NULL) {
> __a4l_err("a4l_assign_driver: "
> "call(alloc) failed\n");
> ret = -ENOMEM;
> @@ -325,27 +323,26 @@ out_assign_driver:
>
> int a4l_release_driver(a4l_cxt_t * cxt)
> {
> - int ret = 0;
> a4l_dev_t *dev = a4l_get_dev(cxt);
> + a4l_subd_t *subd, *tmp;
> + int ret = 0;
>
> if ((ret = dev->driver->detach(dev)) != 0)
> goto out_release_driver;
>
> - /* Decrease module's count
> - so as to allow module unloading */
> module_put(dev->driver->owner);
>
> /* In case, the driver developer did not free the subdevices */
> - while (&dev->subdvsq != dev->subdvsq.next) {
> - struct list_head *this = dev->subdvsq.next;
> - a4l_subd_t *tmp = list_entry(this, a4l_subd_t, list);
> -
> - list_del(this);
> - rtdm_free(tmp);
> - }
> + if (!list_empty(&dev->subdvsq))
> + list_for_each_entry_safe(subd, tmp, &dev->subdvsq, list) {
> + list_del(&subd->list);
> + rtdm_free(subd);
> + }
>
> /* Free the private field */
> - rtdm_free(dev->priv);
> + if (dev->priv)
> + rtdm_free(dev->priv);
> +
> dev->driver = NULL;
>
> out_release_driver:
>
>
Your patch compiles cleanly and works for me.
I tested:
> analogy_config analogy0 analogy_pandadaq
> [...]
> analogy_config -r analogy0
> rmmod analogy_pandadaq
> insmod analogy_pandadaq
> [... and the same from the start]
I didn't see any warnings in dmesg...
A.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-04-12 12:14 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-10 8:58 [Xenomai] Analogy: subdev list not intitialized if driver private data is NULL Andreas Glatz
2014-04-11 13:15 ` Philippe Gerum
2014-04-12 12:14 ` Andreas Glatz
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.