linux-kernel - Re: [PATCH] drm/shmem-helper: Fix locking for drm_gem_shmem_get_pages

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: <d3331cf8-02df-bf15-586b-af9d10830758@asahilina.net>
Date:   Tue, 7 Feb 2023 19:33:05 +0900
From:   Asahi Lina <lina@...hilina.net>
To:     Javier Martinez Canillas <javierm@...hat.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>
Cc:     linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        Noralf Trønnes <noralf@...nnes.org>,
        asahi@...ts.linux.dev, Alyssa Rosenzweig <alyssa@...enzweig.io>
Subject: Re: [PATCH] drm/shmem-helper: Fix locking for
 drm_gem_shmem_get_pages_sgt()

On 07/02/2023 03.47, Javier Martinez Canillas wrote:
> Hello Lina,
> 
> On 2/5/23 13:51, Asahi Lina wrote:
>> Other functions touching shmem->sgt take the pages lock, so do that here
>> too. drm_gem_shmem_get_pages() & co take the same lock, so move to the
>> _locked() variants to avoid recursive locking.
>>
>> Discovered while auditing locking to write the Rust abstractions.
>>
>> Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects")
>> Fixes: 4fa3d66f132b ("drm/shmem: Do dma_unmap_sg before purging pages")
>> Signed-off-by: Asahi Lina <lina@...hilina.net>
>> ---
> 
> Good catch. The patch looks good to me.
> 
> Reviewed-by: Javier Martinez Canillas <javierm@...hat.com>
> 
> What about drm_gem_shmem_free() BTW, I believe that the helper should also
> grab the lock before unmap / free the sgtable?

That's called from driver free callbacks, so it should only be called
when there are no other users left and the refcount is zero, right? If
there's anyone else racing it I think we have bigger problems than the
pages lock at that point, since the last thing it does is `kfree(shmem);` ^^

(In Rust terms this is equivalent to the Drop trait, which takes a
mutable/exclusive reference, which means no other reference to the
object can exist at that point, so no races are possible. And in fact in
my Rust abstraction I trigger a drop of the Rust object embedded in the
shmem object before calling drm_gem_shmem_free(), so if this invariant
doesn't hold that code would be wrong too!)

~~ Lina