From: Jean-Philippe Brucker <jean-philippe@linaro.org> To: Zhangfei Gao <zhangfei.gao@linaro.org> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Arnd Bergmann <arnd@arndb.de>, Herbert Xu <herbert@gondor.apana.org.au>, Wangzhou <wangzhou1@hisilicon.com>, Jonathan Cameron <Jonathan.Cameron@huawei.com>, linux-accelerators@lists.ozlabs.org, linux-kernel@vger.kernel.org, linux-crypto@vger.kernel.org, iommu@lists.linux-foundation.org, Yang Shen <shenyang39@huawei.com> Subject: Re: [PATCH] uacce: fix concurrency of fops_open and uacce_remove Date: Wed, 15 Jun 2022 16:16:02 +0100 [thread overview] Message-ID: <Yqn3spLZHpAkQ9Us@myrica> (raw) In-Reply-To: <20220610123423.27496-1-zhangfei.gao@linaro.org> 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 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. Thanks, Jean > + ret = -ENODEV; > + goto out_with_lock; > + } > + > ret = uacce_bind_queue(uacce, q); > if (ret) > - goto out_with_mem; > + goto out_with_lock; > > q->uacce = uacce; > > @@ -153,7 +160,6 @@ static int uacce_fops_open(struct inode *inode, struct file *filep) > uacce->inode = inode; > q->state = UACCE_Q_INIT; > > - mutex_lock(&uacce->queues_lock); > list_add(&q->list, &uacce->queues); > mutex_unlock(&uacce->queues_lock); > > @@ -161,7 +167,8 @@ static int uacce_fops_open(struct inode *inode, struct file *filep) > > out_with_bond: > uacce_unbind_queue(q); > -out_with_mem: > +out_with_lock: > + mutex_unlock(&uacce->queues_lock); > kfree(q); > return ret; > } > @@ -171,10 +178,10 @@ static int uacce_fops_release(struct inode *inode, struct file *filep) > struct uacce_queue *q = filep->private_data; > > mutex_lock(&q->uacce->queues_lock); > - list_del(&q->list); > - mutex_unlock(&q->uacce->queues_lock); > uacce_put_queue(q); > uacce_unbind_queue(q); > + list_del(&q->list); > + mutex_unlock(&q->uacce->queues_lock); > kfree(q); > > return 0; > @@ -513,10 +520,10 @@ void uacce_remove(struct uacce_device *uacce) > uacce_put_queue(q); > uacce_unbind_queue(q); > } > - mutex_unlock(&uacce->queues_lock); > > /* disable sva now since no opened queues */ > uacce_disable_sva(uacce); > + mutex_unlock(&uacce->queues_lock); > > if (uacce->cdev) > cdev_device_del(uacce->cdev, &uacce->dev); > -- > 2.36.1 >
WARNING: multiple messages have this Message-ID (diff)
From: Jean-Philippe Brucker <jean-philippe@linaro.org> To: Zhangfei Gao <zhangfei.gao@linaro.org> Cc: Yang Shen <shenyang39@huawei.com>, Herbert Xu <herbert@gondor.apana.org.au>, Arnd Bergmann <arnd@arndb.de>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org, linux-crypto@vger.kernel.org, linux-accelerators@lists.ozlabs.org Subject: Re: [PATCH] uacce: fix concurrency of fops_open and uacce_remove Date: Wed, 15 Jun 2022 16:16:02 +0100 [thread overview] Message-ID: <Yqn3spLZHpAkQ9Us@myrica> (raw) In-Reply-To: <20220610123423.27496-1-zhangfei.gao@linaro.org> 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 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. Thanks, Jean > + ret = -ENODEV; > + goto out_with_lock; > + } > + > ret = uacce_bind_queue(uacce, q); > if (ret) > - goto out_with_mem; > + goto out_with_lock; > > q->uacce = uacce; > > @@ -153,7 +160,6 @@ static int uacce_fops_open(struct inode *inode, struct file *filep) > uacce->inode = inode; > q->state = UACCE_Q_INIT; > > - mutex_lock(&uacce->queues_lock); > list_add(&q->list, &uacce->queues); > mutex_unlock(&uacce->queues_lock); > > @@ -161,7 +167,8 @@ static int uacce_fops_open(struct inode *inode, struct file *filep) > > out_with_bond: > uacce_unbind_queue(q); > -out_with_mem: > +out_with_lock: > + mutex_unlock(&uacce->queues_lock); > kfree(q); > return ret; > } > @@ -171,10 +178,10 @@ static int uacce_fops_release(struct inode *inode, struct file *filep) > struct uacce_queue *q = filep->private_data; > > mutex_lock(&q->uacce->queues_lock); > - list_del(&q->list); > - mutex_unlock(&q->uacce->queues_lock); > uacce_put_queue(q); > uacce_unbind_queue(q); > + list_del(&q->list); > + mutex_unlock(&q->uacce->queues_lock); > kfree(q); > > return 0; > @@ -513,10 +520,10 @@ void uacce_remove(struct uacce_device *uacce) > uacce_put_queue(q); > uacce_unbind_queue(q); > } > - mutex_unlock(&uacce->queues_lock); > > /* disable sva now since no opened queues */ > uacce_disable_sva(uacce); > + mutex_unlock(&uacce->queues_lock); > > if (uacce->cdev) > cdev_device_del(uacce->cdev, &uacce->dev); > -- > 2.36.1 > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
next prev parent reply other threads:[~2022-06-15 15:16 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-06-10 12:34 [PATCH] uacce: fix concurrency of fops_open and uacce_remove Zhangfei Gao 2022-06-10 12:34 ` Zhangfei Gao 2022-06-15 15:16 ` Jean-Philippe Brucker [this message] 2022-06-15 15:16 ` Jean-Philippe Brucker 2022-06-16 4:10 ` Zhangfei Gao 2022-06-16 4:10 ` Zhangfei Gao 2022-06-16 8:14 ` Jean-Philippe Brucker 2022-06-16 8:14 ` Jean-Philippe Brucker 2022-06-17 6:05 ` Zhangfei Gao 2022-06-17 6:05 ` Zhangfei Gao 2022-06-17 8:20 ` Zhangfei Gao 2022-06-17 8:20 ` Zhangfei Gao 2022-06-17 14:23 ` Zhangfei Gao 2022-06-17 14:23 ` Zhangfei Gao 2022-06-20 13:25 ` Jean-Philippe Brucker 2022-06-20 13:25 ` Jean-Philippe Brucker 2022-06-20 13:24 ` Jean-Philippe Brucker 2022-06-20 13:24 ` Jean-Philippe Brucker 2022-06-20 13:36 ` Greg Kroah-Hartman 2022-06-20 13:36 ` Greg Kroah-Hartman 2022-06-21 7:37 ` Zhangfei Gao 2022-06-21 7:37 ` Zhangfei Gao 2022-06-21 7:44 ` Greg Kroah-Hartman 2022-06-21 7:44 ` Greg Kroah-Hartman 2022-06-22 8:14 ` Zhangfei Gao 2022-06-22 8:14 ` Zhangfei Gao 2022-06-22 8:24 ` Greg Kroah-Hartman 2022-06-22 8:24 ` Greg Kroah-Hartman 2022-06-20 13:38 ` Greg Kroah-Hartman 2022-06-20 13:38 ` Greg Kroah-Hartman 2022-06-20 20:18 ` [PATCH] uacce: Tidy up locking kernel test robot 2022-06-20 20:18 ` kernel test robot
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=Yqn3spLZHpAkQ9Us@myrica \ --to=jean-philippe@linaro.org \ --cc=Jonathan.Cameron@huawei.com \ --cc=arnd@arndb.de \ --cc=gregkh@linuxfoundation.org \ --cc=herbert@gondor.apana.org.au \ --cc=iommu@lists.linux-foundation.org \ --cc=linux-accelerators@lists.ozlabs.org \ --cc=linux-crypto@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=shenyang39@huawei.com \ --cc=wangzhou1@hisilicon.com \ --cc=zhangfei.gao@linaro.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.