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] [day] [month] [year] [list]

Message-ID: <2f6c2af0-b867-5be5-ed37-c5d71b1ed1ce@huawei.com>
Date:   Sat, 21 Oct 2023 15:25:09 +0800
From:   hejunhao <hejunhao3@...wei.com>
To:     Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        <linuxarm@...wei.com>
CC:     <suzuki.poulose@....com>, <james.clark@....com>,
        <coresight@...ts.linaro.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <yangyicong@...wei.com>,
        <prime.zeng@...ilicon.com>
Subject: Re: [PATCH 1/3] coresight: ultrasoc-smb: fix sleep while close
 preempt in enable_smb

Hi Jonathan,


On 2023/10/19 21:30, Jonathan Cameron wrote:
> On Thu, 12 Oct 2023 17:47:04 +0800
> Junhao He <hejunhao3@...wei.com> wrote:
>
>> When we to enable the SMB by perf, the perf sched will call perf_ctx_lock()
>> to close system preempt in event_function_call(). But SMB::enable_smb() use
>> mutex to lock the critical section, which may sleep.
>>
>>   BUG: sleeping function called from invalid context at kernel/locking/mutex.c:580
>>   in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 153023, name: perf
>>   preempt_count: 2, expected: 0
>>   RCU nest depth: 0, expected: 0
>>   INFO: lockdep is turned off.
>>   irq event stamp: 0
>>   hardirqs last  enabled at (0): [<0000000000000000>] 0x0
>>   hardirqs last disabled at (0): [<ffffa2983f5c5f40>] copy_process+0xae8/0x2b48
>>   softirqs last  enabled at (0): [<ffffa2983f5c5f40>] copy_process+0xae8/0x2b48
>>   softirqs last disabled at (0): [<0000000000000000>] 0x0
>>   CPU: 2 PID: 153023 Comm: perf Kdump: loaded Tainted: G   W  O   6.5.0-rc4+ #1
>>
>>   Call trace:
>>   ...
>>    __mutex_lock+0xbc/0xa70
>>    mutex_lock_nested+0x34/0x48
>>    smb_update_buffer+0x58/0x360 [ultrasoc_smb]
>>    etm_event_stop+0x204/0x2d8 [coresight]
>>    etm_event_del+0x1c/0x30 [coresight]
>>    event_sched_out+0x17c/0x3b8
>>    group_sched_out.part.0+0x5c/0x208
>>    __perf_event_disable+0x15c/0x210
>>    event_function+0xe0/0x230
>>    remote_function+0xb4/0xe8
>>    generic_exec_single+0x160/0x268
>>    smp_call_function_single+0x20c/0x2a0
>>    event_function_call+0x20c/0x220
>>    _perf_event_disable+0x5c/0x90
>>    perf_event_for_each_child+0x58/0xc0
>>    _perf_ioctl+0x34c/0x1250
>>    perf_ioctl+0x64/0x98
>> ...
>>
>> 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.
> I'd like to see a comment on why we no longer need to lock over the copy_to_user
> rather than simply that we can't.

Yes, I will do that.

>> Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver")
>> Signed-off-by: Junhao He <hejunhao3@...wei.com>
> A follow up patch could change a lot of this to use the new cleanup.h (don't want that
> in the fix though as will make back porting trickier.).
> That should let you do
> guard(spin_lock)(&drvdata->spinlock);
> and then use direct returns instead of goto complexity.
>
>
>
> Jonathan

Thanks for sharing.
I will append up a new patch to use guards to reduce gotos.

>
>> @@ -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;
>> -
> Unrelated white space change that shouldn't be here.

Ok, i will drop this white space



Thanks for the comments!

Best regards,
Junhao.

>
>>   	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);
>>   
>> +	dev_dbg(dev, "%zu bytes copied\n", to_copy);
>>   	return to_copy;
>>   }
>
> .
>