Hi Sakari,
On Thursday, 16 November 2017 14:32:37 EET Sakari Ailus wrote:
> On Thu, Nov 16, 2017 at 02:33:48AM +0200, Laurent Pinchart wrote:
> > Device unplug being asynchronous, it naturally races with operations
> > performed by userspace through ioctls or other file operations on video
> > device nodes.
> >
> > This leads to potential access to freed memory or to other resources
> > during device access if unplug occurs during device access. To solve
> > this, we need to wait until all device access completes when unplugging
> > the device, and block all further access when the device is being
> > unplugged.
> >
> > Three new functions are introduced. The video_device_enter() and
> > video_device_exit() functions must be used to mark entry and exit from
> > all code sections where the device can be accessed. The
>
> I wonder if it'd help splitting this patch into two: one that introduces
> the mechanism and the other that uses it. Up to you.
Sure, I'm not opposed to that. The second patch would be a bit small, but that
will change when I'll add support for more system calls.
> Nevertheless, it'd be better to have other system calls covered as well.
>
> > video_device_unplug() function is then used in the unplug handler to
> > mark the device as being unplugged and wait for all access to complete.
> >
> > As an example mark the ioctl handler as a device access section. Other
> > file operations need to be protected too, and blocking ioctls (such as
> > VIDIOC_DQBUF) need to be handled as well.
> >
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > ---
> >
> > drivers/media/v4l2-core/v4l2-dev.c | 57 +++++++++++++++++++++++++++++++++
> > include/media/v4l2-dev.h | 47 +++++++++++++++++++++++++++++++
> > 2 files changed, 104 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-dev.c
> > b/drivers/media/v4l2-core/v4l2-dev.c index c647ba648805..c73c6d49e7cf
> > 100644
> > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > @@ -156,6 +156,52 @@ void video_device_release_empty(struct video_device
> > *vdev)>
> > }
> > EXPORT_SYMBOL(video_device_release_empty);
> >
> > +int video_device_enter(struct video_device *vdev)
> > +{
> > + bool unplugged;
> > +
> > + spin_lock(&vdev->unplug_lock);
> > + unplugged = vdev->unplugged;
> > + if (!unplugged)
> > + vdev->access_refcount++;
> > + spin_unlock(&vdev->unplug_lock);
> > +
> > + return unplugged ? -ENODEV : 0;
> > +}
> > +EXPORT_SYMBOL_GPL(video_device_enter);
> > +
> > +void video_device_exit(struct video_device *vdev)
> > +{
> > + bool wake_up;
> > +
> > + spin_lock(&vdev->unplug_lock);
> > + WARN_ON(--vdev->access_refcount < 0);
> > + wake_up = vdev->access_refcount == 0;
> > + spin_unlock(&vdev->unplug_lock);
> > +
> > + if (wake_up)
> > + wake_up(&vdev->unplug_wait);
> > +}
> > +EXPORT_SYMBOL_GPL(video_device_exit);
>
> Is there a need to export the two, i.e. wouldn't you only call them from
> the framework, or the same module?
There could be a need to call these functions from entry points that are not
controlled by the V4L2 core, such as sysfs or debugfs. We could keep the
functions internal for now and only export them when the need arises, but if
we want to document how drivers need to handle race conditions between device
access and device unbind, we need to have them exported.
> > +
> > +void video_device_unplug(struct video_device *vdev)
> > +{
> > + bool unplug_blocked;
> > +
> > + spin_lock(&vdev->unplug_lock);
> > + unplug_blocked = vdev->access_refcount > 0;
> > + vdev->unplugged = true;
>
> Shouldn't this be set to false in video_register_device()?
Yes it should. I currently rely on the fact that the memory is zeroed when
allocated, but I shouldn't. I'll fix that.
> > + spin_unlock(&vdev->unplug_lock);
> > +
> > + if (!unplug_blocked)
> > + return;
>
> Not necessary, wait_event_timeout() handles this already.
I'll fix this as well.
> > +
> > + if (!wait_event_timeout(vdev->unplug_wait, !vdev->access_refcount,
> > + msecs_to_jiffies(150000)))
> > + WARN(1, "Timeout waiting for device access to complete\n");
>
> Why a timeout? Does this get somehow less problematic over time? :-)
Not quite :-) This should never happen, but driver and/or core bugs could
cause a timeout. In that case I think proceeding after a timeout is a better
option than deadlocking forever.
> > +}
> > +EXPORT_SYMBOL_GPL(video_device_unplug);
> > +
> >
> > static inline void video_get(struct video_device *vdev)
> > {
> >
> > get_device(&vdev->dev);
> >
> > @@ -351,6 +397,10 @@ static long v4l2_ioctl(struct file *filp, unsigned
> > int cmd, unsigned long arg)>
> > struct video_device *vdev = video_devdata(filp);
> > int ret = -ENODEV;
>
> You could leave ret unassigned here.
I'll fix that.
> > + ret = video_device_enter(vdev);
> > + if (ret < 0)
> > + return ret;
> > +
> > if (vdev->fops->unlocked_ioctl) {
> > struct mutex *lock = v4l2_ioctl_get_lock(vdev, cmd);
> >
> > @@ -358,11 +408,14 @@ static long v4l2_ioctl(struct file *filp, unsigned
> > int cmd, unsigned long arg)
> > return -ERESTARTSYS;
> > if (video_is_registered(vdev))
> > ret = vdev->fops->unlocked_ioctl(filp, cmd, arg);
> > + else
> > + ret = -ENODEV;
> > if (lock)
> > mutex_unlock(lock);
> > } else
> > ret = -ENOTTY;
> >
> > + video_device_exit(vdev);
> > return ret;
> > }
> >
> > @@ -841,6 +894,10 @@ int __video_register_device(struct video_device
> > *vdev, int type, int nr,>
> > if (WARN_ON(!vdev->v4l2_dev))
> > return -EINVAL;
> >
> > + /* unplug support */
> > + spin_lock_init(&vdev->unplug_lock);
> > + init_waitqueue_head(&vdev->unplug_wait);
> > +
> > /* v4l2_fh support */
> > spin_lock_init(&vdev->fh_lock);
> > INIT_LIST_HEAD(&vdev->fh_list);
> > diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> > index e657614521e3..365a94f91dc9 100644
> > --- a/include/media/v4l2-dev.h
> > +++ b/include/media/v4l2-dev.h
> > @@ -15,6 +15,7 @@
> > #include <linux/cdev.h>
> > #include <linux/mutex.h>
> > #include <linux/videodev2.h>
> > +#include <linux/wait.h>
> >
> > #include <media/media-entity.h>
> >
> > @@ -178,6 +179,12 @@ struct v4l2_file_operations {
> > * @pipe: &struct media_pipeline
> > * @fops: pointer to &struct v4l2_file_operations for the video device
> > * @device_caps: device capabilities as used in v4l2_capabilities
> > + * @unplugged: when set the device has been unplugged and no device
> > access
> > + * section can be entered
> > + * @access_refcount: number of device access section currently running
> > for the
> > + * device
> > + * @unplug_lock: protects unplugged and access_refcount
> > + * @unplug_wait: wait queue to wait for device access sections to
> > complete
> > * @dev: &struct device for the video device
> > * @cdev: character device
> > * @v4l2_dev: pointer to &struct v4l2_device parent
> > @@ -221,6 +228,12 @@ struct video_device
> >
> > u32 device_caps;
> >
> > + /* unplug handling */
> > + bool unplugged;
> > + int access_refcount;
>
> Could you use refcount_t instead, to avoid integer overflow issues?
I'd love to, but refcount_t has no provision for refcounts that start at 0.
void refcount_inc(refcount_t *r)
{
WARN_ONCE(!refcount_inc_not_zero(r), "refcount_t: increment on 0; use-
after-free.\n");
}
EXPORT_SYMBOL(refcount_inc);
> > + spinlock_t unplug_lock;
> > + wait_queue_head_t unplug_wait;
> > +
> > /* sysfs */
> > struct device dev;
> > struct cdev *cdev;
> > @@ -506,4 +519,38 @@ static inline int video_is_registered(struct
> > video_device *vdev)
> > return test_bit(V4L2_FL_REGISTERED, &vdev->flags);
> > }
> >
> > +/**
> > + * video_device_enter - enter a device access section
> > + * @vdev: the video device
> > + *
> > + * This function marks and protects the beginning of a section that
> > should not
> > + * be entered after the device has been unplugged. The section end is
> > marked
> > + * with a call to video_device_exit(). Calls to this function can be
> > nested.
> > + *
> > + * Returns:
> > + * 0 on success or a negative error code if the device has been
> > unplugged.
> > + */
> > +int video_device_enter(struct video_device *vdev);
> > +
> > +/**
> > + * video_device_exit - exit a device access section
> > + * @vdev: the video device
> > + *
> > + * This function marks the end of a section entered with
> > video_device_enter().
> > + * It wakes up all tasks waiting on video_device_unplug() for device
> > access
> > + * sections to be exited.
> > + */
> > +void video_device_exit(struct video_device *vdev);
> > +
> > +/**
> > + * video_device_unplug - mark a device as unplugged
> > + * @vdev: the video device
> > + *
> > + * Mark a device as unplugged, causing all subsequent calls to
> > + * video_device_enter() to return an error. If a device access section is
> > + * currently being executed the function waits until the section is
> > exited as
> > + * marked by a call to video_device_exit().
> > + */
> > +void video_device_unplug(struct video_device *vdev);
> > +
> >
> > #endif /* _V4L2_DEV_H */
--
Regards,
Laurent Pinchart
