linux-kernel - Re: [PATCH] dma-buf: add ref counting for module as exporter

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAO_48GGjgRP=ab1wjGABguP3BaWPriLou_C-Su2C0kSeMS3Y4Q@mail.gmail.com>
Date:	Thu, 7 May 2015 16:16:47 +0530
From:	Sumit Semwal <sumit.semwal@...aro.org>
To:	Greg KH <gregkh@...uxfoundation.org>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
	DRI mailing list <dri-devel@...ts.freedesktop.org>,
	Linaro MM SIG Mailman List <linaro-mm-sig@...ts.linaro.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	rmk+kernel@....linux.org.uk, Dave Airlie <airlied@...ux.ie>,
	kgene@...nel.org, Thierry Reding <thierry.reding@...il.com>,
	Pawel Osciak <pawel@...iak.com>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Mauro Carvalho Chehab <mchehab@....samsung.com>,
	Linaro Kernel Mailman List <linaro-kernel@...ts.linaro.org>,
	Rob Clark <robdclark@...il.com>,
	Daniel Vetter <daniel@...ll.ch>,
	intel-gfx@...ts.freedesktop.org, linux-tegra@...r.kernel.org,
	inki.dae@...sung.com,
	Maarten Lankhorst <maarten.lankhorst@...onical.com>,
	Tomasz Stanislawski <t.stanislaws@...sung.com>
Subject: Re: [PATCH] dma-buf: add ref counting for module as exporter

On 7 May 2015 at 13:40, Greg KH <gregkh@...uxfoundation.org> wrote:
> On Thu, May 07, 2015 at 01:00:52PM +0530, Sumit Semwal wrote:
>> Add reference counting on a kernel module that exports dma-buf and
>> implements its operations. This prevents the module from being unloaded
>> while DMABUF file is in use.
>>
>> The original patch [1] was submitted by Tomasz, but he's since shifted
>> jobs and a ping didn't elicit any response.
>>
>>   [tomasz: Original author]
>> Signed-off-by: Tomasz Stanislawski <t.stanislaws@...sung.com>
>> Acked-by: Daniel Vetter <daniel@...ll.ch>
>>   [sumits: updated & rebased as per review comments]
>> Signed-off-by: Sumit Semwal <sumit.semwal@...aro.org>
>>
>> [1]: https://lkml.org/lkml/2012/8/8/163
>> ---
>>  drivers/dma-buf/dma-buf.c                      | 9 ++++++++-
>>  drivers/gpu/drm/armada/armada_gem.c            | 1 +
>>  drivers/gpu/drm/drm_prime.c                    | 1 +
>>  drivers/gpu/drm/exynos/exynos_drm_dmabuf.c     | 1 +
>>  drivers/gpu/drm/i915/i915_gem_dmabuf.c         | 1 +
>>  drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c      | 1 +
>>  drivers/gpu/drm/tegra/gem.c                    | 1 +
>>  drivers/gpu/drm/udl/udl_dmabuf.c               | 1 +
>>  drivers/gpu/drm/vmwgfx/vmwgfx_prime.c          | 1 +
>>  drivers/media/v4l2-core/videobuf2-dma-contig.c | 1 +
>>  drivers/media/v4l2-core/videobuf2-dma-sg.c     | 1 +
>>  drivers/media/v4l2-core/videobuf2-vmalloc.c    | 1 +
>>  drivers/staging/android/ion/ion.c              | 1 +
>>  include/linux/dma-buf.h                        | 2 ++
>>  14 files changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>> index c5a9138a6a8d..9583eac0238f 100644
>> --- a/drivers/dma-buf/dma-buf.c
>> +++ b/drivers/dma-buf/dma-buf.c
>> @@ -29,6 +29,7 @@
>>  #include <linux/anon_inodes.h>
>>  #include <linux/export.h>
>>  #include <linux/debugfs.h>
>> +#include <linux/module.h>
>>  #include <linux/seq_file.h>
>>  #include <linux/poll.h>
>>  #include <linux/reservation.h>
>> @@ -64,6 +65,7 @@ static int dma_buf_release(struct inode *inode, struct file *file)
>>       BUG_ON(dmabuf->cb_shared.active || dmabuf->cb_excl.active);
>>
>>       dmabuf->ops->release(dmabuf);
>> +     module_put(dmabuf->ops->owner);
>>
>>       mutex_lock(&db_list.lock);
>>       list_del(&dmabuf->list_node);
>> @@ -302,9 +304,14 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info)
>>               return ERR_PTR(-EINVAL);
>>       }
>>
>> +     if (!try_module_get(exp_info->ops->owner))
>> +             return ERR_PTR(-ENOENT);
>> +
>>       dmabuf = kzalloc(alloc_size, GFP_KERNEL);
>> -     if (dmabuf == NULL)
>> +     if (!dmabuf) {
>> +             module_put(exp_info->ops->owner);
>>               return ERR_PTR(-ENOMEM);
>> +     }
>>
>>       dmabuf->priv = exp_info->priv;
>>       dmabuf->ops = exp_info->ops;
>> diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
>> index 580e10acaa3a..d2c5fc1269b7 100644
>> --- a/drivers/gpu/drm/armada/armada_gem.c
>> +++ b/drivers/gpu/drm/armada/armada_gem.c
>> @@ -524,6 +524,7 @@ armada_gem_dmabuf_mmap(struct dma_buf *buf, struct vm_area_struct *vma)
>>  }
>>
>>  static const struct dma_buf_ops armada_gem_prime_dmabuf_ops = {
>> +     .owner = THIS_MODULE,
>>       .map_dma_buf    = armada_gem_prime_map_dma_buf,
>>       .unmap_dma_buf  = armada_gem_prime_unmap_dma_buf,
>>       .release        = drm_gem_dmabuf_release,
>
> The "easier" way to do this is change the registration function to add
> the module owner automatically, which keeps you from having to modify
> all of the individual drivers.  Look at how pci and usb do this for
> their driver registration functions.  That should result in a much
> smaller patch, that always works properly for everyone (there's no way
> for driver to get it wrong.)
>
Thanks Greg; but of course, you're right! We already have a
DEFINE_DMA_BUF_EXPORT_INFO macro, so this is far easier incorporated
into that.

I will spin up the (much simpler) patch and repost!

> thanks,
>
> greg k-h


Best regards,
Sumit.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/