Re: [PATCH v2 7/9] media: v4l2-device: wait for notifications when unregistering a subdev

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] v4l2: NULL pointer deref or UAF in v4l2_subdev_notify()
--

commit 6588945fb74790836c51288e07f21eed874e7fe8
Author: Sascha Hauer <s.hauer@pengutronix.de>

media: v4l2-device: wait for notifications when unregistering a subdev

This commit introduces SRCU protection for v4l2_subdev_notify() to prevent
race conditions with subdevice unregistration. It adds a boolean flag and an
SRCU read-side critical section to ensure the callback doesn't run after the
bridge driver unbinds.

> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
> --- a/drivers/media/v4l2-core/v4l2-device.c
> +++ b/drivers/media/v4l2-core/v4l2-device.c
[ ... ]
> +/*
> + * Stop notifications to sd->v4l2_dev and wait for those in progress.
> + * Callbacks read sd->v4l2_dev, so it must stay set until this returns.
> + */
> +static void v4l2_subdev_disable_notify(struct v4l2_subdev *sd)
> +{
> +	WRITE_ONCE(sd->notify_enabled, false);
> +	if (sd->v4l2_dev->notify)
> +		synchronize_srcu(&v4l2_subdev_notify_srcu);
> +}
> +

[Severity: High]
If the bridge driver does not provide a notify callback, this skips the SRCU
grace period. Could this break synchronization for a concurrent
v4l2_subdev_notify() reader that just observed notify_enabled as true?

[ ... ]
> +void v4l2_subdev_notify(struct v4l2_subdev *sd, unsigned int notification,
> +			void *arg)
> +{
> +	struct v4l2_device *v4l2_dev;
> +	int idx;
> +
> +	if (!sd)
> +		return;
> +
> +	idx = srcu_read_lock(&v4l2_subdev_notify_srcu);
> +	/* Pairs with smp_store_release() in __v4l2_device_register_subdev() */
> +	if (smp_load_acquire(&sd->notify_enabled)) {
> +		v4l2_dev = sd->v4l2_dev;
> +		if (v4l2_dev->notify)

[Severity: High]
Since the reader unconditionally dereferences v4l2_dev here regardless of
whether the callback is provided, could this lead to a NULL pointer 
dereference or use-after-free?

If a concurrent call to v4l2_subdev_disable_notify() skips the
synchronize_srcu() call because notify is NULL, the unregistering thread
will proceed to clear sd->v4l2_dev and potentially free the v4l2_device. 

If this reader is preempted right after checking notify_enabled, wouldn't
it wake up and dereference a NULL or dangling v4l2_dev pointer?

> +			v4l2_dev->notify(sd, notification, arg);
> +	}
> +	srcu_read_unlock(&v4l2_subdev_notify_srcu, idx);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_notify);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-hdmirx-media-v2-0-c12f641d8b0f@pengutronix.de?part=7