linux-kernel - Re: [Linaro-mm-sig] [PATCH v3 6/9] dma-buf: Move dma-buf attachment to dynamic locking specification

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <e7576ab7-ff1e-e6da-b0fd-0315f1b37ed1@amd.com>
Date:   Wed, 24 Aug 2022 19:45:07 +0200
From:   Christian König <christian.koenig@....com>
To:     Dmitry Osipenko <dmitry.osipenko@...labora.com>,
        Christian König <ckoenig.leichtzumerken@...il.com>,
        David Airlie <airlied@...ux.ie>,
        Gerd Hoffmann <kraxel@...hat.com>,
        Gurchetan Singh <gurchetansingh@...omium.org>,
        Chia-I Wu <olvaffe@...il.com>, Daniel Vetter <daniel@...ll.ch>,
        Daniel Almeida <daniel.almeida@...labora.com>,
        Gert Wollny <gert.wollny@...labora.com>,
        Gustavo Padovan <gustavo.padovan@...labora.com>,
        Daniel Stone <daniel@...ishbar.org>,
        Tomeu Vizoso <tomeu.vizoso@...labora.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Rob Clark <robdclark@...il.com>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        "Pan, Xinhui" <Xinhui.Pan@....com>,
        Thierry Reding <thierry.reding@...il.com>,
        Tomasz Figa <tfiga@...omium.org>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Alex Deucher <alexander.deucher@....com>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>,
        Thomas Hellström <thomas_os@...pmail.org>,
        Qiang Yu <yuq825@...il.com>
Cc:     dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        Dmitry Osipenko <digetx@...il.com>,
        linux-media@...r.kernel.org, linaro-mm-sig@...ts.linaro.org,
        amd-gfx@...ts.freedesktop.org, intel-gfx@...ts.freedesktop.org,
        kernel@...labora.com, virtualization@...ts.linux-foundation.org,
        linux-rdma@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        lima@...ts.freedesktop.org
Subject: Re: [Linaro-mm-sig] [PATCH v3 6/9] dma-buf: Move dma-buf attachment
 to dynamic locking specification

Am 24.08.22 um 17:49 schrieb Dmitry Osipenko:
> On 8/24/22 18:24, Christian König wrote:
>> Am 24.08.22 um 12:22 schrieb Dmitry Osipenko:
>>> Move dma-buf attachment API functions to the dynamic locking
>>> specification.
>>> The strict locking convention prevents deadlock situations for dma-buf
>>> importers and exporters.
>>>
>>> Previously, the "unlocked" versions of the attachment API functions
>>> weren't taking the reservation lock and this patch makes them to take
>>> the lock.
>>>
>>> Intel and AMD GPU drivers already were mapping the attached dma-bufs
>>> under
>>> the held lock during attachment, hence these drivers are updated to use
>>> the locked functions.
>>>
>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@...labora.com>
>>> ---
>>>    drivers/dma-buf/dma-buf.c                  | 115 ++++++++++++++-------
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c    |   4 +-
>>>    drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c |   8 +-
>>>    drivers/gpu/drm/i915/gem/i915_gem_object.c |  12 +++
>>>    include/linux/dma-buf.h                    |  20 ++--
>>>    5 files changed, 110 insertions(+), 49 deletions(-)
>>>
>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>>> index 4556a12bd741..f2a5a122da4a 100644
>>> --- a/drivers/dma-buf/dma-buf.c
>>> +++ b/drivers/dma-buf/dma-buf.c
>>> @@ -559,7 +559,7 @@ static struct file *dma_buf_getfile(struct dma_buf
>>> *dmabuf, int flags)
>>>     * 2. Userspace passes this file-descriptors to all drivers it wants
>>> this buffer
>>>     *    to share with: First the file descriptor is converted to a
>>> &dma_buf using
>>>     *    dma_buf_get(). Then the buffer is attached to the device using
>>> - *    dma_buf_attach().
>>> + *    dma_buf_attach_unlocked().
>> Now I get why this is confusing me so much.
>>
>> The _unlocked postfix implies that there is another function which
>> should be called with the locks already held, but this is not the case
>> for attach/detach (because they always need to grab the lock themselves).
> That's correct. The attach/detach ops of exporter can take the lock
> (like i915 exporter does it), hence importer must not grab the lock
> around dma_buf_attach() invocation.
>
>> So I suggest to drop the _unlocked postfix for the attach/detach
>> functions. Another step would then be to unify attach/detach with
>> dynamic_attach/dynamic_detach when both have the same locking convention
>> anyway.
> It's not a problem to change the name, but it's unclear to me why we
> should do it. The _unlocked postfix tells importer that reservation must
> be unlocked and it must be unlocked in case of dma_buf_attach().
>
> Dropping the postfix will make dma_buf_attach() inconsistent with the
> rest of the _unlocked functions(?). Are you sure we need to rename it?

The idea of the postfix was to distinguish between two different 
versions of the same function, e.g. dma_buf_vmap_unlocked() vs normal 
dma_buf_vmap().

When we don't have those two types of the same function I don't think it 
makes to much sense to keep that. We should just properly document which 
functions expect what and that's what your documentation patch does.

Regards,
Christian.

>
>> Sorry that this is going so much back and forth, it's really complicated
>> to keep all the stuff in my head at the moment :)
> Not a problem at all, I expected that it will take some time for this
> patchset to settle down.
>