v4l: event: Prevent freeing event subscriptions while accessed

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 <[email protected]>

---
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(-)

drivers/media/v4l2-core/v4l2-event.c +18 -17

index 127fe6eb91d9..74161f79e4d3 100644
@@ -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;

drivers/media/v4l2-core/v4l2-fh.c +2 -0

index 3895999bf880..b017dafde907 100644
@@ -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);

include/media/v4l2-fh.h +3 -0

index ea73fef8bdc0..1be45a5f6383 100644
@@ -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 {

Quoted 5 lines · 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.

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.

Quoted 10 lines

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.

Quoted 6 lines

Signed-off-by Sakari Ailus <[email protected]>

---
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.

Quoted 131 lines

 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(-)

drivers/media/v4l2-core/v4l2-event.c +18 -17

index 127fe6eb91d9..74161f79e4d3 100644
@@ -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;

drivers/media/v4l2-core/v4l2-fh.c +2 -0

index 3895999bf880..b017dafde907 100644
@@ -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);

include/media/v4l2-fh.h +3 -0

index ea73fef8bdc0..1be45a5f6383 100644
@@ -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 {

include/media/v4l2-fh.h struct v4l2_fh { · line 62

Quoted 4 lines

include/media/v4l2-fh.h +1 -0

@@ -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'?

Quoted 1 line

Quoted 3 lines

#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 :-)

Quoted 7 lines · Hans Verkuil

Quoted 5 lines · 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.

Quoted 5 lines

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.

Quoted 21 lines

Quoted 8 lines

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.

Quoted 6 lines

Signed-off-by Sakari Ailus <[email protected]>

---
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.

Quoted 4 lines

 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(-)

Quoted 10 lines

include/media/v4l2-fh.h struct v4l2_ctrl_handler; · line 46

Quoted 10 lines

include/media/v4l2-fh.h +2 -0

index ea73fef8bdc0..1be45a5f6383 100644
@@ -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

Quoted 1 line

include/media/v4l2-fh.h struct v4l2_ctrl_handler; · line 47

Quoted 1 line

include/media/v4l2-fh.h +1 -0

+ *

Quoted 15 lines

include/media/v4l2-fh.h struct v4l2_fh { · line 62

Quoted 9 lines

include/media/v4l2-fh.h +0 -0

  * @m2m_ctx: pointer to &struct v4l2_m2m_ctx
  */
 

struct v4l2_fh {

include/media/v4l2-fh.h +1 -0

@@ -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'?

Quoted 2 lines

include/media/v4l2-fh.h +0 -0

 #if IS_ENABLED(CONFIG_V4L2_MEM2MEM_DEV)
 

Quoted 4 lines

include/media/v4l2-fh.h struct v4l2_fh { · line 65

Quoted 1 line

include/media/v4l2-fh.h +0 -0

 	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.

Hello,

Quoted 14 lines · On Wednesday, 12 September 2018 13:00:57 EEST Sakari Ailus wrote:

Quoted 7 lines · Hans Verkuil

Quoted 5 lines · 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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 21 lines

Quoted 21 lines

Quoted 8 lines

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.

Quoted 6 lines

Signed-off-by Sakari Ailus <[email protected]>

---
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.

Quoted 4 lines

 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]

Quoted 10 lines

Quoted 10 lines

include/media/v4l2-fh.h struct v4l2_ctrl_handler; · line 46

Quoted 10 lines

include/media/v4l2-fh.h +2 -0

index ea73fef8bdc0..1be45a5f6383 100644
@@ -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 ?

Quoted 2 lines

Quoted 2 lines

include/media/v4l2-fh.h struct v4l2_ctrl_handler; · line 47

Quoted 2 lines

include/media/v4l2-fh.h +1 -0

+ *

Extra blank line ?

Hi Hans,

Thanks for the quick review.

Quoted 7 lines · Hans Verkuil

Quoted 5 lines · 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.

Quoted 4 lines

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.

Quoted 164 lines

Quoted 11 lines

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.

Quoted 6 lines

Signed-off-by Sakari Ailus <[email protected]>

---
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.

Quoted 131 lines

 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(-)

drivers/media/v4l2-core/v4l2-event.c +18 -17

index 127fe6eb91d9..74161f79e4d3 100644
@@ -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;

drivers/media/v4l2-core/v4l2-fh.c +2 -0

index 3895999bf880..b017dafde907 100644
@@ -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);

include/media/v4l2-fh.h +3 -0

index ea73fef8bdc0..1be45a5f6383 100644
@@ -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 {

include/media/v4l2-fh.h struct v4l2_fh { · line 62

Quoted 4 lines

include/media/v4l2-fh.h +1 -0

@@ -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'?

Quoted 4 lines

include/media/v4l2-fh.h +0 -0

 #if IS_ENABLED(CONFIG_V4L2_MEM2MEM_DEV)
 	struct v4l2_m2m_ctx	*m2m_ctx;
 

Quoted 2 lines

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.

Quoted 17 lines · Sakari Ailus

Hi Hans,

Thanks for the quick review.

Quoted 11 lines · Hans Verkuil

Quoted 5 lines · 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.

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.

Quoted 3 lines

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.

Quoted 168 lines

Quoted 167 lines

Quoted 12 lines

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.

Quoted 6 lines

Signed-off-by Sakari Ailus <[email protected]>

---
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.

Quoted 131 lines

 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(-)

drivers/media/v4l2-core/v4l2-event.c +18 -17

index 127fe6eb91d9..74161f79e4d3 100644
@@ -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;

drivers/media/v4l2-core/v4l2-fh.c +2 -0

index 3895999bf880..b017dafde907 100644
@@ -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);

include/media/v4l2-fh.h +3 -0

index ea73fef8bdc0..1be45a5f6383 100644
@@ -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 {

include/media/v4l2-fh.h struct v4l2_fh { · line 62

Quoted 4 lines

include/media/v4l2-fh.h +1 -0

@@ -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'?

include/media/v4l2-fh.h struct v4l2_fh {

Quoted 4 lines

include/media/v4l2-fh.h +0 -0

 #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 :-)

Quoted 4 lines

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.