| 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
| ||
|
Message-ID: <fdc8d8b0-4e04-78f5-1e8a-4cf44c89a37f@linaro.org>
Date: Thu, 16 Jun 2022 12:10:18 +0800
From: Zhangfei Gao <zhangfei.gao@...aro.org>
To: Jean-Philippe Brucker <jean-philippe@...aro.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Arnd Bergmann <arnd@...db.de>,
Herbert Xu <herbert@...dor.apana.org.au>,
Wangzhou <wangzhou1@...ilicon.com>,
Jonathan Cameron <Jonathan.Cameron@...wei.com>,
linux-accelerators@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
linux-crypto@...r.kernel.org, iommu@...ts.linux-foundation.org,
Yang Shen <shenyang39@...wei.com>
Subject: Re: [PATCH] uacce: fix concurrency of fops_open and uacce_remove
Hi, Jean
On 2022/6/15 下午11:16, Jean-Philippe Brucker wrote:
> Hi,
>
> On Fri, Jun 10, 2022 at 08:34:23PM +0800, Zhangfei Gao wrote:
>> The uacce parent's module can be removed when uacce is working,
>> which may cause troubles.
>>
>> If rmmod/uacce_remove happens just after fops_open: bind_queue,
>> the uacce_remove can not remove the bound queue since it is not
>> added to the queue list yet, which blocks the uacce_disable_sva.
>>
>> Change queues_lock area to make sure the bound queue is added to
>> the list thereby can be searched in uacce_remove.
>>
>> And uacce->parent->driver is checked immediately in case rmmod is
>> just happening.
>>
>> Also the parent driver must always stop DMA before calling
>> uacce_remove.
>>
>> Signed-off-by: Yang Shen <shenyang39@...wei.com>
>> Signed-off-by: Zhangfei Gao <zhangfei.gao@...aro.org>
>> ---
>> drivers/misc/uacce/uacce.c | 19 +++++++++++++------
>> 1 file changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
>> index 281c54003edc..b6219c6bfb48 100644
>> --- a/drivers/misc/uacce/uacce.c
>> +++ b/drivers/misc/uacce/uacce.c
>> @@ -136,9 +136,16 @@ static int uacce_fops_open(struct inode *inode, struct file *filep)
>> if (!q)
>> return -ENOMEM;
>>
>> + mutex_lock(&uacce->queues_lock);
>> +
>> + if (!uacce->parent->driver) {
> I don't think this is useful, because the core clears parent->driver after
> having run uacce_remove():
>
> rmmod hisi_zip open()
> ... uacce_fops_open()
> __device_release_driver() ...
> pci_device_remove()
> hisi_zip_remove()
> hisi_qm_uninit()
> uacce_remove()
> ... ...
> mutex_lock(uacce->queues_lock)
> ... if (!uacce->parent->driver)
> device_unbind_cleanup() /* driver still valid, proceed */
> dev->driver = NULL
The check if (!uacce->parent->driver) is required, otherwise NULL
pointer may happen.
iommu_sva_bind_device
const struct iommu_ops *ops = dev_iommu_ops(dev); ->
dev->iommu->iommu_dev->ops
rmmod has no issue, but remove parent pci device has the issue.
Test:
sleep in fops_open before mutex.
estuary:/mnt$ ./work/a.out &
//sleep in fops_open
echo 1 > /sys/bus/pci/devices/0000:00:02.0/remove &
estuary:/mnt$ [ 22.594348] uacce_remove!
[ 22.594663] pci 0000:00:02.0: Removing from iommu group 2
[ 22.595073] iommu_release_device dev->iommu=0
[ 22.595076] CPU: 2 PID: 229 Comm: ash Not tainted
5.19.0-rc1-15071-gcbcf098c5257-dirty #633
[ 22.595079] Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0
02/06/2015
[ 22.595080] Call trace:
[ 22.595080] dump_backtrace+0xe4/0xf0
[ 22.595085] show_stack+0x20/0x70
[ 22.595086] dump_stack_lvl+0x8c/0xb8
[ 22.595089] dump_stack+0x18/0x34
[ 22.595091] iommu_release_device+0x90/0x98
[ 22.595095] iommu_bus_notifier+0x58/0x68
[ 22.595097] blocking_notifier_call_chain+0x74/0xa8
[ 22.595100] device_del+0x268/0x3b0
[ 22.595102] pci_remove_bus_device+0x84/0x110
[ 22.595106] pci_stop_and_remove_bus_device_locked+0x30/0x60
...
estuary:/mnt$ [ 31.466360] uacce: sleep end!
[ 31.466362] uacce->parent->driver=0
[ 31.466364] uacce->parent->iommu=0
[ 31.466365] uacce_bind_queue!
[ 31.466366] uacce_bind_queue call iommu_sva_bind_device!
[ 31.466367] uacce->parent=d120d0
[ 31.466371] Unable to handle kernel NULL pointer dereference at
virtual address 0000000000000038
[ 31.472870] Mem abort info:
[ 31.473450] ESR = 0x0000000096000004
[ 31.474223] EC = 0x25: DABT (current EL), IL = 32 bits
[ 31.475390] SET = 0, FnV = 0
[ 31.476031] EA = 0, S1PTW = 0
[ 31.476680] FSC = 0x04: level 0 translation fault
[ 31.477687] Data abort info:
[ 31.478294] ISV = 0, ISS = 0x00000004
[ 31.479152] CM = 0, WnR = 0
[ 31.479785] user pgtable: 4k pages, 48-bit VAs, pgdp=00000000714d8000
[ 31.481144] [0000000000000038] pgd=0000000000000000, p4d=0000000000000000
[ 31.482622] Internal error: Oops: 96000004 [#1] PREEMPT SMP
[ 31.483784] Modules linked in: hisi_zip
[ 31.484590] CPU: 2 PID: 228 Comm: a.out Not tainted
5.19.0-rc1-15071-gcbcf098c5257-dirty #633
[ 31.486374] Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0
02/06/2015
[ 31.487862] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS
BTYPE=--)
[ 31.489390] pc : iommu_sva_bind_device+0x44/0xf4
[ 31.490404] lr : uacce_fops_open+0x128/0x234
>
> Since uacce_remove() disabled SVA, the following uacce_bind_queue() will
> fail anyway. However, if uacce->flags does not have UACCE_DEV_SVA set,
> we'll proceed further and call uacce->ops->get_queue(), which does not
> exist anymore since the parent module is gone.
>
> I think we need the global uacce_mutex to serialize uacce_remove() and
> uacce_fops_open(). uacce_remove() would do everything, including
> xa_erase(), while holding that mutex. And uacce_fops_open() would try to
> obtain the uacce object from the xarray while holding the mutex, which
> fails if the uacce object is being removed.
Since fops_open get char device refcount, uacce_release will not happen
until open returns.
So either uacce = xa_load(&uacce_xa, iminor(inode)) is got,
uacce_release release uacce after fops_release.
Or uacce is not got and return -ENODEV．
open:
uacce = xa_load(&uacce_xa, iminor(inode));
if (!uacce)
return -ENODEV;
uacce->dev.release = uacce_release;
uacce_release: kfree(uacce);
Thanks
Powered by blists - more mailing lists