linux-kernel - Re: [PATCH 1/3] coresight: ultrasoc-smb: fix sleep while close preempt in enable

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]

Date:   Thu, 19 Oct 2023 20:45:03 +0800
From:   hejunhao <hejunhao3@...wei.com>
To:     Yicong Yang <yangyicong@...wei.com>
CC:     <suzuki.poulose@....com>, <james.clark@....com>,
        <yangyicong@...ilicon.com>, <coresight@...ts.linaro.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <linuxarm@...wei.com>,
        <jonathan.cameron@...wei.com>, <prime.zeng@...ilicon.com>
Subject: Re: [PATCH 1/3] coresight: ultrasoc-smb: fix sleep while close
 preempt in enable_smb

Hi Yicong,


On 2023/10/19 11:05, Yicong Yang wrote:
> On 2023/10/12 17:47, Junhao He wrote:
>
>
> Use spinlock replace mutex to control driver data access to one at a
> time. But the function copy_to_user() may sleep so spinlock do not to
> lock it.
>
> Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver")
> Signed-off-by: Junhao He <hejunhao3@...wei.com>
> ---
>   drivers/hwtracing/coresight/ultrasoc-smb.c | 36 ++++++++++------------
>   drivers/hwtracing/coresight/ultrasoc-smb.h |  6 ++--
>   2 files changed, 19 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
> index e9a32a97fbee..b08a619d1116 100644
> --- a/drivers/hwtracing/coresight/ultrasoc-smb.c
> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
> @@ -99,7 +99,7 @@ static int smb_open(struct inode *inode, struct file *file)
>   					struct smb_drv_data, miscdev);
>   	int ret = 0;
>   
> -	mutex_lock(&drvdata->mutex);
> +	spin_lock(&drvdata->spinlock);
>   
>   	if (drvdata->reading) {
>   		ret = -EBUSY;
> @@ -115,7 +115,7 @@ static int smb_open(struct inode *inode, struct file *file)
>   
>   	drvdata->reading = true;
>   out:
> -	mutex_unlock(&drvdata->mutex);
> +	spin_unlock(&drvdata->spinlock);
>   
>   	return ret;
>   }
> @@ -132,10 +132,8 @@ static ssize_t smb_read(struct file *file, char __user *data, size_t len,
>   	if (!len)
>   		return 0;
>   
> -	mutex_lock(&drvdata->mutex);
> -
>   	if (!sdb->data_size)
> -		goto out;
> +		return 0;
>   
>   	to_copy = min(sdb->data_size, len);
>   
> @@ -145,20 +143,18 @@ static ssize_t smb_read(struct file *file, char __user *data, size_t len,
>   
>   	if (copy_to_user(data, sdb->buf_base + sdb->buf_rdptr, to_copy)) {
>   		dev_dbg(dev, "Failed to copy data to user\n");
> -		to_copy = -EFAULT;
> -		goto out;
> +		return -EFAULT;
>   	}
>   
> +	spin_lock(&drvdata->spinlock);
>   	*ppos += to_copy;
> -
>   	smb_update_read_ptr(drvdata, to_copy);
>   
> -	dev_dbg(dev, "%zu bytes copied\n", to_copy);
> -out:
>   	if (!sdb->data_size)
>   		smb_reset_buffer(drvdata);
> -	mutex_unlock(&drvdata->mutex);
> +	spin_unlock(&drvdata->spinlock);
> Do we still need the lock here? If we get here, we should have the exclusive
> access to the file, which is protected in open(). Or any other cases?

This is something I've also considered. If someone opens an SMB device
and reads it using multithreading, The SMB device will got out of sync.
I've seen other coresight sink drivers such as etb do not use locks in
the read function.
Maybe it's not necessary here, the buffer synchronization
is guaranteed by the user.

Best regards,
Junhao.