| Message ID | 1383399097-11615-23-git-send-email-m.chehab@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/02/2013 02:31 PM, Mauro Carvalho Chehab wrote: > Dynamic static allocation is evil, as Kernel stack is too low, and > compilation complains about it on some archs: > > drivers/media/v4l2-core/v4l2-async.c:238:1: warning: 'v4l2_async_notifier_unregister' uses dynamic stack allocation [enabled by default] > > Instead, let's enforce a limit for the buffer. > > In this specific case, there's a hard limit imposed by V4L2_MAX_SUBDEVS, > with is currently 128. That means that the buffer size can be up to > 128x8 = 1024 bytes (on a 64bits kernel), with is too big for stack. > > Worse than that, someone could increase it and cause real troubles. > > So, let's use dynamically allocated data, instead. > > Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com> > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > --- > drivers/media/v4l2-core/v4l2-async.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index c85d69da35bd..071596869036 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -189,12 +189,14 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) > struct v4l2_subdev *sd, *tmp; > unsigned int notif_n_subdev = notifier->num_subdevs; > unsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS); > - struct device *dev[n_subdev]; > + struct device **dev; > int i = 0; > > if (!notifier->v4l2_dev) > return; > > + dev = kmalloc(sizeof(*dev) * n_subdev, GFP_KERNEL); > + No check for dev == NULL? Regards, Hans > mutex_lock(&list_lock); > > list_del(¬ifier->list); > @@ -228,6 +230,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) > } > put_device(d); > } > + kfree(dev); > > notifier->v4l2_dev = NULL; > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Mon, 04 Nov 2013 14:15:04 +0100 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > On 11/02/2013 02:31 PM, Mauro Carvalho Chehab wrote: > > Dynamic static allocation is evil, as Kernel stack is too low, and > > compilation complains about it on some archs: > > > > drivers/media/v4l2-core/v4l2-async.c:238:1: warning: 'v4l2_async_notifier_unregister' uses dynamic stack allocation [enabled by default] > > > > Instead, let's enforce a limit for the buffer. > > > > In this specific case, there's a hard limit imposed by V4L2_MAX_SUBDEVS, > > with is currently 128. That means that the buffer size can be up to > > 128x8 = 1024 bytes (on a 64bits kernel), with is too big for stack. > > > > Worse than that, someone could increase it and cause real troubles. > > > > So, let's use dynamically allocated data, instead. > > > > Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com> > > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > --- > > drivers/media/v4l2-core/v4l2-async.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > index c85d69da35bd..071596869036 100644 > > --- a/drivers/media/v4l2-core/v4l2-async.c > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > @@ -189,12 +189,14 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) > > struct v4l2_subdev *sd, *tmp; > > unsigned int notif_n_subdev = notifier->num_subdevs; > > unsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS); > > - struct device *dev[n_subdev]; > > + struct device **dev; > > int i = 0; > > > > if (!notifier->v4l2_dev) > > return; > > > > + dev = kmalloc(sizeof(*dev) * n_subdev, GFP_KERNEL); > > + > > No check for dev == NULL? Well, what should be done in this case? We could do the changes below: void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) { struct v4l2_subdev *sd, *tmp; unsigned int notif_n_subdev = notifier->num_subdevs; unsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS); - struct device *dev[n_subdev]; + struct device **dev; int i = 0; if (!notifier->v4l2_dev) return; + dev = kmalloc(sizeof(*dev) * n_subdev, GFP_KERNEL); + if (!dev) { + WARN_ON(true); + return; + } + mutex_lock(&list_lock); list_del(¬ifier->list); list_for_each_entry_safe(sd, tmp, ¬ifier->done, async_list) { dev[i] = get_device(sd->dev); v4l2_async_cleanup(sd); /* If we handled USB devices, we'd have to lock the parent too */ device_release_driver(dev[i++]); if (notifier->unbind) notifier->unbind(notifier, sd, sd->asd); } mutex_unlock(&list_lock); while (i--) { struct device *d = dev[i]; if (d && device_attach(d) < 0) { const char *name = "(none)"; int lock = device_trylock(d); if (lock && d->driver) name = d->driver->name; dev_err(d, "Failed to re-probe to %s\n", name); if (lock) device_unlock(d); } put_device(d); } + kfree(dev); notifier->v4l2_dev = NULL; /* * Don't care about the waiting list, it is initialised and populated * upon notifier registration. */ } EXPORT_SYMBOL(v4l2_async_notifier_unregister); But I suspect that this will cause an OOPS anyway, as the device will be only half-removed. So, it would likely OOPS at device removal or if the device got probed again, at probing time. So, IMHO, we should have at least a WARN_ON() for this case. Do you have a better idea? Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/11/13 12:36, Mauro Carvalho Chehab wrote: >>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c >>> > > index c85d69da35bd..071596869036 100644 >>> > > --- a/drivers/media/v4l2-core/v4l2-async.c >>> > > +++ b/drivers/media/v4l2-core/v4l2-async.c >>> > > @@ -189,12 +189,14 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) >>> > > struct v4l2_subdev *sd, *tmp; >>> > > unsigned int notif_n_subdev = notifier->num_subdevs; >>> > > unsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS); >>> > > - struct device *dev[n_subdev]; >>> > > + struct device **dev; >>> > > int i = 0; >>> > > >>> > > if (!notifier->v4l2_dev) >>> > > return; >>> > > >>> > > + dev = kmalloc(sizeof(*dev) * n_subdev, GFP_KERNEL); >>> > > + >> > >> > No check for dev == NULL? > Well, what should be done in this case? > > We could do the changes below: > > void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) > { > struct v4l2_subdev *sd, *tmp; > unsigned int notif_n_subdev = notifier->num_subdevs; > unsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS); > - struct device *dev[n_subdev]; > + struct device **dev; > int i = 0; > > if (!notifier->v4l2_dev) > return; > > + dev = kmalloc(sizeof(*dev) * n_subdev, GFP_KERNEL); > + if (!dev) { > + WARN_ON(true); > + return; > + } > + > mutex_lock(&list_lock); > > list_del(¬ifier->list); > > list_for_each_entry_safe(sd, tmp, ¬ifier->done, async_list) { > dev[i] = get_device(sd->dev); > > v4l2_async_cleanup(sd); > > /* If we handled USB devices, we'd have to lock the parent too */ > device_release_driver(dev[i++]); > > if (notifier->unbind) > notifier->unbind(notifier, sd, sd->asd); > } > > mutex_unlock(&list_lock); > > while (i--) { > struct device *d = dev[i]; > > if (d && device_attach(d) < 0) { > const char *name = "(none)"; > int lock = device_trylock(d); > > if (lock && d->driver) > name = d->driver->name; > dev_err(d, "Failed to re-probe to %s\n", name); > if (lock) > device_unlock(d); > } > put_device(d); > } > + kfree(dev); > > notifier->v4l2_dev = NULL; > > /* > * Don't care about the waiting list, it is initialised and populated > * upon notifier registration. > */ > } > EXPORT_SYMBOL(v4l2_async_notifier_unregister); > > But I suspect that this will cause an OOPS anyway, as the device will be > only half-removed. So, it would likely OOPS at device removal or if the > device got probed again, at probing time. > > So, IMHO, we should have at least a WARN_ON() for this case. > > Do you have a better idea? This is how Guennadi's patch looked like when it used dynamic allocation: http://www.spinics.net/lists/linux-sh/msg18194.html -- Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c index c85d69da35bd..071596869036 100644 --- a/drivers/media/v4l2-core/v4l2-async.c +++ b/drivers/media/v4l2-core/v4l2-async.c @@ -189,12 +189,14 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) struct v4l2_subdev *sd, *tmp; unsigned int notif_n_subdev = notifier->num_subdevs; unsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS); - struct device *dev[n_subdev]; + struct device **dev; int i = 0; if (!notifier->v4l2_dev) return; + dev = kmalloc(sizeof(*dev) * n_subdev, GFP_KERNEL); + mutex_lock(&list_lock); list_del(¬ifier->list); @@ -228,6 +230,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) } put_device(d); } + kfree(dev); notifier->v4l2_dev = NULL;
Dynamic static allocation is evil, as Kernel stack is too low, and compilation complains about it on some archs: drivers/media/v4l2-core/v4l2-async.c:238:1: warning: 'v4l2_async_notifier_unregister' uses dynamic stack allocation [enabled by default] Instead, let's enforce a limit for the buffer. In this specific case, there's a hard limit imposed by V4L2_MAX_SUBDEVS, with is currently 128. That means that the buffer size can be up to 128x8 = 1024 bytes (on a 64bits kernel), with is too big for stack. Worse than that, someone could increase it and cause real troubles. So, let's use dynamically allocated data, instead. Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de> --- drivers/media/v4l2-core/v4l2-async.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)