From: "Ruhl, Michael J" <michael.j.ruhl@intel.com>
To: Charan Teja Kalla <charante@codeaurora.org>,
Sumit Semwal <sumit.semwal@linaro.org>,
"David.Laight@ACULAB.COM" <David.Laight@ACULAB.COM>,
"open list:DMA BUFFER SHARING FRAMEWORK"
<linux-media@vger.kernel.org>,
DRI mailing list <dri-devel@lists.freedesktop.org>
Cc: Linaro MM SIG <linaro-mm-sig@lists.linaro.org>,
LKML <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v2] dmabuf: use spinlock to access dmabuf->name
Date: Mon, 22 Jun 2020 11:40:56 +0000 [thread overview]
Message-ID: <14063C7AD467DE4B82DEDB5C278E866301154C1A5E@FMSMSX108.amr.corp.intel.com> (raw)
In-Reply-To: <97f2313e-a690-b5ab-567d-6887384debf5@codeaurora.org>
>-----Original Message-----
>From: charante=codeaurora.org@mg.codeaurora.org
><charante=codeaurora.org@mg.codeaurora.org> On Behalf Of Charan Teja
>Kalla
>Sent: Monday, June 22, 2020 5:26 AM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>; Sumit Semwal
><sumit.semwal@linaro.org>; David.Laight@ACULAB.COM; open list:DMA
>BUFFER SHARING FRAMEWORK <linux-media@vger.kernel.org>; DRI mailing
>list <dri-devel@lists.freedesktop.org>
>Cc: Linaro MM SIG <linaro-mm-sig@lists.linaro.org>; LKML <linux-
>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
>>> <charante=codeaurora.org@mg.codeaurora.org> On Behalf Of Charan
>Teja
>>> Kalla
>>> Sent: Friday, June 19, 2020 7:57 AM
>>> To: Sumit Semwal <sumit.semwal@linaro.org>; Ruhl, Michael J
>>> <michael.j.ruhl@intel.com>; David.Laight@ACULAB.COM; open list:DMA
>>> BUFFER SHARING FRAMEWORK <linux-media@vger.kernel.org>; DRI
>mailing
>>> list <dri-devel@lists.freedesktop.org>
>>> Cc: Linaro MM SIG <linaro-mm-sig@lists.linaro.org>; LKML <linux-
>>> 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: <stable@vger.kernel.org> [5.3+]
>>> Signed-off-by: Charan Teja Reddy <charante@codeaurora.org>
>>> ---
>>>
>>> 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 <michael.j.ruhl@intel.com>
>
>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
next prev parent reply other threads:[~2020-06-22 11:41 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-19 11:57 [PATCH v2] dmabuf: use spinlock to access dmabuf->name Charan Teja Kalla
2020-06-19 13:41 ` Ruhl, Michael J
2020-06-22 9:26 ` Charan Teja Kalla
2020-06-22 11:40 ` Ruhl, Michael J [this message]
2020-06-23 11:34 ` Charan Teja Kalla
2020-06-23 14:23 ` Ruhl, Michael J
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=14063C7AD467DE4B82DEDB5C278E866301154C1A5E@FMSMSX108.amr.corp.intel.com \
--to=michael.j.ruhl@intel.com \
--cc=David.Laight@ACULAB.COM \
--cc=charante@codeaurora.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=linaro-mm-sig@lists.linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=sumit.semwal@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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).