[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