Re: [PATCH v3] media: dvb-core: skip disable_source handler for a released entity

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

On 02/11/2026 15:34, YuanZhe Liu wrote:
> dvb_remove_device() frees the media entity of a dvb_device through
> dvb_media_device_free() and sets dvbdev->entity to NULL.  The media
> device of the adapter can still be registered at that point: au0828,
> for example, only clears mdev->disable_source in au0828_usb_release(),
> which runs after au0828_dvb_unregister() has unregistered the frontend
> device.
> 
> A file descriptor that survives the disconnect then reaches
> dvb_frontend_release() with dvbdev->entity already NULL and calls
> 
> 	fe->dvb->mdev->disable_source(dvbdev->entity);
> 
> au0828_disable_source() dereferences that argument unconditionally:
> 
> 	struct media_device *mdev = entity->graph_obj.mdev;
> 
> which faults:
> 
> 	general protection fault: 0000 [#1] SMP KASAN
> 	KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
> 	RIP: au0828_disable_source+0x27/0xb70 au0828-core.c:445
> 	Call Trace:
> 	 dvb_frontend_release+0x2eb/0x500 dvb_frontend.c:2946
> 
> The error path of __dvb_frontend_open() has the same call.
> 
> Skip the handler when dvbdev->entity is gone.  For that check to be
> sufficient, clear dvbdev->entity in dvb_media_device_free() before the
> entity is unregistered and freed: both frontend paths read the pointer
> while holding mdev->graph_mutex, and media_device_unregister_entity()
> takes the same lock, so a reader either still sees a registered entity,
> for which unregister waits, or sees NULL.  Clearing the pointer after
> the unregister would leave a window in which the entity is read after
> it was already freed.
> 
> Read the pointer only once, into a local variable, while the lock is
> held.  dvb_media_device_free() stores NULL before taking graph_mutex,
> so re-reading the field to pass it as the argument could still observe
> NULL and hand it to the handler.  That store is not serialised with the
> locked readers either, so publish the pointer with WRITE_ONCE() and
> read it with READ_ONCE().
> 
> Stopping the pipeline the handler started is not done here.  Only the
> driver knows how to release the link it owns, and dvb_media_device_free()
> runs for every dvb_device, including the demux, dvr and ca entities that
> never own a media link; calling the handler for those would deactivate a
> link owned by another entity.  The au0828 driver stops the pipeline and
> clears its saved link in au0828_usb_disconnect(), before the frontend
> entity is unregistered, in a separate patch.
> 
> Fixes: 68ff5767ddc4 ("[media] media: dvb-frontend invoke enable/disable_source handlers")
> Cc: stable@vger.kernel.org
> Co-developed-by: Bin Yu <byu@xidian.edu.cn>
> Signed-off-by: Bin Yu <byu@xidian.edu.cn>
> Co-developed-by: Cheng Wen <wencheng@xidian.edu.cn>
> Signed-off-by: Cheng Wen <wencheng@xidian.edu.cn>
> Signed-off-by: YuanZhe Liu <25031212351@stu.xidian.edu.cn>
> ---
> Changes in v3:
> - Publish dvbdev->entity with WRITE_ONCE() and read it with READ_ONCE()
>   at both frontend call sites, so the clear in dvb_media_device_free()
>   is not a plain data race with the readers that hold graph_mutex.
> - State why the active pipeline is torn down by the driver rather than
>   in dvb-core.  The matching au0828 change is attached to this mail.
> 
> Link: https://lore.kernel.org/r/20261102142508.24277-1-25031212351@stu.xidian.edu.cn
> 
> Changes in v2:
> - Read dvbdev->entity once into a local variable under graph_mutex at both
>   call sites.  dvb_media_device_free() clears the pointer without holding
>   that lock, so re-reading the field for the call argument could still pass
>   NULL to the handler.
> ---
>  drivers/media/dvb-core/dvb_frontend.c | 14 ++++++++++----
>  drivers/media/dvb-core/dvbdev.c       | 16 +++++++++++++---
>  2 files changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> index 0286da57f382..bcee88e16576 100644
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
> @@ -2826,9 +2826,12 @@ static int __dvb_frontend_open(struct inode *inode, struct file *file)
>  #ifdef CONFIG_MEDIA_CONTROLLER_DVB
>  	mutex_lock(&fe->dvb->mdev_lock);
>  	if (fe->dvb->mdev) {
> +		struct media_entity *entity;
> +
>  		mutex_lock(&fe->dvb->mdev->graph_mutex);
> -		if (fe->dvb->mdev->disable_source)
> -			fe->dvb->mdev->disable_source(dvbdev->entity);
> +		entity = READ_ONCE(dvbdev->entity);
> +		if (entity && fe->dvb->mdev->disable_source)
> +			fe->dvb->mdev->disable_source(entity);
>  		mutex_unlock(&fe->dvb->mdev->graph_mutex);
>  	}
>  	mutex_unlock(&fe->dvb->mdev_lock);
> @@ -2941,9 +2944,12 @@ static int dvb_frontend_release(struct inode *inode, struct file *file)
>  #ifdef CONFIG_MEDIA_CONTROLLER_DVB
>  		mutex_lock(&fe->dvb->mdev_lock);
>  		if (fe->dvb->mdev) {
> +			struct media_entity *entity;
> +
>  			mutex_lock(&fe->dvb->mdev->graph_mutex);
> -			if (fe->dvb->mdev->disable_source)
> -				fe->dvb->mdev->disable_source(dvbdev->entity);
> +			entity = READ_ONCE(dvbdev->entity);
> +			if (entity && fe->dvb->mdev->disable_source)
> +				fe->dvb->mdev->disable_source(entity);
>  			mutex_unlock(&fe->dvb->mdev->graph_mutex);
>  		}
>  		mutex_unlock(&fe->dvb->mdev_lock);
> diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
> index d753d329502a..bb6369c3abde 100644
> --- a/drivers/media/dvb-core/dvbdev.c
> +++ b/drivers/media/dvb-core/dvbdev.c
> @@ -208,10 +208,20 @@ static void dvb_media_device_free(struct dvb_device *dvbdev)
>  {
>  #if defined(CONFIG_MEDIA_CONTROLLER_DVB)
>  	if (dvbdev->entity) {
> -		media_device_unregister_entity(dvbdev->entity);
> -		kfree(dvbdev->entity);
> +		struct media_entity *entity = dvbdev->entity;
> +
> +		/*
> +		 * Clear the pointer before the entity is unregistered and
> +		 * freed.  The frontend release and open paths read it under
> +		 * mdev->graph_mutex and pass it to the disable_source
> +		 * handler, so a stale entity must not stay reachable.  Publish
> +		 * it with WRITE_ONCE() because the store is not serialised by
> +		 * that lock.
> +		 */
> +		WRITE_ONCE(dvbdev->entity, NULL);

I think this can be simplified: just take call mutex_lock(&fe->dvb->mdev->graph_mutex),
then set dvbdev->entity to NULL and unlock.

In __dvb_frontend_open and dvb_frontend_release you can just check if dvbdev->entity is
non-NULL before calling disable_source.

You basically say that in the comment you added: the store is not serialized by
the lock, so just add that lock here.

Regards,

	Hans

> +		media_device_unregister_entity(entity);
> +		kfree(entity);
>  		kfree(dvbdev->pads);
> -		dvbdev->entity = NULL;
>  		dvbdev->pads = NULL;
>  	}
>