subject:"\[PATCH\/RFC 1\/2\] v4l\: v4l2\-dev\: Add infrastructure to protect device unplug race"

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

2017-12-14 Thread Sakari Ailus

Hi Laurent,

On Tue, Dec 12, 2017 at 04:42:23PM +0200, Laurent Pinchart wrote:
...
> > > 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(>unplug_lock);
> > > + unplugged = vdev->unplugged;
> > > + if (!unplugged)
> > > + vdev->access_refcount++;
> > > + spin_unlock(>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(>unplug_lock);
> > > + WARN_ON(--vdev->access_refcount < 0);
> > > + wake_up = vdev->access_refcount == 0;
> > > + spin_unlock(>unplug_lock);
> > > +
> > > + if (wake_up)
> > > + wake_up(>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.

Ack.

> 
> > > +
> > > +void video_device_unplug(struct video_device *vdev)
> > > +{
> > > + bool unplug_blocked;
> > > +
> > > + spin_lock(>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(>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(15)))
> > > + 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.

This also depends on the frame rate; you could have a very low frame rate
configured on a sensor and the device could be actually in middle of a DMA
operation while the timeout is hit.

I'm not sure if there's a number that can be said to be safe here.

Wouldn't it be better to kill the user space process using the device
instead? Naturally the wait will have to be interruptible.

...

> > > @@ -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);

Ah. I wonder if you could simply initialise in probe and decrement it again
in remove?

You could use refcount_inc_not_zero directly, too.

> 
> > > + spinlock_t unplug_lock;
> > > + wait_queue_head_t unplug_wait;
> > > +
> > >   /* sysfs */
> > >   struct device dev;
> > >   struct cdev *cdev;

-- 
Kind regards,

Sakari Ailus
sakari.ai...@linux.intel.com

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

2017-12-12 Thread Laurent Pinchart

Hi Mauro,

On Tuesday, 12 December 2017 14:39:32 EET Mauro Carvalho Chehab wrote:
> Em Thu, 23 Nov 2017 15:21:01 +0100 Greg Kroah-Hartman escreveu:
> > On Thu, Nov 23, 2017 at 11:07:51AM -0200, Mauro Carvalho Chehab wrote:
> >> Em Thu, 16 Nov 2017 02:33:48 +0200 Laurent Pinchart escreveu:
> >>> 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
> >>> 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
> >>> 
> >>> ---
> >>> 
> >>>  drivers/media/v4l2-core/v4l2-dev.c | 57 ++
> >>>  include/media/v4l2-dev.h   | 47 ++
> >>>  2 files changed, 104 insertions(+)

[snip]

> >> I'm c/c Greg here, as I don't think, that, the way it is, it
> >> belongs at V4L2 core.
> >> 
> >> I mean: if this is a problem that affects all drivers, it would should,
> >> instead, be sitting at the driver's core.
> > 
> > What "problem" is trying to be solved here?  One where your specific
> > device type races with your specific user api?  Doesn't sound very
> > driver-core specific to me :)
> > 
> > As an example, what other bus/device type needs this?  If you can see
> > others that do, then sure, move it into the core.  But for just one, I
> > don't know if that's really needed here, do you?
> 
> The problem that this patch is trying to solve is related to
> hot-unplugging a platform device[1]. Quoting Laurent's comments about
> it on IRC:
> 
>   "it applies to all platform devices at least"

Note how I said "at least" :-) I2C, SPI and PCI devices are affected too, and 
after a closer look at USB today I believe USB devices are affected as well.

>   "I'm actually considering moving that code to the device core as
>it applies to all drivers that have device nodes, but I'm not
>sure that will be feasible it won't hurt other devices
>it applies to I2C and SPI as well at least and PCI too"
> 
> [1] https://linuxtv.org/irc/irclogger_log/media-maint?date=2017-11-23,Thu
> 
> For USB drivers, hot-unplug seems to work fine for media drivers,
> although keeping it working require tests from time to time, as
> it is not hard to break hotplug support. so, currently, I don't see
> the need of anything like that for non-platform drivers.

I2C, SPI and PCI are non-platform drivers, and USB seems to be affected too. 
The race window is small, making it difficult to reproduce the problem, but 
with carefully placed delays it gets much easier to hit the race.

> My point here is that adding a new lock inside the media core that
> would be used for all media drivers, including the ones that don't need
> doesn't sound a good idea.

Why not, if it doesn't affect performances (or anything else) negatively ?

> So, if this is something that applies to all platform drivers (including
> non-media ones), or if are there anything that can be done at driver's core
> that would improve hotplug support for all buses, making it more stable or
> easier to implement, then it would make sense to improve the driver's core.
> If not, this sounds a driver-specific issue whose fix doesn't belong to the
> media core.

It's clearly not a driver-specific issue as most, if not all, drivers are 
affected.

I've replied to Greg's e-mail in this thread with more details, let's try to 
keep the discussion there to avoid splitting it in multiple sub-threads.

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

2017-12-12 Thread Laurent Pinchart

Hi Greg and Mauro,

On Thursday, 23 November 2017 16:21:01 EET Greg Kroah-Hartman wrote:
> On Thu, Nov 23, 2017 at 11:07:51AM -0200, Mauro Carvalho Chehab wrote:
> > Em Thu, 16 Nov 2017 02:33:48 +0200 Laurent Pinchart escreveu:
> >> 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
> >> 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
> >> 
> >> ---
> >> 
> >>  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(>unplug_lock);
> >> +  unplugged = vdev->unplugged;
> >> +  if (!unplugged)
> >> +  vdev->access_refcount++;
> >> +  spin_unlock(>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(>unplug_lock);
> >> +  WARN_ON(--vdev->access_refcount < 0);
> >> +  wake_up = vdev->access_refcount == 0;
> >> +  spin_unlock(>unplug_lock);
> >> +
> >> +  if (wake_up)
> >> +  wake_up(>unplug_wait);
> >> +}
> >> +EXPORT_SYMBOL_GPL(video_device_exit);
> >> +
> >> +void video_device_unplug(struct video_device *vdev)
> >> +{
> >> +  bool unplug_blocked;
> >> +
> >> +  spin_lock(>unplug_lock);
> >> +  unplug_blocked = vdev->access_refcount > 0;
> >> +  vdev->unplugged = true;
> >> +  spin_unlock(>unplug_lock);
> >> +
> >> +  if (!unplug_blocked)
> >> +  return;
> >> +
> >> +  if (!wait_event_timeout(vdev->unplug_wait, !vdev->access_refcount,
> >> +  msecs_to_jiffies(15)))
> >> +  WARN(1, "Timeout waiting for device access to complete\n");
> >> +}
> >> +EXPORT_SYMBOL_GPL(video_device_unplug);
> >> +
> >>  static inline void video_get(struct video_device *vdev)
> >>  {
> >>get_device(>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;
> >> 
> >> +  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(>unplug_lock);
> >> +  init_waitqueue_head(>unplug_wait);
> >> +
> > 
> > I'm c/c Greg here, as I don't think, that, the way it is, it
> > belongs at V4L2 core.
> > 
> > I mean: if this is a problem that affects all drivers, it would should,
> > instead, be sitting at the driver's core.
> 
> What "problem" is trying to be solved here?  One where your specific
> device type races with your specific user

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

2017-12-12 Thread Laurent Pinchart

Hi Mauro,

On Thursday, 23 November 2017 15:07:51 EET Mauro Carvalho Chehab wrote:
> Em Thu, 16 Nov 2017 02:33:48 +0200 Laurent Pinchart escreveu:
> > 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
> > 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
> > 
> > ---
> > 
> >  drivers/media/v4l2-core/v4l2-dev.c | 57 +
> >  include/media/v4l2-dev.h   | 47 +++
> >  2 files changed, 104 insertions(+)

[snip]

> I'm c/c Greg here, as I don't think, that, the way it is, it
> belongs at V4L2 core.
> 
> I mean: if this is a problem that affects all drivers, it would should,
> instead, be sitting at the driver's core.
> 
> If, otherwise, this is specific to rcar-vin (and other platform drivers),
> that's likely should be inside the drivers that require it.
> 
> That's said, I remember we had to add some things in the past for
> USB drivers hot unplug to happen softly. I don't remember the specifics
> anymore, but it was solved by both a V4L2 core and changes at USB
> drivers.
> 
> One of the things that it was added, on that time, was this patch:
> 
>   commit ae6cfaace120f4330715b56265ce0e4a710e1276
>   Author: Hans Verkuil 
>   Date:   Sat Mar 14 08:28:45 2009 -0300
> 
>   V4L/DVB (11044): v4l2-device: add v4l2_device_disconnect
> 
> So, I would expect that a change at V4L2 core (or at driver core) that
> would be applied would also be affecting USB drivers disconnect logic
> and v4l2_device_disconnect() function.

I wasn't aware of v4l2_device_disconnect(), thank you for pointing it out.

I don't know what the full history behind that function is, but I don't see 
why it's needed. struct device instances are refcounted, the struct device 
corresponding to a USB device or USB interface doesn't get freed when a device 
is disconnected as long as a reference is present. We take such a reference in 
v4l2_device_register(), so there should be no problem.

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

2017-12-12 Thread Laurent Pinchart

Hi Hans,

On Friday, 17 November 2017 13:09:20 EET Hans Verkuil wrote:
> On 16/11/17 01:33, 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
> > 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.
> 
> As long as the queue field in struct video_device is filled in properly
> this shouldn't be a problem.
> 
> This looks pretty good, simple and straightforward.

Thank you.

> Do we need something similar for media_device? Other devices?

I believe so, which is why I'm wondering whether this shouldn't somehow go to 
the device core (and in the cdev core). Not all devices will need such an 
infrastructure as some subsystems already protect against device access after 
unbind (USB is one of them if I'm not mistaken), but it certainly shouldn't 
hurt.

DRM is also considering a similar implementation, but based on srcu to lower 
the performance penalty. I feel that's going a bit overboard but I have no 
numbers yet to confirm or infirm the suspicion.

> > Signed-off-by: Laurent Pinchart
> > 
> > ---
> > 
> >  drivers/media/v4l2-core/v4l2-dev.c | 57 +
> >  include/media/v4l2-dev.h   | 47 +++
> >  2 files changed, 104 insertions(+)

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

2017-12-12 Thread Laurent Pinchart

Hi Kieran,

On Thursday, 16 November 2017 16:47:11 EET Kieran Bingham wrote:
> On 16/11/17 12:32, Sakari Ailus wrote:
> > Hi Laurent,
> > 
> > Thank you for the initiative to bring up and address the matter!
> 
> I concur - this looks like a good start towards managing the issue.
> 
> One potential thing spotted on top of Sakari's review inline below, of
> course I suspect this was more of a prototype/consideration patch.
> 
> > 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.
> > 
> > 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
> >> 
> >> ---
> >> 
> >>  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

[snip]

> >> @@ -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.
> > 
> >> +  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;
> 
> It looks like that return -ERESTARTSYS might need a video_device_exit() too?

Oops. Of course. I'll fix that. Thanks for catching the issue.

> >>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;
> >>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

2017-12-12 Thread Laurent Pinchart

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
> > 
> > ---
> > 
> >  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(>unplug_lock);
> > +   unplugged = vdev->unplugged;
> > +   if (!unplugged)
> > +   vdev->access_refcount++;
> > +   spin_unlock(>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(>unplug_lock);
> > +   WARN_ON(--vdev->access_refcount < 0);
> > +   wake_up = vdev->access_refcount == 0;
> > +   spin_unlock(>unplug_lock);
> > +
> > +   if (wake_up)
> > +   wake_up(>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(>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(>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(15)))
> > +   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(>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

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

2017-12-12 Thread Mauro Carvalho Chehab

Em Thu, 23 Nov 2017 15:21:01 +0100
Greg Kroah-Hartman  escreveu:

> On Thu, Nov 23, 2017 at 11:07:51AM -0200, Mauro Carvalho Chehab wrote:
> > Hi Laurent,
> > 
> > Em Thu, 16 Nov 2017 02:33:48 +0200
> > Laurent Pinchart  escreveu:
> >   
> > > 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
> > > 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 
> > > 
> > > ---
> > >  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(>unplug_lock);
> > > + unplugged = vdev->unplugged;
> > > + if (!unplugged)
> > > + vdev->access_refcount++;
> > > + spin_unlock(>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(>unplug_lock);
> > > + WARN_ON(--vdev->access_refcount < 0);
> > > + wake_up = vdev->access_refcount == 0;
> > > + spin_unlock(>unplug_lock);
> > > +
> > > + if (wake_up)
> > > + wake_up(>unplug_wait);
> > > +}
> > > +EXPORT_SYMBOL_GPL(video_device_exit);
> > > +
> > > +void video_device_unplug(struct video_device *vdev)
> > > +{
> > > + bool unplug_blocked;
> > > +
> > > + spin_lock(>unplug_lock);
> > > + unplug_blocked = vdev->access_refcount > 0;
> > > + vdev->unplugged = true;
> > > + spin_unlock(>unplug_lock);
> > > +
> > > + if (!unplug_blocked)
> > > + return;
> > > +
> > > + if (!wait_event_timeout(vdev->unplug_wait, !vdev->access_refcount,
> > > + msecs_to_jiffies(15)))
> > > + WARN(1, "Timeout waiting for device access to complete\n");
> > > +}
> > > +EXPORT_SYMBOL_GPL(video_device_unplug);
> > > +
> > >  static inline void video_get(struct video_device *vdev)
> > >  {
> > >   get_device(>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;
> > >  
> > > + 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(>unplug_lock);
> > > + init_waitqueue_head(>unplug_wait);
> > > +  
> > 
> > I'm c/c Greg here, as I don't think, that, the way it is, it
> > belongs at V4L2 core.
> > 
> > I mean: if this is a problem that affects all drivers, it

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

2017-11-23 Thread Greg Kroah-Hartman

On Thu, Nov 23, 2017 at 11:07:51AM -0200, Mauro Carvalho Chehab wrote:
> Hi Laurent,
> 
> Em Thu, 16 Nov 2017 02:33:48 +0200
> Laurent Pinchart  escreveu:
> 
> > 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
> > 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 
> > ---
> >  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(>unplug_lock);
> > +   unplugged = vdev->unplugged;
> > +   if (!unplugged)
> > +   vdev->access_refcount++;
> > +   spin_unlock(>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(>unplug_lock);
> > +   WARN_ON(--vdev->access_refcount < 0);
> > +   wake_up = vdev->access_refcount == 0;
> > +   spin_unlock(>unplug_lock);
> > +
> > +   if (wake_up)
> > +   wake_up(>unplug_wait);
> > +}
> > +EXPORT_SYMBOL_GPL(video_device_exit);
> > +
> > +void video_device_unplug(struct video_device *vdev)
> > +{
> > +   bool unplug_blocked;
> > +
> > +   spin_lock(>unplug_lock);
> > +   unplug_blocked = vdev->access_refcount > 0;
> > +   vdev->unplugged = true;
> > +   spin_unlock(>unplug_lock);
> > +
> > +   if (!unplug_blocked)
> > +   return;
> > +
> > +   if (!wait_event_timeout(vdev->unplug_wait, !vdev->access_refcount,
> > +   msecs_to_jiffies(15)))
> > +   WARN(1, "Timeout waiting for device access to complete\n");
> > +}
> > +EXPORT_SYMBOL_GPL(video_device_unplug);
> > +
> >  static inline void video_get(struct video_device *vdev)
> >  {
> > get_device(>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;
> >  
> > +   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(>unplug_lock);
> > +   init_waitqueue_head(>unplug_wait);
> > +
> 
> I'm c/c Greg here, as I don't think, that, the way it is, it
> belongs at V4L2 core.
> 
> I mean: if this is a problem that affects all drivers, it would should, 
> instead, be sitting at the driver's core.

What "problem" is trying to be solved here?  One where your specific
device type races with your specific user api?  Doesn't sound very
driver-core specific to me :)

As an example, what other bus/device type needs

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

2017-11-23 Thread Mauro Carvalho Chehab

Hi Laurent,

Em Thu, 16 Nov 2017 02:33:48 +0200
Laurent Pinchart  escreveu:

> 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
> 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 
> ---
>  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(>unplug_lock);
> + unplugged = vdev->unplugged;
> + if (!unplugged)
> + vdev->access_refcount++;
> + spin_unlock(>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(>unplug_lock);
> + WARN_ON(--vdev->access_refcount < 0);
> + wake_up = vdev->access_refcount == 0;
> + spin_unlock(>unplug_lock);
> +
> + if (wake_up)
> + wake_up(>unplug_wait);
> +}
> +EXPORT_SYMBOL_GPL(video_device_exit);
> +
> +void video_device_unplug(struct video_device *vdev)
> +{
> + bool unplug_blocked;
> +
> + spin_lock(>unplug_lock);
> + unplug_blocked = vdev->access_refcount > 0;
> + vdev->unplugged = true;
> + spin_unlock(>unplug_lock);
> +
> + if (!unplug_blocked)
> + return;
> +
> + if (!wait_event_timeout(vdev->unplug_wait, !vdev->access_refcount,
> + msecs_to_jiffies(15)))
> + WARN(1, "Timeout waiting for device access to complete\n");
> +}
> +EXPORT_SYMBOL_GPL(video_device_unplug);
> +
>  static inline void video_get(struct video_device *vdev)
>  {
>   get_device(>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;
>  
> + 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(>unplug_lock);
> + init_waitqueue_head(>unplug_wait);
> +

I'm c/c Greg here, as I don't think, that, the way it is, it
belongs at V4L2 core.

I mean: if this is a problem that affects all drivers, it would should, 
instead, be sitting at the driver's core.

If, otherwise, this is specific to rcar-vin (and other platform drivers),
that's likely should be inside the drivers that require it.

That's said, I remember we had to add some things in the past for
USB drivers hot unplug to happen softly. I don't remember the specifics
anymore, but it was solved by both a V4L2 core and changes at USB
drivers.

One of the things that it was added, on that time, was this patch:

commit

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

2017-11-17 Thread Hans Verkuil

Hi Laurent,

On 16/11/17 01:33, 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
> 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.

As long as the queue field in struct video_device is filled in properly
this shouldn't be a problem.

This looks pretty good, simple and straightforward.

Do we need something similar for media_device? Other devices?

Regards,

Hans

> 
> Signed-off-by: Laurent Pinchart 
> ---
>  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(>unplug_lock);
> + unplugged = vdev->unplugged;
> + if (!unplugged)
> + vdev->access_refcount++;
> + spin_unlock(>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(>unplug_lock);
> + WARN_ON(--vdev->access_refcount < 0);
> + wake_up = vdev->access_refcount == 0;
> + spin_unlock(>unplug_lock);
> +
> + if (wake_up)
> + wake_up(>unplug_wait);
> +}
> +EXPORT_SYMBOL_GPL(video_device_exit);
> +
> +void video_device_unplug(struct video_device *vdev)
> +{
> + bool unplug_blocked;
> +
> + spin_lock(>unplug_lock);
> + unplug_blocked = vdev->access_refcount > 0;
> + vdev->unplugged = true;
> + spin_unlock(>unplug_lock);
> +
> + if (!unplug_blocked)
> + return;
> +
> + if (!wait_event_timeout(vdev->unplug_wait, !vdev->access_refcount,
> + msecs_to_jiffies(15)))
> + WARN(1, "Timeout waiting for device access to complete\n");
> +}
> +EXPORT_SYMBOL_GPL(video_device_unplug);
> +
>  static inline void video_get(struct video_device *vdev)
>  {
>   get_device(>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;
>  
> + 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(>unplug_lock);
> + init_waitqueue_head(>unplug_wait);
> +
>   /* v4l2_fh support */
>   spin_lock_init(>fh_lock);
>   INIT_LIST_HEAD(>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 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  
> @@ -178,6 +179,12 @@ struct v4l2_file_operations {
>   * @pipe:  media_pipeline
>   * @fops: pointer to

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

2017-11-16 Thread Kieran Bingham

Hi Laurent,

On 16/11/17 12:32, Sakari Ailus wrote:
> Hi Laurent,
> 
> Thank you for the initiative to bring up and address the matter!

I concur - this looks like a good start towards managing the issue.

One potential thing spotted on top of Sakari's review inline below, of course I
suspect this was more of a prototype/consideration patch.

> 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.
> 
> 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 
>> ---
>>  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(>unplug_lock);
>> +unplugged = vdev->unplugged;
>> +if (!unplugged)
>> +vdev->access_refcount++;
>> +spin_unlock(>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(>unplug_lock);
>> +WARN_ON(--vdev->access_refcount < 0);
>> +wake_up = vdev->access_refcount == 0;
>> +spin_unlock(>unplug_lock);
>> +
>> +if (wake_up)
>> +wake_up(>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?
> 
>> +
>> +void video_device_unplug(struct video_device *vdev)
>> +{
>> +bool unplug_blocked;
>> +
>> +spin_lock(>unplug_lock);
>> +unplug_blocked = vdev->access_refcount > 0;
>> +vdev->unplugged = true;
> 
> Shouldn't this be set to false in video_register_device()?
> 
>> +spin_unlock(>unplug_lock);
>> +
>> +if (!unplug_blocked)
>> +return;
> 
> Not necessary, wait_event_timeout() handles this already.
> 
>> +
>> +if (!wait_event_timeout(vdev->unplug_wait, !vdev->access_refcount,
>> +msecs_to_jiffies(15)))
>> +WARN(1, "Timeout waiting for device access to complete\n");
> 
> Why a timeout? Does this get somehow less problematic over time? :-)
> 
>> +}
>> +EXPORT_SYMBOL_GPL(video_device_unplug);
>> +
>>  static inline void video_get(struct video_device *vdev)
>>  {
>>  get_device(>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.
> 
>>  
>> +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;


It looks like that return -ERESTARTSYS might need a video_device_exit() too?


>>  if (video_is_registered(vdev))
>>  ret = vdev->fops->unlocked_ioctl(filp, cmd, arg);
>> +else
>> +ret = -ENODEV;
>>  if (lock)
>>  mutex_unlock(lock);
>>  } else
>>  ret =

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

2017-11-16 Thread Sakari Ailus

Hi Laurent,

Thank you for the initiative to bring up and address the matter!

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.

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 
> ---
>  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(>unplug_lock);
> + unplugged = vdev->unplugged;
> + if (!unplugged)
> + vdev->access_refcount++;
> + spin_unlock(>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(>unplug_lock);
> + WARN_ON(--vdev->access_refcount < 0);
> + wake_up = vdev->access_refcount == 0;
> + spin_unlock(>unplug_lock);
> +
> + if (wake_up)
> + wake_up(>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?

> +
> +void video_device_unplug(struct video_device *vdev)
> +{
> + bool unplug_blocked;
> +
> + spin_lock(>unplug_lock);
> + unplug_blocked = vdev->access_refcount > 0;
> + vdev->unplugged = true;

Shouldn't this be set to false in video_register_device()?

> + spin_unlock(>unplug_lock);
> +
> + if (!unplug_blocked)
> + return;

Not necessary, wait_event_timeout() handles this already.

> +
> + if (!wait_event_timeout(vdev->unplug_wait, !vdev->access_refcount,
> + msecs_to_jiffies(15)))
> + WARN(1, "Timeout waiting for device access to complete\n");

Why a timeout? Does this get somehow less problematic over time? :-)

> +}
> +EXPORT_SYMBOL_GPL(video_device_unplug);
> +
>  static inline void video_get(struct video_device *vdev)
>  {
>   get_device(>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.

>  
> + 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(>unplug_lock);
> + init_waitqueue_head(>unplug_wait);
> +
>   /* v4l2_fh support */
>   spin_lock_init(>fh_lock);
>

[PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

2017-11-15 Thread Laurent Pinchart

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
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 
---
 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(>unplug_lock);
+   unplugged = vdev->unplugged;
+   if (!unplugged)
+   vdev->access_refcount++;
+   spin_unlock(>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(>unplug_lock);
+   WARN_ON(--vdev->access_refcount < 0);
+   wake_up = vdev->access_refcount == 0;
+   spin_unlock(>unplug_lock);
+
+   if (wake_up)
+   wake_up(>unplug_wait);
+}
+EXPORT_SYMBOL_GPL(video_device_exit);
+
+void video_device_unplug(struct video_device *vdev)
+{
+   bool unplug_blocked;
+
+   spin_lock(>unplug_lock);
+   unplug_blocked = vdev->access_refcount > 0;
+   vdev->unplugged = true;
+   spin_unlock(>unplug_lock);
+
+   if (!unplug_blocked)
+   return;
+
+   if (!wait_event_timeout(vdev->unplug_wait, !vdev->access_refcount,
+   msecs_to_jiffies(15)))
+   WARN(1, "Timeout waiting for device access to complete\n");
+}
+EXPORT_SYMBOL_GPL(video_device_unplug);
+
 static inline void video_get(struct video_device *vdev)
 {
get_device(>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;
 
+   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(>unplug_lock);
+   init_waitqueue_head(>unplug_wait);
+
/* v4l2_fh support */
spin_lock_init(>fh_lock);
INIT_LIST_HEAD(>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 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -178,6 +179,12 @@ struct v4l2_file_operations {
  * @pipe:  media_pipeline
  * @fops: pointer to  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:  device for the video device
  *

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

[PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

14 matches

Site Navigation

Mail list logo

Footer information