[PATCH 1/1] v4l: event: Prevent freeing event subscriptions while accessed

[PATCH 1/1] v4l: event: Prevent freeing event subscriptions while accessed

[Sakari Ailus]

The event subscriptions are added to the subscribed event list while
holding a spinlock, but that lock is subsequently released while still
accessing the subscription object. This makes it possible to unsubscribe
the event --- and freeing the subscription object's memory --- while
the subscription object is simultaneously accessed.

Prevent this by adding a mutex to serialise the event subscription and
unsubscription. This also gives a guarantee to the callback ops that the
add op has returned before the del op is called.

This change also results in making the elems field less special:
subscriptions are only added to the event list once they are fully
initialised.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
Hi folks,

I noticed this while working to add support for media events. This seems
like material for the stable trees.

 drivers/media/v4l2-core/v4l2-event.c | 35 ++++++++++++++++++-----------------
 drivers/media/v4l2-core/v4l2-fh.c    |  2 ++
 include/media/v4l2-fh.h              |  4 ++++
 3 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c
index 127fe6eb91d9..74161f79e4d3 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -115,14 +115,6 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *e
 	if (sev == NULL)
 		return;
 
-	/*
-	 * If the event has been added to the fh->subscribed list, but its
-	 * add op has not completed yet elems will be 0, treat this as
-	 * not being subscribed.
-	 */
-	if (!sev->elems)
-		return;
-
 	/* Increase event sequence number on fh. */
 	fh->sequence++;
 
@@ -208,6 +200,7 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 	struct v4l2_subscribed_event *sev, *found_ev;
 	unsigned long flags;
 	unsigned i;
+	int ret = 0;
 
 	if (sub->type == V4L2_EVENT_ALL)
 		return -EINVAL;
@@ -225,31 +218,35 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 	sev->flags = sub->flags;
 	sev->fh = fh;
 	sev->ops = ops;
+	sev->elems = elems;
+
+	mutex_lock(&fh->mutex);
 
 	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
 	found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
-	if (!found_ev)
-		list_add(&sev->list, &fh->subscribed);
 	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
 
 	if (found_ev) {
+		/* Already listening */
 		kvfree(sev);
-		return 0; /* Already listening */
+		goto out_unlock;
 	}
 
 	if (sev->ops && sev->ops->add) {
-		int ret = sev->ops->add(sev, elems);
+		ret = sev->ops->add(sev, elems);
 		if (ret) {
-			sev->ops = NULL;
-			v4l2_event_unsubscribe(fh, sub);
-			return ret;
+			kvfree(sev);
+			goto out_unlock;
 		}
 	}
 
 	/* Mark as ready for use */
-	sev->elems = elems;
+	list_add(&sev->list, &fh->subscribed);
 
-	return 0;
+out_unlock:
+	mutex_unlock(&fh->mutex);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
 
@@ -288,6 +285,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 		return 0;
 	}
 
+	mutex_lock(&fh->mutex);
+
 	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
 
 	sev = v4l2_event_subscribed(fh, sub->type, sub->id);
@@ -305,6 +304,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 	if (sev && sev->ops && sev->ops->del)
 		sev->ops->del(sev);
 
+	mutex_unlock(&fh->mutex);
+
 	kvfree(sev);
 
 	return 0;
diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c
index 3895999bf880..b017dafde907 100644
--- a/drivers/media/v4l2-core/v4l2-fh.c
+++ b/drivers/media/v4l2-core/v4l2-fh.c
@@ -45,6 +45,7 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
 	INIT_LIST_HEAD(&fh->available);
 	INIT_LIST_HEAD(&fh->subscribed);
 	fh->sequence = -1;
+	mutex_init(&fh->mutex);
 }
 EXPORT_SYMBOL_GPL(v4l2_fh_init);
 
@@ -90,6 +91,7 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
 		return;
 	v4l_disable_media_source(fh->vdev);
 	v4l2_event_unsubscribe_all(fh);
+	mutex_destroy(&fh->mutex);
 	fh->vdev = NULL;
 }
 EXPORT_SYMBOL_GPL(v4l2_fh_exit);
diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
index ea73fef8bdc0..1be45a5f6383 100644
--- a/include/media/v4l2-fh.h
+++ b/include/media/v4l2-fh.h
@@ -42,6 +42,9 @@ struct v4l2_ctrl_handler;
  * @available: list of events waiting to be dequeued
  * @navailable: number of available events at @available list
  * @sequence: event sequence number
+ * @mutex: hold event subscriptions during subscribing;
+ *	   guarantee that the add and del event callbacks are orderly called
+ *
  * @m2m_ctx: pointer to &struct v4l2_m2m_ctx
  */
 struct v4l2_fh {
@@ -56,6 +59,7 @@ struct v4l2_fh {
 	struct list_head	available;
 	unsigned int		navailable;
 	u32			sequence;
+	struct mutex		mutex;
 
 #if IS_ENABLED(CONFIG_V4L2_MEM2MEM_DEV)
 	struct v4l2_m2m_ctx	*m2m_ctx;
-- 
2.11.0

Re: [PATCH 1/1] v4l: event: Prevent freeing event subscriptions while accessed

[Hans Verkuil]

On 09/12/18 10:52, Sakari Ailus wrote:
> The event subscriptions are added to the subscribed event list while
> holding a spinlock, but that lock is subsequently released while still
> accessing the subscription object. This makes it possible to unsubscribe
> the event --- and freeing the subscription object's memory --- while
> the subscription object is simultaneously accessed.

Hmm, the (un)subscribe ioctls are serialized through the ioctl lock,
so this could only be a scenario with drivers that do not use this
lock. Off-hand the only driver I know that does this is uvc. Unfortunately,
that's a rather popular one.

> 
> Prevent this by adding a mutex to serialise the event subscription and
> unsubscription. This also gives a guarantee to the callback ops that the
> add op has returned before the del op is called.
> 
> This change also results in making the elems field less special:
> subscriptions are only added to the event list once they are fully
> initialised.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Hi folks,
> 
> I noticed this while working to add support for media events. This seems
> like material for the stable trees.

I'd say 'no need for this' if it wasn't for uvc.

> 
>  drivers/media/v4l2-core/v4l2-event.c | 35 ++++++++++++++++++-----------------
>  drivers/media/v4l2-core/v4l2-fh.c    |  2 ++
>  include/media/v4l2-fh.h              |  4 ++++
>  3 files changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c
> index 127fe6eb91d9..74161f79e4d3 100644
> --- a/drivers/media/v4l2-core/v4l2-event.c
> +++ b/drivers/media/v4l2-core/v4l2-event.c
> @@ -115,14 +115,6 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *e
>  	if (sev == NULL)
>  		return;
>  
> -	/*
> -	 * If the event has been added to the fh->subscribed list, but its
> -	 * add op has not completed yet elems will be 0, treat this as
> -	 * not being subscribed.
> -	 */
> -	if (!sev->elems)
> -		return;
> -
>  	/* Increase event sequence number on fh. */
>  	fh->sequence++;
>  
> @@ -208,6 +200,7 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>  	struct v4l2_subscribed_event *sev, *found_ev;
>  	unsigned long flags;
>  	unsigned i;
> +	int ret = 0;
>  
>  	if (sub->type == V4L2_EVENT_ALL)
>  		return -EINVAL;
> @@ -225,31 +218,35 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>  	sev->flags = sub->flags;
>  	sev->fh = fh;
>  	sev->ops = ops;
> +	sev->elems = elems;
> +
> +	mutex_lock(&fh->mutex);
>  
>  	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>  	found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
> -	if (!found_ev)
> -		list_add(&sev->list, &fh->subscribed);
>  	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>  
>  	if (found_ev) {
> +		/* Already listening */
>  		kvfree(sev);
> -		return 0; /* Already listening */
> +		goto out_unlock;
>  	}
>  
>  	if (sev->ops && sev->ops->add) {
> -		int ret = sev->ops->add(sev, elems);
> +		ret = sev->ops->add(sev, elems);
>  		if (ret) {
> -			sev->ops = NULL;
> -			v4l2_event_unsubscribe(fh, sub);
> -			return ret;
> +			kvfree(sev);
> +			goto out_unlock;
>  		}
>  	}
>  
>  	/* Mark as ready for use */
> -	sev->elems = elems;
> +	list_add(&sev->list, &fh->subscribed);
>  
> -	return 0;
> +out_unlock:
> +	mutex_unlock(&fh->mutex);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
>  
> @@ -288,6 +285,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>  		return 0;
>  	}
>  
> +	mutex_lock(&fh->mutex);
> +
>  	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>  
>  	sev = v4l2_event_subscribed(fh, sub->type, sub->id);
> @@ -305,6 +304,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>  	if (sev && sev->ops && sev->ops->del)
>  		sev->ops->del(sev);
>  
> +	mutex_unlock(&fh->mutex);
> +
>  	kvfree(sev);
>  
>  	return 0;
> diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c
> index 3895999bf880..b017dafde907 100644
> --- a/drivers/media/v4l2-core/v4l2-fh.c
> +++ b/drivers/media/v4l2-core/v4l2-fh.c
> @@ -45,6 +45,7 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
>  	INIT_LIST_HEAD(&fh->available);
>  	INIT_LIST_HEAD(&fh->subscribed);
>  	fh->sequence = -1;
> +	mutex_init(&fh->mutex);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fh_init);
>  
> @@ -90,6 +91,7 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
>  		return;
>  	v4l_disable_media_source(fh->vdev);
>  	v4l2_event_unsubscribe_all(fh);
> +	mutex_destroy(&fh->mutex);
>  	fh->vdev = NULL;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fh_exit);
> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> index ea73fef8bdc0..1be45a5f6383 100644
> --- a/include/media/v4l2-fh.h
> +++ b/include/media/v4l2-fh.h
> @@ -42,6 +42,9 @@ struct v4l2_ctrl_handler;
>   * @available: list of events waiting to be dequeued
>   * @navailable: number of available events at @available list
>   * @sequence: event sequence number
> + * @mutex: hold event subscriptions during subscribing;
> + *	   guarantee that the add and del event callbacks are orderly called
> + *
>   * @m2m_ctx: pointer to &struct v4l2_m2m_ctx
>   */
>  struct v4l2_fh {
> @@ -56,6 +59,7 @@ struct v4l2_fh {
>  	struct list_head	available;
>  	unsigned int		navailable;
>  	u32			sequence;
> +	struct mutex		mutex;

I don't like the name 'mutex'. Perhaps something more descriptive like:
'subscribe_lock'?

>  
>  #if IS_ENABLED(CONFIG_V4L2_MEM2MEM_DEV)
>  	struct v4l2_m2m_ctx	*m2m_ctx;
> 

Overall I think this patch makes sense. The code is cleaner and easier to follow.
Just give 'mutex' a better name :-)

Regards,

	Hans

Re: [PATCH 1/1] v4l: event: Prevent freeing event subscriptions while accessed

[Sakari Ailus]

Hi Hans,

Thanks for the quick review.

On Wed, Sep 12, 2018 at 11:27:35AM +0200, Hans Verkuil wrote:
> On 09/12/18 10:52, Sakari Ailus wrote:
> > The event subscriptions are added to the subscribed event list while
> > holding a spinlock, but that lock is subsequently released while still
> > accessing the subscription object. This makes it possible to unsubscribe
> > the event --- and freeing the subscription object's memory --- while
> > the subscription object is simultaneously accessed.
> 
> Hmm, the (un)subscribe ioctls are serialized through the ioctl lock,
> so this could only be a scenario with drivers that do not use this
> lock. Off-hand the only driver I know that does this is uvc. Unfortunately,
> that's a rather popular one.

On video nodes, perhaps. But how about sub-device nodes? Generally drivers
tend to do locking themselves, whether or not that is the best for most
drivers.

> 
> > 
> > Prevent this by adding a mutex to serialise the event subscription and
> > unsubscription. This also gives a guarantee to the callback ops that the
> > add op has returned before the del op is called.
> > 
> > This change also results in making the elems field less special:
> > subscriptions are only added to the event list once they are fully
> > initialised.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > Hi folks,
> > 
> > I noticed this while working to add support for media events. This seems
> > like material for the stable trees.
> 
> I'd say 'no need for this' if it wasn't for uvc.
> 
> > 
> >  drivers/media/v4l2-core/v4l2-event.c | 35 ++++++++++++++++++-----------------
> >  drivers/media/v4l2-core/v4l2-fh.c    |  2 ++
> >  include/media/v4l2-fh.h              |  4 ++++
> >  3 files changed, 24 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c
> > index 127fe6eb91d9..74161f79e4d3 100644
> > --- a/drivers/media/v4l2-core/v4l2-event.c
> > +++ b/drivers/media/v4l2-core/v4l2-event.c
> > @@ -115,14 +115,6 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *e
> >  	if (sev == NULL)
> >  		return;
> >  
> > -	/*
> > -	 * If the event has been added to the fh->subscribed list, but its
> > -	 * add op has not completed yet elems will be 0, treat this as
> > -	 * not being subscribed.
> > -	 */
> > -	if (!sev->elems)
> > -		return;
> > -
> >  	/* Increase event sequence number on fh. */
> >  	fh->sequence++;
> >  
> > @@ -208,6 +200,7 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> >  	struct v4l2_subscribed_event *sev, *found_ev;
> >  	unsigned long flags;
> >  	unsigned i;
> > +	int ret = 0;
> >  
> >  	if (sub->type == V4L2_EVENT_ALL)
> >  		return -EINVAL;
> > @@ -225,31 +218,35 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> >  	sev->flags = sub->flags;
> >  	sev->fh = fh;
> >  	sev->ops = ops;
> > +	sev->elems = elems;
> > +
> > +	mutex_lock(&fh->mutex);
> >  
> >  	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> >  	found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
> > -	if (!found_ev)
> > -		list_add(&sev->list, &fh->subscribed);
> >  	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> >  
> >  	if (found_ev) {
> > +		/* Already listening */
> >  		kvfree(sev);
> > -		return 0; /* Already listening */
> > +		goto out_unlock;
> >  	}
> >  
> >  	if (sev->ops && sev->ops->add) {
> > -		int ret = sev->ops->add(sev, elems);
> > +		ret = sev->ops->add(sev, elems);
> >  		if (ret) {
> > -			sev->ops = NULL;
> > -			v4l2_event_unsubscribe(fh, sub);
> > -			return ret;
> > +			kvfree(sev);
> > +			goto out_unlock;
> >  		}
> >  	}
> >  
> >  	/* Mark as ready for use */
> > -	sev->elems = elems;
> > +	list_add(&sev->list, &fh->subscribed);
> >  
> > -	return 0;
> > +out_unlock:
> > +	mutex_unlock(&fh->mutex);
> > +
> > +	return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
> >  
> > @@ -288,6 +285,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> >  		return 0;
> >  	}
> >  
> > +	mutex_lock(&fh->mutex);
> > +
> >  	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> >  
> >  	sev = v4l2_event_subscribed(fh, sub->type, sub->id);
> > @@ -305,6 +304,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> >  	if (sev && sev->ops && sev->ops->del)
> >  		sev->ops->del(sev);
> >  
> > +	mutex_unlock(&fh->mutex);
> > +
> >  	kvfree(sev);
> >  
> >  	return 0;
> > diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c
> > index 3895999bf880..b017dafde907 100644
> > --- a/drivers/media/v4l2-core/v4l2-fh.c
> > +++ b/drivers/media/v4l2-core/v4l2-fh.c
> > @@ -45,6 +45,7 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
> >  	INIT_LIST_HEAD(&fh->available);
> >  	INIT_LIST_HEAD(&fh->subscribed);
> >  	fh->sequence = -1;
> > +	mutex_init(&fh->mutex);
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_fh_init);
> >  
> > @@ -90,6 +91,7 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
> >  		return;
> >  	v4l_disable_media_source(fh->vdev);
> >  	v4l2_event_unsubscribe_all(fh);
> > +	mutex_destroy(&fh->mutex);
> >  	fh->vdev = NULL;
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_fh_exit);
> > diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> > index ea73fef8bdc0..1be45a5f6383 100644
> > --- a/include/media/v4l2-fh.h
> > +++ b/include/media/v4l2-fh.h
> > @@ -42,6 +42,9 @@ struct v4l2_ctrl_handler;
> >   * @available: list of events waiting to be dequeued
> >   * @navailable: number of available events at @available list
> >   * @sequence: event sequence number
> > + * @mutex: hold event subscriptions during subscribing;
> > + *	   guarantee that the add and del event callbacks are orderly called
> > + *
> >   * @m2m_ctx: pointer to &struct v4l2_m2m_ctx
> >   */
> >  struct v4l2_fh {
> > @@ -56,6 +59,7 @@ struct v4l2_fh {
> >  	struct list_head	available;
> >  	unsigned int		navailable;
> >  	u32			sequence;
> > +	struct mutex		mutex;
> 
> I don't like the name 'mutex'. Perhaps something more descriptive like:
> 'subscribe_lock'?
> 
> >  
> >  #if IS_ENABLED(CONFIG_V4L2_MEM2MEM_DEV)
> >  	struct v4l2_m2m_ctx	*m2m_ctx;
> > 
> 
> Overall I think this patch makes sense. The code is cleaner and easier to follow.
> Just give 'mutex' a better name :-)

How about "subscribe_mutex"? It's a mutex... "subscribe_lock" would use a
similar convention elsewhere in V4L2 where mutexes are commonly called
locks, so I'm certainly fine with that as well.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 1/1] v4l: event: Prevent freeing event subscriptions while accessed

[Laurent Pinchart]

Hello,

On Wednesday, 12 September 2018 13:00:57 EEST Sakari Ailus wrote:
> On Wed, Sep 12, 2018 at 11:27:35AM +0200, Hans Verkuil wrote:
> > On 09/12/18 10:52, Sakari Ailus wrote:
> >> The event subscriptions are added to the subscribed event list while
> >> holding a spinlock, but that lock is subsequently released while still
> >> accessing the subscription object. This makes it possible to unsubscribe
> >> the event --- and freeing the subscription object's memory --- while
> >> the subscription object is simultaneously accessed.
> > 
> > Hmm, the (un)subscribe ioctls are serialized through the ioctl lock,
> > so this could only be a scenario with drivers that do not use this
> > lock. Off-hand the only driver I know that does this is uvc.
> > Unfortunately,
> > that's a rather popular one.
> 
> On video nodes, perhaps. But how about sub-device nodes? Generally drivers
> tend to do locking themselves, whether or not that is the best for most
> drivers.

I tend to agree with Sakari. Furthermore, having fine-grained locking is 
better in my opinion than locking everything at the ioctl level, for drivers 
that wish to do so. We should thus strive for self-contained locking in the 
different helper libraries of V4L2.

> >> Prevent this by adding a mutex to serialise the event subscription and
> >> unsubscription. This also gives a guarantee to the callback ops that the
> >> add op has returned before the del op is called.
> >> 
> >> This change also results in making the elems field less special:
> >> subscriptions are only added to the event list once they are fully
> >> initialised.
> >> 
> >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >> ---
> >> Hi folks,
> >> 
> >> I noticed this while working to add support for media events. This seems
> >> like material for the stable trees.
> > 
> > I'd say 'no need for this' if it wasn't for uvc.
> > 
> >>  drivers/media/v4l2-core/v4l2-event.c | 35 ++++++++++++++++-------------
> >>  drivers/media/v4l2-core/v4l2-fh.c    |  2 ++
> >>  include/media/v4l2-fh.h              |  4 ++++
> >>  3 files changed, 24 insertions(+), 17 deletions(-)

[snip]

> >> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> >> index ea73fef8bdc0..1be45a5f6383 100644
> >> --- a/include/media/v4l2-fh.h
> >> +++ b/include/media/v4l2-fh.h
> >> @@ -42,6 +42,9 @@ struct v4l2_ctrl_handler;
> >>   * @available: list of events waiting to be dequeued
> >>   * @navailable: number of available events at @available list
> >>   * @sequence: event sequence number
> >> + * @mutex: hold event subscriptions during subscribing;
> >> + *	   guarantee that the add and del event callbacks are orderly called

Could you try to describe what this mutex protects in terms of data ?

> >> + *

Extra blank line ?

> >>   * @m2m_ctx: pointer to &struct v4l2_m2m_ctx
> >>   */
> >>  
> >>  struct v4l2_fh {
> >> @@ -56,6 +59,7 @@ struct v4l2_fh {
> >>  	struct list_head	available;
> >>  	unsigned int		navailable;
> >>  	u32			sequence;
> >> +	struct mutex		mutex;
> > 
> > I don't like the name 'mutex'. Perhaps something more descriptive like:
> > 'subscribe_lock'?
> > 
> >>  #if IS_ENABLED(CONFIG_V4L2_MEM2MEM_DEV)
> >>  
> >>  	struct v4l2_m2m_ctx	*m2m_ctx;
> > 
> > Overall I think this patch makes sense. The code is cleaner and easier to
> > follow. Just give 'mutex' a better name :-)
> 
> How about "subscribe_mutex"? It's a mutex... "subscribe_lock" would use a
> similar convention elsewhere in V4L2 where mutexes are commonly called
> locks, so I'm certainly fine with that as well.

We indeed use lock for both mutexes and spinlocks. I have a slight preference 
for the name lock myself, but I don't mind too much.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/1] v4l: event: Prevent freeing event subscriptions while accessed

[Hans Verkuil]

On 09/12/18 12:00, Sakari Ailus wrote:
> Hi Hans,
> 
> Thanks for the quick review.
> 
> On Wed, Sep 12, 2018 at 11:27:35AM +0200, Hans Verkuil wrote:
>> On 09/12/18 10:52, Sakari Ailus wrote:
>>> The event subscriptions are added to the subscribed event list while
>>> holding a spinlock, but that lock is subsequently released while still
>>> accessing the subscription object. This makes it possible to unsubscribe
>>> the event --- and freeing the subscription object's memory --- while
>>> the subscription object is simultaneously accessed.
>>
>> Hmm, the (un)subscribe ioctls are serialized through the ioctl lock,
>> so this could only be a scenario with drivers that do not use this
>> lock. Off-hand the only driver I know that does this is uvc. Unfortunately,
>> that's a rather popular one.
> 
> On video nodes, perhaps. But how about sub-device nodes? Generally drivers
> tend to do locking themselves, whether or not that is the best for most
> drivers.

That's a whole different discussion :-)

I've never been convinced by this since 1) it's hard to prove correctness
if drivers handle locking and 2) I've never seen any proof that it actually
improves performance.

Anyway, it's unrelated to this patch.

> 
>>
>>>
>>> Prevent this by adding a mutex to serialise the event subscription and
>>> unsubscription. This also gives a guarantee to the callback ops that the
>>> add op has returned before the del op is called.
>>>
>>> This change also results in making the elems field less special:
>>> subscriptions are only added to the event list once they are fully
>>> initialised.
>>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>> ---
>>> Hi folks,
>>>
>>> I noticed this while working to add support for media events. This seems
>>> like material for the stable trees.
>>
>> I'd say 'no need for this' if it wasn't for uvc.
>>
>>>
>>>  drivers/media/v4l2-core/v4l2-event.c | 35 ++++++++++++++++++-----------------
>>>  drivers/media/v4l2-core/v4l2-fh.c    |  2 ++
>>>  include/media/v4l2-fh.h              |  4 ++++
>>>  3 files changed, 24 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c
>>> index 127fe6eb91d9..74161f79e4d3 100644
>>> --- a/drivers/media/v4l2-core/v4l2-event.c
>>> +++ b/drivers/media/v4l2-core/v4l2-event.c
>>> @@ -115,14 +115,6 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *e
>>>  	if (sev == NULL)
>>>  		return;
>>>  
>>> -	/*
>>> -	 * If the event has been added to the fh->subscribed list, but its
>>> -	 * add op has not completed yet elems will be 0, treat this as
>>> -	 * not being subscribed.
>>> -	 */
>>> -	if (!sev->elems)
>>> -		return;
>>> -
>>>  	/* Increase event sequence number on fh. */
>>>  	fh->sequence++;
>>>  
>>> @@ -208,6 +200,7 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>>>  	struct v4l2_subscribed_event *sev, *found_ev;
>>>  	unsigned long flags;
>>>  	unsigned i;
>>> +	int ret = 0;
>>>  
>>>  	if (sub->type == V4L2_EVENT_ALL)
>>>  		return -EINVAL;
>>> @@ -225,31 +218,35 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>>>  	sev->flags = sub->flags;
>>>  	sev->fh = fh;
>>>  	sev->ops = ops;
>>> +	sev->elems = elems;
>>> +
>>> +	mutex_lock(&fh->mutex);
>>>  
>>>  	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>>>  	found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
>>> -	if (!found_ev)
>>> -		list_add(&sev->list, &fh->subscribed);
>>>  	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>>>  
>>>  	if (found_ev) {
>>> +		/* Already listening */
>>>  		kvfree(sev);
>>> -		return 0; /* Already listening */
>>> +		goto out_unlock;
>>>  	}
>>>  
>>>  	if (sev->ops && sev->ops->add) {
>>> -		int ret = sev->ops->add(sev, elems);
>>> +		ret = sev->ops->add(sev, elems);
>>>  		if (ret) {
>>> -			sev->ops = NULL;
>>> -			v4l2_event_unsubscribe(fh, sub);
>>> -			return ret;
>>> +			kvfree(sev);
>>> +			goto out_unlock;
>>>  		}
>>>  	}
>>>  
>>>  	/* Mark as ready for use */
>>> -	sev->elems = elems;
>>> +	list_add(&sev->list, &fh->subscribed);
>>>  
>>> -	return 0;
>>> +out_unlock:
>>> +	mutex_unlock(&fh->mutex);
>>> +
>>> +	return ret;
>>>  }
>>>  EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
>>>  
>>> @@ -288,6 +285,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>>>  		return 0;
>>>  	}
>>>  
>>> +	mutex_lock(&fh->mutex);
>>> +
>>>  	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>>>  
>>>  	sev = v4l2_event_subscribed(fh, sub->type, sub->id);
>>> @@ -305,6 +304,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>>>  	if (sev && sev->ops && sev->ops->del)
>>>  		sev->ops->del(sev);
>>>  
>>> +	mutex_unlock(&fh->mutex);
>>> +
>>>  	kvfree(sev);
>>>  
>>>  	return 0;
>>> diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c
>>> index 3895999bf880..b017dafde907 100644
>>> --- a/drivers/media/v4l2-core/v4l2-fh.c
>>> +++ b/drivers/media/v4l2-core/v4l2-fh.c
>>> @@ -45,6 +45,7 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
>>>  	INIT_LIST_HEAD(&fh->available);
>>>  	INIT_LIST_HEAD(&fh->subscribed);
>>>  	fh->sequence = -1;
>>> +	mutex_init(&fh->mutex);
>>>  }
>>>  EXPORT_SYMBOL_GPL(v4l2_fh_init);
>>>  
>>> @@ -90,6 +91,7 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
>>>  		return;
>>>  	v4l_disable_media_source(fh->vdev);
>>>  	v4l2_event_unsubscribe_all(fh);
>>> +	mutex_destroy(&fh->mutex);
>>>  	fh->vdev = NULL;
>>>  }
>>>  EXPORT_SYMBOL_GPL(v4l2_fh_exit);
>>> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
>>> index ea73fef8bdc0..1be45a5f6383 100644
>>> --- a/include/media/v4l2-fh.h
>>> +++ b/include/media/v4l2-fh.h
>>> @@ -42,6 +42,9 @@ struct v4l2_ctrl_handler;
>>>   * @available: list of events waiting to be dequeued
>>>   * @navailable: number of available events at @available list
>>>   * @sequence: event sequence number
>>> + * @mutex: hold event subscriptions during subscribing;
>>> + *	   guarantee that the add and del event callbacks are orderly called
>>> + *
>>>   * @m2m_ctx: pointer to &struct v4l2_m2m_ctx
>>>   */
>>>  struct v4l2_fh {
>>> @@ -56,6 +59,7 @@ struct v4l2_fh {
>>>  	struct list_head	available;
>>>  	unsigned int		navailable;
>>>  	u32			sequence;
>>> +	struct mutex		mutex;
>>
>> I don't like the name 'mutex'. Perhaps something more descriptive like:
>> 'subscribe_lock'?
>>
>>>  
>>>  #if IS_ENABLED(CONFIG_V4L2_MEM2MEM_DEV)
>>>  	struct v4l2_m2m_ctx	*m2m_ctx;
>>>
>>
>> Overall I think this patch makes sense. The code is cleaner and easier to follow.
>> Just give 'mutex' a better name :-)
> 
> How about "subscribe_mutex"? It's a mutex... "subscribe_lock" would use a
> similar convention elsewhere in V4L2 where mutexes are commonly called
> locks, so I'm certainly fine with that as well.
> 

Like Laurent I have a slight preference for using _lock.

I prefer _slock for spinlocks, and _lock for mutexes.

Regards,

	Hans

Re: [PATCH 1/1] v4l: event: Prevent freeing event subscriptions while accessed

[Sakari Ailus]

Hi Hans,

On Wed, Sep 12, 2018 at 02:32:52PM +0200, Hans Verkuil wrote:
> On 09/12/18 12:00, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > Thanks for the quick review.
> > 
> > On Wed, Sep 12, 2018 at 11:27:35AM +0200, Hans Verkuil wrote:
> >> On 09/12/18 10:52, Sakari Ailus wrote:
> >>> The event subscriptions are added to the subscribed event list while
> >>> holding a spinlock, but that lock is subsequently released while still
> >>> accessing the subscription object. This makes it possible to unsubscribe
> >>> the event --- and freeing the subscription object's memory --- while
> >>> the subscription object is simultaneously accessed.
> >>
> >> Hmm, the (un)subscribe ioctls are serialized through the ioctl lock,
> >> so this could only be a scenario with drivers that do not use this
> >> lock. Off-hand the only driver I know that does this is uvc. Unfortunately,
> >> that's a rather popular one.
> > 
> > On video nodes, perhaps. But how about sub-device nodes? Generally drivers
> > tend to do locking themselves, whether or not that is the best for most
> > drivers.
> 
> That's a whole different discussion :-)

Not entirely: the sub-device drivers do expose the sub-device event interface
to the user space and thus the bug affects those as well.

> 
> I've never been convinced by this since 1) it's hard to prove correctness
> if drivers handle locking and 2) I've never seen any proof that it actually
> improves performance.
> 
> Anyway, it's unrelated to this patch.

Yes, I agree on that.

> 
> > 
> >>
> >>>
> >>> Prevent this by adding a mutex to serialise the event subscription and
> >>> unsubscription. This also gives a guarantee to the callback ops that the
> >>> add op has returned before the del op is called.
> >>>
> >>> This change also results in making the elems field less special:
> >>> subscriptions are only added to the event list once they are fully
> >>> initialised.
> >>>
> >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>> ---
> >>> Hi folks,
> >>>
> >>> I noticed this while working to add support for media events. This seems
> >>> like material for the stable trees.
> >>
> >> I'd say 'no need for this' if it wasn't for uvc.
> >>
> >>>
> >>>  drivers/media/v4l2-core/v4l2-event.c | 35 ++++++++++++++++++-----------------
> >>>  drivers/media/v4l2-core/v4l2-fh.c    |  2 ++
> >>>  include/media/v4l2-fh.h              |  4 ++++
> >>>  3 files changed, 24 insertions(+), 17 deletions(-)
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c
> >>> index 127fe6eb91d9..74161f79e4d3 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-event.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-event.c
> >>> @@ -115,14 +115,6 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *e
> >>>  	if (sev == NULL)
> >>>  		return;
> >>>  
> >>> -	/*
> >>> -	 * If the event has been added to the fh->subscribed list, but its
> >>> -	 * add op has not completed yet elems will be 0, treat this as
> >>> -	 * not being subscribed.
> >>> -	 */
> >>> -	if (!sev->elems)
> >>> -		return;
> >>> -
> >>>  	/* Increase event sequence number on fh. */
> >>>  	fh->sequence++;
> >>>  
> >>> @@ -208,6 +200,7 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> >>>  	struct v4l2_subscribed_event *sev, *found_ev;
> >>>  	unsigned long flags;
> >>>  	unsigned i;
> >>> +	int ret = 0;
> >>>  
> >>>  	if (sub->type == V4L2_EVENT_ALL)
> >>>  		return -EINVAL;
> >>> @@ -225,31 +218,35 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> >>>  	sev->flags = sub->flags;
> >>>  	sev->fh = fh;
> >>>  	sev->ops = ops;
> >>> +	sev->elems = elems;
> >>> +
> >>> +	mutex_lock(&fh->mutex);
> >>>  
> >>>  	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> >>>  	found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
> >>> -	if (!found_ev)
> >>> -		list_add(&sev->list, &fh->subscribed);
> >>>  	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> >>>  
> >>>  	if (found_ev) {
> >>> +		/* Already listening */
> >>>  		kvfree(sev);
> >>> -		return 0; /* Already listening */
> >>> +		goto out_unlock;
> >>>  	}
> >>>  
> >>>  	if (sev->ops && sev->ops->add) {
> >>> -		int ret = sev->ops->add(sev, elems);
> >>> +		ret = sev->ops->add(sev, elems);
> >>>  		if (ret) {
> >>> -			sev->ops = NULL;
> >>> -			v4l2_event_unsubscribe(fh, sub);
> >>> -			return ret;
> >>> +			kvfree(sev);
> >>> +			goto out_unlock;
> >>>  		}
> >>>  	}
> >>>  
> >>>  	/* Mark as ready for use */
> >>> -	sev->elems = elems;
> >>> +	list_add(&sev->list, &fh->subscribed);
> >>>  
> >>> -	return 0;
> >>> +out_unlock:
> >>> +	mutex_unlock(&fh->mutex);
> >>> +
> >>> +	return ret;
> >>>  }
> >>>  EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
> >>>  
> >>> @@ -288,6 +285,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> >>>  		return 0;
> >>>  	}
> >>>  
> >>> +	mutex_lock(&fh->mutex);
> >>> +
> >>>  	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> >>>  
> >>>  	sev = v4l2_event_subscribed(fh, sub->type, sub->id);
> >>> @@ -305,6 +304,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> >>>  	if (sev && sev->ops && sev->ops->del)
> >>>  		sev->ops->del(sev);
> >>>  
> >>> +	mutex_unlock(&fh->mutex);
> >>> +
> >>>  	kvfree(sev);
> >>>  
> >>>  	return 0;
> >>> diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c
> >>> index 3895999bf880..b017dafde907 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-fh.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-fh.c
> >>> @@ -45,6 +45,7 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
> >>>  	INIT_LIST_HEAD(&fh->available);
> >>>  	INIT_LIST_HEAD(&fh->subscribed);
> >>>  	fh->sequence = -1;
> >>> +	mutex_init(&fh->mutex);
> >>>  }
> >>>  EXPORT_SYMBOL_GPL(v4l2_fh_init);
> >>>  
> >>> @@ -90,6 +91,7 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
> >>>  		return;
> >>>  	v4l_disable_media_source(fh->vdev);
> >>>  	v4l2_event_unsubscribe_all(fh);
> >>> +	mutex_destroy(&fh->mutex);
> >>>  	fh->vdev = NULL;
> >>>  }
> >>>  EXPORT_SYMBOL_GPL(v4l2_fh_exit);
> >>> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> >>> index ea73fef8bdc0..1be45a5f6383 100644
> >>> --- a/include/media/v4l2-fh.h
> >>> +++ b/include/media/v4l2-fh.h
> >>> @@ -42,6 +42,9 @@ struct v4l2_ctrl_handler;
> >>>   * @available: list of events waiting to be dequeued
> >>>   * @navailable: number of available events at @available list
> >>>   * @sequence: event sequence number
> >>> + * @mutex: hold event subscriptions during subscribing;
> >>> + *	   guarantee that the add and del event callbacks are orderly called
> >>> + *
> >>>   * @m2m_ctx: pointer to &struct v4l2_m2m_ctx
> >>>   */
> >>>  struct v4l2_fh {
> >>> @@ -56,6 +59,7 @@ struct v4l2_fh {
> >>>  	struct list_head	available;
> >>>  	unsigned int		navailable;
> >>>  	u32			sequence;
> >>> +	struct mutex		mutex;
> >>
> >> I don't like the name 'mutex'. Perhaps something more descriptive like:
> >> 'subscribe_lock'?
> >>
> >>>  
> >>>  #if IS_ENABLED(CONFIG_V4L2_MEM2MEM_DEV)
> >>>  	struct v4l2_m2m_ctx	*m2m_ctx;
> >>>
> >>
> >> Overall I think this patch makes sense. The code is cleaner and easier to follow.
> >> Just give 'mutex' a better name :-)
> > 
> > How about "subscribe_mutex"? It's a mutex... "subscribe_lock" would use a
> > similar convention elsewhere in V4L2 where mutexes are commonly called
> > locks, so I'm certainly fine with that as well.
> > 
> 
> Like Laurent I have a slight preference for using _lock.
> 
> I prefer _slock for spinlocks, and _lock for mutexes.

I'll use subscribe_lock.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 1/1] v4l: event: Prevent freeing event subscriptions while accessed

[Sakari Ailus]

Hi Laurent,

On Wed, Sep 12, 2018 at 02:57:20PM +0300, Laurent Pinchart wrote:
> Hello,
> 
> On Wednesday, 12 September 2018 13:00:57 EEST Sakari Ailus wrote:
> > On Wed, Sep 12, 2018 at 11:27:35AM +0200, Hans Verkuil wrote:
> > > On 09/12/18 10:52, Sakari Ailus wrote:
> > >> The event subscriptions are added to the subscribed event list while
> > >> holding a spinlock, but that lock is subsequently released while still
> > >> accessing the subscription object. This makes it possible to unsubscribe
> > >> the event --- and freeing the subscription object's memory --- while
> > >> the subscription object is simultaneously accessed.
> > > 
> > > Hmm, the (un)subscribe ioctls are serialized through the ioctl lock,
> > > so this could only be a scenario with drivers that do not use this
> > > lock. Off-hand the only driver I know that does this is uvc.
> > > Unfortunately,
> > > that's a rather popular one.
> > 
> > On video nodes, perhaps. But how about sub-device nodes? Generally drivers
> > tend to do locking themselves, whether or not that is the best for most
> > drivers.
> 
> I tend to agree with Sakari. Furthermore, having fine-grained locking is 
> better in my opinion than locking everything at the ioctl level, for drivers 
> that wish to do so. We should thus strive for self-contained locking in the 
> different helper libraries of V4L2.
> 
> > >> Prevent this by adding a mutex to serialise the event subscription and
> > >> unsubscription. This also gives a guarantee to the callback ops that the
> > >> add op has returned before the del op is called.
> > >> 
> > >> This change also results in making the elems field less special:
> > >> subscriptions are only added to the event list once they are fully
> > >> initialised.
> > >> 
> > >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > >> ---
> > >> Hi folks,
> > >> 
> > >> I noticed this while working to add support for media events. This seems
> > >> like material for the stable trees.
> > > 
> > > I'd say 'no need for this' if it wasn't for uvc.
> > > 
> > >>  drivers/media/v4l2-core/v4l2-event.c | 35 ++++++++++++++++-------------
> > >>  drivers/media/v4l2-core/v4l2-fh.c    |  2 ++
> > >>  include/media/v4l2-fh.h              |  4 ++++
> > >>  3 files changed, 24 insertions(+), 17 deletions(-)
> 
> [snip]
> 
> > >> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> > >> index ea73fef8bdc0..1be45a5f6383 100644
> > >> --- a/include/media/v4l2-fh.h
> > >> +++ b/include/media/v4l2-fh.h
> > >> @@ -42,6 +42,9 @@ struct v4l2_ctrl_handler;
> > >>   * @available: list of events waiting to be dequeued
> > >>   * @navailable: number of available events at @available list
> > >>   * @sequence: event sequence number
> > >> + * @mutex: hold event subscriptions during subscribing;
> > >> + *	   guarantee that the add and del event callbacks are orderly called
> 
> Could you try to describe what this mutex protects in terms of data ?

The purpose actually changed a bit after I wrote the text. That could be
the reason why it's sort of detached from the reality. :-) How about this:

mutex: serialise changes to the subscribed list; guarantee that the add and
       del event callbacks are orderly called

> 
> > >> + *
> 
> Extra blank line ?

A line that separates the event related fields from the m2m context.
There's one above the event related ones, too. I didn't think it's worth
mentioning that in the patch description. :-)

-- 
Sakari Ailus
sakari.ailus@linux.intel.com