Re: [PATCHv2 22/29] v4l2-async: Don't use dynamic static allocation

[Mauro Carvalho Chehab]

Em Mon, 04 Nov 2013 14:15:04 +0100
Hans Verkuil <hverk...@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.che...@samsung.com>
> > Cc: Guennadi Liakhovetski <g.liakhovet...@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(&notifier->list);
 
        list_for_each_entry_safe(sd, tmp, &notifier->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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html