Re: [PATCH] media: pvrusb2: fix disconnect and teardown races

[Hans Verkuil <hverkuil+cisco@kernel.org>]

On 4/20/26 08:06, Sangyun Kim wrote:
> pvr2_context_disconnect() queues a notification to the pvrusb2-context
> kthread before it stores mp->disconnect_flag:
> 
>     pvr2_hdw_disconnect(mp->hdw);
>     if (!pvr2_context_shutok())
>         pvr2_context_notify(mp);
>     mp->disconnect_flag = !0;
> 
> The context thread only destroys a context when disconnect_flag is set
> and mc_first is NULL. If the notification wakes the thread before the
> flag store becomes visible, the thread dequeues the context, runs
> pvr2_context_check() with disconnect_flag still observed as 0, decides
> that the destroy condition is not met yet, and goes back to sleep.
> Nothing wakes the thread again once the flag is finally stored, so the
> context stays on the global exist list forever and
> pvr2_context_global_done() blocks module unload. commit 0a0b79ea55de
> ("media: pvrusb2: fix uaf in pvr2_context_set_notify") made this
> liveness failure easier to hit by moving the notify earlier in the
> disconnect path.
> 
> The same teardown sequence still contains a use-after-free.
> pvr2_context_exit() inspects disconnect_flag after releasing mp->mutex
> and may then call pvr2_context_notify(mp) after the context thread has
> already freed the object via pvr2_context_destroy()/kfree(). The hdw
> completion callback registered through pvr2_hdw_initialize() can race
> the same way. Reordering the disconnect path alone closes the unload
> hang, but it still leaves late notifiers able to touch freed memory.
> 
> Fix both problems together:
> 
> - Split pvr2_context_set_notify() into a locked helper
>   (pvr2_context_set_notify_locked()) and a wrapper that acquires
>   pvr2_context_mutex. This lets callers update several pieces of
>   related state inside a single critical section without relocking.
> 
> - In pvr2_context_disconnect(), set disconnect_flag and enqueue the
>   thread notification under pvr2_context_mutex. The context thread
>   manipulates the notify list under the same mutex, so when it observes
>   the queued entry it is guaranteed to observe disconnect_flag = 1 as
>   well and the destroy condition evaluates correctly. This eliminates
>   the original notify-before-flag liveness hole.
> 
> - Add a per-context refcount_t. pvr2_context_create() initialises it to
>   1 (creator reference). pvr2_channel_init() and pvr2_channel_done()
>   take and drop a reference around each channel's lifetime.
>   pvr2_context_disconnect() takes a temporary reference across its body
>   so the context cannot be freed while disconnect is still touching it.
>   pvr2_context_destroy() no longer calls kfree() directly; it drops its
>   reference via pvr2_context_put(), and whichever caller drops the last
>   reference performs the actual kfree. This keeps the object alive
>   until disconnect and the final channel teardown finish, regardless of
>   how the context thread, channel close, and USB disconnect paths
>   interleave.
> 
> - Add a destroying_flag that pvr2_context_destroy() sets under
>   pvr2_context_mutex before unlinking the context from the notify and
>   exist lists. pvr2_context_set_notify_locked() refuses to re-enqueue a
>   context whose destroying_flag is set, so a late notifier arriving
>   after destroy has started cannot resurrect the context on the notify
>   list. The dequeue path (fl == 0) still proceeds unconditionally
>   because destroy itself must be able to remove any still-queued entry.
> 
> - Update pvr2_context_exit() to enqueue through
>   pvr2_context_set_notify_locked() after releasing mp->mutex. The
>   caller (channel close or disconnect) always holds a reference, so the
>   object is stable across the mp->mutex / pvr2_context_mutex hand-off
>   and a concurrent destroy cannot free it under us. If destroy has
>   already won the race, destroying_flag short-circuits the enqueue into
>   a no-op.
> 
> Lock ordering: pvr2_context_mutex is only acquired after mp->mutex is
> released; no path holds pvr2_context_mutex while acquiring mp->mutex,
> so no AB/BA deadlock is introduced. wake_up() on
> pvr2_context_sync_data is moved outside pvr2_context_mutex in every
> path that grew a new locked section, matching the existing style.

Huge commit log, weird patch. All the hallmarks of AI slop.

Rejected.

Regards,

	Hans

> 
> Fixes: 0a0b79ea55de ("media: pvrusb2: fix uaf in pvr2_context_set_notify")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sangyun Kim <sangyun.kim@snu.ac.kr>
> ---
>  drivers/media/usb/pvrusb2/pvrusb2-context.c | 56 ++++++++++++++++++---
>  drivers/media/usb/pvrusb2/pvrusb2-context.h |  3 ++
>  2 files changed, 51 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-context.c b/drivers/media/usb/pvrusb2/pvrusb2-context.c
> index 93f5da65ead9..fb9bdbf5c886 100644
> --- a/drivers/media/usb/pvrusb2/pvrusb2-context.c
> +++ b/drivers/media/usb/pvrusb2/pvrusb2-context.c
> @@ -27,11 +27,19 @@ static int pvr2_context_cleaned_flag;
>  static struct task_struct *pvr2_context_thread_ptr;
>  
>  
> -static void pvr2_context_set_notify(struct pvr2_context *mp, int fl)
> +static void pvr2_context_put(struct pvr2_context *mp)
> +{
> +	if (refcount_dec_and_test(&mp->refcount))
> +		kfree(mp);
> +}
> +
> +static int pvr2_context_set_notify_locked(struct pvr2_context *mp, int fl)
>  {
>  	int signal_flag = 0;
> -	mutex_lock(&pvr2_context_mutex);
> +
>  	if (fl) {
> +		if (mp->destroying_flag)
> +			return 0;
>  		if (!mp->notify_flag) {
>  			signal_flag = (pvr2_context_notify_first == NULL);
>  			mp->notify_prev = pvr2_context_notify_last;
> @@ -59,6 +67,15 @@ static void pvr2_context_set_notify(struct pvr2_context *mp, int fl)
>  			}
>  		}
>  	}
> +	return signal_flag;
> +}
> +
> +static void pvr2_context_set_notify(struct pvr2_context *mp, int fl)
> +{
> +	int signal_flag = 0;
> +
> +	mutex_lock(&pvr2_context_mutex);
> +	signal_flag = pvr2_context_set_notify_locked(mp, fl);
>  	mutex_unlock(&pvr2_context_mutex);
>  	if (signal_flag) wake_up(&pvr2_context_sync_data);
>  }
> @@ -66,10 +83,13 @@ static void pvr2_context_set_notify(struct pvr2_context *mp, int fl)
>  
>  static void pvr2_context_destroy(struct pvr2_context *mp)
>  {
> +	int signal_flag = 0;
> +
>  	pvr2_trace(PVR2_TRACE_CTXT,"pvr2_context %p (destroy)",mp);
>  	pvr2_hdw_destroy(mp->hdw);
> -	pvr2_context_set_notify(mp, 0);
>  	mutex_lock(&pvr2_context_mutex);
> +	mp->destroying_flag = !0;
> +	pvr2_context_set_notify_locked(mp, 0);
>  	if (mp->exist_next) {
>  		mp->exist_next->exist_prev = mp->exist_prev;
>  	} else {
> @@ -83,10 +103,12 @@ static void pvr2_context_destroy(struct pvr2_context *mp)
>  	if (!pvr2_context_exist_first) {
>  		/* Trigger wakeup on control thread in case it is waiting
>  		   for an exit condition. */
> -		wake_up(&pvr2_context_sync_data);
> +		signal_flag = !0;
>  	}
>  	mutex_unlock(&pvr2_context_mutex);
> -	kfree(mp);
> +	if (signal_flag)
> +		wake_up(&pvr2_context_sync_data);
> +	pvr2_context_put(mp);
>  }
>  
>  
> @@ -209,6 +231,7 @@ struct pvr2_context *pvr2_context_create(
>  	pvr2_trace(PVR2_TRACE_CTXT,"pvr2_context %p (create)",mp);
>  	mp->setup_func = setup_func;
>  	mutex_init(&mp->mutex);
> +	refcount_set(&mp->refcount, 1);
>  	mutex_lock(&pvr2_context_mutex);
>  	mp->exist_prev = pvr2_context_exist_last;
>  	mp->exist_next = NULL;
> @@ -256,25 +279,41 @@ static void pvr2_context_enter(struct pvr2_context *mp)
>  static void pvr2_context_exit(struct pvr2_context *mp)
>  {
>  	int destroy_flag = 0;
> +	int signal_flag = 0;
>  	if (!(mp->mc_first || !mp->disconnect_flag)) {
>  		destroy_flag = !0;
>  	}
>  	mutex_unlock(&mp->mutex);
> -	if (destroy_flag) pvr2_context_notify(mp);
> +	if (destroy_flag) {
> +		mutex_lock(&pvr2_context_mutex);
> +		signal_flag = pvr2_context_set_notify_locked(mp, !0);
> +		mutex_unlock(&pvr2_context_mutex);
> +		if (signal_flag)
> +			wake_up(&pvr2_context_sync_data);
> +	}
>  }
>  
>  
>  void pvr2_context_disconnect(struct pvr2_context *mp)
>  {
> +	int signal_flag = 0;
> +
> +	refcount_inc(&mp->refcount);
>  	pvr2_hdw_disconnect(mp->hdw);
> -	if (!pvr2_context_shutok())
> -		pvr2_context_notify(mp);
> +	mutex_lock(&pvr2_context_mutex);
>  	mp->disconnect_flag = !0;
> +	if (!pvr2_context_shutok())
> +		signal_flag = pvr2_context_set_notify_locked(mp, !0);
> +	mutex_unlock(&pvr2_context_mutex);
> +	if (signal_flag)
> +		wake_up(&pvr2_context_sync_data);
> +	pvr2_context_put(mp);
>  }
>  
>  
>  void pvr2_channel_init(struct pvr2_channel *cp,struct pvr2_context *mp)
>  {
> +	refcount_inc(&mp->refcount);
>  	pvr2_context_enter(mp);
>  	cp->hdw = mp->hdw;
>  	cp->mc_head = mp;
> @@ -318,6 +357,7 @@ void pvr2_channel_done(struct pvr2_channel *cp)
>  	}
>  	cp->hdw = NULL;
>  	pvr2_context_exit(mp);
> +	pvr2_context_put(mp);
>  }
>  
>  
> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-context.h b/drivers/media/usb/pvrusb2/pvrusb2-context.h
> index 5840b2ce8f1e..4e06530eccb8 100644
> --- a/drivers/media/usb/pvrusb2/pvrusb2-context.h
> +++ b/drivers/media/usb/pvrusb2/pvrusb2-context.h
> @@ -7,6 +7,7 @@
>  #define __PVRUSB2_CONTEXT_H
>  
>  #include <linux/mutex.h>
> +#include <linux/refcount.h>
>  #include <linux/usb.h>
>  #include <linux/workqueue.h>
>  
> @@ -33,9 +34,11 @@ struct pvr2_context {
>  	struct pvr2_hdw *hdw;
>  	struct pvr2_context_stream video_stream;
>  	struct mutex mutex;
> +	refcount_t refcount;
>  	int notify_flag;
>  	int initialized_flag;
>  	int disconnect_flag;
> +	int destroying_flag;
>  
>  	/* Called after pvr2_context initialization is complete */
>  	void (*setup_func)(struct pvr2_context *);