From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A20D9C433DF for ; Mon, 22 Jun 2020 11:41:00 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 7BB0320716 for ; Mon, 22 Jun 2020 11:41:00 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7BB0320716 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id E92A66E4D2; Mon, 22 Jun 2020 11:40:59 +0000 (UTC) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTPS id 34EA86E0A6 for ; Mon, 22 Jun 2020 11:40:58 +0000 (UTC) IronPort-SDR: LyDTQfPjhDcl6zAJ/VSDXwQw+aOmpYsPSH9t8Ugeb5yk2knD76eToJex1Fbk+mVqJuh9TAIImv fuiRS8hjpTXA== X-IronPort-AV: E=McAfee;i="6000,8403,9659"; a="161813672" X-IronPort-AV: E=Sophos;i="5.75,266,1589266800"; d="scan'208";a="161813672" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Jun 2020 04:40:57 -0700 IronPort-SDR: a34dBW4tc9fUUjFHOSHElAWnkcRXlTv8h9hbjzF4peJrOc5trSCZHC+xqIpVOSyuMx8vQqCxkn QPNv5wijiIbA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.75,266,1589266800"; d="scan'208";a="262943724" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga007.fm.intel.com with ESMTP; 22 Jun 2020 04:40:57 -0700 Received: from fmsmsx113.amr.corp.intel.com (10.18.116.7) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 22 Jun 2020 04:40:57 -0700 Received: from fmsmsx108.amr.corp.intel.com ([169.254.9.193]) by FMSMSX113.amr.corp.intel.com ([169.254.13.85]) with mapi id 14.03.0439.000; Mon, 22 Jun 2020 04:40:57 -0700 From: "Ruhl, Michael J" To: Charan Teja Kalla , Sumit Semwal , "David.Laight@ACULAB.COM" , "open list:DMA BUFFER SHARING FRAMEWORK" , DRI mailing list Subject: RE: [PATCH v2] dmabuf: use spinlock to access dmabuf->name Thread-Topic: [PATCH v2] dmabuf: use spinlock to access dmabuf->name Thread-Index: AQHWRjDU8ynrfmeG7EGyHovmSs13oqjf8L+wgATmmYD//64hEA== Date: Mon, 22 Jun 2020 11:40:56 +0000 Message-ID: <14063C7AD467DE4B82DEDB5C278E866301154C1A5E@FMSMSX108.amr.corp.intel.com> References: <14063C7AD467DE4B82DEDB5C278E866301154BAE9E@FMSMSX108.amr.corp.intel.com> <97f2313e-a690-b5ab-567d-6887384debf5@codeaurora.org> In-Reply-To: <97f2313e-a690-b5ab-567d-6887384debf5@codeaurora.org> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.1.200.108] MIME-Version: 1.0 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Linaro MM SIG , LKML Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" >-----Original Message----- >From: charante=codeaurora.org@mg.codeaurora.org > On Behalf Of Charan Teja >Kalla >Sent: Monday, June 22, 2020 5:26 AM >To: Ruhl, Michael J ; Sumit Semwal >; David.Laight@ACULAB.COM; open list:DMA >BUFFER SHARING FRAMEWORK ; DRI mailing >list >Cc: Linaro MM SIG ; LKML kernel@vger.kernel.org> >Subject: Re: [PATCH v2] dmabuf: use spinlock to access dmabuf->name > >Hello Mike, > >On 6/19/2020 7:11 PM, Ruhl, Michael J wrote: >>> -----Original Message----- >>> From: charante=codeaurora.org@mg.codeaurora.org >>> On Behalf Of Charan >Teja >>> Kalla >>> Sent: Friday, June 19, 2020 7:57 AM >>> To: Sumit Semwal ; Ruhl, Michael J >>> ; David.Laight@ACULAB.COM; open list:DMA >>> BUFFER SHARING FRAMEWORK ; DRI >mailing >>> list >>> Cc: Linaro MM SIG ; LKML >> kernel@vger.kernel.org> >>> Subject: [PATCH v2] dmabuf: use spinlock to access dmabuf->name >>> >>> There exists a sleep-while-atomic bug while accessing the dmabuf->name >>> under mutex in the dmabuffs_dname(). This is caused from the SELinux >>> permissions checks on a process where it tries to validate the inherited >>> files from fork() by traversing them through iterate_fd() (which >>> traverse files under spin_lock) and call >>> match_file(security/selinux/hooks.c) where the permission checks >happen. >>> This audit information is logged using dump_common_audit_data() where >it >>> calls d_path() to get the file path name. If the file check happen on >>> the dmabuf's fd, then it ends up in ->dmabuffs_dname() and use mutex to >>> access dmabuf->name. The flow will be like below: >>> flush_unauthorized_files() >>> iterate_fd() >>> spin_lock() --> Start of the atomic section. >>> match_file() >>> file_has_perm() >>> avc_has_perm() >>> avc_audit() >>> slow_avc_audit() >>> common_lsm_audit() >>> dump_common_audit_data() >>> audit_log_d_path() >>> d_path() >>> dmabuffs_dname() >>> mutex_lock()--> Sleep while atomic. >>> >>> Call trace captured (on 4.19 kernels) is below: >>> ___might_sleep+0x204/0x208 >>> __might_sleep+0x50/0x88 >>> __mutex_lock_common+0x5c/0x1068 >>> __mutex_lock_common+0x5c/0x1068 >>> mutex_lock_nested+0x40/0x50 >>> dmabuffs_dname+0xa0/0x170 >>> d_path+0x84/0x290 >>> audit_log_d_path+0x74/0x130 >>> common_lsm_audit+0x334/0x6e8 >>> slow_avc_audit+0xb8/0xf8 >>> avc_has_perm+0x154/0x218 >>> file_has_perm+0x70/0x180 >>> match_file+0x60/0x78 >>> iterate_fd+0x128/0x168 >>> selinux_bprm_committing_creds+0x178/0x248 >>> security_bprm_committing_creds+0x30/0x48 >>> install_exec_creds+0x1c/0x68 >>> load_elf_binary+0x3a4/0x14e0 >>> search_binary_handler+0xb0/0x1e0 >>> >>> So, use spinlock to access dmabuf->name to avoid sleep-while-atomic. >>> >>> Cc: [5.3+] >>> Signed-off-by: Charan Teja Reddy >>> --- >>> >>> Changes in V2: Addressed review comments from Ruhl, Michael J >>> >>> Changes in V1: https://lore.kernel.org/patchwork/patch/1255055/ >>> >>> drivers/dma-buf/dma-buf.c | 11 +++++++---- >>> include/linux/dma-buf.h | 1 + >>> 2 files changed, 8 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >>> index 01ce125..d81d298 100644 >>> --- a/drivers/dma-buf/dma-buf.c >>> +++ b/drivers/dma-buf/dma-buf.c >>> @@ -45,10 +45,10 @@ static char *dmabuffs_dname(struct dentry >*dentry, >>> char *buffer, int buflen) >>> size_t ret = 0; >>> >>> dmabuf = dentry->d_fsdata; >>> - dma_resv_lock(dmabuf->resv, NULL); >>> + spin_lock(&dmabuf->name_lock); >>> if (dmabuf->name) >>> ret = strlcpy(name, dmabuf->name, DMA_BUF_NAME_LEN); >>> - dma_resv_unlock(dmabuf->resv); >>> + spin_unlock(&dmabuf->name_lock); >>> >>> return dynamic_dname(dentry, buffer, buflen, "/%s:%s", >>> dentry->d_name.name, ret > 0 ? name : ""); >>> @@ -341,8 +341,10 @@ static long dma_buf_set_name(struct dma_buf >>> *dmabuf, const char __user *buf) >>> kfree(name); >>> goto out_unlock; >>> } >>> + spin_lock(&dmabuf->name_lock); >>> kfree(dmabuf->name); >>> dmabuf->name = name; >>> + spin_unlock(&dmabuf->name_lock); >> >> While this code path is ok, I would have separated the protection of the >> attachment list and the name manipulation. >> >> dma_resv_lock(resv) >> if (!list_empty(attachment) >> ret = -EBUSY >> dma_resv_unlock(resv) >> >> if (ret) { >> kfree(name) >> return ret; >> } > >Is it that the name should be visible before importer attaches to the >dmabuf,(using dma_buf_attach()), thus _buf_set_name() is under the >_resv_lock() as well? That is the name that was being freed in the error path of the lock block. Alternatively: dma_resv_lock(resv) if (!list_empty(attachment) { ret = -EBUSY kfree(name) } dma_resv_unlock(resv) if (ret) return ret; I was limiting what was happening in the lock block. You have two distinct locks, that protect two distinct items: dmabuf->attachment dmabuf->name Nesting the locking is ok, but if the code ever changes you can get that nesting wrong, so: long ret = 0; if (IS_ERR(name)) return PTR_ERR(name); dma_resv_lock(dmabuf->resv, NULL); if (!list_empty(&dmabuf->attachments)) { ret = -EBUSY; kfree(name); } dma_resv_unlock(dmabuf->resv); if (ret) return ret; spinlock(dmabuf->name_lock) kfree(dmabuf->name); dmabuf->name = name; spinunlock(dmabuf->name_lock) return 0; } M > >> >> spinlock(nam_lock) >> ... >> >> Nesting locks that don't need to be nested always makes me nervous >> for future use that misses the lock/unlock pattern. >> >> However, this looks reasonable. >> >> With this current code, or if you update to the above pattern: >> >> Reviewed-by: Michael J. Ruhl > >Thanks for the ACK. >> >> Mike >> >> >>> out_unlock: >>> dma_resv_unlock(dmabuf->resv); >>> @@ -405,10 +407,10 @@ static void dma_buf_show_fdinfo(struct seq_file >>> *m, struct file *file) >>> /* Don't count the temporary reference taken inside procfs seq_show >>> */ >>> seq_printf(m, "count:\t%ld\n", file_count(dmabuf->file) - 1); >>> seq_printf(m, "exp_name:\t%s\n", dmabuf->exp_name); >>> - dma_resv_lock(dmabuf->resv, NULL); >>> + spin_lock(&dmabuf->name_lock); >>> if (dmabuf->name) >>> seq_printf(m, "name:\t%s\n", dmabuf->name); >>> - dma_resv_unlock(dmabuf->resv); >>> + spin_unlock(&dmabuf->name_lock); >>> } >>> >>> static const struct file_operations dma_buf_fops = { >>> @@ -546,6 +548,7 @@ struct dma_buf *dma_buf_export(const struct >>> dma_buf_export_info *exp_info) >>> dmabuf->size = exp_info->size; >>> dmabuf->exp_name = exp_info->exp_name; >>> dmabuf->owner = exp_info->owner; >>> + spin_lock_init(&dmabuf->name_lock); >>> init_waitqueue_head(&dmabuf->poll); >>> dmabuf->cb_excl.poll = dmabuf->cb_shared.poll = &dmabuf->poll; >>> dmabuf->cb_excl.active = dmabuf->cb_shared.active = 0; >>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h >>> index ab0c156..93108fd 100644 >>> --- a/include/linux/dma-buf.h >>> +++ b/include/linux/dma-buf.h >>> @@ -311,6 +311,7 @@ struct dma_buf { >>> void *vmap_ptr; >>> const char *exp_name; >>> const char *name; >>> + spinlock_t name_lock; >>> struct module *owner; >>> struct list_head list_node; >>> void *priv; >>> -- >>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora >>> Forum, a Linux Foundation Collaborative Project > >-- >The Qualcomm Innovation Center, Inc. is a member of the Code Aurora >Forum, a Linux Foundation Collaborative Project _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel