linux-kernel - Re: [PATCH v6 14/22] dma-buf: Introduce new locking convention

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: <7372dd1b-06f7-5336-4738-15f9b4d4d4b3@collabora.com>
Date:   Mon, 30 May 2022 16:57:22 +0300
From:   Dmitry Osipenko <dmitry.osipenko@...labora.com>
To:     Christian König <christian.koenig@....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 Herring <robh@...nel.org>,
        Steven Price <steven.price@....com>,
        Alyssa Rosenzweig <alyssa.rosenzweig@...labora.com>,
        Rob Clark <robdclark@...il.com>,
        Emil Velikov <emil.l.velikov@...il.com>,
        Robin Murphy <robin.murphy@....com>,
        Qiang Yu <yuq825@...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>
Cc:     dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        virtualization@...ts.linux-foundation.org,
        Dmitry Osipenko <digetx@...il.com>,
        linux-tegra@...r.kernel.org, linux-media@...r.kernel.org,
        linaro-mm-sig@...ts.linaro.org, amd-gfx@...ts.freedesktop.org,
        intel-gfx@...ts.freedesktop.org, kernel@...labora.com
Subject: Re: [PATCH v6 14/22] dma-buf: Introduce new locking convention

On 5/30/22 16:41, Christian König wrote:
> Hi Dmitry,
> 
> Am 30.05.22 um 15:26 schrieb Dmitry Osipenko:
>> Hello Christian,
>>
>> On 5/30/22 09:50, Christian König wrote:
>>> Hi Dmitry,
>>>
>>> First of all please separate out this patch from the rest of the series,
>>> since this is a complex separate structural change.
>> I assume all the patches will go via the DRM tree in the end since the
>> rest of the DRM patches in this series depend on this dma-buf change.
>> But I see that separation may ease reviewing of the dma-buf changes, so
>> let's try it.
> 
> That sounds like you are underestimating a bit how much trouble this
> will be.
> 
>>> I have tried this before and failed because catching all the locks in
>>> the right code paths are very tricky. So expect some fallout from this
>>> and make sure the kernel test robot and CI systems are clean.
>> Sure, I'll fix up all the reported things in the next iteration.
>>
>> BTW, have you ever posted yours version of the patch? Will be great if
>> we could compare the changed code paths.
> 
> No, I never even finished creating it after realizing how much work it
> would be.
> 
>>>> This patch introduces new locking convention for dma-buf users. From
>>>> now
>>>> on all dma-buf importers are responsible for holding dma-buf
>>>> reservation
>>>> lock around operations performed over dma-bufs.
>>>>
>>>> This patch implements the new dma-buf locking convention by:
>>>>
>>>>     1. Making dma-buf API functions to take the reservation lock.
>>>>
>>>>     2. Adding new locked variants of the dma-buf API functions for
>>>> drivers
>>>>        that need to manage imported dma-bufs under the held lock.
>>> Instead of adding new locked variants please mark all variants which
>>> expect to be called without a lock with an _unlocked postfix.
>>>
>>> This should make it easier to remove those in a follow up patch set and
>>> then fully move the locking into the importer.
>> Do we really want to move all the locks to the importers? Seems the
>> majority of drivers should be happy with the dma-buf helpers handling
>> the locking for them.
> 
> Yes, I clearly think so.
> 
>>
>>>>     3. Converting all drivers to the new locking scheme.
>>> I have strong doubts that you got all of them. At least radeon and
>>> nouveau should grab the reservation lock in their ->attach callbacks
>>> somehow.
>> Radeon and Nouveau use gem_prime_import_sg_table() and they take resv
>> lock already, seems they should be okay (?)
> 
> You are looking at the wrong side. You need to fix the export code path,
> not the import ones.
> 
> See for example attach on radeon works like this
> drm_gem_map_attach->drm_gem_pin->radeon_gem_prime_pin->radeon_bo_reserve->ttm_bo_reserve->dma_resv_lock.

Yeah, I was looking at the both sides, but missed this one.

> Same for nouveau and probably a few other exporters as well. That will
> certainly cause a deadlock if you don't fix it.
> 
> I strongly suggest to do this step by step, first attach/detach and then
> the rest.

Thank you very much for the suggestions. I'll implement them in the next
version.

-- 
Best regards,
Dmitry