[PATCH v2] media: au0828: release the media source when the device is disconnected

[YuanZhe Liu]

au0828_enable_source() starts the media pipeline and saves the link, its
owner and the source and sink entities in struct au0828_dev.  The matching
au0828_disable_source() is only called from the frontend open and release
paths.

When the device is disconnected, au0828_dvb_unregister() removes the
frontend entity through dvb_unregister_frontend().  dvb_remove_device()
frees the entity unconditionally and __media_entity_remove_links() frees
every link of it, but the source is not released: the pipeline is never
stopped and dev->active_link, dev->active_link_owner, dev->active_source
and dev->active_sink keep pointing at freed memory.

struct au0828_dev stays alive until the last V4L2 file descriptor is
closed, because the video device holds a reference to v4l2_device.  A
later close therefore reaches au0828_disable_source() through
v4l2_fh_exit() and dereferences the freed link and entity.

Stop the pipeline and deactivate the link in au0828_usb_disconnect(),
before the frontend entity is unregistered and while holding graph_mutex.
Move the cleanup tail of au0828_disable_source() to
au0828_release_active_link() so that both paths share it; the disconnect
path must not be skipped by the dev->users > 1 check, which only applies
to a video owner.  Nothing has to be released when no link was activated,
so the call is guarded by dev->active_link_owner, and the guard is
evaluated again under graph_mutex: a close that runs in between can clear
the field before the disconnect path acquires the lock.

au0828_disable_source() stops the pipeline itself on the way to its
deactivate_link label, so the label deactivates the link and clears the
saved state only (au0828_deactivate_link()), while the disconnect path
calls au0828_release_active_link(), which stops the pipeline first.
Calling __media_pipeline_stop() a second time for the same pipeline trips
its WARN_ON(!pipe) check.

au0828_enable_source() refuses to start a pipeline once DEV_DISCONNECTED
is set.  Without that check a frontend open that happens while
au0828_dvb_unregister() shuts the rest of the device down can reactivate
the link after the disconnect path released it, and the frontend entity
is then freed with an active pipeline.

Fixes: c94903f1f7cc ("[media] media: au0828 add enable, disable source handlers")
Cc: stable@xxxxxxxxxxxxxxx
Co-developed-by: Bin Yu <byu@xxxxxxxxxxxxx>
Signed-off-by: Bin Yu <byu@xxxxxxxxxxxxx>
Co-developed-by: Cheng Wen <wencheng@xxxxxxxxxxxxx>
Signed-off-by: Cheng Wen <wencheng@xxxxxxxxxxxxx>
Signed-off-by: YuanZhe Liu <25031212351@xxxxxxxxxxxxxxxxx>
---
Changes in v2:
- Re-check dev->active_link_owner under graph_mutex.  A close running on
  another CPU can clear it while the disconnect path waits for the lock,
  and au0828_release_active_link() would then dereference NULL.
- Split the shared cleanup into au0828_deactivate_link(), so the
  deactivate_link label of au0828_disable_source() does not stop the
  pipeline a second time.  __media_pipeline_stop() warns on an unbalanced
  call.
- Reject au0828_enable_source() after DEV_DISCONNECTED, so a frontend
  open cannot reactivate the link while the device is being torn down.
- The NULL entity that dvb_frontend_release() passes after
  dvb_media_device_free() is a dvb-core problem, fixed separately by
  https://lore.kernel.org/r/20261102143415.26942-1-25031212351@xxxxxxxxxxxxxxxxx
---
 drivers/media/usb/au0828/au0828-core.c | 64 ++++++++++++++++++++------
 1 file changed, 50 insertions(+), 14 deletions(-)

diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
index 445cbeb7abae..0d4528a26c7d 100644
--- a/drivers/media/usb/au0828/au0828-core.c
+++ b/drivers/media/usb/au0828/au0828-core.c
@@ -120,6 +120,7 @@ static int recv_control_msg(struct au0828_dev *dev, u16 request, u32 value,
 #ifdef CONFIG_MEDIA_CONTROLLER
 static void au0828_media_graph_notify(struct media_entity *new,
 				      void *notify_data);
+static void au0828_release_active_link(struct au0828_dev *dev);
 #endif
 
 static void au0828_unregister_media_device(struct au0828_dev *dev)
@@ -175,6 +176,19 @@ static void au0828_usb_disconnect(struct usb_interface *interface)
 	set_bit(DEV_DISCONNECTED, &dev->dev_state);
 
 	au0828_rc_unregister(dev);
+#ifdef CONFIG_MEDIA_CONTROLLER
+	/*
+	 * The frontend entity can own the active link.  Its entity is released
+	 * by au0828_dvb_unregister() below, so stop the pipeline and deactivate
+	 * the link while the entity is still registered.
+	 */
+	if (dev->media_dev) {
+		mutex_lock(&dev->media_dev->graph_mutex);
+		if (dev->active_link_owner)
+			au0828_release_active_link(dev);
+		mutex_unlock(&dev->media_dev->graph_mutex);
+	}
+#endif
 	/* Digital TV */
 	au0828_dvb_unregister(dev);
 
@@ -300,6 +314,10 @@ static int au0828_enable_source(struct media_entity *entity,
 
 	dev = mdev->source_priv;
 
+	/* Do not start a new pipeline on a device that is going away */
+	if (test_bit(DEV_DISCONNECTED, &dev->dev_state))
+		return -ENODEV;
+
 	/*
 	 * For Audio and V4L2 entity, find the link to which decoder
 	 * is the sink. Look for an active link between decoder and
@@ -438,6 +456,37 @@ static int au0828_enable_source(struct media_entity *entity,
 	return ret;
 }
 
+/* Callers should hold graph_mutex */
+static void au0828_deactivate_link(struct au0828_dev *dev)
+{
+	int ret;
+
+	ret = __media_entity_setup_link(dev->active_link, 0);
+	if (ret)
+		pr_err("Deactivate link Error %d\n", ret);
+
+	pr_info("Disabled Source: %s->%s->%s Ret %d\n",
+		dev->active_source->name, dev->active_sink->name,
+		dev->active_link_owner->name, ret);
+
+	dev->active_link = NULL;
+	dev->active_link_owner = NULL;
+	dev->active_source = NULL;
+	dev->active_sink = NULL;
+	dev->active_link_shared = false;
+	dev->active_link_user = NULL;
+	dev->active_link_user_pipe = NULL;
+}
+
+/* Callers should hold graph_mutex */
+static void au0828_release_active_link(struct au0828_dev *dev)
+{
+	__media_pipeline_stop(dev->active_link_owner->pads);
+	pr_debug("Pipeline stop for %s\n", dev->active_link_owner->name);
+
+	au0828_deactivate_link(dev);
+}
+
 /* Callers should hold graph_mutex */
 static void au0828_disable_source(struct media_entity *entity)
 {
@@ -537,20 +586,7 @@ static void au0828_disable_source(struct media_entity *entity)
 			dev->active_link_owner->name);
 
 deactivate_link:
-		ret = __media_entity_setup_link(dev->active_link, 0);
-		if (ret)
-			pr_err("Deactivate link Error %d\n", ret);
-
-		pr_info("Disabled Source: %s->%s->%s Ret %d\n",
-			 dev->active_source->name, dev->active_sink->name,
-			 dev->active_link_owner->name, ret);
-
-		dev->active_link = NULL;
-		dev->active_link_owner = NULL;
-		dev->active_source = NULL;
-		dev->active_sink = NULL;
-		dev->active_link_shared = false;
-		dev->active_link_user = NULL;
+		au0828_deactivate_link(dev);
 	}
 }
 #endif
-- 
2.34.1