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=-6.7 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 4CE4EC433DF for ; Fri, 19 Jun 2020 13:41:08 +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 22997208C7 for ; Fri, 19 Jun 2020 13:41:07 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 22997208C7 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 5BBD56ECC0; Fri, 19 Jun 2020 13:41:07 +0000 (UTC) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTPS id 9E6596ECC0 for ; Fri, 19 Jun 2020 13:41:05 +0000 (UTC) IronPort-SDR: 8QxQo3ej9YPIAWryP+UZ5CYvLPKJzvvDrgbwnnG5i1IK1rhr7dVGjUbYTqosUfRlLtHqNi+mLe xaUhvKQkpCbg== X-IronPort-AV: E=McAfee;i="6000,8403,9656"; a="161063857" X-IronPort-AV: E=Sophos;i="5.75,255,1589266800"; d="scan'208";a="161063857" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Jun 2020 06:41:05 -0700 IronPort-SDR: 7a6fRNvxQocMKFtpy6dheILuMZlNC+47soiadq6ZNoMhv0FDBFbc26o7AeG0lq/RmKEjJnIqPR NRo405rUk2Vg== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.75,255,1589266800"; d="scan'208";a="300061811" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga004.fm.intel.com with ESMTP; 19 Jun 2020 06:41:04 -0700 Received: from fmsmsx158.amr.corp.intel.com (10.18.116.75) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.439.0; Fri, 19 Jun 2020 06:41:04 -0700 Received: from fmsmsx108.amr.corp.intel.com ([169.254.9.193]) by fmsmsx158.amr.corp.intel.com ([169.254.15.146]) with mapi id 14.03.0439.000; Fri, 19 Jun 2020 06:41:04 -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+w Date: Fri, 19 Jun 2020 13:41:03 +0000 Message-ID: <14063C7AD467DE4B82DEDB5C278E866301154BAE9E@FMSMSX108.amr.corp.intel.com> References: In-Reply-To: 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: 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; } 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 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel