[PATCH] media: subdev: Fix validation state lockdep issue

[PATCH] media: subdev: Fix validation state lockdep issue

[Tomi Valkeinen]

The new subdev state code has a possible deadlock scenario during link
validation when the pipeline contains subdevs that support state and
that do not support state.

The current code locks the states of the subdevs on both ends of the
link when starting the link validation, locking the sink side first,
then the source. If either (or both) of the subdevs does not support
state, nothing is done for that subdev at this point, and instead the
locking is handled the old way, i.e. the subdev's ops do the locking
internally.

The issue arises when the sink doesn't support state, but source does,
so the validation code locks the source for the duration of the
validation, and then the sink is locked only when the get_fmt op is
called. So lockdep sees the source locked first, then the sink.

Later, when the streaming is started, the sink's s_stream op is called,
which probably takes the subdev's lock. The op then calls the source's
s_stream, which takes the source's lock. So, the sink is locked first,
then the source.

Note that link validation and stream starting is not done at the same
time, so an actual deadlock should never happen. However, it's still a
clear bug.

Fix this by locking the subdev states only if both subdevs support
state. In other words, we have two scenarios:

1. Both subdevs support state. Lock sink first, then source, and keep
   the locks while validating the link.
2. At least one of the subdevs do not support state. Take the lock only
   for the duration of the operation (get_fmt or looking at the
   routing), and release after the op is done.

Obviously 1. is better, as we have a more consistent view of the states
of the subdevs during validation. 2. is how it has been so far, so it's
no worse than this used to be.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 82 +++++++++++++++++----------
 1 file changed, 52 insertions(+), 30 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index dff1d9be7841..ceee50694c24 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -1057,32 +1057,45 @@ EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate_default);
 
 static int
 v4l2_subdev_link_validate_get_format(struct media_pad *pad, u32 stream,
-				     struct v4l2_subdev_format *fmt)
+				     struct v4l2_subdev_format *fmt,
+				     bool states_locked)
 {
-	if (is_media_entity_v4l2_subdev(pad->entity)) {
-		struct v4l2_subdev *sd =
-			media_entity_to_v4l2_subdev(pad->entity);
+	struct v4l2_subdev_state *state;
+	struct v4l2_subdev *sd;
+	int ret;
 
-		fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE;
-		fmt->pad = pad->index;
-		fmt->stream = stream;
+	if (!is_media_entity_v4l2_subdev(pad->entity)) {
+		WARN(pad->entity->function != MEDIA_ENT_F_IO_V4L,
+		     "Driver bug! Wrong media entity type 0x%08x, entity %s\n",
+		     pad->entity->function, pad->entity->name);
 
-		return v4l2_subdev_call(sd, pad, get_fmt,
-					v4l2_subdev_get_locked_active_state(sd),
-					fmt);
+		return -EINVAL;
 	}
 
-	WARN(pad->entity->function != MEDIA_ENT_F_IO_V4L,
-	     "Driver bug! Wrong media entity type 0x%08x, entity %s\n",
-	     pad->entity->function, pad->entity->name);
+	sd = media_entity_to_v4l2_subdev(pad->entity);
 
-	return -EINVAL;
+	fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE;
+	fmt->pad = pad->index;
+	fmt->stream = stream;
+
+	if (states_locked)
+		state = v4l2_subdev_get_locked_active_state(sd);
+	else
+		state = v4l2_subdev_lock_and_get_active_state(sd);
+
+	ret = v4l2_subdev_call(sd, pad, get_fmt, state, fmt);
+
+	if (!states_locked && state)
+		v4l2_subdev_unlock_state(state);
+
+	return ret;
 }
 
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 
 static void __v4l2_link_validate_get_streams(struct media_pad *pad,
-					     u64 *streams_mask)
+					     u64 *streams_mask,
+					     bool states_locked)
 {
 	struct v4l2_subdev_route *route;
 	struct v4l2_subdev_state *state;
@@ -1092,7 +1105,11 @@ static void __v4l2_link_validate_get_streams(struct media_pad *pad,
 
 	*streams_mask = 0;
 
-	state = v4l2_subdev_get_locked_active_state(subdev);
+	if (states_locked)
+		state = v4l2_subdev_get_locked_active_state(subdev);
+	else
+		state = v4l2_subdev_lock_and_get_active_state(subdev);
+
 	if (WARN_ON(!state))
 		return;
 
@@ -1113,12 +1130,16 @@ static void __v4l2_link_validate_get_streams(struct media_pad *pad,
 
 		*streams_mask |= BIT_ULL(route_stream);
 	}
+
+	if (!states_locked)
+		v4l2_subdev_unlock_state(state);
 }
 
 #endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */
 
 static void v4l2_link_validate_get_streams(struct media_pad *pad,
-					   u64 *streams_mask)
+					   u64 *streams_mask,
+					   bool states_locked)
 {
 	struct v4l2_subdev *subdev = media_entity_to_v4l2_subdev(pad->entity);
 
@@ -1129,14 +1150,14 @@ static void v4l2_link_validate_get_streams(struct media_pad *pad,
 	}
 
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
-	__v4l2_link_validate_get_streams(pad, streams_mask);
+	__v4l2_link_validate_get_streams(pad, streams_mask, states_locked);
 #else
 	/* This shouldn't happen */
 	*streams_mask = 0;
 #endif
 }
 
-static int v4l2_subdev_link_validate_locked(struct media_link *link)
+static int v4l2_subdev_link_validate_locked(struct media_link *link, bool states_locked)
 {
 	struct v4l2_subdev *sink_subdev =
 		media_entity_to_v4l2_subdev(link->sink->entity);
@@ -1151,8 +1172,8 @@ static int v4l2_subdev_link_validate_locked(struct media_link *link)
 		link->source->entity->name, link->source->index,
 		link->sink->entity->name, link->sink->index);
 
-	v4l2_link_validate_get_streams(link->source, &source_streams_mask);
-	v4l2_link_validate_get_streams(link->sink, &sink_streams_mask);
+	v4l2_link_validate_get_streams(link->source, &source_streams_mask, states_locked);
+	v4l2_link_validate_get_streams(link->sink, &sink_streams_mask, states_locked);
 
 	/*
 	 * It is ok to have more source streams than sink streams as extra
@@ -1180,7 +1201,7 @@ static int v4l2_subdev_link_validate_locked(struct media_link *link)
 			link->sink->entity->name, link->sink->index, stream);
 
 		ret = v4l2_subdev_link_validate_get_format(link->source, stream,
-							   &source_fmt);
+							   &source_fmt, states_locked);
 		if (ret < 0) {
 			dev_dbg(dev,
 				"Failed to get format for \"%s\":%u:%u (but that's ok)\n",
@@ -1190,7 +1211,7 @@ static int v4l2_subdev_link_validate_locked(struct media_link *link)
 		}
 
 		ret = v4l2_subdev_link_validate_get_format(link->sink, stream,
-							   &sink_fmt);
+							   &sink_fmt, states_locked);
 		if (ret < 0) {
 			dev_dbg(dev,
 				"Failed to get format for \"%s\":%u:%u (but that's ok)\n",
@@ -1222,6 +1243,7 @@ int v4l2_subdev_link_validate(struct media_link *link)
 {
 	struct v4l2_subdev *source_sd, *sink_sd;
 	struct v4l2_subdev_state *source_state, *sink_state;
+	bool states_locked;
 	int ret;
 
 	sink_sd = media_entity_to_v4l2_subdev(link->sink->entity);
@@ -1230,19 +1252,19 @@ int v4l2_subdev_link_validate(struct media_link *link)
 	sink_state = v4l2_subdev_get_unlocked_active_state(sink_sd);
 	source_state = v4l2_subdev_get_unlocked_active_state(source_sd);
 
-	if (sink_state)
-		v4l2_subdev_lock_state(sink_state);
+	states_locked = sink_state && source_state;
 
-	if (source_state)
+	if (states_locked) {
+		v4l2_subdev_lock_state(sink_state);
 		v4l2_subdev_lock_state(source_state);
+	}
 
-	ret = v4l2_subdev_link_validate_locked(link);
+	ret = v4l2_subdev_link_validate_locked(link, states_locked);
 
-	if (sink_state)
+	if (states_locked) {
 		v4l2_subdev_unlock_state(sink_state);
-
-	if (source_state)
 		v4l2_subdev_unlock_state(source_state);
+	}
 
 	return ret;
 }
-- 
2.34.1

Re: [PATCH] media: subdev: Fix validation state lockdep issue

[Sakari Ailus]

Moi,

On Fri, Mar 03, 2023 at 05:52:49PM +0200, Tomi Valkeinen wrote:
> The new subdev state code has a possible deadlock scenario during link
> validation when the pipeline contains subdevs that support state and
> that do not support state.
> 
> The current code locks the states of the subdevs on both ends of the
> link when starting the link validation, locking the sink side first,
> then the source. If either (or both) of the subdevs does not support
> state, nothing is done for that subdev at this point, and instead the
> locking is handled the old way, i.e. the subdev's ops do the locking
> internally.
> 
> The issue arises when the sink doesn't support state, but source does,
> so the validation code locks the source for the duration of the
> validation, and then the sink is locked only when the get_fmt op is
> called. So lockdep sees the source locked first, then the sink.
> 
> Later, when the streaming is started, the sink's s_stream op is called,
> which probably takes the subdev's lock. The op then calls the source's
> s_stream, which takes the source's lock. So, the sink is locked first,
> then the source.
> 
> Note that link validation and stream starting is not done at the same
> time, so an actual deadlock should never happen. However, it's still a
> clear bug.
> 
> Fix this by locking the subdev states only if both subdevs support
> state. In other words, we have two scenarios:
> 
> 1. Both subdevs support state. Lock sink first, then source, and keep
>    the locks while validating the link.
> 2. At least one of the subdevs do not support state. Take the lock only
>    for the duration of the operation (get_fmt or looking at the
>    routing), and release after the op is done.
> 
> Obviously 1. is better, as we have a more consistent view of the states
> of the subdevs during validation. 2. is how it has been so far, so it's
> no worse than this used to be.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Thanks for the patch, Tomi!

I'll add this to my next fixes PR, probably tomorrow, unless there are
objections. I don't think this is a false positive: the same locks may well
be used for the same purpose in different video nodes.

-- 
Terveisin,

Sakari Ailus