[PATCH] media: mc: Remove spurious WARN_ON in __media_pipeline_start

Niharika Khare <[email protected]>
Newsgroups org.kernel.vger.linux-media,org.kernel.vger.linux-kernel
Message-ID <[email protected]>
Vimc driver's entity topology has three capture nodes and two of
those (Raw Capture 0 and RGB/YUV Capture) are reachable from each
other via enabled links. The crash report and the C repro show that
the two capture nodes attempt to invoke a STREAMON session one after
the other, which resulted in the WARNING getting triggered by the
media controller.

  vimc link validate: Sensor A:src:640x480 (0x42474752, 0, 0, 0, 0) Raw Capture 0:snk:640x480 (0x42474752, 8, 0, 0, 0)
  vimc link validate: Scaler:src:640x480 (0x33424752, 8, 0, 0, 0) RGB/YUV Capture:snk:640x480 (0x33424752, 8, 0, 0, 0)
  origin->pipe && origin->pipe != pipe
  WARNING: drivers/media/mc/mc-entity.c:785 at __media_pipeline_start+0x1add/0x22b0, CPU#1: syz.0.17/5912

The graph walk originating from Raw Capture 0 (first STREAMON in C
repro on /dev/video0) reaches RGB/YUV Capture and marks its origin
pad's pipe with its own pipe. When RGB/YUV Capture's graph walk starts
(second STREAMON in C repro on /dev/video4), the pad is already
claimed. This is a legitimate userspace request but not something that
kernel should allow while the pad remains claimed. Hence, the kernel
checks the pipe and returns -EBUSY instead of raising a bug, same as
the other non-origin pads. The check on origin pad's pipe needs to stay
otherwise the origin pad case would go through the increment on the
start_count path, even though there exists a loop few lines below this
block to check the pad and return -EBUSY. The check also provides early
failure detection and prevents the complete graph walk.

Fixes: ae219872834a ("media: mc: entity: Rewrite media_pipeline_start()")
Reported-by: [email protected]
Closes: https://syzkaller.appspot.com/bug?extid=68e901d044baaea1f60c
Tested-by: [email protected]
Signed-off-by: Niharika Khare <[email protected]>
---
 drivers/media/mc/mc-entity.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index 3fa0bc687..7b4ef2c95 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -778,12 +778,18 @@ __must_check int __media_pipeline_start(struct media_pad *origin,
 
 	lockdep_assert_held(&mdev->graph_mutex);
 
-	/*
-	 * If the pad is already part of a pipeline, that pipeline must be the
-	 * same as the pipe given to media_pipeline_start().
-	 */
-	if (WARN_ON(origin->pipe && origin->pipe != pipe))
-		return -EINVAL;
+	/*
+	 * If the pad is part of a different pipeline than the one passed on to
+	 * media_pipeline_start(), the pad must wait for the ongoing session on
+	 * this previous pipeline to complete before it starts a new session.
+	 * The request for starting the pipeline, although valid, cannot be
+	 * served until the ongoing request finishes.
+	 */
+	if (origin->pipe && origin->pipe != pipe) {
+		dev_dbg(mdev->dev, "Failed to start pipeline: pad '%s':%u busy\n",
+			origin->entity->name, origin->index);
+		return -EBUSY;
+	}
 
 	/*
 	 * If the pipeline has already been started, it is guaranteed to be
-- 
2.53.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.