linux-kernel - Re: [PATCH v2 2/2] media: drivers/media/dvb-core: Refactor dvb_frontend

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240816101642.5ef4e461@foz.lan>
Date: Fri, 16 Aug 2024 10:16:51 +0200
From: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To: Ricardo Ribalda <ribalda@...omium.org>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>, linux-media@...r.kernel.org,
 linux-kernel@...r.kernel.org, Hans Verkuil <hverkuil-cisco@...all.nl>
Subject: Re: [PATCH v2 2/2] media: drivers/media/dvb-core: Refactor
 dvb_frontend_open locking

Em Wed, 14 Aug 2024 14:10:23 +0000
Ricardo Ribalda <ribalda@...omium.org> escreveu:

> Split out the wait function, and introduce some new toys: guard and
> lockdep.
> 
> This fixes the following cocci warnings:
> drivers/media/dvb-core/dvb_frontend.c:2897:1-7: preceding lock on line 2776
> drivers/media/dvb-core/dvb_frontend.c:2897:1-7: preceding lock on line 2786
> drivers/media/dvb-core/dvb_frontend.c:2897:1-7: preceding lock on line 2809

Hi Ricardo,

Every time someone tries to fix this lock, we end having regression reports,
because of the diversity of devices, and the way they registers there.

That's specially true for devices with multiple frontends and custom
zigzag methods.

On what devices have you tested this patch?

Regards,
Mauro

> 
> Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> ---
>  drivers/media/dvb-core/dvb_frontend.c | 58 ++++++++++++++++++++++-------------
>  1 file changed, 37 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> index e81b9996530e..7f5d0c297464 100644
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
> @@ -30,6 +30,7 @@
>  #include <linux/kthread.h>
>  #include <linux/ktime.h>
>  #include <linux/compat.h>
> +#include <linux/lockdep.h>
>  #include <asm/processor.h>
>  
>  #include <media/dvb_frontend.h>
> @@ -2826,6 +2827,34 @@ static int __dvb_frontend_open(struct inode *inode, struct file *file)
>  	return ret;
>  }
>  
> +static int wait_dvb_frontend(struct dvb_adapter *adapter,
> +			     struct dvb_device *mfedev)
> +{
> +	struct dvb_frontend *mfe = mfedev->priv;
> +	struct dvb_frontend_private *mfepriv = mfe->frontend_priv;
> +	int mferetry = (dvb_mfe_wait_time << 1);
> +	int ret = 0;
> +
> +	lockdep_assert_held(&adapter->mfe_lock);
> +
> +	if (mfedev->users == -1 && !mfepriv->thread)
> +		return 0;
> +
> +	mutex_unlock(&adapter->mfe_lock);
> +
> +	while (mferetry-- && (mfedev->users != -1 || mfepriv->thread)) {
> +		if (msleep_interruptible(500))
> +			if (signal_pending(current)) {
> +				ret = -EINTR;
> +				break;
> +			}
> +	}
> +
> +	mutex_lock(&adapter->mfe_lock);
> +
> +	return ret;
> +}
> +
>  static int dvb_frontend_open(struct inode *inode, struct file *file)
>  {
>  	struct dvb_device *dvbdev = file->private_data;
> @@ -2840,19 +2869,16 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)
>  	if (!adapter->mfe_shared)
>  		return __dvb_frontend_open(inode, file);
>  
> +	guard(mutex)(&adapter->mfe_lock);
> +
>  	if (adapter->mfe_shared == 2) {
> -		mutex_lock(&adapter->mfe_lock);
>  		if ((file->f_flags & O_ACCMODE) != O_RDONLY) {
>  			if (adapter->mfe_dvbdev &&
> -			    !adapter->mfe_dvbdev->writers) {
> -				mutex_unlock(&adapter->mfe_lock);
> +			    !adapter->mfe_dvbdev->writers)
>  				return -EBUSY;
> -			}
>  			adapter->mfe_dvbdev = dvbdev;
>  		}
>  	} else {
> -		mutex_lock(&adapter->mfe_lock);
> -
>  		if (!adapter->mfe_dvbdev) {
>  			adapter->mfe_dvbdev = dvbdev;
>  		} else if (adapter->mfe_dvbdev != dvbdev) {
> @@ -2862,34 +2888,24 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)
>  				*mfe = mfedev->priv;
>  			struct dvb_frontend_private
>  				*mfepriv = mfe->frontend_priv;
> -			int mferetry = (dvb_mfe_wait_time << 1);
> -
> -			mutex_unlock(&adapter->mfe_lock);
> -			while (mferetry-- && (mfedev->users != -1 ||
> -					      mfepriv->thread)) {
> -				if (msleep_interruptible(500)) {
> -					if (signal_pending(current))
> -						return -EINTR;
> -				}
> -			}
>  
> -			mutex_lock(&adapter->mfe_lock);
> +			ret = wait_dvb_frontend(adapter, mfedev);
> +			if (ret)
> +				return ret;
> +
>  			if (adapter->mfe_dvbdev != dvbdev) {
>  				mfedev = adapter->mfe_dvbdev;
>  				mfe = mfedev->priv;
>  				mfepriv = mfe->frontend_priv;
>  				if (mfedev->users != -1 ||
> -				    mfepriv->thread) {
> -					mutex_unlock(&adapter->mfe_lock);
> +				    mfepriv->thread)
>  					return -EBUSY;
> -				}
>  				adapter->mfe_dvbdev = dvbdev;
>  			}
>  		}
>  	}
>  
>  	ret = __dvb_frontend_open(inode, file);
> -	mutex_unlock(&adapter->mfe_lock);
>  
>  	return ret;
>  }
> 



Thanks,
Mauro