Received: by 2002:a6b:fb09:0:0:0:0:0 with SMTP id h9csp1225765iog; Thu, 30 Jun 2022 20:49:39 -0700 (PDT) X-Google-Smtp-Source: AGRyM1sxwPeafio8jRj9mXRUOrRCfz07HM8L0uah9bHxQ2ZYi1c4LZc2gxElEJBmRoshiUCPQnpI X-Received: by 2002:a17:907:1c12:b0:726:a6a3:751c with SMTP id nc18-20020a1709071c1200b00726a6a3751cmr11435571ejc.683.1656647379762; Thu, 30 Jun 2022 20:49:39 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1656647379; cv=none; d=google.com; s=arc-20160816; b=kZkuZ5HNzy9jHa4/Ptaz3Fatv9mip8rbBDjYdHIxiZzhgMcc3jR9EFjOENIZwkoyPb LwRmehU8cEOrT3aJ4EbcW5K7jVFs7xYj3UVcMIkP0j/YuPXruKCc/2XA3swMNyW+DdXV TtEr2L2pJIGXSueBqLYPhGeb9/0ThE6gyT4To1aqN/IRbJMeawjz3dN0/fKxTuRVBX6e fYcktD9oMsNRrlB7UIzWInGm4cfAUFx/SAITEag004YIq0+ovg61IKJyP4+P1FYexjOe lNoX1tTr92y3QIOWBj01hZNwDbK38CqiGcQ5j8xYd/p0mANE3klUFiLm5uCZnx7Dnrkr Gq7g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=moDrQDciiD9zvGpAvAvRu8+57Oada2iCKxbwjOQrpi0=; b=dAojgnecRGWcCXx5j/VA7+7Uy2qV4BmnAQkkNoAeNi3D4EwF7GzuQuMNeZmUsOL15b xYsnbgXrpeA9Gd/60vnw41Y981ZnXu3K6SVrBCBPLiYMjrUy2tRmFQAMuxC3ddR1UVgs 8X27pVOA0tUY5uc2pX1qq3YaTCYUukL1ikkT0t+TLYqGJCVns9x9MfkPgdcEclb7ozye Fd3gHJtc36oozvClhXznIRZ60A1LMyB3k+O+bGgyHqS2yxKAJSBrfbzpYIaIlll7OUn4 YeLzqa7buVpp4iP5HAlrbSqCNTDsNAd1KvGmucBkdoQw+b8sLM6kGH2wuW7HjTj2Rjhe YkBw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=yOpvXYiP; spf=pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-crypto-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id l18-20020a056402255200b0043761340274si3862939edb.127.2022.06.30.20.49.02; Thu, 30 Jun 2022 20:49:39 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=yOpvXYiP; spf=pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-crypto-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234050AbiGADs7 (ORCPT + 99 others); Thu, 30 Jun 2022 23:48:59 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60848 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232992AbiGADs7 (ORCPT ); Thu, 30 Jun 2022 23:48:59 -0400 Received: from mail-pf1-x430.google.com (mail-pf1-x430.google.com [IPv6:2607:f8b0:4864:20::430]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DFDE765D70 for ; Thu, 30 Jun 2022 20:48:57 -0700 (PDT) Received: by mail-pf1-x430.google.com with SMTP id k9so1279282pfg.5 for ; Thu, 30 Jun 2022 20:48:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=moDrQDciiD9zvGpAvAvRu8+57Oada2iCKxbwjOQrpi0=; b=yOpvXYiPN8Vj0CBxMyk71T1T0K2ZeNuEFa4bUEXZiDdRtO7/KTr/w4NsKnSwrz4+Cy eKsenOc6SbkjxWMsV7uoaogkh+r56JSdRcVB8kpc5w9dl1qMLUzNzhhCPwcti3QXzlBJ svHktb37/GesFbHHjthzbtI90ETY+WTTvcGTKuc/E0jirNsJRr2pCucyjw01iUrpIpWX jRIEIrlDbf6n2/vxYguLivxC5tRC7LOpQ68S4j8lRGtVbdYfI9ovePowDguCLqWgvHn/ Cqy4ir3gQY7jkVHxdOZQZVeD1IixQqPuBewO8DXk0qLl6lwuatWJUCsDiKYaPW9ZXZ6B lbAA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=moDrQDciiD9zvGpAvAvRu8+57Oada2iCKxbwjOQrpi0=; b=L7C2fnjuF2KRsBf1zhdLnVmKy7omNXAWELamDssVkbyuNX6VdgWe1GVPUopt8hFqzI Z1tGHgi0rrlkhf4k2BLHTAGCL6s8Yf+6Rq0K8HkvF0xA85ZLuqRnrrSK2l0piS7aekpg lOAPuh5W53MnADhqYDm8JDBRIOgQ+ZeaPYwUZ3GA30hf++xMjJLaRXENfOwN2brX0j78 gto95Jx5xMJIH6Rzlvw9rvB4PvNKuBPuG3xe/sJ2+NxNf7vSRy6Lb4rXBJGp+KsKEUbX nhcl5dDexOClJq5k8tZLyW27BDy6Yl46XmEotupX1CsW2Del98Pu/GOLg9ftPzCe9Ccc GjCw== X-Gm-Message-State: AJIora+mm3weHbay/sirgl0/i8/BKtezUsDsVFc4TaN0e/mNYywkl3R9 AvX2T64HibLLb7yA88q85axn1Q== X-Received: by 2002:a63:1566:0:b0:405:3aa3:23de with SMTP id 38-20020a631566000000b004053aa323demr10377420pgv.528.1656647337397; Thu, 30 Jun 2022 20:48:57 -0700 (PDT) Received: from localhost.localdomain ([199.101.192.109]) by smtp.gmail.com with ESMTPSA id p66-20020a625b45000000b0051b9ecb53e6sm14344328pfb.105.2022.06.30.20.48.50 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Thu, 30 Jun 2022 20:48:57 -0700 (PDT) From: Zhangfei Gao To: Greg Kroah-Hartman , Arnd Bergmann , Herbert Xu , jean-philippe , Wangzhou , Jonathan Cameron Cc: acc@openeuler.org, linux-kernel@vger.kernel.org, linux-crypto@vger.kernel.org, iommu@lists.linux.dev, Yang Shen , Zhangfei Gao Subject: [PATCH v3] uacce: Handle parent device removal or parent driver module rmmod Date: Fri, 1 Jul 2022 11:48:43 +0800 Message-Id: <20220701034843.7502-1-zhangfei.gao@linaro.org> X-Mailer: git-send-email 2.36.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-crypto@vger.kernel.org From: Jean-Philippe Brucker The uacce driver must deal with a possible removal of the parent device or parent driver module rmmod at any time. Although uacce_remove(), called on device removal and on driver unbind, prevents future use of the uacce fops by removing the cdev, fops that were called before that point may still be running. Serialize uacce_fops_open() and uacce_remove() with uacce->mutex. Serialize other fops against uacce_remove() with q->mutex. Since we need to protect uacce_fops_poll() which gets called on the fast path, replace uacce->queues_lock with q->mutex to improve scalability. The other fops are only used during setup. uacce_queue_is_valid(), checked under q->mutex or uacce->mutex, denotes whether uacce_remove() has disabled all queues. If that is the case, don't go any further since the parent device is being removed and uacce->ops should not be called anymore. Reported-by: Yang Shen Signed-off-by: Zhangfei Gao Signed-off-by: Jean-Philippe Brucker --- v3: Drop patch 1, which set cdev owner with parent driver onwer, to block parent driver rmmod. Suggested from Greg, this behavior introduce layering violation. Only keep patch 2, which can cover both parent moduel rmmod and parent device removal. drivers/misc/uacce/uacce.c | 133 ++++++++++++++++++++++++------------- include/linux/uacce.h | 6 +- 2 files changed, 91 insertions(+), 48 deletions(-) diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c index 281c54003edc..b70a013139c7 100644 --- a/drivers/misc/uacce/uacce.c +++ b/drivers/misc/uacce/uacce.c @@ -9,43 +9,38 @@ static struct class *uacce_class; static dev_t uacce_devt; -static DEFINE_MUTEX(uacce_mutex); static DEFINE_XARRAY_ALLOC(uacce_xa); -static int uacce_start_queue(struct uacce_queue *q) +/* + * If the parent driver or the device disappears, the queue state is invalid and + * ops are not usable anymore. + */ +static bool uacce_queue_is_valid(struct uacce_queue *q) { - int ret = 0; + return q->state == UACCE_Q_INIT || q->state == UACCE_Q_STARTED; +} - mutex_lock(&uacce_mutex); +static int uacce_start_queue(struct uacce_queue *q) +{ + int ret; - if (q->state != UACCE_Q_INIT) { - ret = -EINVAL; - goto out_with_lock; - } + if (q->state != UACCE_Q_INIT) + return -EINVAL; if (q->uacce->ops->start_queue) { ret = q->uacce->ops->start_queue(q); if (ret < 0) - goto out_with_lock; + return ret; } q->state = UACCE_Q_STARTED; - -out_with_lock: - mutex_unlock(&uacce_mutex); - - return ret; + return 0; } static int uacce_put_queue(struct uacce_queue *q) { struct uacce_device *uacce = q->uacce; - mutex_lock(&uacce_mutex); - - if (q->state == UACCE_Q_ZOMBIE) - goto out; - if ((q->state == UACCE_Q_STARTED) && uacce->ops->stop_queue) uacce->ops->stop_queue(q); @@ -54,8 +49,6 @@ static int uacce_put_queue(struct uacce_queue *q) uacce->ops->put_queue(q); q->state = UACCE_Q_ZOMBIE; -out: - mutex_unlock(&uacce_mutex); return 0; } @@ -65,20 +58,36 @@ static long uacce_fops_unl_ioctl(struct file *filep, { struct uacce_queue *q = filep->private_data; struct uacce_device *uacce = q->uacce; + long ret = -ENXIO; + + /* + * uacce->ops->ioctl() may take the mmap_lock when copying arg to/from + * user. Avoid a circular lock dependency with uacce_fops_mmap(), which + * gets called with mmap_lock held, by taking uacce->mutex instead of + * q->mutex. Doing this in uacce_fops_mmap() is not possible because + * uacce_fops_open() calls iommu_sva_bind_device(), which takes + * mmap_lock, while holding uacce->mutex. + */ + mutex_lock(&uacce->mutex); + if (!uacce_queue_is_valid(q)) + goto out_unlock; switch (cmd) { case UACCE_CMD_START_Q: - return uacce_start_queue(q); - + ret = uacce_start_queue(q); + break; case UACCE_CMD_PUT_Q: - return uacce_put_queue(q); - + ret = uacce_put_queue(q); + break; default: - if (!uacce->ops->ioctl) - return -EINVAL; - - return uacce->ops->ioctl(q, cmd, arg); + if (uacce->ops->ioctl) + ret = uacce->ops->ioctl(q, cmd, arg); + else + ret = -EINVAL; } +out_unlock: + mutex_unlock(&uacce->mutex); + return ret; } #ifdef CONFIG_COMPAT @@ -136,6 +145,13 @@ static int uacce_fops_open(struct inode *inode, struct file *filep) if (!q) return -ENOMEM; + mutex_lock(&uacce->mutex); + + if (!uacce->parent) { + ret = -EINVAL; + goto out_with_mem; + } + ret = uacce_bind_queue(uacce, q); if (ret) goto out_with_mem; @@ -152,10 +168,9 @@ static int uacce_fops_open(struct inode *inode, struct file *filep) filep->private_data = q; uacce->inode = inode; q->state = UACCE_Q_INIT; - - mutex_lock(&uacce->queues_lock); + mutex_init(&q->mutex); list_add(&q->list, &uacce->queues); - mutex_unlock(&uacce->queues_lock); + mutex_unlock(&uacce->mutex); return 0; @@ -163,18 +178,20 @@ static int uacce_fops_open(struct inode *inode, struct file *filep) uacce_unbind_queue(q); out_with_mem: kfree(q); + mutex_unlock(&uacce->mutex); return ret; } static int uacce_fops_release(struct inode *inode, struct file *filep) { struct uacce_queue *q = filep->private_data; + struct uacce_device *uacce = q->uacce; - mutex_lock(&q->uacce->queues_lock); - list_del(&q->list); - mutex_unlock(&q->uacce->queues_lock); + mutex_lock(&uacce->mutex); uacce_put_queue(q); uacce_unbind_queue(q); + list_del(&q->list); + mutex_unlock(&uacce->mutex); kfree(q); return 0; @@ -217,10 +234,9 @@ static int uacce_fops_mmap(struct file *filep, struct vm_area_struct *vma) vma->vm_private_data = q; qfr->type = type; - mutex_lock(&uacce_mutex); - - if (q->state != UACCE_Q_INIT && q->state != UACCE_Q_STARTED) { - ret = -EINVAL; + mutex_lock(&q->mutex); + if (!uacce_queue_is_valid(q)) { + ret = -ENXIO; goto out_with_lock; } @@ -248,12 +264,12 @@ static int uacce_fops_mmap(struct file *filep, struct vm_area_struct *vma) } q->qfrs[type] = qfr; - mutex_unlock(&uacce_mutex); + mutex_unlock(&q->mutex); return ret; out_with_lock: - mutex_unlock(&uacce_mutex); + mutex_unlock(&q->mutex); kfree(qfr); return ret; } @@ -262,12 +278,20 @@ static __poll_t uacce_fops_poll(struct file *file, poll_table *wait) { struct uacce_queue *q = file->private_data; struct uacce_device *uacce = q->uacce; + __poll_t ret = 0; + + mutex_lock(&q->mutex); + if (!uacce_queue_is_valid(q)) + goto out_unlock; poll_wait(file, &q->wait, wait); + if (uacce->ops->is_q_updated && uacce->ops->is_q_updated(q)) - return EPOLLIN | EPOLLRDNORM; + ret = EPOLLIN | EPOLLRDNORM; - return 0; +out_unlock: + mutex_unlock(&q->mutex); + return ret; } static const struct file_operations uacce_fops = { @@ -450,7 +474,7 @@ struct uacce_device *uacce_alloc(struct device *parent, goto err_with_uacce; INIT_LIST_HEAD(&uacce->queues); - mutex_init(&uacce->queues_lock); + mutex_init(&uacce->mutex); device_initialize(&uacce->dev); uacce->dev.devt = MKDEV(MAJOR(uacce_devt), uacce->dev_id); uacce->dev.class = uacce_class; @@ -507,13 +531,23 @@ void uacce_remove(struct uacce_device *uacce) if (uacce->inode) unmap_mapping_range(uacce->inode->i_mapping, 0, 0, 1); + /* + * uacce_fops_open() may be running concurrently, even after we remove + * the cdev. Holding uacce->mutex ensures that open() does not obtain a + * removed uacce device. + */ + mutex_lock(&uacce->mutex); /* ensure no open queue remains */ - mutex_lock(&uacce->queues_lock); list_for_each_entry_safe(q, next_q, &uacce->queues, list) { + /* + * Taking q->mutex ensures that fops do not use the defunct + * uacce->ops after the queue is disabled. + */ + mutex_lock(&q->mutex); uacce_put_queue(q); + mutex_unlock(&q->mutex); uacce_unbind_queue(q); } - mutex_unlock(&uacce->queues_lock); /* disable sva now since no opened queues */ uacce_disable_sva(uacce); @@ -521,6 +555,13 @@ void uacce_remove(struct uacce_device *uacce) if (uacce->cdev) cdev_device_del(uacce->cdev, &uacce->dev); xa_erase(&uacce_xa, uacce->dev_id); + /* + * uacce exists as long as there are open fds, but ops will be freed + * now. Ensure that bugs cause NULL deref rather than use-after-free. + */ + uacce->ops = NULL; + uacce->parent = NULL; + mutex_unlock(&uacce->mutex); put_device(&uacce->dev); } EXPORT_SYMBOL_GPL(uacce_remove); diff --git a/include/linux/uacce.h b/include/linux/uacce.h index 48e319f40275..9ce88c28b0a8 100644 --- a/include/linux/uacce.h +++ b/include/linux/uacce.h @@ -70,6 +70,7 @@ enum uacce_q_state { * @wait: wait queue head * @list: index into uacce queues list * @qfrs: pointer of qfr regions + * @mutex: protects queue state * @state: queue state machine * @pasid: pasid associated to the mm * @handle: iommu_sva handle returned by iommu_sva_bind_device() @@ -80,6 +81,7 @@ struct uacce_queue { wait_queue_head_t wait; struct list_head list; struct uacce_qfile_region *qfrs[UACCE_MAX_REGION]; + struct mutex mutex; enum uacce_q_state state; u32 pasid; struct iommu_sva *handle; @@ -97,9 +99,9 @@ struct uacce_queue { * @dev_id: id of the uacce device * @cdev: cdev of the uacce * @dev: dev of the uacce + * @mutex: protects uacce operation * @priv: private pointer of the uacce * @queues: list of queues - * @queues_lock: lock for queues list * @inode: core vfs */ struct uacce_device { @@ -113,9 +115,9 @@ struct uacce_device { u32 dev_id; struct cdev *cdev; struct device dev; + struct mutex mutex; void *priv; struct list_head queues; - struct mutex queues_lock; struct inode *inode; }; -- 2.36.1