[PATCH AUTOSEL 6.19-6.6] media: v4l2-async: Fix error handling on steps after finding a match

[Sasha Levin <sashal@kernel.org>]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

[ Upstream commit 7345d6d356336c448d6b9230ed8704f39679fd12 ]

Once an async connection is found to be matching with an fwnode, a
sub-device may be registered (in case it wasn't already), its bound
operation is called, ancillary links are created, the async connection
is added to the sub-device's list of connections and removed from the
global waiting connection list. Further on, the sub-device's possible own
notifier is searched for possible additional matches.

Fix these specific issues:

- If v4l2_async_match_notify() failed before the sub-notifier handling,
  the async connection was unbound and its entry removed from the
  sub-device's async connection list. The latter part was also done in
  v4l2_async_match_notify().

- The async connection's sd field was only set after creating ancillary
  links in v4l2_async_match_notify(). It was however dereferenced in
  v4l2_async_unbind_subdev_one(), which was called on error path of
  v4l2_async_match_notify() failure.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Tested-by: "Yew, Chang Ching" <chang.ching.yew@intel.com>
Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

## Analysis of media: v4l2-async: Fix error handling on steps after
finding a match

### 1. COMMIT MESSAGE ANALYSIS

The subject explicitly says "Fix error handling" — this is a bug fix.
The commit message describes two specific issues:

1. **Double cleanup of async connection list entry**: When
   `v4l2_async_match_notify()` failed before sub-notifier handling, the
   async connection was unbound and its entry removed from the sub-
   device's async connection list *twice* — once inside
   `v4l2_async_match_notify()` and once in the caller's error path.

2. **NULL pointer dereference**: The `asc->sd` field was only set
   *after* creating ancillary links, but
   `v4l2_async_unbind_subdev_one()` (called on the error path)
   dereferences `asc->sd`. If `v4l2_async_match_notify()` failed before
   `asc->sd` was assigned, this would cause a NULL pointer dereference.

These are real bugs — a double list_del (list corruption) and a NULL
pointer dereference (crash).

### 2. CODE CHANGE ANALYSIS

The patch restructures `v4l2_async_match_notify()` by:

**a) Splitting the function**: The sub-notifier discovery logic is
extracted into a new function `v4l2_async_nf_try_subdev_notifier()`.
This separates concerns so that `v4l2_async_match_notify()` handles only
the matching/binding, and the sub-notifier logic is called separately by
the caller.

**b) Fixing error paths in `v4l2_async_match_notify()`**:
- The error labels are moved to *after* the success return (`return 0`),
  ensuring that on success the function returns cleanly without falling
  through to error handling.
- The `err_call_unbind` label properly calls
  `v4l2_async_nf_call_unbind()` and removes the list entry.
- The `err_unregister_subdev` label only unregisters if `registered` is
  true.
- Crucially, `asc->sd` is set *before* the error labels, so
  `v4l2_async_unbind_subdev_one()` in the caller won't dereference NULL.

**c) Fixing caller error handling in `__v4l2_async_register_subdev()`**:
- New error labels: `err_unlock` (just unlocks mutex) and
  `err_unbind_one` (unbinds single connection).
- If `v4l2_async_match_notify()` fails, it jumps to `err_unlock` — the
  function already cleaned up internally.
- If `v4l2_async_nf_try_subdev_notifier()` fails, it jumps to
  `err_unbind_one`.
- If `v4l2_async_nf_try_complete()` fails, it jumps to `err_unbind`
  (full unbind).
- The redundant `if (asc)` check is removed since `asc` is always valid
  at that point.

**d) Same pattern in `v4l2_async_nf_try_all_subdevs()`**: The sub-
notifier call is now done separately after `v4l2_async_match_notify()`.

### 3. BUG CLASSIFICATION

- **NULL pointer dereference**: `asc->sd` dereferenced when NULL — this
  is a crash bug.
- **List corruption / double cleanup**: Removing an entry from a list
  twice can corrupt the list, leading to crashes or undefined behavior.

Both are serious bugs that can cause kernel crashes.

### 4. SCOPE AND RISK ASSESSMENT

- **Files changed**: 1 file (`drivers/media/v4l2-core/v4l2-async.c`)
- **Nature of changes**: Refactoring a function into two pieces and
  fixing error handling labels
- **Lines changed**: Moderate (~80 lines of diff), but the logic changes
  are focused on error paths
- **Risk**: Medium — the refactoring is non-trivial as it splits a
  function and changes error flow in two call sites. However, the logic
  is straightforward and the changes are well-contained.

### 5. USER IMPACT

The V4L2 async framework is used by many camera/media subsystems,
especially on embedded platforms (ARM SoCs with ISPs). The bugs are
triggered on error paths during device probe, which can happen when:
- Hardware initialization fails
- Firmware nodes don't match properly
- Resource allocation fails

This affects embedded/SoC users running stable kernels (Android,
embedded Linux).

### 6. STABILITY INDICATORS

- **Tested-by**: "Yew, Chang Ching" — indicates real testing
- **Author**: Sakari Ailus — a well-known V4L2/media subsystem
  maintainer
- **Signed-off by**: Hans Verkuil — the V4L2 subsystem maintainer
- Both bugs are clearly identified and the fix is logical

### 7. DEPENDENCY CHECK

The patch is self-contained within `v4l2-async.c`. It refactors existing
code rather than depending on new APIs. The main concern would be
whether the code being patched exists in older stable trees in the same
form. The `v4l2_async_connection` type and the specific function
signatures suggest this is relatively recent code, but the fix should
apply to any tree containing these functions.

### 8. CONCERNS

- The patch is somewhat large due to the function extraction, not just a
  simple one-liner. However, the refactoring is necessary to properly
  separate the error handling concerns.
- The function split makes the error handling cleaner and more correct,
  which is exactly what stable trees need.
- The changes need careful backporting review to ensure they apply
  cleanly to each target stable tree.

### CONCLUSION

This commit fixes two real bugs: a NULL pointer dereference and a double
list cleanup (list corruption). Both can cause kernel crashes. The fix
is authored and reviewed by the V4L2 subsystem maintainers, has been
tested, and is contained to a single file. While the patch is moderately
sized due to the function extraction, the changes are logically sound
and necessary to fix the error handling properly. The bugs affect real
embedded/media users.

**YES**

 drivers/media/v4l2-core/v4l2-async.c | 45 +++++++++++++++++++---------
 1 file changed, 31 insertions(+), 14 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index ee884a8221fbd..1c08bba9ecb91 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -343,7 +343,6 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
 				   struct v4l2_subdev *sd,
 				   struct v4l2_async_connection *asc)
 {
-	struct v4l2_async_notifier *subdev_notifier;
 	bool registered = false;
 	int ret;
 
@@ -389,6 +388,25 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
 	dev_dbg(notifier_dev(notifier), "v4l2-async: %s bound (ret %d)\n",
 		dev_name(sd->dev), ret);
 
+	return 0;
+
+err_call_unbind:
+	v4l2_async_nf_call_unbind(notifier, sd, asc);
+	list_del(&asc->asc_subdev_entry);
+
+err_unregister_subdev:
+	if (registered)
+		v4l2_device_unregister_subdev(sd);
+
+	return ret;
+}
+
+static int
+v4l2_async_nf_try_subdev_notifier(struct v4l2_async_notifier *notifier,
+				  struct v4l2_subdev *sd)
+{
+	struct v4l2_async_notifier *subdev_notifier;
+
 	/*
 	 * See if the sub-device has a notifier. If not, return here.
 	 */
@@ -404,16 +422,6 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
 	subdev_notifier->parent = notifier;
 
 	return v4l2_async_nf_try_all_subdevs(subdev_notifier);
-
-err_call_unbind:
-	v4l2_async_nf_call_unbind(notifier, sd, asc);
-	list_del(&asc->asc_subdev_entry);
-
-err_unregister_subdev:
-	if (registered)
-		v4l2_device_unregister_subdev(sd);
-
-	return ret;
 }
 
 /* Test all async sub-devices in a notifier for a match. */
@@ -445,6 +453,10 @@ v4l2_async_nf_try_all_subdevs(struct v4l2_async_notifier *notifier)
 		if (ret < 0)
 			return ret;
 
+		ret = v4l2_async_nf_try_subdev_notifier(notifier, sd);
+		if (ret < 0)
+			return ret;
+
 		/*
 		 * v4l2_async_match_notify() may lead to registering a
 		 * new notifier and thus changing the async subdevs
@@ -829,7 +841,11 @@ int __v4l2_async_register_subdev(struct v4l2_subdev *sd, struct module *module)
 			ret = v4l2_async_match_notify(notifier, v4l2_dev, sd,
 						      asc);
 			if (ret)
-				goto err_unbind;
+				goto err_unlock;
+
+			ret = v4l2_async_nf_try_subdev_notifier(notifier, sd);
+			if (ret)
+				goto err_unbind_one;
 
 			ret = v4l2_async_nf_try_complete(notifier);
 			if (ret)
@@ -853,9 +869,10 @@ int __v4l2_async_register_subdev(struct v4l2_subdev *sd, struct module *module)
 	if (subdev_notifier)
 		v4l2_async_nf_unbind_all_subdevs(subdev_notifier);
 
-	if (asc)
-		v4l2_async_unbind_subdev_one(notifier, asc);
+err_unbind_one:
+	v4l2_async_unbind_subdev_one(notifier, asc);
 
+err_unlock:
 	mutex_unlock(&list_lock);
 
 	sd->owner = NULL;
-- 
2.51.0