Messages in this thread | | | Subject | Re: [PATCH] uacce: fix concurrency of fops_open and uacce_remove | From | Zhangfei Gao <> | Date | Thu, 16 Jun 2022 12:10:18 +0800 |
| |
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@huawei.com> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.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
| |