On Tue, Feb 18, 2020 at 12:05:53PM +0530, Sumit Semwal wrote:
> Hello Martin,
>
> Thanks for your patches - they look decent.
>
> May I please request you to run get_maintainers.pl (the patches need
> to be sent to a couple of other MLs too for wider review).
>
> Best,
> Sumit.
Sorry for the late reply. Sure, I will include more MLs for wider
review. Thanks for the suggestion.
> On Tue, 14 Jan 2020 at 20:37, Martin Liu <liumartin@xxxxxxxxxx> wrote:
> >
> > We introduced setname ioctl in commit bb2bb9030425 ("dma-buf:
> > add DMA_BUF_SET_NAME ioctls") that provides userpsace
> > to attach a free-form name for tracking and counting shared
> > buffers. However the d_dname callback could be called in atomic
> > context. This call path comes from selinux that verifies all
> > inherited open files from exec call. To verify all inherited
> > open files, kernel would iterate all fds which need to hold
> > spin_lock to get denty name by calling d_dname operation.
> > In dma-buf d_dname callback, we use mutex lock to prevent the
> > race from setname causing this issue.
> >
> > This commit adds a spinlock to protect set/get name operation
> > to fix this issue.
> >
> > [ 165.617090] Call trace:
> > [ 165.620504] ___might_sleep+0x114/0x118
> > [ 165.625344] __might_sleep+0x50/0x84
> > [ 165.629928] __mutex_lock_common+0x5c/0x10b0
> > [ 165.635215] mutex_lock_nested+0x40/0x50
> > [ 165.640157] dmabuffs_dname+0x48/0xdc
> > [ 165.644821] d_path+0x78/0x1e4
> > [ 165.648870] audit_log_d_path+0x68/0x134
> > [ 165.653807] common_lsm_audit+0x33c/0x6f4
> > [ 165.658832] slow_avc_audit+0xb4/0xf0
> > [ 165.663503] avc_has_perm+0xdc/0x1a4
> > [ 165.668081] file_has_perm+0x70/0x154
> > [ 165.672750] match_file+0x54/0x6c
> > [ 165.677064] iterate_fd+0x74/0xac
> > [ 165.681369] selinux_bprm_committing_creds+0xfc/0x210
> > [ 165.687459] security_bprm_committing_creds+0x2c/0x40
> > [ 165.693546] install_exec_creds+0x1c/0x68
> > [ 165.698569] load_elf_binary+0x3a0/0x13c8
> > [ 165.703590] search_binary_handler+0xb8/0x1e4
> > [ 165.708964] __do_execve_file+0x6e4/0x9c8
> > [ 165.713984] __arm64_sys_execve+0x44/0x54
> > [ 165.719008] el0_svc_common+0xa8/0x168
> > [ 165.723765] el0_svc_handler+0x78/0x94
> > [ 165.728522] el0_svc+0x8/0xc
> >
> > Signed-off-by: Martin Liu <liumartin@xxxxxxxxxx>
> > ---
> > drivers/dma-buf/dma-buf.c | 11 +++++++----
> > include/linux/dma-buf.h | 2 ++
> > 2 files changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> > index ce41cd9b758a..7cbcb22ad0e4 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 : "");
> > @@ -335,6 +335,7 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf)
> > return PTR_ERR(name);
> >
> > dma_resv_lock(dmabuf->resv, NULL);
> > + spin_lock(&dmabuf->name_lock);
> > if (!list_empty(&dmabuf->attachments)) {
> > ret = -EBUSY;
> > kfree(name);
> > @@ -344,6 +345,7 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf)
> > dmabuf->name = name;
> >
> > out_unlock:
> > + spin_unlock(&dmabuf->name_lock);
> > dma_resv_unlock(dmabuf->resv);
> > return ret;
> > }
> > @@ -403,10 +405,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 = {
> > @@ -561,6 +563,7 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info)
> > dmabuf->file = file;
> >
> > mutex_init(&dmabuf->lock);
> > + spin_lock_init(&dmabuf->name_lock);
> > INIT_LIST_HEAD(&dmabuf->attachments);
> >
> > mutex_lock(&db_list.lock);
> > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> > index af73f835c51c..1b138580f746 100644
> > --- a/include/linux/dma-buf.h
> > +++ b/include/linux/dma-buf.h
> > @@ -292,6 +292,7 @@ struct dma_buf_ops {
> > * @exp_name: name of the exporter; useful for debugging.
> > * @name: userspace-provided name; useful for accounting and debugging,
> > * protected by @resv.
> > + * @name_lock: lock to protect name.
> > * @owner: pointer to exporter module; used for refcounting when exporter is a
> > * kernel module.
> > * @list_node: node for dma_buf accounting and debugging.
> > @@ -320,6 +321,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;
> > --
> > 2.25.0.rc1.283.g88dfdc4193-goog
> >
>
>
> --
> Thanks and regards,
>
> Sumit Semwal
> Linaro Consumer Group - Kernel Team Lead
> Linaro.org │ Open source software for ARM SoCs
